diff --git a/app/models/concerns/remote_synchronizable.rb b/app/models/concerns/remote_synchronizable.rb new file mode 100644 index 00000000..6354ccec --- /dev/null +++ b/app/models/concerns/remote_synchronizable.rb @@ -0,0 +1,68 @@ +# Enhances a model with lifecycle callbacks to synchronise it with a remote resource using a client +# class (conventionally located in `app/clients/`). +# +# Example: +# ```ruby +# class Foo < ApplicationRecord +# include RemoteSynchronizable +# remote_synchronize with: BarApi::FooClient +# end +# ``` +# +# If the remote operation fails, the record will not be created/updated/destroyed, and will be +# marked invalid. +module RemoteSynchronizable + extend ActiveSupport::Concern + + included do + # Client class to use for synchronisation + class_attribute :client_class + + # Create and update the remote resource using the client during ActiveRecord lifecycle events. + # + # Normally we would avoid using ActiveRecord callbacks to make network calls, but as the core + # purpose of Search Admin is to provide an interface to manage resources on various remote APIs, + # this is part of its core domain. + before_create :create_remote, unless: :skip_remote_synchronization_on_create + before_update :update_remote + before_destroy :destroy_remote + + # Skips the creation of the remote synchronisation on create. + # + # This allows to create new instances of a record without a remote counterpart, for example + # when importing existing remote resources, or as part of test setup (see `spec/factories.rb`). + attr_accessor :skip_remote_synchronization_on_create + end + + class_methods do + # Set the class to be used for synchronisation for this model. It must allow initialisation with + # a record, and respond to `#create`, `#update` and `#delete`. + def remote_synchronize(with:) + self.client_class = with + end + end + +private + + def create_remote + client.create(self) # rubocop:disable Rails/SaveBang (not an ActiveRecord model) + rescue ClientError + raise ActiveRecord::RecordInvalid, self + end + + def update_remote + client.update(self) # rubocop:disable Rails/SaveBang (not an ActiveRecord model) + rescue ClientError + raise ActiveRecord::RecordInvalid, self + end + + def destroy_remote + client.delete(self) + rescue ClientError + throw :abort + end + + def client + @client ||= client_class.new + end +end diff --git a/app/models/control.rb b/app/models/control.rb index 7b3ae03a..d2824a1d 100644 --- a/app/models/control.rb +++ b/app/models/control.rb @@ -10,6 +10,9 @@ # see # https://cloud.google.com/ruby/docs/reference/google-cloud-discovery_engine-v1/latest/Google-Cloud-DiscoveryEngine-V1-Control class Control < ApplicationRecord + include RemoteSynchronizable + remote_synchronize with: DiscoveryEngine::ControlClient + delegated_type :action, types: %w[BoostAction FilterAction], dependent: :destroy accepts_nested_attributes_for :action diff --git a/config/locales/en.yml b/config/locales/en.yml index a8a40c48..d7539450 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -90,6 +90,13 @@ en: keywords: Keywords comment: Comments + errors: + models: + control: + remote_error: | + We couldn't save this control on Vertex AI Search because of an unexpected error. + This error has been logged. Please try again later. + controls: index: page_title: Controls diff --git a/spec/factories.rb b/spec/factories.rb index 10dce222..a9289135 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -1,5 +1,22 @@ FactoryBot.define do - factory :control do + # Unlike fixtures, record instances created through FactoryBot go through the full Rails callback + # lifecycle on creation. That is often desirable, but not in the case of our models using the + # `RemoteSynchronizable` concern to create counterparts on remote APIs automatically whenever you + # create a new record. + # + # This trait sets the `skip_remote_synchronization_on_create` flag to true on records created + # through factories that include it, so that this behaviour is skipped. + # + # If you _do_ want to specifically test remote synchronization on a record, you can override this + # manually when you build your model using `FactoryBot.build`: + # ```ruby + # build(:my_model, skip_remote_synchronization_on_create: false) + # ``` + trait :remote_synchronizable do + skip_remote_synchronization_on_create { true } + end + + factory :control, traits: %i[remote_synchronizable] do display_name { "Control" } action factory: :control_boost_action diff --git a/spec/models/control_spec.rb b/spec/models/control_spec.rb index 86b3b0a6..1632484b 100644 --- a/spec/models/control_spec.rb +++ b/spec/models/control_spec.rb @@ -1,4 +1,6 @@ RSpec.describe Control, type: :model do + it_behaves_like "RemoteSynchronizable", DiscoveryEngine::ControlClient + describe "validations" do subject(:control) { build(:control) } diff --git a/spec/support/shared_examples/concerns/remote_synchronizable.rb b/spec/support/shared_examples/concerns/remote_synchronizable.rb new file mode 100644 index 00000000..d7d86f56 --- /dev/null +++ b/spec/support/shared_examples/concerns/remote_synchronizable.rb @@ -0,0 +1,83 @@ +RSpec.shared_examples "RemoteSynchronizable" do |client_class| + let(:client) do + instance_double(client_class, create: true, update: true, delete: true) + end + let(:factory) { described_class.model_name.param_key } + + before do + allow(client_class).to receive(:new).and_return(client) + end + + describe "when creating a new record" do + subject(:record) { build(factory, skip_remote_synchronization_on_create: false) } + + it "creates the remote resource using the client" do + record.save! + + expect(client).to have_received(:create).with(record) + end + + context "when the remote resource creation fails" do + let(:error) { ClientError.new("Uh oh") } + + before do + allow(client).to receive(:create).and_raise(error) + + record.save # rubocop:disable Rails/SaveBang (we're checking record state) + end + + it "stops the record from being created" do + expect(record).not_to be_persisted + end + end + end + + describe "when updating an existing record" do + subject(:record) { create(factory) } + + it "updates the remote resource" do + record.save! + + expect(client).to have_received(:update).with(record) + end + + context "when the remote resource update fails" do + let(:error) { ClientError.new("Uh oh") } + + before do + allow(client).to receive(:update).and_raise(error) + + record.updated_at = Time.current # change something so we can check it won't save + record.save # rubocop:disable Rails/SaveBang (we're checking record state) + end + + it "stops the record from being persisted" do + expect(record).to be_changed + end + end + end + + describe "when destroying an existing record" do + subject(:record) { create(factory) } + + it "deletes the remote resource" do + record.destroy! + + expect(client).to have_received(:delete).with(record) + end + + context "when the remote resource deletion fails with an internal error" do + let(:error) { ClientError.new("Uh oh") } + + before do + allow(client).to receive(:delete).and_raise(error) + + record.destroy # rubocop:disable Rails/SaveBang (we're checking record state) + end + + it "stops the record from being destroyed" do + expect(described_class.exists?(record.id)).to be(true) + end + end + end +end diff --git a/spec/system/controls_spec.rb b/spec/system/controls_spec.rb index e2e84813..0940afc9 100644 --- a/spec/system/controls_spec.rb +++ b/spec/system/controls_spec.rb @@ -1,4 +1,12 @@ RSpec.describe "Controls", type: :system do + let(:control) do + instance_double(DiscoveryEngine::ControlClient, create: true, update: true, delete: true) + end + + before do + allow(DiscoveryEngine::ControlClient).to receive(:new).and_return(control) + end + scenario "Viewing controls" do given_several_controls_exist