THREESCALE-10758: Filter email notifications by service

[jlledom]

What this PR does / why we need it:

Clients complain their inboxes are being spammed with non-relevant emails about notifications they don't care about because their users don't have permissions to manage the services the notifications are about.

We already implement a system to filter such notifications, this is how it works:

1. An event happens
2. It's listened by PublishNotificationEventSubscriber:

porta/app/lib/event_store/repository.rb

Lines 117 to 167 in a897958

	# applications/cinstances
	subscribe_for_notification(:application_created, Applications::ApplicationCreatedEvent)
	subscribe_for_notification(:cinstance_cancellation, Cinstances::CinstanceCancellationEvent)
	subscribe_for_notification(:cinstance_expired_trial, Cinstances::CinstanceExpiredTrialEvent)
	subscribe_for_notification(:cinstance_plan_changed, Cinstances::CinstancePlanChangedEvent)
	subscribe_for_notification(:application_plan_change_requested, Applications::ApplicationPlanChangeRequestedEvent)
	# accounts
	subscribe_for_notification(:account_created, Accounts::AccountCreatedEvent)
	subscribe_for_notification(:account_deleted, Accounts::AccountDeletedEvent)
	subscribe_for_notification(:account_plan_change_requested, Accounts::AccountPlanChangeRequestedEvent)
	subscribe_for_notification(:account_state_changed, Accounts::AccountStateChangedEvent)
	subscribe_for_notification(:expired_credit_card_provider, Accounts::ExpiredCreditCardProviderEvent)
	# alerts
	subscribe_for_notification(:limit_violation_reached_provider, Alerts::LimitViolationReachedProviderEvent)
	subscribe_for_notification(:limit_alert_reached_provider, Alerts::LimitAlertReachedProviderEvent)
	# invoices
	subscribe_for_notification(:unsuccessfully_charged_invoice_provider, Invoices::UnsuccessfullyChargedInvoiceProviderEvent)
	subscribe_for_notification(:unsuccessfully_charged_invoice_final_provider, Invoices::UnsuccessfullyChargedInvoiceFinalProviderEvent)
	subscribe_for_notification(:invoices_to_review, Invoices::InvoicesToReviewEvent)
	# service contracts
	subscribe_for_notification(:service_contract_cancellation, ServiceContracts::ServiceContractCancellationEvent)
	subscribe_for_notification(:service_contract_created, ServiceContracts::ServiceContractCreatedEvent)
	subscribe_for_notification(:service_contract_plan_changed, ServiceContracts::ServiceContractPlanChangedEvent)
	# plans
	subscribe_for_notification(:plan_downgraded, Plans::PlanDowngradedEvent)
	# messages
	subscribe_for_notification(:message_received, Messages::MessageReceivedEvent)
	# posts
	subscribe_for_notification(:post_created, Posts::PostCreatedEvent)
	# reports
	subscribe_for_notification(:csv_data_export, Reports::CsvDataExportEvent)
	# services
	subscribe_for_notification(:service_deleted, Services::ServiceDeletedEvent)
	subscribe_for_notification(:service_plan_change_requested, Services::ServicePlanChangeRequestedEvent)
	subscribe_event(PublishZyncEventSubscriber.new,
	Applications::ApplicationCreatedEvent,
	Applications::ApplicationUpdatedEvent,
	Applications::ApplicationDeletedEvent,
	Applications::ApplicationEnabledChangedEvent,
	OIDC::ProxyChangedEvent,
	OIDC::ServiceChangedEvent,
	Domains::ProviderDomainsChangedEvent,
	Domains::ProxyDomainsChangedEvent
	)
	subscribe_event(ServiceTokenEventSubscriber.new, ServiceTokenDeletedEvent)
	subscribe_event(ServiceDeletionSubscriber.new, Services::ServiceScheduledForDeletionEvent)
	subscribe_event(ServiceDeletedSubscriber.new, Services::ServiceDeletedEvent)
	subscribe_event(ApplicationDeletedSubscriber.new, Applications::ApplicationDeletedEvent)
	subscribe_event(ProxyConfigEventSubscriber.new, ProxyConfigs::AffectingObjectChangedEvent)
	subscribe_event(ZyncSubscriber.new, ZyncEvent)

porta/app/lib/event_store/repository.rb

Lines 186 to 188 in a897958

	def subscribe_for_notification(name, event_class)
	client.subscribe(PublishNotificationEventSubscriber.new(name), [event_class])
	end

3. A NotificationEvent is created:

porta/app/subscribers/publish_notification_event_subscriber.rb

Lines 12 to 18 in 25fa9bc

	def call(event)
	notification_event = NotificationEvent.create(system_name, event)
	publish_event(notification_event, event.event_id) if notification_enabled?(event)
	notification_event
	end

4. AFAIK nobody subscribes to it, but after being added, a ProcessNotificationEventWorker job is enqueued:
   

   porta/app/events/notification_event.rb

   Lines 20 to 22 in 46eb05a

   	def after_commit
   	ProcessNotificationEventWorker.enqueue(self)
   	end

5. When performed, one UserNotificationWorker job is enqueued for each active user in the provider
   

   porta/app/workers/process_notification_event_worker.rb

   Lines 37 to 55 in 7efaabd

   	def create_notifications(event)
   	provider = Provider.find(event.provider_id)
   	if provider.suspended_or_scheduled_for_deletion?
   	Rails.logger.info "[Notification] skipping notifications for event #{event.event_id} of #{provider.state} account #{event.provider_id}"
   	return
   	end
   	parallelize do
   	provider.users.active.but_impersonation_admin.find_each do |user|
   	UserNotificationWorker.perform_async(user.id, event.event_id, event.system_name)
   	end
   	end
   	provider
   	rescue ActiveRecord::RecordNotFound => e
   	::Rails.logger.error e.message
   	false
   	end

6. For every user, should_deliver? is called:
   

   porta/app/workers/process_notification_event_worker.rb

   Line 26 in 7efaabd

   notification.deliver! if notification.should_deliver?

7. Which calls permitted?:
   

   porta/app/models/notification.rb

   Lines 38 to 40 in 9833dc4

   	def should_deliver?
   	enabled? && subscribed? && permitted?
   	end

8. Which checks the permissions:
   

   porta/app/models/notification.rb

   Lines 72 to 74 in 9833dc4

   	def permitted?
   	Ability.new(user).can?(:show, parent_event)
   	end

9. The event can be BillingRelatedEvent, AccountRelatedEvent or ServiceRelatedEvent.

   • Billing events are sent to everybody having the :finance permission
   • Service events must include the service field in their data, and can be notified if the user has access to the service.
   • Account events can include the service field in their data or not, and can be notified if the user has access to service or the event doesn't contain a service:

porta/config/abilities/provider_any.rb

Lines 30 to 41 in af8ef71

	can [:show], BillingRelatedEvent if user.has_permission?(:finance)
	if user.has_permission?(:partners)
	can [:show], AccountRelatedEvent do |event|
	service_id = event.try(:service)&.id || event.try(:service_id)
	!service_id || user.has_access_to_service?(service_id)
	end
	can [:show], ServiceRelatedEvent do |event|
	user.has_access_to_service?(event.try(:service) || event.service_id)
	end
	end

This works fine for those events that can be easily associated to a service, for instance creating an application; but some other events are not, for instance sending a message.

In particular, the client complains about:

1. An account is created with a service subscription of any other product, for example ProductB.
2. An account is created without any service subscription.
3. An account is deleted regardless of any service subscription.
4. A developer user sends a message regardless the service subscriptions the account has.

• 1 and 2 trigger Accounts::AccountCreatedEvent
• 3 triggers Accounts::AccountDeletedEvent
• 4 triggers Messages::MessageReceivedEvent

In order to fix AccountCreatedEvent and MessageReceivedEvent I made some changes in the ability so it can handle multiple services rather than only one, if any of the services in the event are permitted, then the notification is sent: 0110eb1

Then I added the relevant services to the events: c066485 and 1b45331

Also made all application events related to Service rather than Account, I think it's more correct: 0eaca5e

About AccountDeletedEvent we have a problem here. The event is created from an observer:

porta/app/observers/account_observer.rb

Lines 34 to 37 in 25fa9bc

	def publish_account_deleted_event!(account)
	event = Accounts::AccountDeletedEvent.create(account)
	Rails.application.config.event_store.publish_event(event)
	end

But the received account is already deleted and not persisted, so its bought_service_contracts association is empty and we can't get the service subscriptions. Maybe the corresponding ServiceContract records still exist in DB at this point and can be retrieved, but I think that's error prone so I didn't even try.

For that reason I couldn't implement the filtering for this kind of event.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-10758
https://issues.redhat.com/browse/THREESCALE-8720

[jlledom]

Please read the description and then tell me your opinion about:

1. Message inbox screen: all messages will appear there, also those from users not subscribed to the services the current user has permissions over: /p/admin/messages

• Should we hide not relevant messages from this screen as well?

2. Considering AccountDeletedEvent can't be fixed and there are not so many AccountCreatedEvent, is this PR even worth it?

[mayorova]

@jlledom Have you seen any AccountRelatedEvent that was already including service_ids?

On the other hand, you are now adding services property in AccountRelatedEvent, while this seems to be the only place where it is used. And if we only need the IDs, maybe this is what we can actually pass? service_ids: some_scope.pluck(:id), instead of services: some_scope.to_a

[jlledom]

@jlledom Have you seen any AccountRelatedEvent that was already including service_ids?

No

On the other hand, you are now adding services property in AccountRelatedEvent, while this seems to be the only place where it is used. And if we only need the IDs, maybe this is what we can actually pass? service_ids: some_scope.pluck(:id), instead of services: some_scope.to_a

Yeah, I'll do that.

[akostadinov]

if user has not subscribed to any services, then we send notification? Shouldn't we not send in fact?

[jlledom]

We could do it that way if you have a strong opinion and think it's the correct thing to do.

Anyway, old code already notified everyone for account related events that aren't connected to any service:

 !service_id || user.has_access_to_service?(service_id)

If no service, return true; otherwise, return whether the user has access to that service.

In the new code I'm restricting that to only notify admins about events not connected to services.

I think that's fine because an admin could be interested in e.g. new buyers signups even when there is no default service for them. Also, admins and all users can disable any from their notification preferences.

[akostadinov]

wrt the observer, can we move the event to before_destroy/before_delete or something? I mean in case service contracts are still there.

[mayorova]

Please read the description and then tell me your opinion about:

1. Message inbox screen: all messages will appear there, also those from users not subscribed to the services the current user has permissions over: /p/admin/messages

• Should we hide not relevant messages from this screen as well?

I think Messages inbox should only be shown to the members that have "partners" permission, and in this case I believe they can see the list of all accounts, regardless of whether they have subscribed to any of the services that the member has access to (through some other permission, such as "plans"). And in this case all messages should be seen to.

2. Considering AccountDeletedEvent can't be fixed and there are not so many AccountCreatedEvent, is this PR even worth it?

Cannot we somehow preload the list of "bought contracts" in before_destroy and then include the IDs of these contracts in the AccountDeletedEvent ?

I think we are doing something similar here:

porta/app/lib/backend/model_extensions/cinstance.rb

Line 8 in 36eeb45

before_destroy :preload_used_associations

But I might be wrong...

[jlledom]

I rebased to solve the conflicts

[jlledom]

• Should we hide not relevant messages from this screen as well?

I think Messages inbox should only be shown to the members that have "partners" permission, and in this case I believe they can see the list of all accounts, regardless of whether they have subscribed to any of the services that the member has access to (through some other permission, such as "plans"). And in this case all messages should be seen to.

OKI

2. Considering AccountDeletedEvent can't be fixed and there are not so many AccountCreatedEvent, is this PR even worth it?

Cannot we somehow preload the list of "bought contracts" in before_destroy and then include the IDs of these contracts in the AccountDeletedEvent ?

I'll take a look

[jlledom]

@mayorova I followed your sugestion to use only service ids: b3ff82d

About AccountDeletedEvent, I couldn't add the service ids, this is what I tried:

• Publish the event from the observer, before_destroy (@akostadinov suggestion)
• Cache the association from the observer, before_destroy (@mayorova suggestion)
• Cache the association from the model callback, also before_destroy
• Cache the association from the api controller: Admin::Api::AccountsController#destroy
  • Inside and outside the transaction
• For caching, I tried calling record.bought_service_contracts and ActiveRecord::Associations::Preloader.new(records: [record], associations: [:bought_service_contracts]).call

Nothing worked, because it doesn't matter if the association was cached or not, dependent: destroy always clear the cache if it existed. So I'm really blocked on this, I can think only on dramatic solutions like use thread variables, which I don't like and I'm not sure they would work either.

Any other ideas?