refactor notification #184

alexskr · 2024-02-02T19:11:46Z

notification refactor follow up to #148

Rename variables and methods to make a clear distinction between
OntoPortal site admins, ontology admins and support.
Add new feature to optionally disable notifications send to OntoPortal
admins for each event associated with ontology creation, new user, etc
LinkedData.setting.support_contact_email is a new configuration option
for setting end user support contact email.
BioPortal requires different emails for contacting BioPortal site
admins and end user support

In order to clarify the distinction between the OntoPortal site admins and
ontology owners/admins the following settings and methods are renamed:

LinkedData.setting.admin_emails is renamed to ontoportal_admin_emails
LinkedData::Utils::Notifier.admin_mails -> ontology_admin_emails
LinkedData::Utils::support_mails -> ontoportal_admin_emails
LinkedData::Utils::notify_support_grouped -> notify_ontoportal_admins_grouped

Refactor: notifications

- Rename variables and methods to make a clear distinction between OntoPortal site admins, ontology admins and support. - Add new feature to optionally disable notifications send to OntoPortal admins for each event associated with ontology creation, new user, etc - LinkedData.setting.support_contact_email is a new configuration option for setting end user support contact email. BioPortal requires different emails for contacting BioPortal site admins and end user support In order to clarify the distinction between the OntoPortal site admins and ontology owners/admins the following settings and methods are renamed: LinkedData.setting.admin_emails is renamed to ontoportal_admin_emails LinkedData::Utils::Notifier.admin_mails -> ontology_admin_emails LinkedData::Utils::support_mails -> ontoportal_admin_emails LinkedData::Utils::notify_support_grouped -> notify_ontoportal_admins_grouped

alexskr · 2024-02-02T19:15:01Z

@syphax-bouazzouni please review the follow up changes and let me know if there is anything that should be tweaked in order to fit your code base

codecov · 2024-02-02T19:24:54Z

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (4b6b7f4) 80.76% compared to head (d0b6ad8) 80.66%.
Report is 4 commits behind head on develop.

❗ Current head d0b6ad8 differs from pull request most recent head 87afcc4. Consider uploading reports for the commit 87afcc4 to get more accurate results

Files	Patch %	Lines
lib/ontologies_linked_data/utils/notifications.rb	61.97%	27 Missing ⚠️
lib/ontologies_linked_data/utils/notifier.rb	94.82%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #184      +/-   ##
===========================================
- Coverage    80.76%   80.66%   -0.10%     
===========================================
  Files           63       64       +1     
  Lines         4917     5337     +420     
===========================================
+ Hits          3971     4305     +334     
- Misses         946     1032      +86

Flag	Coverage Δ
unittests	`80.66% <76.92%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

syphax-bouazzouni

Hey 👋 , thanks on working on this. Fine by me.
Added some optional minor remarks that you can

syphax-bouazzouni · 2024-02-02T21:26:02Z

lib/ontologies_linked_data/config/config.rb

-      puts "Error: 'redis_host' is not a valid conf parameter."
-      puts '        Redis databases were split into multiple hosts (09/22/13).'
-      raise Exception, 'redis_host is not a valid conf parameter.'
+    unless @settings.admin_emails.nil?


why this part?

admin_emails setting was renamed to ontoportal_admin_emails so thats a way to notify folks who have been relying on that setting to update. I would be more than happy to remove if you think thats an overkill.

Yes, it is overkill, this sort of code, is always the one remaining 10 years after, with no one knowing why they are here and too screed to remove it.

removed in 87afcc4

syphax-bouazzouni · 2024-02-02T21:30:12Z

lib/ontologies_linked_data/config/config.rb

@@ -69,7 +65,8 @@ def config(&block)

    # Email settings
    @settings.enable_notifications          ||= false
-    @settings.email_sender                  ||= '[email protected]' # Default sender for emails
+    # Default sender From email address
+    @settings.email_sender                  ||= '[email protected]'
    @settings.email_override                ||= '[email protected]' # By default, all email gets sent here. Disable with email_override_disable.


email_override and email_disable_override are never really used, in test or development environments config the email_sender on it self is email_override

we use email_override in staging environment to troubleshoot send notifications to a dedicated email address used for debugging only. Sending all overwritten/debugging emails to the admin account creates too much noise so having this options works well for us.

yes but I mean in your staging env, you can set admin_email as the one that you override with ? anyways no big deal.

syphax-bouazzouni · 2024-02-02T21:46:35Z

lib/ontologies_linked_data/utils/notifier.rb

+
+      def self.ontoportal_admin_emails
+
+        if !LinkedData.settings.ontoportal_admin_emails.nil? &&


the condition can be replaced with only, which handle nil case also

LinkedData.settings.ontoportal_admin_emails.is_a?(Array)

or all the function by Array(LinkedData.settings.ontoportal_admin_emails)

addressed in d0b6ad8

syphax-bouazzouni · 2024-02-02T22:14:33Z

lib/ontologies_linked_data/utils/notifier.rb

+        end
+        recipients
+      end
+


you can add (and use) the following helper/facets/alias here

def self.support_mail Array(LinkedData.settings.support_contact_email) end

def self.notification_enabled? LinkedData.settings.enable_administrative_notifications end

this will make it easier to read

thanks for the suggestion

addressed in d0b6ad8

syphax-bouazzouni · 2024-02-02T22:15:18Z

lib/ontologies_linked_data/utils/notifier.rb

+      end
+
+      def self.notify_ontoportal_admins(subject, body)
+        notify_mails_grouped subject, body, ontoportal_admin_emails if LinkedData.settings.enable_administrative_notifications


no need of the if LinkedData.settings.enable_administrative_notifications as notify already test that

LinkedData.settings.enable_administrative_notifications is not the same as LinkedData.settings.enable_notifications

enable_notifications enables all notifications for all users
enable_administrative_notifications enables notifications send to the admin team (or support team in your case) for things like new user creation event. BioPortal team doesn't have the need to see those notifications.

Ah ok sorry, miss reading the thing,
Good for me

syphax-bouazzouni · 2024-02-02T22:15:25Z

lib/ontologies_linked_data/utils/notifier.rb

+      end
+
+      def self.notify_ontoportal_admins_grouped(subject, body)
+        notify_mails_grouped subject, body, ontoportal_admin_emails if LinkedData.settings.enable_administrative_notifications


no need of the if LinkedData.settings.enable_administrative_notifications as notify already test that

syphax-bouazzouni and others added 20 commits April 4, 2022 14:30

auto lint refactor

574da21

remove the Bioportal harder coded constant in notification subjects

b7a4856

send the ontology pull failure notification also to the portal admins

d837cea

adding a notification for new users creation

339a268

add a notification for new ontology creation

402e361

removing duplicate if found in the notify recipients

c66eaf0

configurize the hardcoded Bioportal name

c18270d

fix empty admin_emails value test

fcd5bbd

extract the notifier (sender) from the notifications (the content)

a31727c

add the ontology administrators to the processing notification targets

90798af

replace ui_host with ui_name

31d146e

use ui_name in the tests

ee73818

Merge branch 'master' into pl/refacotor-notification

90bea9d

fix mailer configuration when using login auth option

10d3248

refactor notifications of notes and reset password

f5b9478

use id_url_prefix in generating note id instead of of hardcoded value

e6f1dac

Merge pull request #148 from ontoportal-lirmm/pl/refacotor-notification

dca560d

Refactor: notifications

add enable_administrative_notifications condition

0913930

add unit test for disabling administrative notifications

12daf06

syphax-bouazzouni approved these changes Feb 2, 2024

View reviewed changes

alexskr added 2 commits February 2, 2024 23:19

minor optimizations

d0b6ad8

Remove backward compatibility check.

87afcc4

alexskr merged commit 247fb4d into develop Feb 5, 2024
2 of 4 checks passed

This was referenced Feb 5, 2024

Improve notifications in BioPortal ncbo/bioportal-project#291

Open

Notifications: ontology processing notification sends out misleading email about failed archived submission #185

Closed

Merge Develop to Master, release v5.28.0 #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor notification #184

refactor notification #184

alexskr commented Feb 2, 2024

alexskr commented Feb 2, 2024

codecov bot commented Feb 2, 2024 •

edited

Loading

syphax-bouazzouni left a comment

syphax-bouazzouni Feb 2, 2024

alexskr Feb 3, 2024

syphax-bouazzouni Feb 5, 2024

alexskr Feb 5, 2024

syphax-bouazzouni Feb 2, 2024

alexskr Feb 3, 2024

syphax-bouazzouni Feb 5, 2024

syphax-bouazzouni Feb 2, 2024

syphax-bouazzouni Feb 2, 2024

alexskr Feb 3, 2024

syphax-bouazzouni Feb 2, 2024

alexskr Feb 3, 2024

syphax-bouazzouni Feb 2, 2024

alexskr Feb 3, 2024

syphax-bouazzouni Feb 5, 2024

syphax-bouazzouni Feb 2, 2024


		def self.ontoportal_admin_emails

		if !LinkedData.settings.ontoportal_admin_emails.nil? &&

refactor notification #184

refactor notification #184

Conversation

alexskr commented Feb 2, 2024

alexskr commented Feb 2, 2024

codecov bot commented Feb 2, 2024 • edited Loading

Codecov Report

syphax-bouazzouni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2024 •

edited

Loading