From 8f79cfd43b0d410a06124d9582a2f55b84f5ef4e Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Tue, 14 Jan 2025 20:30:45 +0200 Subject: [PATCH 1/5] fixed double mail sending --- .../requests/views/test_node_request_list.py | 4 ++-- osf/utils/machines.py | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/api_tests/requests/views/test_node_request_list.py b/api_tests/requests/views/test_node_request_list.py index f94d3b4acf2..b49e2840816 100644 --- a/api_tests/requests/views/test_node_request_list.py +++ b/api_tests/requests/views/test_node_request_list.py @@ -80,7 +80,7 @@ def test_requests_disabled_list(self, app, url, create_payload, project, admin): res = app.get(url, create_payload, auth=admin.auth, expect_errors=True) assert res.status_code == 403 - @mock.patch('website.mails.mails.send_mail') + @mock.patch('website.mails.send_mail') def test_email_sent_to_all_admins_on_submit(self, mock_mail, app, project, noncontrib, url, create_payload, second_admin): project.is_public = True project.save() @@ -88,7 +88,7 @@ def test_email_sent_to_all_admins_on_submit(self, mock_mail, app, project, nonco assert res.status_code == 201 assert mock_mail.call_count == 2 - @mock.patch('website.mails.mails.send_mail') + @mock.patch('website.mails.send_mail') def test_email_not_sent_to_parent_admins_on_submit(self, mock_mail, app, project, noncontrib, url, create_payload, second_admin): component = NodeFactory(parent=project, creator=second_admin) component.is_public = True diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 9687e19749d..ac63b1b7894 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -226,15 +226,15 @@ def notify_submit(self, ev): context = self.get_context() context['contributors_url'] = f'{self.machineable.target.absolute_url}contributors/' context['project_settings_url'] = f'{self.machineable.target.absolute_url}settings/' - - for admin in self.machineable.target.get_users_with_perm(permissions.ADMIN): - mails.send_mail( - admin.username, - mails.ACCESS_REQUEST_SUBMITTED, - admin=admin, - osf_contact_email=OSF_CONTACT_EMAIL, - **context - ) + if not self.machineable.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value: + for admin in self.machineable.target.get_users_with_perm(permissions.ADMIN): + mails.send_mail( + admin.username, + mails.ACCESS_REQUEST_SUBMITTED, + admin=admin, + osf_contact_email=OSF_CONTACT_EMAIL, + **context + ) def notify_resubmit(self, ev): """ Notify admins that someone is requesting access again From d007d63d6a228a572d8494476a028805f589f95d Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Tue, 14 Jan 2025 22:02:12 +0200 Subject: [PATCH 2/5] reverted changes to test_node_request_list --- api_tests/requests/views/test_node_request_list.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api_tests/requests/views/test_node_request_list.py b/api_tests/requests/views/test_node_request_list.py index b49e2840816..f94d3b4acf2 100644 --- a/api_tests/requests/views/test_node_request_list.py +++ b/api_tests/requests/views/test_node_request_list.py @@ -80,7 +80,7 @@ def test_requests_disabled_list(self, app, url, create_payload, project, admin): res = app.get(url, create_payload, auth=admin.auth, expect_errors=True) assert res.status_code == 403 - @mock.patch('website.mails.send_mail') + @mock.patch('website.mails.mails.send_mail') def test_email_sent_to_all_admins_on_submit(self, mock_mail, app, project, noncontrib, url, create_payload, second_admin): project.is_public = True project.save() @@ -88,7 +88,7 @@ def test_email_sent_to_all_admins_on_submit(self, mock_mail, app, project, nonco assert res.status_code == 201 assert mock_mail.call_count == 2 - @mock.patch('website.mails.send_mail') + @mock.patch('website.mails.mails.send_mail') def test_email_not_sent_to_parent_admins_on_submit(self, mock_mail, app, project, noncontrib, url, create_payload, second_admin): component = NodeFactory(parent=project, creator=second_admin) component.is_public = True From b2680033198f3a0ae7444f8b7a1a23bbfd04f69c Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Wed, 15 Jan 2025 15:15:24 +0200 Subject: [PATCH 3/5] added testcases to check email send once --- admin/base/settings/defaults.py | 4 +++- .../views/test_node_request_institutional_access.py | 8 ++++++++ osf/features.yaml | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/admin/base/settings/defaults.py b/admin/base/settings/defaults.py index f0647492f67..6ca67ee5075 100644 --- a/admin/base/settings/defaults.py +++ b/admin/base/settings/defaults.py @@ -40,7 +40,9 @@ CSRF_COOKIE_HTTPONLY = False ALLOWED_HOSTS = [ - '.osf.io' + '.osf.io', + 'localhost', + '127.0.0.1' ] AUTH_PASSWORD_VALIDATORS = [ diff --git a/api_tests/requests/views/test_node_request_institutional_access.py b/api_tests/requests/views/test_node_request_institutional_access.py index 72b0fcce03b..16a3ec42626 100644 --- a/api_tests/requests/views/test_node_request_institutional_access.py +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -194,6 +194,14 @@ def test_institutional_admin_unauth_institution(self, app, project, institution_ assert res.status_code == 403 assert 'Institutional request access is not enabled.' in res.json['errors'][0]['detail'] + @mock.patch('website.mails.send_mail') + def test_email_send_to_all_admins_once_on_institutional_request(self, mock_mail, app, project, url, create_payload, institutional_admin): + project.is_public = True + project.save() + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + assert res.status_code == 201 + assert mock_mail.call_count == 1 + @mock.patch('api.requests.serializers.send_mail') def test_email_not_sent_without_recipient(self, mock_mail, app, project, institutional_admin, url, create_payload, institution): diff --git a/osf/features.yaml b/osf/features.yaml index 1b41e4b2cdc..ad57a9e3b06 100644 --- a/osf/features.yaml +++ b/osf/features.yaml @@ -192,6 +192,7 @@ flags: - flag_name: INSTITUTIONAL_DASHBOARD_2024 name: institutional_dashboard_2024 note: whether to surface older or updated (in 2024) institutional metrics + everyone: false switches: - flag_name: DISABLE_ENGAGEMENT_EMAILS From bb243a1fc99d91351034a2d843d0078ff546311e Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Wed, 15 Jan 2025 16:57:31 +0200 Subject: [PATCH 4/5] removed useless code --- admin/base/settings/defaults.py | 2 -- osf/features.yaml | 1 - 2 files changed, 3 deletions(-) diff --git a/admin/base/settings/defaults.py b/admin/base/settings/defaults.py index 6ca67ee5075..4554355821d 100644 --- a/admin/base/settings/defaults.py +++ b/admin/base/settings/defaults.py @@ -41,8 +41,6 @@ ALLOWED_HOSTS = [ '.osf.io', - 'localhost', - '127.0.0.1' ] AUTH_PASSWORD_VALIDATORS = [ diff --git a/osf/features.yaml b/osf/features.yaml index ad57a9e3b06..1b41e4b2cdc 100644 --- a/osf/features.yaml +++ b/osf/features.yaml @@ -192,7 +192,6 @@ flags: - flag_name: INSTITUTIONAL_DASHBOARD_2024 name: institutional_dashboard_2024 note: whether to surface older or updated (in 2024) institutional metrics - everyone: false switches: - flag_name: DISABLE_ENGAGEMENT_EMAILS From 979b1577270335a3469b58b1345129a5702c4d30 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Wed, 15 Jan 2025 17:07:50 +0200 Subject: [PATCH 5/5] refactored test --- admin/base/settings/defaults.py | 2 +- .../test_node_request_institutional_access.py | 53 ++++++++++++++++--- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/admin/base/settings/defaults.py b/admin/base/settings/defaults.py index 4554355821d..f0647492f67 100644 --- a/admin/base/settings/defaults.py +++ b/admin/base/settings/defaults.py @@ -40,7 +40,7 @@ CSRF_COOKIE_HTTPONLY = False ALLOWED_HOSTS = [ - '.osf.io', + '.osf.io' ] AUTH_PASSWORD_VALIDATORS = [ diff --git a/api_tests/requests/views/test_node_request_institutional_access.py b/api_tests/requests/views/test_node_request_institutional_access.py index 16a3ec42626..c3fdc4e111b 100644 --- a/api_tests/requests/views/test_node_request_institutional_access.py +++ b/api_tests/requests/views/test_node_request_institutional_access.py @@ -194,13 +194,52 @@ def test_institutional_admin_unauth_institution(self, app, project, institution_ assert res.status_code == 403 assert 'Institutional request access is not enabled.' in res.json['errors'][0]['detail'] - @mock.patch('website.mails.send_mail') - def test_email_send_to_all_admins_once_on_institutional_request(self, mock_mail, app, project, url, create_payload, institutional_admin): - project.is_public = True - project.save() - res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) - assert res.status_code == 201 - assert mock_mail.call_count == 1 + @mock.patch('api.requests.serializers.send_mail') + @mock.patch('osf.utils.machines.mails.send_mail') + def test_email_send_institutional_request_specific_email( + self, + mock_send_mail_machines, + mock_send_mail_serializers, + user_with_affiliation, + app, + project, + url, + create_payload, + institutional_admin, + institution + ): + """ + Test that the institutional request triggers email notifications to appropriate recipients. + """ + # Set up mock behaviors + project.is_public = True + project.save() + + # Perform the action + res = app.post_json_api(url, create_payload, auth=institutional_admin.auth) + + # Ensure response is successful + assert res.status_code == 201 + + assert mock_send_mail_serializers.call_count == 1 + assert mock_send_mail_machines.call_count == 0 + + # Check calls for osf.utils.machines.mails.send_mail + mock_send_mail_serializers.assert_called_once_with( + to_addr=user_with_affiliation.username, + mail=NODE_REQUEST_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + bcc_addr=None, + reply_to=None, + **{ + 'sender': institutional_admin, + 'recipient': user_with_affiliation, + 'comment': create_payload['data']['attributes']['comment'], + 'institution': institution, + 'osf_url': mock.ANY, + 'node': project, + } + ) @mock.patch('api.requests.serializers.send_mail') def test_email_not_sent_without_recipient(self, mock_mail, app, project, institutional_admin, url,