From 608cb755ea9ac045d4f9c2f6d1a8c6a41825ad05 Mon Sep 17 00:00:00 2001 From: Vlad0n20 <137097005+Vlad0n20@users.noreply.github.com> Date: Thu, 20 Feb 2025 18:44:05 +0200 Subject: [PATCH 1/5] [ENG-6526] Add new admins/moderators for a given institution page (#10984) ## Purpose Add a new page - admins/moderators for a given institution ## Changes Add 2 new views in the admin app ## Ticket https://openscience.atlassian.net/browse/ENG-6526 --- admin/institutions/urls.py | 5 ++ admin/institutions/views.py | 73 +++++++++++++++++++ admin/templates/institutions/detail.html | 3 + .../institutions/edit_moderators.html | 63 ++++++++++++++++ 4 files changed, 144 insertions(+) create mode 100644 admin/templates/institutions/edit_moderators.html diff --git a/admin/institutions/urls.py b/admin/institutions/urls.py index bc052be7b83..3b9b3aaf1f9 100644 --- a/admin/institutions/urls.py +++ b/admin/institutions/urls.py @@ -15,4 +15,9 @@ re_path(r'^(?P[0-9]+)/cannot_delete/$', views.CannotDeleteInstitution.as_view(), name='cannot_delete'), re_path(r'^(?P[0-9]+)/nodes/$', views.InstitutionNodeList.as_view(), name='nodes'), re_path(r'^(?P[0-9]+)/register/$', views.InstitutionalMetricsAdminRegister.as_view(), name='register_metrics_admin'), + re_path(r'^(?P[0-9]+)/list_and_add_admin_or_moderator/$', views.InstitutionListAndAddAdminOrModerator.as_view(), + name='list_and_add_admin_or_moderator'), + re_path(r'^(?P[0-9]+)/remove_admins_and_moderators/$', views.InstitutionRemoveAdminOrModerator.as_view(), + name='remove_admins_and_moderators'), + ] diff --git a/admin/institutions/views.py b/admin/institutions/views.py index 2c7286fe242..b83460b6aa6 100644 --- a/admin/institutions/views.py +++ b/admin/institutions/views.py @@ -164,6 +164,79 @@ def get_context_data(self, **kwargs): kwargs.setdefault('logohost', settings.OSF_URL) return super().get_context_data(**kwargs) +class InstitutionAdminAndModeratorBaseView(PermissionRequiredMixin, ListView): + permission_required = 'osf.change_institution' + template_name = 'institutions/edit_moderators.html' + raise_exception = True + + def get_queryset(self): + return Institution.objects.get(id=self.kwargs['institution_id']) + + def get_context_data(self, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + context = super().get_context_data(**kwargs) + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + context['institution'] = institution + context['admins'] = admin_group.user_set.all() + context['moderators'] = institution.contributors.exclude(id__in=admin_group.user_set.values_list('id', flat=True)) + return context + + +class InstitutionListAndAddAdminOrModerator(InstitutionAdminAndModeratorBaseView): + + def get_permission_required(self): + if self.request.method == 'GET': + return ('osf.view_institution',) + return (self.permission_required,) + + def post(self, request, *args, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + data = dict(request.POST) + del data['csrfmiddlewaretoken'] # just to remove the key from the form dict + + target_user = OSFUser.load(data['add-moderators-form'][0]) + if target_user is None: + messages.error(request, f'User for guid: {data["add-moderators-form"][0]} could not be found') + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) + + if 'admin' in data: + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + admin_group.user_set.add(target_user) + target_type = 'admin' + else: + institution.contributors.add(target_user) + target_type = 'moderator' + + messages.success(request, f'The following {target_type} was successfully added: {target_user.fullname} ({target_user.username})') + + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) + +class InstitutionRemoveAdminOrModerator(InstitutionAdminAndModeratorBaseView): + + def post(self, request, *args, **kwargs): + institution = Institution.objects.get(id=self.kwargs['institution_id']) + data = dict(request.POST) + del data['csrfmiddlewaretoken'] # just to remove the key from the form dict + + to_be_removed = list(data.keys()) + removed_admins = [admin.replace('Admin-', '') for admin in to_be_removed if 'Admin-' in admin] + removed_moderators = [moderator.replace('Moderator-', '') for moderator in to_be_removed if + 'Moderator-' in moderator] + moderators = OSFUser.objects.filter(id__in=removed_moderators) + admins = OSFUser.objects.filter(id__in=removed_admins) + admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() + admin_group.user_set.remove(*admins) + institution.contributors.remove(*moderators) + + if moderators: + moderator_names = ' ,'.join(moderators.values_list('fullname', flat=True)) + messages.success(request, f'The following moderators were successfully removed: {moderator_names}') + + if admins: + admin_names = ' ,'.join(admins.values_list('fullname', flat=True)) + messages.success(request, f'The following admins were successfully removed: {admin_names}') + + return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) class DeleteInstitution(PermissionRequiredMixin, DeleteView): permission_required = 'osf.delete_institution' diff --git a/admin/templates/institutions/detail.html b/admin/templates/institutions/detail.html index 0f83e361ac7..196b6559e4f 100644 --- a/admin/templates/institutions/detail.html +++ b/admin/templates/institutions/detail.html @@ -59,6 +59,9 @@

Banner:

Create Moderator/Admin
+
+ Contributors list +
{% endif %} diff --git a/admin/templates/institutions/edit_moderators.html b/admin/templates/institutions/edit_moderators.html new file mode 100644 index 00000000000..ddef91f5ef4 --- /dev/null +++ b/admin/templates/institutions/edit_moderators.html @@ -0,0 +1,63 @@ +{% extends "base.html" %} +{% load static %} +{% load render_bundle from webpack_loader %} +{% block title %} + Institution contributors +{% endblock title %} +{% block content %} +
+
+ {% if messages %} +
    + {% for message in messages %} + {{ message }} + {% endfor %} +
+ {% endif %} +
+
+
+

{{ institution.name }}

+
+
+
+
+
+ {% csrf_token %} + + + + +
+
+
+
+
+
+
+ {% csrf_token %} + + + + + {% for moderator in moderators %} + + + + + + {% endfor %} + {% for admin in admins %} + + + + + + {% endfor %} +
NameType
{{ moderator.fullname }} ({{moderator.username}})Moderator
{{ admin.fullname }} ({{admin.username}})Admin
+ +
+
+
+
+{% endblock content %} \ No newline at end of file From 1d5c7e4954084666321413c5314cac87e4a94675 Mon Sep 17 00:00:00 2001 From: "Brian J. Geiger" Date: Thu, 20 Feb 2025 12:45:59 -0500 Subject: [PATCH 2/5] =?UTF-8?q?Revert=20"[ENG-6526]=20Add=20new=20admins/m?= =?UTF-8?q?oderators=20for=20a=20given=20institution=20page=20(#1=E2=80=A6?= =?UTF-8?q?"=20(#10989)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 608cb755ea9ac045d4f9c2f6d1a8c6a41825ad05. --- admin/institutions/urls.py | 5 -- admin/institutions/views.py | 73 ------------------- admin/templates/institutions/detail.html | 3 - .../institutions/edit_moderators.html | 63 ---------------- 4 files changed, 144 deletions(-) delete mode 100644 admin/templates/institutions/edit_moderators.html diff --git a/admin/institutions/urls.py b/admin/institutions/urls.py index 3b9b3aaf1f9..bc052be7b83 100644 --- a/admin/institutions/urls.py +++ b/admin/institutions/urls.py @@ -15,9 +15,4 @@ re_path(r'^(?P[0-9]+)/cannot_delete/$', views.CannotDeleteInstitution.as_view(), name='cannot_delete'), re_path(r'^(?P[0-9]+)/nodes/$', views.InstitutionNodeList.as_view(), name='nodes'), re_path(r'^(?P[0-9]+)/register/$', views.InstitutionalMetricsAdminRegister.as_view(), name='register_metrics_admin'), - re_path(r'^(?P[0-9]+)/list_and_add_admin_or_moderator/$', views.InstitutionListAndAddAdminOrModerator.as_view(), - name='list_and_add_admin_or_moderator'), - re_path(r'^(?P[0-9]+)/remove_admins_and_moderators/$', views.InstitutionRemoveAdminOrModerator.as_view(), - name='remove_admins_and_moderators'), - ] diff --git a/admin/institutions/views.py b/admin/institutions/views.py index b83460b6aa6..2c7286fe242 100644 --- a/admin/institutions/views.py +++ b/admin/institutions/views.py @@ -164,79 +164,6 @@ def get_context_data(self, **kwargs): kwargs.setdefault('logohost', settings.OSF_URL) return super().get_context_data(**kwargs) -class InstitutionAdminAndModeratorBaseView(PermissionRequiredMixin, ListView): - permission_required = 'osf.change_institution' - template_name = 'institutions/edit_moderators.html' - raise_exception = True - - def get_queryset(self): - return Institution.objects.get(id=self.kwargs['institution_id']) - - def get_context_data(self, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - context = super().get_context_data(**kwargs) - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - context['institution'] = institution - context['admins'] = admin_group.user_set.all() - context['moderators'] = institution.contributors.exclude(id__in=admin_group.user_set.values_list('id', flat=True)) - return context - - -class InstitutionListAndAddAdminOrModerator(InstitutionAdminAndModeratorBaseView): - - def get_permission_required(self): - if self.request.method == 'GET': - return ('osf.view_institution',) - return (self.permission_required,) - - def post(self, request, *args, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - data = dict(request.POST) - del data['csrfmiddlewaretoken'] # just to remove the key from the form dict - - target_user = OSFUser.load(data['add-moderators-form'][0]) - if target_user is None: - messages.error(request, f'User for guid: {data["add-moderators-form"][0]} could not be found') - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) - - if 'admin' in data: - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - admin_group.user_set.add(target_user) - target_type = 'admin' - else: - institution.contributors.add(target_user) - target_type = 'moderator' - - messages.success(request, f'The following {target_type} was successfully added: {target_user.fullname} ({target_user.username})') - - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) - -class InstitutionRemoveAdminOrModerator(InstitutionAdminAndModeratorBaseView): - - def post(self, request, *args, **kwargs): - institution = Institution.objects.get(id=self.kwargs['institution_id']) - data = dict(request.POST) - del data['csrfmiddlewaretoken'] # just to remove the key from the form dict - - to_be_removed = list(data.keys()) - removed_admins = [admin.replace('Admin-', '') for admin in to_be_removed if 'Admin-' in admin] - removed_moderators = [moderator.replace('Moderator-', '') for moderator in to_be_removed if - 'Moderator-' in moderator] - moderators = OSFUser.objects.filter(id__in=removed_moderators) - admins = OSFUser.objects.filter(id__in=removed_admins) - admin_group = Group.objects.filter(name__startswith=f'institution_{institution._id}').first() - admin_group.user_set.remove(*admins) - institution.contributors.remove(*moderators) - - if moderators: - moderator_names = ' ,'.join(moderators.values_list('fullname', flat=True)) - messages.success(request, f'The following moderators were successfully removed: {moderator_names}') - - if admins: - admin_names = ' ,'.join(admins.values_list('fullname', flat=True)) - messages.success(request, f'The following admins were successfully removed: {admin_names}') - - return redirect('institutions:list_and_add_admin_or_moderator', institution_id=institution.id) class DeleteInstitution(PermissionRequiredMixin, DeleteView): permission_required = 'osf.delete_institution' diff --git a/admin/templates/institutions/detail.html b/admin/templates/institutions/detail.html index 196b6559e4f..0f83e361ac7 100644 --- a/admin/templates/institutions/detail.html +++ b/admin/templates/institutions/detail.html @@ -59,9 +59,6 @@

Banner:

Create Moderator/Admin
-
- Contributors list -
{% endif %} diff --git a/admin/templates/institutions/edit_moderators.html b/admin/templates/institutions/edit_moderators.html deleted file mode 100644 index ddef91f5ef4..00000000000 --- a/admin/templates/institutions/edit_moderators.html +++ /dev/null @@ -1,63 +0,0 @@ -{% extends "base.html" %} -{% load static %} -{% load render_bundle from webpack_loader %} -{% block title %} - Institution contributors -{% endblock title %} -{% block content %} -
-
- {% if messages %} -
    - {% for message in messages %} - {{ message }} - {% endfor %} -
- {% endif %} -
-
-
-

{{ institution.name }}

-
-
-
-
-
- {% csrf_token %} - - - - -
-
-
-
-
-
-
- {% csrf_token %} - - - - - {% for moderator in moderators %} - - - - - - {% endfor %} - {% for admin in admins %} - - - - - - {% endfor %} -
NameType
{{ moderator.fullname }} ({{moderator.username}})Moderator
{{ admin.fullname }} ({{admin.username}})Admin
- -
-
-
-
-{% endblock content %} \ No newline at end of file From ba7a25d03d418e1555e8666df6698ba36c463ae7 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Tue, 25 Feb 2025 11:30:53 -0500 Subject: [PATCH 3/5] update merge function to better distinguish between the requesting user addons and the user being merged addons --- osf/models/mixins.py | 19 ++++++++++++++++++- osf/models/user.py | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 7c82044fd1a..702fbabac09 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -52,7 +52,7 @@ ReviewTriggers, ) -from osf.utils.requests import get_request_and_user_id +from osf.utils.requests import get_request_and_user_id, get_current_request from website.project import signals as project_signals from website import settings, mails, language from website.project.licenses import set_license @@ -496,6 +496,23 @@ def get_addons(self, service_type: str | None = None): for config in self.ADDONS_AVAILABLE ] if _f] + def get_addons_for_self(self): + """ + This refers to the model self, not the user making the request. + """ + request = get_current_request() + if flag_is_active(request, features.ENABLE_GV): + osf_addons = filter( + lambda x: x is not None, + (self.get_addon(addon) for addon in self.OSF_HOSTED_ADDONS) + ) + return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=self._id)) + + return [_f for _f in [ + self.get_addon(config.short_name) + for config in self.ADDONS_AVAILABLE + ] if _f] + def get_oauth_addons(self): # TODO: Using hasattr is a dirty hack - we should be using issubclass(). # We can't, because importing the parent classes here causes a diff --git a/osf/models/user.py b/osf/models/user.py index 5a1183f9547..af96359311b 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -720,7 +720,7 @@ def contributed(self): @property def can_be_merged(self): """The ability of the `merge_user` method to fully merge the user""" - return all(addon.can_be_merged for addon in self.get_addons()) + return all(addon.can_be_merged for addon in self.get_addons_for_self()) def merge_user(self, user): """Merge a registered user into this account. This user will be From 63f36cbb6351f813b207ca9bcd528d045adcd479 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Wed, 26 Feb 2025 09:05:26 -0500 Subject: [PATCH 4/5] break-up test file and enable GV for merging users in tests --- osf_tests/test_merging_users.py | 296 ++++++++++++++++++++++++++++++++ osf_tests/test_user.py | 271 ----------------------------- 2 files changed, 296 insertions(+), 271 deletions(-) create mode 100644 osf_tests/test_merging_users.py diff --git a/osf_tests/test_merging_users.py b/osf_tests/test_merging_users.py new file mode 100644 index 00000000000..1a35e856211 --- /dev/null +++ b/osf_tests/test_merging_users.py @@ -0,0 +1,296 @@ +import pytest +from unittest import mock +import datetime as dt +from django.utils import timezone +from tests.base import OsfTestCase + +from framework.celery_tasks import handlers +from website import settings +from website.project.signals import contributor_added +from website.project.views.contributor import notify_added_contributor +from website.util.metrics import OsfSourceTags +from framework.auth import Auth + +from osf_tests.factories import ( + UserFactory, + UnconfirmedUserFactory, + ProjectFactory, + UnregUserFactory, + ExternalAccountFactory, +) +from importlib import import_module +from django.conf import settings as django_conf_settings +from osf.models import UserSessionMap +from tests.utils import run_celery_tasks +from waffle.testutils import override_flag +from osf.features import ENABLE_GV + +SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore + + +@pytest.mark.enable_implicit_clean +@pytest.mark.enable_bookmark_creation +class TestUserMerging(OsfTestCase): + def setUp(self): + super().setUp() + self.user = UserFactory() + with self.context: + handlers.celery_before_request() + + def _add_unconfirmed_user(self): + self.unconfirmed = UnconfirmedUserFactory() + + self.user.add_system_tag('user') + self.user.add_system_tag('shared') + self.unconfirmed.add_system_tag('unconfirmed') + self.unconfirmed.add_system_tag('shared') + + def _add_unregistered_user(self): + self.unregistered = UnregUserFactory() + + self.project_with_unreg_contrib = ProjectFactory() + self.project_with_unreg_contrib.add_unregistered_contributor( + fullname='Unreg', + email=self.unregistered.username, + auth=Auth(self.project_with_unreg_contrib.creator) + ) + self.project_with_unreg_contrib.save() + + @pytest.mark.enable_enqueue_task + @mock.patch('website.mailchimp_utils.get_mailchimp_api') + def test_merge(self, mock_get_mailchimp_api): + def is_mrm_field(value): + return 'RelatedManager' in str(value.__class__) + + other_user = UserFactory() + other_user.save() + + # create session for other_user + other_user_session = SessionStore() + other_user_session.create() + UserSessionMap.objects.create(user=other_user, session_key=other_user_session.session_key) + + # define values for users' fields + today = timezone.now() + yesterday = today - dt.timedelta(days=1) + + self.user.comments_viewed_timestamp['shared_gt'] = today + other_user.comments_viewed_timestamp['shared_gt'] = yesterday + self.user.comments_viewed_timestamp['shared_lt'] = yesterday + other_user.comments_viewed_timestamp['shared_lt'] = today + self.user.comments_viewed_timestamp['user'] = yesterday + other_user.comments_viewed_timestamp['other'] = yesterday + + self.user.email_verifications = {'user': {'email': 'a'}} + other_user.email_verifications = {'other': {'email': 'b'}} + + self.user.notifications_configured = {'abc12': True} + other_user.notifications_configured = {'123ab': True} + + self.user.external_accounts.add(ExternalAccountFactory()) + other_user.external_accounts.add(ExternalAccountFactory()) + + self.user.mailchimp_mailing_lists = { + } + other_user.mailchimp_mailing_lists = { + settings.MAILCHIMP_GENERAL_LIST: True + } + + self.user.security_messages = { + 'user': today, + 'shared': today, + } + other_user.security_messages = { + 'other': today, + 'shared': today, + } + + self.user.add_system_tag('user') + self.user.add_system_tag('shared') + other_user.add_system_tag('other') + other_user.add_system_tag('shared') + + self.user.save() + other_user.save() + + # define expected behavior for ALL FIELDS of the User object + default_to_master_user_fields = [ + 'id', + 'date_confirmed', + 'date_disabled', + 'date_last_login', + 'date_registered', + 'email_last_sent', + 'external_identity', + 'family_name', + 'fullname', + 'given_name', + 'is_invited', + 'is_registered', + 'jobs', + 'locale', + 'merged_by', + 'middle_names', + 'password', + 'schools', + 'social', + 'suffix', + 'timezone', + 'username', + 'verification_key', + 'verification_key_v2', + 'contributor_added_email_records', + 'requested_deactivation', + ] + + calculated_fields = { + 'comments_viewed_timestamp': { + 'user': yesterday, + 'other': yesterday, + 'shared_gt': today, + 'shared_lt': today, + }, + 'email_verifications': { + 'user': {'email': 'a'}, + 'other': {'email': 'b'}, + }, + 'notifications_configured': { + '123ab': True, 'abc12': True, + }, + 'emails': { + other_user.emails.first().id, + self.user.emails.first().id, + }, + 'external_accounts': { + self.user.external_accounts.first().id, + other_user.external_accounts.first().id, + }, + 'recently_added': set(), + 'mailchimp_mailing_lists': { + settings.MAILCHIMP_GENERAL_LIST: True + }, + 'osf_mailing_lists': { + 'Open Science Framework Help': True + }, + 'security_messages': { + 'user': today, + 'other': today, + 'shared': today, + }, + 'unclaimed_records': {}, + } + + # from the explicit rules above, compile expected field/value pairs + expected = {} + expected.update(calculated_fields) + for key in default_to_master_user_fields: + if is_mrm_field(getattr(self.user, key)): + expected[key] = set(list(getattr(self.user, key).all().values_list('id', flat=True))) + else: + expected[key] = getattr(self.user, key) + + # ensure all fields of the user object have an explicit expectation + all_field_names = {each.name for each in self.user._meta.get_fields()} + assert set(expected.keys()).issubset(all_field_names) + + # mock mailchimp + mock_client = mock.MagicMock() + mock_get_mailchimp_api.return_value = mock_client + + with run_celery_tasks(): + # perform the merge + with override_flag(ENABLE_GV, active=True): + self.user.merge_user(other_user) + self.user.save() + + self.user.reload() + + # check each field/value pair + for k, v in expected.items(): + if is_mrm_field(getattr(self.user, k)): + assert set(list(getattr(self.user, k).all().values_list('id', flat=True))) == v, f'{k} doesn\'t match expectations' + else: + assert getattr(self.user, k) == v, f'{k} doesn\'t match expectation' + + assert sorted(self.user.system_tags) == ['other', 'shared', 'user'] + + # check fields set on merged user + assert other_user.merged_by == self.user + + assert not SessionStore().exists(session_key=other_user_session.session_key) + + def test_merge_unconfirmed(self): + self._add_unconfirmed_user() + unconfirmed_username = self.unconfirmed.username + self.user.merge_user(self.unconfirmed) + + assert self.unconfirmed.is_merged is True + assert self.unconfirmed.merged_by == self.user + + assert self.user.is_registered is True + assert self.user.is_invited is False + + assert sorted(self.user.system_tags) == sorted(['shared', 'user', 'unconfirmed', OsfSourceTags.Osf.value]) + + assert self.unconfirmed.email_verifications == {} + assert self.unconfirmed.password[0] == '!' + assert self.unconfirmed.verification_key is None + # The mergee's email no longer needs to be confirmed by merger + unconfirmed_emails = [record['email'] for record in self.user.email_verifications.values()] + assert unconfirmed_username not in unconfirmed_emails + + def test_merge_preserves_external_identity(self): + verified_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}}) + linking_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'LINK'}}) + creating_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'CREATE'}}) + different_id_user = UserFactory(external_identity={'ORCID': {'4321-4321-4321-4321': 'VERIFIED'}}) + no_id_user = UserFactory(external_identity={'ORCID': {}}) + no_provider_user = UserFactory(external_identity={}) + + with override_flag(ENABLE_GV, active=True): + linking_user.merge_user(creating_user) + assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'LINK'}} + + with override_flag(ENABLE_GV, active=True): + linking_user.merge_user(verified_user) + assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} + with override_flag(ENABLE_GV, active=True): + linking_user.merge_user(no_id_user) + assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} + with override_flag(ENABLE_GV, active=True): + linking_user.merge_user(no_provider_user) + assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} + with override_flag(ENABLE_GV, active=True): + linking_user.merge_user(different_id_user) + assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} + + assert creating_user.external_identity == {} + assert verified_user.external_identity == {} + assert no_id_user.external_identity == {} + assert no_provider_user.external_identity == {} + + with override_flag(ENABLE_GV, active=True): + no_provider_user.merge_user(linking_user) + assert linking_user.external_identity == {} + assert no_provider_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} + + def test_merge_unregistered(self): + # test only those behaviors that are not tested with unconfirmed users + self._add_unregistered_user() + + with override_flag(ENABLE_GV, active=True): + self.user.merge_user(self.unregistered) + + self.project_with_unreg_contrib.reload() + assert self.user.is_invited is True + assert self.user in self.project_with_unreg_contrib.contributors + + @mock.patch('website.project.views.contributor.mails.send_mail') + def test_merge_doesnt_send_signal(self, mock_notify): + #Explictly reconnect signal as it is disconnected by default for test + contributor_added.connect(notify_added_contributor) + other_user = UserFactory() + with override_flag(ENABLE_GV, active=True): + self.user.merge_user(other_user) + assert other_user.merged_by._id == self.user._id + assert mock_notify.called is False diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index c5774823ccd..c031fcc344a 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -19,13 +19,9 @@ from framework.auth.signals import user_account_merged from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError -from framework.celery_tasks import handlers from website import settings from website import filters -from website.project.signals import contributor_added -from website.project.views.contributor import notify_added_contributor from website.views import find_bookmark_collection -from website.util.metrics import OsfSourceTags from osf.models import ( AbstractNode, @@ -71,7 +67,6 @@ DraftNodeFactory ) from tests.base import OsfTestCase -from tests.utils import run_celery_tasks SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore @@ -1891,272 +1886,6 @@ def test_remove_all_affiliated_institutions(self): assert user.get_affiliated_institutions().count() == 0 -# Copied from tests/models/test_user.py -@pytest.mark.enable_implicit_clean -@pytest.mark.enable_bookmark_creation -class TestUserMerging(OsfTestCase): - def setUp(self): - super().setUp() - self.user = UserFactory() - with self.context: - handlers.celery_before_request() - - def _add_unconfirmed_user(self): - self.unconfirmed = UnconfirmedUserFactory() - - self.user.add_system_tag('user') - self.user.add_system_tag('shared') - self.unconfirmed.add_system_tag('unconfirmed') - self.unconfirmed.add_system_tag('shared') - - def _add_unregistered_user(self): - self.unregistered = UnregUserFactory() - - self.project_with_unreg_contrib = ProjectFactory() - self.project_with_unreg_contrib.add_unregistered_contributor( - fullname='Unreg', - email=self.unregistered.username, - auth=Auth(self.project_with_unreg_contrib.creator) - ) - self.project_with_unreg_contrib.save() - - @pytest.mark.enable_enqueue_task - @mock.patch('website.mailchimp_utils.get_mailchimp_api') - def test_merge(self, mock_get_mailchimp_api): - def is_mrm_field(value): - return 'RelatedManager' in str(value.__class__) - - other_user = UserFactory() - other_user.save() - - # create session for other_user - other_user_session = SessionStore() - other_user_session.create() - UserSessionMap.objects.create(user=other_user, session_key=other_user_session.session_key) - - # define values for users' fields - today = timezone.now() - yesterday = today - dt.timedelta(days=1) - - self.user.comments_viewed_timestamp['shared_gt'] = today - other_user.comments_viewed_timestamp['shared_gt'] = yesterday - self.user.comments_viewed_timestamp['shared_lt'] = yesterday - other_user.comments_viewed_timestamp['shared_lt'] = today - self.user.comments_viewed_timestamp['user'] = yesterday - other_user.comments_viewed_timestamp['other'] = yesterday - - self.user.email_verifications = {'user': {'email': 'a'}} - other_user.email_verifications = {'other': {'email': 'b'}} - - self.user.notifications_configured = {'abc12': True} - other_user.notifications_configured = {'123ab': True} - - self.user.external_accounts.add(ExternalAccountFactory()) - other_user.external_accounts.add(ExternalAccountFactory()) - - self.user.mailchimp_mailing_lists = { - } - other_user.mailchimp_mailing_lists = { - settings.MAILCHIMP_GENERAL_LIST: True - } - - self.user.security_messages = { - 'user': today, - 'shared': today, - } - other_user.security_messages = { - 'other': today, - 'shared': today, - } - - self.user.add_system_tag('user') - self.user.add_system_tag('shared') - other_user.add_system_tag('other') - other_user.add_system_tag('shared') - - self.user.save() - other_user.save() - - # define expected behavior for ALL FIELDS of the User object - default_to_master_user_fields = [ - 'id', - 'date_confirmed', - 'date_disabled', - 'date_last_login', - 'date_registered', - 'email_last_sent', - 'external_identity', - 'family_name', - 'fullname', - 'given_name', - 'is_invited', - 'is_registered', - 'jobs', - 'locale', - 'merged_by', - 'middle_names', - 'password', - 'schools', - 'social', - 'suffix', - 'timezone', - 'username', - 'verification_key', - 'verification_key_v2', - 'contributor_added_email_records', - 'requested_deactivation', - ] - - calculated_fields = { - 'comments_viewed_timestamp': { - 'user': yesterday, - 'other': yesterday, - 'shared_gt': today, - 'shared_lt': today, - }, - 'email_verifications': { - 'user': {'email': 'a'}, - 'other': {'email': 'b'}, - }, - 'notifications_configured': { - '123ab': True, 'abc12': True, - }, - 'emails': { - other_user.emails.first().id, - self.user.emails.first().id, - }, - 'external_accounts': { - self.user.external_accounts.first().id, - other_user.external_accounts.first().id, - }, - 'recently_added': set(), - 'mailchimp_mailing_lists': { - settings.MAILCHIMP_GENERAL_LIST: True - }, - 'osf_mailing_lists': { - 'Open Science Framework Help': True - }, - 'security_messages': { - 'user': today, - 'other': today, - 'shared': today, - }, - 'unclaimed_records': {}, - } - - # from the explicit rules above, compile expected field/value pairs - expected = {} - expected.update(calculated_fields) - for key in default_to_master_user_fields: - if is_mrm_field(getattr(self.user, key)): - expected[key] = set(list(getattr(self.user, key).all().values_list('id', flat=True))) - else: - expected[key] = getattr(self.user, key) - - # ensure all fields of the user object have an explicit expectation - all_field_names = {each.name for each in self.user._meta.get_fields()} - assert set(expected.keys()).issubset(all_field_names) - - # mock mailchimp - mock_client = mock.MagicMock() - mock_get_mailchimp_api.return_value = mock_client - - with run_celery_tasks(): - # perform the merge - self.user.merge_user(other_user) - self.user.save() - - self.user.reload() - - # check each field/value pair - for k, v in expected.items(): - if is_mrm_field(getattr(self.user, k)): - assert set(list(getattr(self.user, k).all().values_list('id', flat=True))) == v, f'{k} doesn\'t match expectations' - else: - assert getattr(self.user, k) == v, f'{k} doesn\'t match expectation' - - assert sorted(self.user.system_tags) == ['other', 'shared', 'user'] - - # check fields set on merged user - assert other_user.merged_by == self.user - - assert not SessionStore().exists(session_key=other_user_session.session_key) - - def test_merge_unconfirmed(self): - self._add_unconfirmed_user() - unconfirmed_username = self.unconfirmed.username - self.user.merge_user(self.unconfirmed) - - assert self.unconfirmed.is_merged is True - assert self.unconfirmed.merged_by == self.user - - assert self.user.is_registered is True - assert self.user.is_invited is False - - # TODO: test profile fields - jobs, schools, social - # TODO: test security_messages - # TODO: test mailing_lists - - assert sorted(self.user.system_tags) == sorted(['shared', 'user', 'unconfirmed', OsfSourceTags.Osf.value]) - - # TODO: test emails - # TODO: test external_accounts - - assert self.unconfirmed.email_verifications == {} - assert self.unconfirmed.password[0] == '!' - assert self.unconfirmed.verification_key is None - # The mergee's email no longer needs to be confirmed by merger - unconfirmed_emails = [record['email'] for record in self.user.email_verifications.values()] - assert unconfirmed_username not in unconfirmed_emails - - def test_merge_preserves_external_identity(self): - verified_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}}) - linking_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'LINK'}}) - creating_user = UserFactory(external_identity={'ORCID': {'1234-1234-1234-1234': 'CREATE'}}) - different_id_user = UserFactory(external_identity={'ORCID': {'4321-4321-4321-4321': 'VERIFIED'}}) - no_id_user = UserFactory(external_identity={'ORCID': {}}) - no_provider_user = UserFactory(external_identity={}) - - linking_user.merge_user(creating_user) - assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'LINK'}} - linking_user.merge_user(verified_user) - assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} - linking_user.merge_user(no_id_user) - assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} - linking_user.merge_user(no_provider_user) - assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED'}} - linking_user.merge_user(different_id_user) - assert linking_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} - - assert creating_user.external_identity == {} - assert verified_user.external_identity == {} - assert no_id_user.external_identity == {} - assert no_provider_user.external_identity == {} - - no_provider_user.merge_user(linking_user) - assert linking_user.external_identity == {} - assert no_provider_user.external_identity == {'ORCID': {'1234-1234-1234-1234': 'VERIFIED', '4321-4321-4321-4321': 'VERIFIED'}} - - def test_merge_unregistered(self): - # test only those behaviors that are not tested with unconfirmed users - self._add_unregistered_user() - - self.user.merge_user(self.unregistered) - - self.project_with_unreg_contrib.reload() - assert self.user.is_invited is True - assert self.user in self.project_with_unreg_contrib.contributors - - @mock.patch('website.project.views.contributor.mails.send_mail') - def test_merge_doesnt_send_signal(self, mock_notify): - #Explictly reconnect signal as it is disconnected by default for test - contributor_added.connect(notify_added_contributor) - other_user = UserFactory() - self.user.merge_user(other_user) - assert other_user.merged_by._id == self.user._id - assert mock_notify.called is False - - @pytest.mark.enable_implicit_clean class TestUserValidation(OsfTestCase): From 226808e37d5141d3eca81dc1a3adc7ff4c0b943d Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Wed, 26 Feb 2025 10:10:27 -0500 Subject: [PATCH 5/5] merge methods for clarity and DRYness --- osf/models/mixins.py | 34 ++++++++++++++------------------- osf/models/user.py | 2 +- osf_tests/test_merging_users.py | 3 ++- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 702fbabac09..1af6459c257 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -52,7 +52,7 @@ ReviewTriggers, ) -from osf.utils.requests import get_request_and_user_id, get_current_request +from osf.utils.requests import get_request_and_user_id from website.project import signals as project_signals from website import settings, mails, language from website.project.licenses import set_license @@ -482,31 +482,25 @@ def get_addon_key(cls, config): def addons(self): return self.get_addons() - def get_addons(self, service_type: str | None = None): - request, user_id = get_request_and_user_id() - if flag_is_active(request, features.ENABLE_GV): - osf_addons = filter( - lambda x: x is not None, - (self.get_addon(addon) for addon in self.OSF_HOSTED_ADDONS) - ) - return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id, service_type=service_type)) + def get_addons(self, service_type: str | None = None, in_request_context: bool = True): + ''' + This gets all a user's addons whether that user is the model user (self.) or the user making the request (the + user signing off on whatever auth mechicanism such as token or basic auth. - return [_f for _f in [ - self.get_addon(config.short_name) - for config in self.ADDONS_AVAILABLE - ] if _f] + service_type is the addon type such as "storage" or "citations" + in_request_context is the addon for the requesting user? or is it outside the request context. + ''' + if in_request_context: + request, user_id = get_request_and_user_id() + else: + user_id = self._id - def get_addons_for_self(self): - """ - This refers to the model self, not the user making the request. - """ - request = get_current_request() - if flag_is_active(request, features.ENABLE_GV): + if not in_request_context or (in_request_context and flag_is_active(request, features.ENABLE_GV)): osf_addons = filter( lambda x: x is not None, (self.get_addon(addon) for addon in self.OSF_HOSTED_ADDONS) ) - return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=self._id)) + return itertools.chain(osf_addons, self._get_addons_from_gv(requesting_user_id=user_id, service_type=service_type)) return [_f for _f in [ self.get_addon(config.short_name) diff --git a/osf/models/user.py b/osf/models/user.py index af96359311b..0b00d256369 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -720,7 +720,7 @@ def contributed(self): @property def can_be_merged(self): """The ability of the `merge_user` method to fully merge the user""" - return all(addon.can_be_merged for addon in self.get_addons_for_self()) + return all(addon.can_be_merged for addon in self.get_addons(in_request_context=False)) def merge_user(self, user): """Merge a registered user into this account. This user will be diff --git a/osf_tests/test_merging_users.py b/osf_tests/test_merging_users.py index 1a35e856211..0bb124c4f13 100644 --- a/osf_tests/test_merging_users.py +++ b/osf_tests/test_merging_users.py @@ -222,7 +222,8 @@ def is_mrm_field(value): def test_merge_unconfirmed(self): self._add_unconfirmed_user() unconfirmed_username = self.unconfirmed.username - self.user.merge_user(self.unconfirmed) + with override_flag(ENABLE_GV, active=True): + self.user.merge_user(self.unconfirmed) assert self.unconfirmed.is_merged is True assert self.unconfirmed.merged_by == self.user