[ENG-8410] Refactor Notifications Phase 1 by Johnetordoff · Pull Request #11176 · CenterForOpenScience/osf.io

Johnetordoff · 2025-06-09T13:42:19Z

Purpose

This is the first phase of the notifications refactor project, that will update and improve the notifications model. Theis first phase is to remove all the code assassinated with notifications no longer being sent.

Changes

removes OSF Groups
removes notifications for OSF Meetings
removes notifications for OSF Comments
removes OSF Quickfiles code
add mailhog integration to improve email tests

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

Johnetordoff · 2025-07-10T19:34:57Z

    view_category = 'files'
    view_name = 'file-detail'

-    def get_serializer_class(self):


removed for being extraneous

brianjgeiger

Drive by: a good ratio of deleted to added lines. I have a couple of questions around logs and migrations. Expect a more thorough review from @cslzchen.

Johnetordoff · 2025-08-12T12:37:11Z

-                boa_support_email=boa_settings.BOA_SUPPORT_EMAIL,
-                osf_support_email=osf_settings.OSF_SUPPORT_EMAIL,
-            )
+            self.mock_send_grid.assert_called()


A bit lazy just to assert this but asserting every single thing in a template is unmaintainable.

hmm ... why is it unmaintainable? Did the test fail and we can't figure out how to fix it? The reason why we assert the details for send_mail is because we want to make sure we send the email with correct information.

removes notifications for OSF Meetings removes notifications for OSF Comments removes OSF Quickfiles code add mailhog integration to improve email tests

brianjgeiger

One last historical migration change to revert.

brianjgeiger · 2025-08-18T12:39:55Z

        migrations.RunSQL(
            [
                """
-                CREATE UNIQUE INDEX one_quickfiles_per_user ON public.osf_abstractnode USING btree (creator_id, type, is_deleted) WHERE (((type)::text = 'osf.quickfilesnode'::text) AND (is_deleted = false));


Shouldn't this be reverted as well?

into refactor-notifications

cslzchen

The review is for future references in phase 2, some of the comments may have been outdated when I submit the review. In addition, some issues are too repetitive to comment on every occurrence.

My major concern is the way how we test send_mail. With your change, it no longer tests the correctness of the mail (i.e. the arguments that are passed in to build the template). Some even only test that send_mail is called while some test limited info such as the subject. This is something that needs to be fixed in phase 2.
One suggestion (for future work like this) is to avoid one big PR. Removing group, removing notifications, removing quickfile and the mailhog work seem to be independent of each other (maybe I am wrong). I think 4 small phases/releases would have been better.

cslzchen · 2025-08-12T16:04:32Z

-  - Needed for quickfiles feature:
-      ```bash
-      docker compose up -d ember_osf_web
-      ```
+  - Needed for ember app:
+    - `docker-compose up -d ember_osf_web`


keep the format consistent (i.e. use ``` instead of `)

docker compose

cslzchen · 2025-08-12T16:14:07Z

-                boa_support_email=boa_settings.BOA_SUPPORT_EMAIL,
-                osf_support_email=osf_settings.OSF_SUPPORT_EMAIL,
-            )
+            self.mock_send_grid.assert_called()


hmm ... why is it unmaintainable? Did the test fail and we can't figure out how to fix it? The reason why we assert the details for send_mail is because we want to make sure we send the email with correct information.

cslzchen · 2025-08-12T19:10:07Z


    # overrides GenericAPIView
-    def get_queryset(self):
+    def get_queryset(self, *args, **kwargs):


GenericAPIView has get_queryset(self). Why do we have to change the signature?

cslzchen · 2025-08-12T19:18:48Z

-        with mock.patch('framework.auth.views.mails.send_mail') as mock_send_mail:
-            context_data = self.make_mailgun_payload(crossref_response=error_xml)
-            app.post(url, context_data)
-        assert mock_send_mail.called
+        context_data = self.make_mailgun_payload(crossref_response=error_xml)
+        app.post(url, context_data)
+        assert mock_send_grid.called


For myself, same question on why we can't assert sent_mail but switched to test send_grid (many occurrences), maybe related to one of the changes?

add mailhog integration to improve email tests

cslzchen · 2025-08-12T19:23:47Z

        # Python 3.6 does not support mock.call_args.args/kwargs
        # Instead, mock.call_args[0] is positional args, mock.call_args[1] is kwargs
        # (note, this is compatible with later versions)


Unrelated to your changes but since we are on python 3.12, should we remote this comment and optionally update the code too?

cslzchen · 2025-08-14T17:44:41Z

-        forgot_password = 'forgotpassword' if settings.DOMAIN.endswith('/') else '/forgotpassword'
-        mock_send_mail.assert_called_with(
-            to_addr=user.username,
-            mail=mails.INSTITUTION_DEACTIVATION,
-            user=user,
-            forgot_password_link=f'{settings.DOMAIN}{forgot_password}',
-            osf_support_email=settings.OSF_SUPPORT_EMAIL
-        )
+        mock_send_grid.assert_called()


same here, we no longer test the correctness of the email content, which beats the goal of the test.

cslzchen · 2025-08-14T17:50:52Z

-    @mock.patch('website.project.views.contributor.mails.send_mail')
-    def test_add_contributors_sends_contributor_added_signal(self, mock_send_mail, node, auth):
+    def test_add_contributors_sends_contributor_added_signal(self, node, auth):


If I understand this correctly, we no longer need to mock send mail because we have mail hog running to actually send the mail during testing.

That's not accurate mailhog isn't used here.

cslzchen · 2025-08-14T17:54:58Z

-    def test_set_permissions_raises_error_if_only_admins_permissions_are_reduced(self, node):
-        # creator is the only admin
-        with pytest.raises(NodeStateError) as excinfo:
-            node.set_permissions(node.creator, permissions=WRITE)
-        assert excinfo.value.args[0] == 'Must have at least one registered admin contributor'


Should this part be kept?

cslzchen · 2025-08-14T18:01:04Z

                users, auth=auth, save=True,
            )

-    def test_manage_contributors_no_registered_admins(self, node, auth):


Not sure if possible, can this be changed to test without using group?

cslzchen · 2025-08-14T18:05:18Z

+        assert len(mock_send_grid.call_args_list) == 2
+        admin_message, contrib_message = mock_send_grid.call_args_list

-        assert admin_message == call(


Same concern in this file on email correctness test

Yeah I actually was basing my feeling on this off @jwalz said about "Change detection tests" basically the tests shouldn't check for template variables because wording changes could break tests, so I wasn't just being lazy something here, by not checking.

Johnetordoff changed the title ~~[WIP] Refactor Notifications~~ [WIP] Refactor Notifications Phase 1 Jun 9, 2025

Johnetordoff marked this pull request as ready for review June 16, 2025 14:30

Johnetordoff changed the base branch from develop to feature/pbs-25-10 June 16, 2025 14:35

Johnetordoff changed the base branch from feature/pbs-25-10 to develop July 3, 2025 13:49

Johnetordoff force-pushed the refactor-notifications branch 2 times, most recently from ccfc523 to afc3815 Compare July 10, 2025 19:32

Johnetordoff commented Jul 10, 2025

View reviewed changes

Johnetordoff force-pushed the refactor-notifications branch from afc3815 to b6bbeed Compare July 11, 2025 13:02

Johnetordoff changed the title ~~[WIP] Refactor Notifications Phase 1~~ Refactor Notifications Phase 1 Jul 11, 2025

cslzchen self-requested a review July 22, 2025 16:22

Johnetordoff changed the title ~~Refactor Notifications Phase 1~~ [ENG-8410] Refactor Notifications Phase 1 Jul 22, 2025

brianjgeiger reviewed Jul 23, 2025

View reviewed changes

Comment thread osf/models/nodelog.py

Comment thread osf/migrations/0001_initial.py

Comment thread website/static/js/anonymousLogActionsList.json

Comment thread website/static/js/logActionsList.json

Johnetordoff force-pushed the refactor-notifications branch from 15289a0 to 4c50c6c Compare August 1, 2025 19:03

Johnetordoff commented Aug 12, 2025

View reviewed changes

Johnetordoff changed the base branch from develop to feature/pbs-25-13 August 12, 2025 13:18

Johnetordoff changed the base branch from feature/pbs-25-13 to develop August 12, 2025 13:44

Johnetordoff force-pushed the refactor-notifications branch 6 times, most recently from 911befe to e2a16ac Compare August 13, 2025 11:50

removes OSF Groups

a2dd86a

removes notifications for OSF Meetings removes notifications for OSF Comments removes OSF Quickfiles code add mailhog integration to improve email tests

Johnetordoff force-pushed the refactor-notifications branch from e2a16ac to a2dd86a Compare August 13, 2025 13:05

Merge branch 'develop' into refactor-notifications

46e8726

brianjgeiger requested changes Aug 18, 2025

View reviewed changes

reintroduce quickfiles db constraint

944692a

brianjgeiger approved these changes Aug 18, 2025

View reviewed changes

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

57d7a1a

into refactor-notifications

cslzchen reviewed Aug 29, 2025

View reviewed changes

cslzchen merged commit ee1f0e1 into develop Aug 29, 2025
20 of 21 checks passed

Ostap-Zherebetskyi mentioned this pull request Oct 22, 2025

[ENG-9619] Tests for api3 branch turned off #11385

Merged

cslzchen deleted the refactor-notifications branch December 17, 2025 20:19

Conversation

Johnetordoff commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianjgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Johnetordoff commented Jun 9, 2025 •

edited

Loading