From 8c5a6747fccdd0c17778c8c416c313d4818e936e Mon Sep 17 00:00:00 2001 From: jimbali <40831617+jimbali@users.noreply.github.com> Date: Tue, 11 Jun 2024 14:02:06 +0100 Subject: [PATCH] feat: Send cross_supplier_move_* notification to cross-deck supplier (#2275) Where the receiving location is operated by another supplier, we now send them a `cross_supplier_move_add` notification at the same time as we send the initial supplier a `create_move` notification. This means that they can add it to their systems to be ready for when they take over. When a MoveRedirect causes a move to no longer be cross-deck, we send a `cross_supplier_move_remove` notification to any supplier that previously received a `cross_supplier_move_add` notification, to let them know that they no longer need to be aware of this move. While a move is cross-deck, update and update status notifications will be sent to the receiving supplier as `cross_supplier_move_update` and `cross_supplier_move_update_status` respectively. [MAP-322] --- app/jobs/prepare_base_notifications_job.rb | 70 ++++++++--- app/jobs/prepare_lodging_notifications_job.rb | 2 +- app/models/generic_event/move_redirect.rb | 10 ++ app/models/move.rb | 4 + .../prepare_move_notifications_job_spec.rb | 115 ++++++++++++++++++ .../generic_event/move_redirect_spec.rb | 38 ++++++ spec/models/move_spec.rb | 16 +++ .../api/allocations_controller_update_spec.rb | 2 +- .../api/moves_controller_create_v2_spec.rb | 53 ++++++++ .../api/moves_controller_update_v2_spec.rb | 56 +++++++++ 10 files changed, 350 insertions(+), 16 deletions(-) diff --git a/app/jobs/prepare_base_notifications_job.rb b/app/jobs/prepare_base_notifications_job.rb index 4cf9273da..cdee85a06 100644 --- a/app/jobs/prepare_base_notifications_job.rb +++ b/app/jobs/prepare_base_notifications_job.rb @@ -7,15 +7,13 @@ def perform(topic_id:, action_name:, queue_as:, send_webhooks: true, send_emails topic = find_topic(topic_id) move = associated_move(topic) - subscriptions(move, only_supplier_id:).find_each do |subscription| + subscriptions(move, action_name:, only_supplier_id:).find_each do |subscription| if send_webhooks && subscription.callback_url.present? && should_webhook?(subscription, move, action_name) - notification = build_notification(subscription, NotificationType::WEBHOOK, topic, action_name) - NotifyWebhookJob.perform_later(notification_id: notification.id, queue_as:) + build_and_send_notifications(subscription, NotificationType::WEBHOOK, topic, action_name, queue_as) end if send_emails && subscription.email_address.present? && should_email?(move) - notification = build_notification(subscription, NotificationType::EMAIL, topic, action_name) - NotifyEmailJob.perform_later(notification_id: notification.id, queue_as:) + build_and_send_notifications(subscription, NotificationType::EMAIL, topic, action_name, queue_as) end end end @@ -30,20 +28,36 @@ def associated_move(topic) raise NotImplementedError end - def subscriptions(move, only_supplier_id: nil) - suppliers = [move.supplier || move.suppliers].flatten.filter do |supplier| + def subscriptions(move, action_name:, only_supplier_id: nil) + # only cross-deck suppliers get `cross_supplier_add` or `cross_supplier_remove` notifications + case action_name + when 'cross_supplier_add' + return Subscription.kept.enabled.where(supplier: move.to_location&.suppliers || []) + when 'cross_supplier_remove' + notified_sub_ids = Notification.where(topic: move, event_type: 'cross_supplier_move_add').pluck(:subscription_id) + return Subscription.kept.enabled.where(id: notified_sub_ids) + end + + suppliers = [move.supplier || move.suppliers].flatten + + if move.cross_deck? + suppliers += move.to_location&.suppliers || [] + end + + suppliers = suppliers.uniq.filter do |supplier| only_supplier_id.nil? || only_supplier_id == supplier.id end Subscription.kept.enabled.where(supplier: suppliers) end - def build_notification(subscription, type_id, topic, action_name) - subscription.notifications.create!( + def build_and_send_notifications(subscription, type_id, topic, action_name, queue_as) + notification = subscription.notifications.create!( notification_type_id: type_id, topic:, - event_type: event_type(action_name, topic, type_id), + event_type: event_type(action_name, topic, type_id, subscription), ) + notify_job(type_id).perform_later(notification_id: notification.id, queue_as:) end def should_webhook?(subscription, move, action_name) @@ -62,17 +76,45 @@ def should_email?(move) move.current? && VALID_EMAIL_STATUSES.include?(move.status) end - def event_type(action_name, topic, type_id) + CROSS_SUPPLIER_EQUIVALENT = { + 'update_move' => 'cross_supplier_move_update', + 'update_move_status' => 'cross_supplier_move_update_status', + }.freeze + + def event_type(action_name, topic, type_id, subscription) action = { 'create' => 'create_move', 'update' => 'update_move', 'update_status' => 'update_move_status', 'destroy' => 'destroy_move', + 'cross_supplier_add' => 'cross_supplier_move_add', + 'cross_supplier_remove' => 'cross_supplier_move_remove', }.fetch(action_name, action_name) - return action unless action == 'update_move_status' + # make sure we send a create_move notification if we haven't sent one yet + if action == 'update_move_status' + create_notification = topic.notifications.find_by(event_type: 'create_move', notification_type_id: type_id) + action = 'create_move' if create_notification.nil? && !topic.cancelled? + end + + # send create notification as `cross_supplier_move_add` if we are notifying a cross-deck supplier + if action == 'create_move' && !topic.from_location.suppliers.include?(subscription.supplier) + action = 'cross_supplier_move_add' + end + + # make sure we send a cross_supplier_move_add notification if we haven't sent one yet for a cross-deck supplier + if %w[update_move update_move_status].include?(action) && !topic.from_location.suppliers.include?(subscription.supplier) + add_notification = topic.notifications.find_by(event_type: 'cross_supplier_move_add', notification_type_id: type_id) + action = add_notification.nil? ? 'cross_supplier_move_add' : CROSS_SUPPLIER_EQUIVALENT[action] + end + + action + end - create_notification = topic.notifications.find_by(event_type: 'create_move', notification_type_id: type_id) - create_notification.nil? && !topic.cancelled? ? 'create_move' : 'update_move_status' + def notify_job(type_id) + { + NotificationType::WEBHOOK => NotifyWebhookJob, + NotificationType::EMAIL => NotifyEmailJob, + }[type_id] end end diff --git a/app/jobs/prepare_lodging_notifications_job.rb b/app/jobs/prepare_lodging_notifications_job.rb index b5820fda4..bec859e87 100644 --- a/app/jobs/prepare_lodging_notifications_job.rb +++ b/app/jobs/prepare_lodging_notifications_job.rb @@ -11,7 +11,7 @@ def associated_move(topic) topic.move end - def event_type(action_name, topic, _) + def event_type(action_name, topic, _, _) "#{action_name}_#{topic.class.name&.underscore}" end end diff --git a/app/models/generic_event/move_redirect.rb b/app/models/generic_event/move_redirect.rb index 3ced56db7..19a435693 100644 --- a/app/models/generic_event/move_redirect.rb +++ b/app/models/generic_event/move_redirect.rb @@ -25,8 +25,18 @@ class MoveRedirect < GenericEvent delegate :generic_events, to: :eventable def trigger(*) + was_cross_deck = eventable.cross_deck? + eventable.to_location = to_location eventable.move_type = move_type if move_type.present? + + if !was_cross_deck && eventable.cross_deck? + Notifier.prepare_notifications(topic: eventable, action_name: 'cross_supplier_add') + end + + if was_cross_deck && !eventable.cross_deck? + Notifier.prepare_notifications(topic: eventable, action_name: 'cross_supplier_remove') + end end end end diff --git a/app/models/move.rb b/app/models/move.rb index 584de213c..b34a65c88 100644 --- a/app/models/move.rb +++ b/app/models/move.rb @@ -382,6 +382,10 @@ def expected_collection_time generic_events.select { |event| event.type == 'GenericEvent::MoveNotifyPremisesOfExpectedCollectionTime' }.max_by(&:occurred_at)&.expected_at end + def cross_deck? + from_location&.suppliers != to_location&.suppliers + end + private def date_to_after_date_from diff --git a/spec/jobs/prepare_move_notifications_job_spec.rb b/spec/jobs/prepare_move_notifications_job_spec.rb index 93f9ea37a..97772b0a3 100644 --- a/spec/jobs/prepare_move_notifications_job_spec.rb +++ b/spec/jobs/prepare_move_notifications_job_spec.rb @@ -97,6 +97,87 @@ it_behaves_like 'it schedules NotifyEmailJob' it_behaves_like 'it does not schedule NotifyWebhookJob' end + + context 'when it is a cross-deck move' do + let!(:initial_supplier) { create(:supplier) } + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: initial_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let(:move) { create :move, from_location: location, to_location:, supplier: } + + it 'sends the create_move and cross_supplier_move_add notifications to the respective suppliers' do + perform + expect(Notification.webhooks.order(:created_at).pluck(:subscription_id, :event_type)).to contain_exactly( + [subscription.id, 'create_move'], + [subscription2.id, 'cross_supplier_move_add'], + ) + end + end + end + + context 'when updating a move' do + let(:action_name) { 'update' } + + context 'when a subscription has both a webhook and email addresses' do + it_behaves_like 'it creates a webhook notification record' + it_behaves_like 'it creates an email notification record' + it_behaves_like 'it schedules NotifyWebhookJob' + it_behaves_like 'it schedules NotifyEmailJob' + end + + context 'when a subscription has no email addresses' do + let(:subscription) { create :subscription, :no_email_address } + + it_behaves_like 'it creates a webhook notification record' + it_behaves_like 'it does not create an email notification record' + it_behaves_like 'it schedules NotifyWebhookJob' + it_behaves_like 'it does not schedule NotifyEmailJob' + end + + context 'when a subscription has no webhook' do + let(:subscription) { create :subscription, :no_callback_url } + + it_behaves_like 'it does not create a webhook notification record' + it_behaves_like 'it creates an email notification record' + it_behaves_like 'it schedules NotifyEmailJob' + it_behaves_like 'it does not schedule NotifyWebhookJob' + end + + context 'when it is updated to become a cross-deck move' do + let!(:initial_supplier) { create(:supplier) } + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: initial_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let(:move) { create :move, from_location: location, to_location:, supplier: } + + it 'sends the update_move and cross_supplier_move_add notifications to the respective suppliers' do + perform + expect(Notification.webhooks.order(:created_at).pluck(:subscription_id, :event_type)).to contain_exactly( + [subscription.id, 'update_move'], + [subscription2.id, 'cross_supplier_move_add'], + ) + end + end + + context 'when it is a cross-deck move that has already been notified' do + let!(:initial_supplier) { create(:supplier) } + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: initial_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let(:move) { create :move, from_location: location, to_location:, supplier: } + let!(:existing_notification) { create(:notification, event_type: 'cross_supplier_move_add', topic: move, subscription: subscription2) } + + it 'sends the update_move and cross_supplier_move_update notifications to the suppliers' do + perform + expect(Notification.webhooks.order(:created_at).last(2).pluck(:subscription_id, :event_type)).to contain_exactly( + [subscription.id, 'update_move'], + [subscription2.id, 'cross_supplier_move_update'], + ) + end + end end context 'when updating move status' do @@ -159,6 +240,40 @@ end end + context 'when explicitly notifying a cross-deck move' do + let(:action_name) { 'cross_supplier_add' } + let!(:initial_supplier) { create(:supplier) } + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: initial_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let(:move) { create :move, from_location: location, to_location:, supplier: } + + it 'sends the cross_supplier_move_add notification only to the receiving supplier' do + perform + expect(Notification.webhooks.order(:created_at).pluck(:subscription_id, :event_type)).to contain_exactly( + [subscription2.id, 'cross_supplier_move_add'], + ) + end + end + + context 'when explicitly notifying that a move is no longer cross-deck' do + let(:action_name) { 'cross_supplier_remove' } + let!(:initial_supplier) { create(:supplier) } + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: initial_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let(:move) { create :move, from_location: location, to_location:, supplier: } + let!(:notification) { create(:notification, :webhook, event_type: 'cross_supplier_move_add', subscription: subscription2, topic: move) } + + it 'sends the cross_supplier_move_remove notification only to the supplier who received the cross_supplier_move_add' do + perform + expect(Notification.webhooks.where.not(event_type: 'cross_supplier_move_add').pluck(:subscription_id, :event_type)) + .to contain_exactly([subscription2.id, 'cross_supplier_move_remove']) + end + end + context 'when confirming a person escort record' do let(:action_name) { 'confirm_person_escort_record' } diff --git a/spec/models/generic_event/move_redirect_spec.rb b/spec/models/generic_event/move_redirect_spec.rb index 52be8f2f3..b97b38310 100644 --- a/spec/models/generic_event/move_redirect_spec.rb +++ b/spec/models/generic_event/move_redirect_spec.rb @@ -123,5 +123,43 @@ expect { generic_event.trigger }.to change { generic_event.eventable.move_type }.from('prison_transfer').to('court_appearance') end end + + context 'when it becomes a cross-deck move' do + subject(:generic_event) { build(:event_move_redirect, details:, eventable:) } + + let(:departing_supplier) { create(:supplier) } + let(:receiving_supplier) { create(:supplier) } + let(:from_location) { create(:location, :court, suppliers: [departing_supplier]) } + let(:old_to_location) { create(:location, :court, suppliers: [departing_supplier]) } + let(:new_to_location) { create(:location, :court, suppliers: [receiving_supplier]) } + let(:eventable) { build(:move, move_type: 'court_appearance', from_location:, to_location: old_to_location) } + let(:details) { { reason: 'other', to_location_id: new_to_location.id } } + + before { allow(Notifier).to receive(:prepare_notifications) } + + it 'sends a cross_supplier_move_add notification to the receiving supplier' do + generic_event.trigger + expect(Notifier).to have_received(:prepare_notifications).with(topic: eventable, action_name: 'cross_supplier_add') + end + end + + context 'when it ceases to be a cross-deck move' do + subject(:generic_event) { build(:event_move_redirect, details:, eventable:) } + + let(:departing_supplier) { create(:supplier) } + let(:receiving_supplier) { create(:supplier) } + let(:from_location) { create(:location, :court, suppliers: [departing_supplier]) } + let(:old_to_location) { create(:location, :court, suppliers: [receiving_supplier]) } + let(:new_to_location) { create(:location, :court, suppliers: [departing_supplier]) } + let(:eventable) { build(:move, move_type: 'court_appearance', from_location:, to_location: old_to_location) } + let(:details) { { reason: 'other', to_location_id: new_to_location.id } } + + before { allow(Notifier).to receive(:prepare_notifications) } + + it 'sends a cross_supplier_move_remove notification to the receiving supplier' do + generic_event.trigger + expect(Notifier).to have_received(:prepare_notifications).with(topic: eventable, action_name: 'cross_supplier_remove') + end + end end end diff --git a/spec/models/move_spec.rb b/spec/models/move_spec.rb index 5181bc5b3..3040e00a8 100644 --- a/spec/models/move_spec.rb +++ b/spec/models/move_spec.rb @@ -1049,4 +1049,20 @@ expect(journey3.date).to eq(Time.zone.today + 7) end end + + describe '#cross_deck?' do + let(:move) { create(:move) } + + it { expect(move.cross_deck?).to be false } + + context 'when the origin and destination suppliers are different' do + let!(:supplier1) { create(:supplier) } + let!(:supplier2) { create(:supplier) } + let!(:from_location) { create(:location, suppliers: [supplier1]) } + let!(:to_location) { create(:location, :court, suppliers: [supplier2]) } + let(:move) { create(:move, from_location:, to_location:) } + + it { expect(move.cross_deck?).to be true } + end + end end diff --git a/spec/requests/api/allocations_controller_update_spec.rb b/spec/requests/api/allocations_controller_update_spec.rb index 0f80750db..32758fe75 100644 --- a/spec/requests/api/allocations_controller_update_spec.rb +++ b/spec/requests/api/allocations_controller_update_spec.rb @@ -61,7 +61,7 @@ it 'creates notifications for each move' do perform_enqueued_jobs(only: [PrepareMoveNotificationsJob, NotifyWebhookJob]) do expect { patch_allocations } - .to change { subscription.notifications.where(event_type: 'update_move').count } + .to change { subscription.notifications.count } .by(2) end end diff --git a/spec/requests/api/moves_controller_create_v2_spec.rb b/spec/requests/api/moves_controller_create_v2_spec.rb index 18873e5a4..edbd7ba07 100644 --- a/spec/requests/api/moves_controller_create_v2_spec.rb +++ b/spec/requests/api/moves_controller_create_v2_spec.rb @@ -730,6 +730,59 @@ before { do_post } end end + + context 'when it is a cross-deck move' do + before do + create(:notification_type, :webhook) + allow(Faraday).to receive(:new).and_return(faraday_client) + end + + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier: another_supplier) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + + let(:faraday_client) do + class_double( + Faraday, + headers: {}, + post: instance_double(Faraday::Response, success?: true, status: 202), + ) + end + + let(:expected_notification_attributes) do + { + 'topic_id' => move.id, + 'topic_type' => 'Move', + 'delivery_attempts' => 1, + 'delivery_attempted_at' => be_within(5.seconds).of(Time.zone.now), + 'delivered_at' => be_within(5.seconds).of(Time.zone.now), + 'discarded_at' => nil, + 'response_id' => nil, + 'notification_type_id' => 'webhook', + } + end + + it 'notifies the initial supplier' do + perform_enqueued_jobs(only: [PrepareMoveNotificationsJob, NotifyWebhookJob]) do + do_post + end + + expect(subscription.notifications.last.attributes).to include_json( + expected_notification_attributes.merge({ 'event_type' => 'create_move' }), + ) + end + + it 'notifies the receiving supplier' do + perform_enqueued_jobs(only: [PrepareMoveNotificationsJob, NotifyWebhookJob]) do + do_post + end + + expect(subscription2.notifications.last.attributes).to include_json( + expected_notification_attributes.merge({ 'event_type' => 'cross_supplier_move_add' }), + ) + end + end end def do_post diff --git a/spec/requests/api/moves_controller_update_v2_spec.rb b/spec/requests/api/moves_controller_update_v2_spec.rb index 2bab368ff..cfbc73d99 100644 --- a/spec/requests/api/moves_controller_update_v2_spec.rb +++ b/spec/requests/api/moves_controller_update_v2_spec.rb @@ -340,6 +340,62 @@ end end + context 'when it is a cross-deck move' do + let!(:receiving_supplier) { create(:supplier) } + let!(:subscription) { create(:subscription, :no_email_address, supplier:) } + let!(:subscription2) { create(:subscription, :no_email_address, supplier: receiving_supplier) } + let(:to_location) { create :location, :court, suppliers: [receiving_supplier] } + let!(:move) { create :move, :proposed, :prison_recall, from_location:, to_location:, profile:, supplier: } + + let(:faraday_client) do + class_double( + Faraday, + headers: {}, + post: instance_double(Faraday::Response, success?: true, status: 202), + ) + end + + let(:expected_notification_attributes) do + { + 'topic_id' => move.id, + 'topic_type' => 'Move', + 'delivery_attempts' => 1, + 'delivery_attempted_at' => be_within(5.seconds).of(Time.zone.now), + 'delivered_at' => be_within(5.seconds).of(Time.zone.now), + 'discarded_at' => nil, + 'response_id' => nil, + 'notification_type_id' => 'webhook', + } + end + + before do + create(:notification_type, :webhook) + allow(Faraday).to receive(:new).and_return(faraday_client) + create(:notification, topic: move, event_type: 'create_move') + create(:notification, topic: move, event_type: 'cross_supplier_move_add') + end + + it 'notifies the initial supplier' do + perform_enqueued_jobs(only: [PrepareMoveNotificationsJob, NotifyWebhookJob]) do + do_patch + end + + expect(subscription.notifications.last.attributes).to include_json( + expected_notification_attributes.merge({ 'event_type' => 'update_move_status' }), + ) + end + + it 'notifies the receiving supplier' do + perform_enqueued_jobs(only: [PrepareMoveNotificationsJob, NotifyWebhookJob]) do + do_patch + end + + expect(subscription2.notifications.last.attributes).to include_json( + expected_notification_attributes.merge({ 'event_type' => 'cross_supplier_move_update_status' }), + ) + end + end + context 'when updating an existing requested move without a change of move_status' do let!(:move) { create :move, :requested, move_type: 'prison_recall', from_location:, supplier: }