From 1db9adcb7ea525a5d5967816b09a42070ba7a0e8 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 14 Jul 2023 12:05:31 -0700 Subject: [PATCH 01/13] Add outline for class + command to merge two users in simple cases --- coldfront/core/project/utils.py | 16 + .../user/management/commands/merge_users.py | 62 +++ .../core/user/utils_/user_merge_utils.py | 389 ++++++++++++++++++ 3 files changed, 467 insertions(+) create mode 100644 coldfront/core/user/management/commands/merge_users.py create mode 100644 coldfront/core/user/utils_/user_merge_utils.py diff --git a/coldfront/core/project/utils.py b/coldfront/core/project/utils.py index cb4179a10..fb005ba40 100644 --- a/coldfront/core/project/utils.py +++ b/coldfront/core/project/utils.py @@ -7,6 +7,7 @@ from coldfront.core.allocation.utils_.accounting_utils import set_service_units from coldfront.core.project.models import Project from coldfront.core.project.models import ProjectStatusChoice +from coldfront.core.project.models import ProjectUserRoleChoice from coldfront.core.resource.utils import get_compute_resource_names from coldfront.core.resource.utils import get_primary_compute_resource_name from coldfront.core.utils.common import display_time_zone_current_date @@ -220,3 +221,18 @@ def is_primary_cluster_project(project): project_compute_resource_name = get_project_compute_resource_name(project) primary_cluster_resource_name = get_primary_compute_resource_name() return project_compute_resource_name == primary_cluster_resource_name + + +def higher_project_user_role(role_1, role_2): + """Given two ProjectUserRoleChoices, return the "higher" (more + privileged) of the two.""" + assert isinstance(role_1, ProjectUserRoleChoice) + assert isinstance(role_2, ProjectUserRoleChoice) + roles_ascending = ['User', 'Manager', 'Principal Investigator'] + assert role_1.name in roles_ascending + assert role_2.name in roles_ascending + role_1_index = roles_ascending.index(role_1.name) + role_2_index = roles_ascending.index(role_2.name) + if roles_ascending[role_1_index] >= roles_ascending[role_2_index]: + return role_1 + return role_2 diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py new file mode 100644 index 000000000..2fd97d873 --- /dev/null +++ b/coldfront/core/user/management/commands/merge_users.py @@ -0,0 +1,62 @@ +import logging +import sys + +from django.contrib.auth.models import User +from django.core.management.base import BaseCommand + +from coldfront.core.user.utils_.user_merge_utils import UserMergeRunner +from coldfront.core.utils.common import add_argparse_dry_run_argument + + +"""An admin command that merges two Users into one.""" + + +class Command(BaseCommand): + + help = ( + 'Merge two Users into one. The command chooses one instance, transfers ' + 'that instance\'s relationships, requests, etc. to the other, and then ' + 'deletes it.') + + logger = logging.getLogger(__name__) + + def add_arguments(self, parser): + add_argparse_dry_run_argument(parser) + parser.add_argument( + 'username_1', help='The username of the first user.', type=str) + parser.add_argument( + 'username_2', help='The username of the second user.', type=str) + + def handle(self, *args, **options): + dry_run = options['dry_run'] + if not dry_run: + user_confirmation = input( + 'Are you sure you wish to proceed? [Y/y/N/n]: ') + if user_confirmation.strip().lower() != 'y': + self.stdout.write(self.style.WARNING('Merge aborted.')) + sys.exit(0) + + username_1 = options['username_1'] + try: + user_1 = User.objects.get(username=username_1) + except User.DoesNotExist: + self.stderr.write( + self.style.ERROR(f'User "{username_1}" does not exist.')) + return + + username_2 = options['username_2'] + try: + user_2 = User.objects.get(username=username_2) + except User.DoesNotExist: + self.stderr.write( + self.style.ERROR(f'User "{username_2}" does not exist.')) + return + + # TODO: Check that the first and last names of the two users match. + + user_merge_runner = UserMergeRunner(user_1, user_2) + + print(f'Src: {user_merge_runner.src_user}') + print(f'Dst: {user_merge_runner.dst_user}') + + user_merge_runner.run() diff --git a/coldfront/core/user/utils_/user_merge_utils.py b/coldfront/core/user/utils_/user_merge_utils.py new file mode 100644 index 000000000..58167624f --- /dev/null +++ b/coldfront/core/user/utils_/user_merge_utils.py @@ -0,0 +1,389 @@ +import inspect +import logging + +from abc import ABC +from abc import abstractmethod + +from django.contrib.auth.models import User +from django.contrib.admin.utils import NestedObjects +from django.core.exceptions import ObjectDoesNotExist +from django.db import DEFAULT_DB_ALIAS +from django.db import transaction + +from flags.state import flag_enabled + +from coldfront.core.allocation.models import AllocationUser +from coldfront.core.allocation.models import AllocationUserStatusChoice +from coldfront.core.allocation.utils import has_cluster_access +from coldfront.core.project.models import ProjectUser +from coldfront.core.project.models import ProjectUserStatusChoice +from coldfront.core.project.utils import higher_project_user_role + + +class UserMergeRunner(object): + """TODO""" + + def __init__(self, user_1, user_2): + """TODO""" + # Merge src_user's data into dst_user. + self._src_user = None + self._dst_user = None + self._identify_src_and_dst_users(user_1, user_2) + # TODO: + # Error out if both users have cluster access. + # Warn if the destination user is inactive. + + @property + def dst_user(self): + return self._dst_user + + @property + def src_user(self): + return self._src_user + + def run(self): + """TODO""" + with transaction.atomic(): + self._process_dependencies() + self._src_user.delete() + # TODO: Remove this. + raise Exception('Rolling back.') + + def _identify_src_and_dst_users(self, user_1, user_2): + """Given two Users, determine which should be the source (the + one having its data merged and then deleted) and which should be + the destination (the one having data merged into it).""" + user_1_has_cluster_access = has_cluster_access(user_1) + user_2_has_cluster_access = has_cluster_access(user_2) + if not (user_1_has_cluster_access or user_2_has_cluster_access): + src, dst = user_2, user_1 + elif user_1_has_cluster_access and not user_2_has_cluster_access: + src, dst = user_2, user_1 + elif not user_1_has_cluster_access and user_2_has_cluster_access: + src, dst = user_1, user_2 + else: + raise NotImplementedError( + 'Both Users have cluster access. This case is not currently ' + 'handled.') + self._src_user = src + self._dst_user = dst + + def _process_dependencies(self): + """TODO""" + collector = NestedObjects(using=DEFAULT_DB_ALIAS) + collector.collect([self._src_user]) + objects = collector.nested() + + assert len(objects) == 2 + assert isinstance(objects[0], User) + assert isinstance(objects[1], list) + + for obj in self._yield_nested_objects(objects[1]): + class_handler_factory = ClassHandlerFactory() + try: + handler = class_handler_factory.get_handler( + obj.__class__, self._src_user, self._dst_user, obj) + handler.run() + except ValueError: + print( + f'Found no handler for object with class {obj.__class__}.') + except Exception as e: + # TODO + print(e) + + def _yield_nested_objects(self, objects): + """TODO""" + for obj in objects: + if isinstance(obj, list): + yield from self._yield_nested_objects(obj) + else: + yield obj + + +class ClassHandlerFactory(object): + """A factory for returning a class that handles merging data from a + source object into a destination object of a given class when + merging User accounts.""" + + def get_handler(self, klass, *args, **kwargs): + """Return an instantiated handler for the given class with the + given arguments and keyword arguments.""" + assert inspect.isclass(klass) + return self._get_handler_class(klass)(*args, **kwargs) + + @staticmethod + def _get_handler_class(klass): + """Return the appropriate handler class for the given class. If + none are applicable, raise a ValueError.""" + handler_class_name = f'{klass.__name__}Handler' + try: + return globals()[handler_class_name] + except KeyError: + raise ValueError(f'No handler for class {klass.__name__}.') + + +class ClassHandler(ABC): + """TODO""" + + @abstractmethod + def __init__(self, src_user, dst_user, src_obj): + """TODO""" + self._src_user = src_user + self._dst_user = dst_user + self._src_obj = src_obj + self._dst_obj = None + + self._logger = logging.getLogger(__name__) + + def dry_run(self): + """TODO""" + # TODO: Display what would(n't) happen. + # # E.g., not overriding an existing billing_activity. + pass + + def run(self): + """TODO""" + with transaction.atomic(): + # TODO: Consider whether special handling may need to happen first. + if self._dst_obj: + self._set_falsy_attrs() + self._run_special_handling() + + def _get_settable_if_falsy_attrs(self): + """TODO""" + return [] + + def _run_special_handling(self): + """TODO""" + raise NotImplementedError + + def _set_attr_if_falsy(self, attr_name, dry=False): + """TODO""" + assert hasattr(self._src_obj, attr_name) + assert hasattr(self._dst_obj, attr_name) + + src_attr = getattr(self._src_obj, attr_name) + dst_attr = getattr(self._dst_obj, attr_name) + + if src_attr and not dst_attr: + if dry: + # TODO: Print. + pass + else: + setattr(self._dst_obj, attr_name, src_attr) + # TODO: Log. + # Only flush to the log at the end of the transaction, or + # Log that the transaction is being rolled back. + # Include a UUID in each log message to identify the merge. + + def _set_falsy_attrs(self): + """TODO""" + for attr_name in self._get_settable_if_falsy_attrs(): + self._set_attr_if_falsy(attr_name) + self._dst_obj.save() + + def _transfer_src_obj_to_dst_user(self): + """TODO""" + self._src_obj.user = self._dst_user + self._src_obj.save() + + +class UserProfileHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._dst_obj = self._dst_user.userprofile + + def _get_settable_if_falsy_attrs(self): + return [ + 'is_pi', + # Only the destination user should have a cluster UID. + # 'cluster_uid', + 'phone_number', + 'access_agreement_signed_date', + 'billing_activity', + ] + + def _run_special_handling(self): + """TODO""" + self._set_host_user() + + def _set_host_user(self): + if flag_enabled('LRC_ONLY'): + # TODO + # Deal with conflicts. + # Handle LBL users. + self._set_attr_if_falsy('host_user') + + +class SocialAccountHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + """TODO""" + self._transfer_src_obj_to_dst_user() + + +class EmailAddressHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + """TODO""" + # TODO + # Consider allowing a new primary to be set. + self._src_obj.primary = False + self._transfer_src_obj_to_dst_user() + + +class AllocationUserHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + try: + self._dst_obj = AllocationUser.objects.get( + allocation=self._src_obj.allocation, user=self._dst_user) + except ObjectDoesNotExist: + self._dst_obj = None + + def _run_special_handling(self): + """TODO""" + allocation = self._src_obj.allocation + + # TODO: Note that only compute Allocations are handled for now. + + assert allocation.resources.count() == 1 + resource = allocation.resources.first() + assert resource.name.endswith(' Compute') + + if self._dst_obj: + self._src_obj.delete() + + active_allocation_user_status = \ + AllocationUserStatusChoice.objects.get(name='Active') + if (self._dst_obj.status != active_allocation_user_status and + self._dst_obj.status == active_allocation_user_status): + self._dst_obj.status = self._src_obj.status + self._dst_obj.save() + else: + self._src_obj.user = self._dst_user + self._src_obj.save() + + +class AllocationUserAttributeHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + # TODO: This block is duplicated. Factor it out. + try: + self._src_obj.refresh_from_db() + except ObjectDoesNotExist: + # The object was deleted. + # TODO: Log and write to output. + return + else: + # The object was transferred to the destination user. + # TODO: Log and write to output. + return + + +class AllocationUserAttributeUsageHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + try: + self._src_obj.refresh_from_db() + except ObjectDoesNotExist: + # The object was deleted. + # TODO: Log and write to output. + return + else: + # The object was transferred to the destination user. + # TODO: Log and write to output. + return + + +class ClusterAccessRequestHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + try: + self._src_obj.refresh_from_db() + except ObjectDoesNotExist: + # The object was deleted. + # TODO: Log and write to output. + return + else: + # The object was transferred to the destination user. + # TODO: Log and write to output. + return + + +class ProjectUserHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + try: + self._dst_obj = ProjectUser.objects.get( + project=self._src_obj.project, user=self._dst_user) + except ObjectDoesNotExist: + self._dst_obj = None + + def _run_special_handling(self): + if self._dst_obj: + self._dst_obj.role = higher_project_user_role( + self._dst_obj.role, self._src_obj.role) + + active_project_user_status = ProjectUserStatusChoice.objects.get( + name='Active') + if (self._dst_obj.status != active_project_user_status and + self._src_obj.status == active_project_user_status): + self._dst_obj.status = self._src_obj.status + self._dst_obj.save() + + self._src_obj.delete() + else: + self._src_obj.user = self._dst_user + self._src_obj.save() + + # TODO: Run the runner? + + +class ProjectUserJoinRequestHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + try: + self._src_obj.refresh_from_db() + except ObjectDoesNotExist: + # The object was deleted. + # TODO: Log and write to output. + return + else: + # The object was transferred to the destination user. + # TODO: Log and write to output. + return + + +class SavioProjectAllocationRequestHandler(ClassHandler): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + def _run_special_handling(self): + if self._src_obj.requester == self._src_user: + self._src_obj.requester = self._dst_user + if self._src_obj.pi == self._src_user: + self._src_obj.pi = self._dst_user + self._src_obj.save() From a38ed946bd88411f127c68573792f1da3345df2b Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 26 Jul 2023 14:00:50 -0700 Subject: [PATCH 02/13] Refactor user merge classes across files --- .../user/management/commands/merge_users.py | 4 +- .../core/user/utils_/merge_users/__init__.py | 6 + .../class_handlers.py} | 101 ++------------- .../core/user/utils_/merge_users/runner.py | 117 ++++++++++++++++++ 4 files changed, 136 insertions(+), 92 deletions(-) create mode 100644 coldfront/core/user/utils_/merge_users/__init__.py rename coldfront/core/user/utils_/{user_merge_utils.py => merge_users/class_handlers.py} (73%) create mode 100644 coldfront/core/user/utils_/merge_users/runner.py diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py index 2fd97d873..61a128e44 100644 --- a/coldfront/core/user/management/commands/merge_users.py +++ b/coldfront/core/user/management/commands/merge_users.py @@ -4,7 +4,7 @@ from django.contrib.auth.models import User from django.core.management.base import BaseCommand -from coldfront.core.user.utils_.user_merge_utils import UserMergeRunner +from coldfront.core.user.utils_.merge_users import UserMergeRunner from coldfront.core.utils.common import add_argparse_dry_run_argument @@ -59,4 +59,6 @@ def handle(self, *args, **options): print(f'Src: {user_merge_runner.src_user}') print(f'Dst: {user_merge_runner.dst_user}') + # TODO: Call run or dry_run. + user_merge_runner.run() diff --git a/coldfront/core/user/utils_/merge_users/__init__.py b/coldfront/core/user/utils_/merge_users/__init__.py new file mode 100644 index 000000000..f9bf3c137 --- /dev/null +++ b/coldfront/core/user/utils_/merge_users/__init__.py @@ -0,0 +1,6 @@ +from coldfront.core.user.utils_.merge_users.runner import UserMergeRunner + + +__all__ = [ + 'UserMergeRunner', +] diff --git a/coldfront/core/user/utils_/user_merge_utils.py b/coldfront/core/user/utils_/merge_users/class_handlers.py similarity index 73% rename from coldfront/core/user/utils_/user_merge_utils.py rename to coldfront/core/user/utils_/merge_users/class_handlers.py index 58167624f..f869da708 100644 --- a/coldfront/core/user/utils_/user_merge_utils.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -4,102 +4,18 @@ from abc import ABC from abc import abstractmethod -from django.contrib.auth.models import User -from django.contrib.admin.utils import NestedObjects from django.core.exceptions import ObjectDoesNotExist -from django.db import DEFAULT_DB_ALIAS from django.db import transaction from flags.state import flag_enabled from coldfront.core.allocation.models import AllocationUser from coldfront.core.allocation.models import AllocationUserStatusChoice -from coldfront.core.allocation.utils import has_cluster_access from coldfront.core.project.models import ProjectUser from coldfront.core.project.models import ProjectUserStatusChoice from coldfront.core.project.utils import higher_project_user_role -class UserMergeRunner(object): - """TODO""" - - def __init__(self, user_1, user_2): - """TODO""" - # Merge src_user's data into dst_user. - self._src_user = None - self._dst_user = None - self._identify_src_and_dst_users(user_1, user_2) - # TODO: - # Error out if both users have cluster access. - # Warn if the destination user is inactive. - - @property - def dst_user(self): - return self._dst_user - - @property - def src_user(self): - return self._src_user - - def run(self): - """TODO""" - with transaction.atomic(): - self._process_dependencies() - self._src_user.delete() - # TODO: Remove this. - raise Exception('Rolling back.') - - def _identify_src_and_dst_users(self, user_1, user_2): - """Given two Users, determine which should be the source (the - one having its data merged and then deleted) and which should be - the destination (the one having data merged into it).""" - user_1_has_cluster_access = has_cluster_access(user_1) - user_2_has_cluster_access = has_cluster_access(user_2) - if not (user_1_has_cluster_access or user_2_has_cluster_access): - src, dst = user_2, user_1 - elif user_1_has_cluster_access and not user_2_has_cluster_access: - src, dst = user_2, user_1 - elif not user_1_has_cluster_access and user_2_has_cluster_access: - src, dst = user_1, user_2 - else: - raise NotImplementedError( - 'Both Users have cluster access. This case is not currently ' - 'handled.') - self._src_user = src - self._dst_user = dst - - def _process_dependencies(self): - """TODO""" - collector = NestedObjects(using=DEFAULT_DB_ALIAS) - collector.collect([self._src_user]) - objects = collector.nested() - - assert len(objects) == 2 - assert isinstance(objects[0], User) - assert isinstance(objects[1], list) - - for obj in self._yield_nested_objects(objects[1]): - class_handler_factory = ClassHandlerFactory() - try: - handler = class_handler_factory.get_handler( - obj.__class__, self._src_user, self._dst_user, obj) - handler.run() - except ValueError: - print( - f'Found no handler for object with class {obj.__class__}.') - except Exception as e: - # TODO - print(e) - - def _yield_nested_objects(self, objects): - """TODO""" - for obj in objects: - if isinstance(obj, list): - yield from self._yield_nested_objects(obj) - else: - yield obj - - class ClassHandlerFactory(object): """A factory for returning a class that handles merging data from a source object into a destination object of a given class when @@ -134,12 +50,12 @@ def __init__(self, src_user, dst_user, src_obj): self._dst_obj = None self._logger = logging.getLogger(__name__) + self._dry = False def dry_run(self): """TODO""" - # TODO: Display what would(n't) happen. - # # E.g., not overriding an existing billing_activity. - pass + self._dry = True + self.run() def run(self): """TODO""" @@ -148,6 +64,8 @@ def run(self): if self._dst_obj: self._set_falsy_attrs() self._run_special_handling() + if self._dst_obj: + self._dst_obj.save() def _get_settable_if_falsy_attrs(self): """TODO""" @@ -157,7 +75,7 @@ def _run_special_handling(self): """TODO""" raise NotImplementedError - def _set_attr_if_falsy(self, attr_name, dry=False): + def _set_attr_if_falsy(self, attr_name): """TODO""" assert hasattr(self._src_obj, attr_name) assert hasattr(self._dst_obj, attr_name) @@ -166,7 +84,7 @@ def _set_attr_if_falsy(self, attr_name, dry=False): dst_attr = getattr(self._dst_obj, attr_name) if src_attr and not dst_attr: - if dry: + if self._dry: # TODO: Print. pass else: @@ -184,8 +102,9 @@ def _set_falsy_attrs(self): def _transfer_src_obj_to_dst_user(self): """TODO""" - self._src_obj.user = self._dst_user - self._src_obj.save() + if not self._dry: + self._src_obj.user = self._dst_user + self._src_obj.save() class UserProfileHandler(ClassHandler): diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py new file mode 100644 index 000000000..d43a9931e --- /dev/null +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -0,0 +1,117 @@ +from django.contrib.auth.models import User +from django.contrib.admin.utils import NestedObjects +from django.db import DEFAULT_DB_ALIAS +from django.db import transaction + +from coldfront.core.allocation.utils import has_cluster_access +from coldfront.core.user.utils_.merge_users.class_handlers import ClassHandlerFactory + + +class UserMergeRunner(object): + """A class that merges two User objects into one. + + It identifies one User as a source and the other as a destination, + merges the source's relationships, requests, etc. into the + destination, and then deletes the source. + + It currently only supports merging when only one of the given Users + has cluster access. + """ + + def __init__(self, user_1, user_2): + """Identify which of the two Users should be merged into.""" + # src_user's data will be merged into dst_user. + self._src_user = None + self._dst_user = None + self._identify_src_and_dst_users(user_1, user_2) + + @property + def dst_user(self): + return self._dst_user + + @property + def src_user(self): + return self._src_user + + @transaction.atomic + def run(self): + """Transfer dependencies from the source User to the destination + User, then delete the source User.""" + try: + with transaction.atomic(): + self._process_src_user_dependencies() + self._src_user.delete() + + self._dst_user.refresh_from_db() + self._display_dst_user_dependencies() + raise Exception('Rolling back.') + except Exception as e: + # TODO + print(e) + + def _identify_src_and_dst_users(self, user_1, user_2): + """Given two Users, determine which should be the source (the + one having its data merged and then deleted) and which should be + the destination (the one having data merged into it).""" + user_1_has_cluster_access = has_cluster_access(user_1) + user_2_has_cluster_access = has_cluster_access(user_2) + if not (user_1_has_cluster_access or user_2_has_cluster_access): + src, dst = user_2, user_1 + elif user_1_has_cluster_access and not user_2_has_cluster_access: + src, dst = user_2, user_1 + elif not user_1_has_cluster_access and user_2_has_cluster_access: + src, dst = user_1, user_2 + else: + raise NotImplementedError( + 'Both Users have cluster access. This case is not currently ' + 'handled.') + self._src_user = src + self._dst_user = dst + + def _process_src_user_dependencies(self): + """Process each database object associated with the source User + on a case-by-case basis.""" + collector = NestedObjects(using=DEFAULT_DB_ALIAS) + collector.collect([self._src_user]) + objects = collector.nested() + + assert len(objects) == 2 + assert isinstance(objects[0], User) + assert isinstance(objects[1], list) + + for obj in self._yield_nested_objects(objects[1]): + class_handler_factory = ClassHandlerFactory() + + # Block other threads from retrieving this object until the end of + # the transaction. + obj = obj.__class__.objects.select_for_update().get(pk=obj.pk) + + try: + handler = class_handler_factory.get_handler( + obj.__class__, self._src_user, self._dst_user, obj) + handler.run() + except ValueError: + print( + f'Found no handler for object with class {obj.__class__}.') + except Exception as e: + # TODO + print(e) + + def _display_dst_user_dependencies(self): + """TODO""" + collector = NestedObjects(using=DEFAULT_DB_ALIAS) + collector.collect([self._dst_user]) + objects = collector.nested() + + for obj in self._yield_nested_objects(objects[1]): + print(obj.__class__, obj, obj.__dict__) + + def _yield_nested_objects(self, objects): + """Given a list that contains objects and lists of potentially- + nested objects, return a generator that recursively yields + objects.""" + for obj in objects: + if isinstance(obj, list): + yield from self._yield_nested_objects(obj) + else: + yield obj From 4153589bb512f0b4063e02d73bdac6a367de2f0c Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 26 Jul 2023 14:19:55 -0700 Subject: [PATCH 03/13] Skip deleting objects that will get deleted in cascading fashion later --- coldfront/core/user/utils_/merge_users/class_handlers.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/coldfront/core/user/utils_/merge_users/class_handlers.py b/coldfront/core/user/utils_/merge_users/class_handlers.py index f869da708..d3c19e8a2 100644 --- a/coldfront/core/user/utils_/merge_users/class_handlers.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -179,8 +179,6 @@ def _run_special_handling(self): assert resource.name.endswith(' Compute') if self._dst_obj: - self._src_obj.delete() - active_allocation_user_status = \ AllocationUserStatusChoice.objects.get(name='Active') if (self._dst_obj.status != active_allocation_user_status and @@ -268,8 +266,6 @@ def _run_special_handling(self): self._src_obj.status == active_project_user_status): self._dst_obj.status = self._src_obj.status self._dst_obj.save() - - self._src_obj.delete() else: self._src_obj.user = self._dst_user self._src_obj.save() From 6ee568c8588556ca7fe48bfcdee322d878522ba9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 26 Jul 2023 15:25:11 -0700 Subject: [PATCH 04/13] Skip transfer of legacy EmailAddress instances --- coldfront/core/user/utils_/merge_users/runner.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py index d43a9931e..7b7fc847c 100644 --- a/coldfront/core/user/utils_/merge_users/runner.py +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -4,6 +4,7 @@ from django.db import transaction from coldfront.core.allocation.utils import has_cluster_access +from coldfront.core.user.models import EmailAddress as OldEmailAddress from coldfront.core.user.utils_.merge_users.class_handlers import ClassHandlerFactory @@ -41,7 +42,6 @@ def run(self): with transaction.atomic(): self._process_src_user_dependencies() self._src_user.delete() - self._dst_user.refresh_from_db() self._display_dst_user_dependencies() raise Exception('Rolling back.') @@ -49,6 +49,14 @@ def run(self): # TODO print(e) + @staticmethod + def _classes_to_ignore(): + """Return a set of classes for which no processing should be + done.""" + return { + OldEmailAddress, + } + def _identify_src_and_dst_users(self, user_1, user_2): """Given two Users, determine which should be the source (the one having its data merged and then deleted) and which should be @@ -79,7 +87,13 @@ def _process_src_user_dependencies(self): assert isinstance(objects[0], User) assert isinstance(objects[1], list) + classes_to_ignore = self._classes_to_ignore() + for obj in self._yield_nested_objects(objects[1]): + + if obj.__class__ in classes_to_ignore: + continue + class_handler_factory = ClassHandlerFactory() # Block other threads from retrieving this object until the end of From 31031574efe7ba856eb5a7849d68af750ec129c4 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Jul 2023 10:52:11 -0700 Subject: [PATCH 05/13] Implement dry run of user merge by rolling back transaction --- .../user/management/commands/merge_users.py | 15 ++-- .../core/user/utils_/merge_users/runner.py | 72 ++++++++++++++----- 2 files changed, 62 insertions(+), 25 deletions(-) diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py index 61a128e44..993ead2fc 100644 --- a/coldfront/core/user/management/commands/merge_users.py +++ b/coldfront/core/user/management/commands/merge_users.py @@ -52,13 +52,14 @@ def handle(self, *args, **options): self.style.ERROR(f'User "{username_2}" does not exist.')) return - # TODO: Check that the first and last names of the two users match. - user_merge_runner = UserMergeRunner(user_1, user_2) - print(f'Src: {user_merge_runner.src_user}') - print(f'Dst: {user_merge_runner.dst_user}') - - # TODO: Call run or dry_run. + self.stdout.write( + self.style.WARNING(f'Source: {user_merge_runner.src_user}')) + self.stdout.write( + self.style.WARNING(f'Destination: {user_merge_runner.dst_user}')) - user_merge_runner.run() + if dry_run: + user_merge_runner.dry_run() + else: + user_merge_runner.run() diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py index 7b7fc847c..36e3918f4 100644 --- a/coldfront/core/user/utils_/merge_users/runner.py +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -8,6 +8,14 @@ from coldfront.core.user.utils_.merge_users.class_handlers import ClassHandlerFactory +class UserMergeError(Exception): + pass + + +class UserMergeRollback(Exception): + pass + + class UserMergeRunner(object): """A class that merges two User objects into one. @@ -21,9 +29,11 @@ class UserMergeRunner(object): def __init__(self, user_1, user_2): """Identify which of the two Users should be merged into.""" + self._dry = False # src_user's data will be merged into dst_user. self._src_user = None self._dst_user = None + self._src_user_pk = None self._identify_src_and_dst_users(user_1, user_2) @property @@ -34,20 +44,29 @@ def dst_user(self): def src_user(self): return self._src_user + def dry_run(self): + """Attempt to run the merge, but rollback before committing + changes.""" + self._dry = True + self.run() + @transaction.atomic def run(self): """Transfer dependencies from the source User to the destination User, then delete the source User.""" try: with transaction.atomic(): + self._select_users_for_update() self._process_src_user_dependencies() self._src_user.delete() - self._dst_user.refresh_from_db() - self._display_dst_user_dependencies() - raise Exception('Rolling back.') + if self._dry: + self._rollback() + except UserMergeRollback: + # The dry run succeeded, and the transaction was rolled back. + self._reset_users() except Exception as e: - # TODO - print(e) + self._reset_users() + raise e @staticmethod def _classes_to_ignore(): @@ -75,6 +94,9 @@ def _identify_src_and_dst_users(self, user_1, user_2): 'handled.') self._src_user = src self._dst_user = dst + # Store the primary key of src_user, used to restore the object after + # dry run rollback. + self._src_user_pk = self._src_user.pk def _process_src_user_dependencies(self): """Process each database object associated with the source User @@ -105,20 +127,34 @@ def _process_src_user_dependencies(self): obj.__class__, self._src_user, self._dst_user, obj) handler.run() except ValueError: - print( - f'Found no handler for object with class {obj.__class__}.') + raise UserMergeError( + f'No handler for object with class {obj.__class__}.') except Exception as e: - # TODO - print(e) - - def _display_dst_user_dependencies(self): - """TODO""" - collector = NestedObjects(using=DEFAULT_DB_ALIAS) - collector.collect([self._dst_user]) - objects = collector.nested() - - for obj in self._yield_nested_objects(objects[1]): - print(obj.__class__, obj, obj.__dict__) + raise UserMergeError( + f'Failed to process object with class {obj.__class__} and ' + f'primary key {obj.pk}. Details:\n{e}') + + def _reset_users(self): + """Reset user objects because the values of a model's fields + won't be reverted when a transaction rollback happens. + + Source: https://docs.djangoproject.com/en/3.2/topics/db/transactions/#controlling-transactions-explicitly + """ + self._src_user = User.objects.get(pk=self._src_user_pk) + self._dst_user.refresh_from_db() + + def _rollback(self): + """Raise a UserMergeRollback exception to roll the enclosing + transaction back.""" + raise UserMergeRollback('Rolling back.') + + def _select_users_for_update(self): + """Block other threads from retrieving the users until the end + of the transaction.""" + self._src_user = User.objects.select_for_update().get( + pk=self._src_user.pk) + self._dst_user = User.objects.select_for_update().get( + pk=self._dst_user.pk) def _yield_nested_objects(self, objects): """Given a list that contains objects and lists of potentially- From 4e0895a243e45fc1bfe12b020178bf58554af085 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Jul 2023 15:26:44 -0700 Subject: [PATCH 06/13] Correct bug in higher_project_user_role --- coldfront/core/project/utils.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/coldfront/core/project/utils.py b/coldfront/core/project/utils.py index fb005ba40..886740bc2 100644 --- a/coldfront/core/project/utils.py +++ b/coldfront/core/project/utils.py @@ -233,6 +233,4 @@ def higher_project_user_role(role_1, role_2): assert role_2.name in roles_ascending role_1_index = roles_ascending.index(role_1.name) role_2_index = roles_ascending.index(role_2.name) - if roles_ascending[role_1_index] >= roles_ascending[role_2_index]: - return role_1 - return role_2 + return role_1 if role_1_index >= role_2_index else role_2 From 146ff4b46d92b8e46ad1aa3676ca8168d8318af0 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Jul 2023 15:27:24 -0700 Subject: [PATCH 07/13] Add classes for recording messages via Django command, printing, or logging --- coldfront/core/utils/reporting/__init__.py | 0 .../reporting/report_message_strategy.py | 84 +++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 coldfront/core/utils/reporting/__init__.py create mode 100644 coldfront/core/utils/reporting/report_message_strategy.py diff --git a/coldfront/core/utils/reporting/__init__.py b/coldfront/core/utils/reporting/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/utils/reporting/report_message_strategy.py b/coldfront/core/utils/reporting/report_message_strategy.py new file mode 100644 index 000000000..5ebbd31d7 --- /dev/null +++ b/coldfront/core/utils/reporting/report_message_strategy.py @@ -0,0 +1,84 @@ +import logging + +from abc import ABC + +from django.core.management.base import BaseCommand +from django.utils.termcolors import colorize + + +class ReportMessageStrategy(ABC): + """An interface that uses the Strategy design pattern to vary how + messages are reported.""" + + def error(self, message): + raise NotImplementedError + + def success(self, message): + raise NotImplementedError + + def warning(self, message): + raise NotImplementedError + + +class EnqueueForLoggingStrategy(ReportMessageStrategy): + """A strategy for enqueueing messages to be written to a logging instance + later.""" + + def __init__(self, logger): + assert isinstance(logger, logging.Logger) + self._logger = logger + # Tuples of the form (logging_func, message). + self._queue = [] + + def error(self, message): + logging_func = self._logger.error + self._queue.append((logging_func, message)) + + def success(self, message): + logging_func = self._logger.info + self._queue.append((logging_func, message)) + + def warning(self, message): + logging_func = self._logger.warning + self._queue.append((logging_func, message)) + + def log_queued_messages(self): + for logging_func, message in self._queue: + logging_func(message) + + +class PrintStrategy(ReportMessageStrategy): + """A strategy for printing messages.""" + + def error(self, message): + print(colorize(text=message, fg="red")) + + def success(self, message): + print(colorize(text=message, fg="green")) + + def warning(self, message): + print(colorize(text=message, fg="yellow")) + + +class WriteViaCommandStrategy(ReportMessageStrategy): + """A strategy for writing messages to stdout/stderr via a Django + management command.""" + + def __init__(self, command): + assert isinstance(command, BaseCommand) + self._command = command + + def error(self, message): + stream = self._command.stderr + style = self._command.style.ERROR + stream.write(style(message)) + + def success(self, message): + stream = self._command.stdout + style = self._command.style.SUCCESS + stream.write(style(message)) + + def warning(self, message): + stream = self._command.stdout + style = self._command.style.WANRING + stream.write(style(message)) From 4643bf63d4727b559a2bc6d94b774d7a7644c06e Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Jul 2023 15:33:56 -0700 Subject: [PATCH 08/13] Report uesr merge messages to Django command + log; refactor common logic --- .../user/management/commands/merge_users.py | 21 +- .../user/utils_/merge_users/class_handlers.py | 223 ++++++++++-------- .../core/user/utils_/merge_users/runner.py | 11 +- 3 files changed, 149 insertions(+), 106 deletions(-) diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py index 993ead2fc..13b169e40 100644 --- a/coldfront/core/user/management/commands/merge_users.py +++ b/coldfront/core/user/management/commands/merge_users.py @@ -6,6 +6,8 @@ from coldfront.core.user.utils_.merge_users import UserMergeRunner from coldfront.core.utils.common import add_argparse_dry_run_argument +from coldfront.core.utils.reporting.report_message_strategy import EnqueueForLoggingStrategy +from coldfront.core.utils.reporting.report_message_strategy import WriteViaCommandStrategy """An admin command that merges two Users into one.""" @@ -52,14 +54,27 @@ def handle(self, *args, **options): self.style.ERROR(f'User "{username_2}" does not exist.')) return - user_merge_runner = UserMergeRunner(user_1, user_2) + write_via_command_strategy = WriteViaCommandStrategy(self) + enqueue_for_logging_strategy = EnqueueForLoggingStrategy(self.logger) + reporting_strategies = [ + write_via_command_strategy, enqueue_for_logging_strategy] + user_merge_runner = UserMergeRunner( + user_1, user_2, reporting_strategies=reporting_strategies) + + src_user = user_merge_runner.src_user + dst_user = user_merge_runner.dst_user self.stdout.write( - self.style.WARNING(f'Source: {user_merge_runner.src_user}')) + self.style.WARNING( + f'Source: {src_user.username} ({src_user.first_name} ' + f'{src_user.last_name})')) self.stdout.write( - self.style.WARNING(f'Destination: {user_merge_runner.dst_user}')) + self.style.WARNING( + f'Destination: {dst_user.username} ({dst_user.first_name} ' + f'{dst_user.last_name})')) if dry_run: user_merge_runner.dry_run() else: user_merge_runner.run() + enqueue_for_logging_strategy.log_queued_messages() diff --git a/coldfront/core/user/utils_/merge_users/class_handlers.py b/coldfront/core/user/utils_/merge_users/class_handlers.py index d3c19e8a2..294465768 100644 --- a/coldfront/core/user/utils_/merge_users/class_handlers.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -1,5 +1,4 @@ import inspect -import logging from abc import ABC from abc import abstractmethod @@ -17,9 +16,8 @@ class ClassHandlerFactory(object): - """A factory for returning a class that handles merging data from a - source object into a destination object of a given class when - merging User accounts.""" + """A factory for returning a concrete instance of ClassHandler for a + particular class.""" def get_handler(self, klass, *args, **kwargs): """Return an instantiated handler for the given class with the @@ -39,28 +37,31 @@ def _get_handler_class(klass): class ClassHandler(ABC): - """TODO""" + """A class that handles transferring data from a source object of a + particular class, and which belongs to a source user, to a + destination user, when merging User accounts.""" @abstractmethod - def __init__(self, src_user, dst_user, src_obj): - """TODO""" + def __init__(self, src_user, dst_user, src_obj, reporting_strategies=None): self._src_user = src_user self._dst_user = dst_user self._src_obj = src_obj + # A corresponding object may or may not exist for the destination User. + # Attempt to retrieve it in each concrete child class. self._dst_obj = None - self._logger = logging.getLogger(__name__) - self._dry = False + self._class_name = self._src_obj.__class__.__name__ - def dry_run(self): - """TODO""" - self._dry = True - self.run() + # Report messages using each of the given strategies. + self._reporting_strategies = [] + if isinstance(reporting_strategies, list): + for strategy in reporting_strategies: + self._reporting_strategies.append(strategy) def run(self): - """TODO""" + """Transfer the source object from the source user to the + destination user.""" with transaction.atomic(): - # TODO: Consider whether special handling may need to happen first. if self._dst_obj: self._set_falsy_attrs() self._run_special_handling() @@ -68,31 +69,60 @@ def run(self): self._dst_obj.save() def _get_settable_if_falsy_attrs(self): - """TODO""" + """Return a list of attributes that, if falsy in the + destination, should be updated to the value of the corresponding + attribute in the source object.""" return [] + def _handle_associated_object(self): + """An object B may be associated with a User through a different + object A. When A is deleted, B may be deleted with it. When A is + transferred, B is transferred with it. Record that this has + occurred.""" + try: + self._src_obj.refresh_from_db() + except ObjectDoesNotExist: + # The object was deleted. + message = ( + f'{self._class_name}({self._src_obj.pk}): indirectly deleted') + self._report_success_message(message) + else: + # The object was transferred to the destination user. + self._record_update( + self._src_obj.pk, 'user (indirectly associated)', + self._src_user, self._dst_user) + + def _report_success_message(self, message): + """Record a success message with the given text to each of the + reporting strategies.""" + for strategy in self._reporting_strategies: + strategy.success(message) + + def _record_update(self, pk, attr_name, pre_value, post_value): + """Record that the object of this class and with the given + primary key had its attribute with the given name updated from + pre_value to post_value.""" + message = ( + f'{self._class_name}({pk}).{attr_name}: {pre_value} --> ' + f'{post_value}') + self._report_success_message(message) + def _run_special_handling(self): - """TODO""" + """Run handling specific to a particular class, implemented by + each child class.""" raise NotImplementedError def _set_attr_if_falsy(self, attr_name): - """TODO""" + """If the attribute with the given name is falsy in the + destination object but not in the source object, update the + former's value to the latter's.""" assert hasattr(self._src_obj, attr_name) assert hasattr(self._dst_obj, attr_name) - src_attr = getattr(self._src_obj, attr_name) dst_attr = getattr(self._dst_obj, attr_name) - if src_attr and not dst_attr: - if self._dry: - # TODO: Print. - pass - else: - setattr(self._dst_obj, attr_name, src_attr) - # TODO: Log. - # Only flush to the log at the end of the transaction, or - # Log that the transaction is being rolled back. - # Include a UUID in each log message to identify the merge. + setattr(self._dst_obj, attr_name, src_attr) + self._record_update(self._dst_obj.pk, attr_name, dst_attr, src_attr) def _set_falsy_attrs(self): """TODO""" @@ -100,11 +130,12 @@ def _set_falsy_attrs(self): self._set_attr_if_falsy(attr_name) self._dst_obj.save() - def _transfer_src_obj_to_dst_user(self): + def _transfer_src_obj_to_dst_user(self, attr_name='user'): """TODO""" - if not self._dry: - self._src_obj.user = self._dst_user - self._src_obj.save() + setattr(self._src_obj, attr_name, self._dst_user) + self._src_obj.save() + self._record_update( + self._src_obj.pk, attr_name, self._src_user, self._dst_user) class UserProfileHandler(ClassHandler): @@ -124,15 +155,15 @@ def _get_settable_if_falsy_attrs(self): ] def _run_special_handling(self): - """TODO""" self._set_host_user() def _set_host_user(self): if flag_enabled('LRC_ONLY'): + raise NotImplementedError # TODO # Deal with conflicts. # Handle LBL users. - self._set_attr_if_falsy('host_user') + # self._set_attr_if_falsy('host_user') class SocialAccountHandler(ClassHandler): @@ -141,7 +172,6 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - """TODO""" self._transfer_src_obj_to_dst_user() @@ -151,9 +181,6 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - """TODO""" - # TODO - # Consider allowing a new primary to be set. self._src_obj.primary = False self._transfer_src_obj_to_dst_user() @@ -169,7 +196,6 @@ def __init__(self, *args, **kwargs): self._dst_obj = None def _run_special_handling(self): - """TODO""" allocation = self._src_obj.allocation # TODO: Note that only compute Allocations are handled for now. @@ -179,15 +205,26 @@ def _run_special_handling(self): assert resource.name.endswith(' Compute') if self._dst_obj: - active_allocation_user_status = \ - AllocationUserStatusChoice.objects.get(name='Active') - if (self._dst_obj.status != active_allocation_user_status and - self._dst_obj.status == active_allocation_user_status): - self._dst_obj.status = self._src_obj.status + status_updated = self._update_status() + if status_updated: self._dst_obj.save() else: - self._src_obj.user = self._dst_user - self._src_obj.save() + self._transfer_src_obj_to_dst_user() + + def _update_status(self): + """Update the status of the destination if it is not "Active" + but the source's is. Return whether an update occurred.""" + active_allocation_user_status = \ + AllocationUserStatusChoice.objects.get(name='Active') + dst_obj_status = self._dst_obj.status + if (dst_obj_status != active_allocation_user_status and + self._src_obj.status == active_allocation_user_status): + self._dst_obj.status = self._src_obj.status + self._record_update( + self._dst_obj.pk, 'status', dst_obj_status.name, + 'Active') + return True + return False class AllocationUserAttributeHandler(ClassHandler): @@ -196,17 +233,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - # TODO: This block is duplicated. Factor it out. - try: - self._src_obj.refresh_from_db() - except ObjectDoesNotExist: - # The object was deleted. - # TODO: Log and write to output. - return - else: - # The object was transferred to the destination user. - # TODO: Log and write to output. - return + self._handle_associated_object() class AllocationUserAttributeUsageHandler(ClassHandler): @@ -215,16 +242,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - try: - self._src_obj.refresh_from_db() - except ObjectDoesNotExist: - # The object was deleted. - # TODO: Log and write to output. - return - else: - # The object was transferred to the destination user. - # TODO: Log and write to output. - return + self._handle_associated_object() class ClusterAccessRequestHandler(ClassHandler): @@ -233,16 +251,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - try: - self._src_obj.refresh_from_db() - except ObjectDoesNotExist: - # The object was deleted. - # TODO: Log and write to output. - return - else: - # The object was transferred to the destination user. - # TODO: Log and write to output. - return + self._handle_associated_object() class ProjectUserHandler(ClassHandler): @@ -257,21 +266,43 @@ def __init__(self, *args, **kwargs): def _run_special_handling(self): if self._dst_obj: - self._dst_obj.role = higher_project_user_role( - self._dst_obj.role, self._src_obj.role) - - active_project_user_status = ProjectUserStatusChoice.objects.get( - name='Active') - if (self._dst_obj.status != active_project_user_status and - self._src_obj.status == active_project_user_status): - self._dst_obj.status = self._src_obj.status + role_updated = self._update_role() + status_updated = self._update_status() + if role_updated or status_updated: self._dst_obj.save() else: - self._src_obj.user = self._dst_user - self._src_obj.save() + self._transfer_src_obj_to_dst_user() # TODO: Run the runner? + def _update_role(self): + """Update the role of the destination if the source's is higher. + Return whether an update occurred.""" + dst_obj_role = self._dst_obj.role + self._dst_obj.role = higher_project_user_role( + dst_obj_role, self._src_obj.role) + if self._dst_obj.role != dst_obj_role: + self._record_update( + self._dst_obj.pk, 'role', dst_obj_role.name, + self._dst_obj.role.name) + return True + return False + + def _update_status(self): + """Update the status of the destination if it is not "Active" + but the source's is. Return whether an update occurred.""" + active_project_user_status = ProjectUserStatusChoice.objects.get( + name='Active') + dst_obj_status = self._dst_obj.status + if (self._dst_obj.status != active_project_user_status and + self._src_obj.status == active_project_user_status): + self._dst_obj.status = self._src_obj.status + self._record_update( + self._dst_obj.pk, 'status', dst_obj_status.name, + 'Active') + return True + return False + class ProjectUserJoinRequestHandler(ClassHandler): @@ -279,16 +310,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - try: - self._src_obj.refresh_from_db() - except ObjectDoesNotExist: - # The object was deleted. - # TODO: Log and write to output. - return - else: - # The object was transferred to the destination user. - # TODO: Log and write to output. - return + self._handle_associated_object() class SavioProjectAllocationRequestHandler(ClassHandler): @@ -298,7 +320,6 @@ def __init__(self, *args, **kwargs): def _run_special_handling(self): if self._src_obj.requester == self._src_user: - self._src_obj.requester = self._dst_user + self._transfer_src_obj_to_dst_user(attr_name='requester') if self._src_obj.pi == self._src_user: - self._src_obj.pi = self._dst_user - self._src_obj.save() + self._transfer_src_obj_to_dst_user(attr_name='pi') diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py index 36e3918f4..9f1ecf4db 100644 --- a/coldfront/core/user/utils_/merge_users/runner.py +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -27,7 +27,7 @@ class UserMergeRunner(object): has cluster access. """ - def __init__(self, user_1, user_2): + def __init__(self, user_1, user_2, reporting_strategies=None): """Identify which of the two Users should be merged into.""" self._dry = False # src_user's data will be merged into dst_user. @@ -36,6 +36,12 @@ def __init__(self, user_1, user_2): self._src_user_pk = None self._identify_src_and_dst_users(user_1, user_2) + # Report messages using each of the given strategies. + self._reporting_strategies = [] + if isinstance(reporting_strategies, list): + for strategy in reporting_strategies: + self._reporting_strategies.append(strategy) + @property def dst_user(self): return self._dst_user @@ -124,7 +130,8 @@ def _process_src_user_dependencies(self): try: handler = class_handler_factory.get_handler( - obj.__class__, self._src_user, self._dst_user, obj) + obj.__class__, self._src_user, self._dst_user, obj, + reporting_strategies=self._reporting_strategies) handler.run() except ValueError: raise UserMergeError( From c3a61c0fcb7716cea4081d94ceb0d890372a7c8a Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 2 Aug 2023 11:37:14 -0700 Subject: [PATCH 09/13] Including pre- and post- logging messages --- .../user/management/commands/merge_users.py | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py index 13b169e40..e54b999a2 100644 --- a/coldfront/core/user/management/commands/merge_users.py +++ b/coldfront/core/user/management/commands/merge_users.py @@ -63,18 +63,30 @@ def handle(self, *args, **options): user_1, user_2, reporting_strategies=reporting_strategies) src_user = user_merge_runner.src_user + src_user_str = ( + f'{src_user.username} ({src_user.pk}, {src_user.first_name} ' + f'{src_user.last_name})') dst_user = user_merge_runner.dst_user - self.stdout.write( - self.style.WARNING( - f'Source: {src_user.username} ({src_user.first_name} ' - f'{src_user.last_name})')) - self.stdout.write( - self.style.WARNING( - f'Destination: {dst_user.username} ({dst_user.first_name} ' - f'{dst_user.last_name})')) + dst_user_str = ( + f'{dst_user.username} ({dst_user.pk}, {dst_user.first_name} ' + f'{dst_user.last_name})') + + self.stdout.write(self.style.WARNING(f'Source: {src_user_str}')) + self.stdout.write(self.style.WARNING(f'Destination: {dst_user_str}')) if dry_run: user_merge_runner.dry_run() else: - user_merge_runner.run() - enqueue_for_logging_strategy.log_queued_messages() + enqueue_for_logging_strategy.warning( + f'Initiating a merge of source User {src_user_str} into ' + f'destination User {dst_user_str}.') + try: + user_merge_runner.run() + except Exception as e: + # TODO + pass + else: + enqueue_for_logging_strategy.success( + f'Successfully merged source User {src_user_str} into ' + f'destination User {dst_user_str}.') + enqueue_for_logging_strategy.log_queued_messages() From 9a06694374ef468cc7f22fdc8cbaf1cf6529d1c9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 2 Aug 2023 12:04:13 -0700 Subject: [PATCH 10/13] Write success messages to stdout --- coldfront/core/user/management/commands/merge_users.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coldfront/core/user/management/commands/merge_users.py b/coldfront/core/user/management/commands/merge_users.py index e54b999a2..9b13c098b 100644 --- a/coldfront/core/user/management/commands/merge_users.py +++ b/coldfront/core/user/management/commands/merge_users.py @@ -76,6 +76,7 @@ def handle(self, *args, **options): if dry_run: user_merge_runner.dry_run() + self.stdout.write(self.style.WARNING('Dry run of merge complete.')) else: enqueue_for_logging_strategy.warning( f'Initiating a merge of source User {src_user_str} into ' @@ -86,6 +87,7 @@ def handle(self, *args, **options): # TODO pass else: + self.stdout.write(self.style.SUCCESS('Merge complete.')) enqueue_for_logging_strategy.success( f'Successfully merged source User {src_user_str} into ' f'destination User {dst_user_str}.') From 76467614d50f32c2ec2872fc7a398c5d7a2f4494 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 2 Aug 2023 12:49:18 -0700 Subject: [PATCH 11/13] Delete objects when processing done so that associated objects are reported correctly --- .../core/user/utils_/merge_users/class_handlers.py | 1 + coldfront/core/user/utils_/merge_users/runner.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/coldfront/core/user/utils_/merge_users/class_handlers.py b/coldfront/core/user/utils_/merge_users/class_handlers.py index 294465768..f4850d058 100644 --- a/coldfront/core/user/utils_/merge_users/class_handlers.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -67,6 +67,7 @@ def run(self): self._run_special_handling() if self._dst_obj: self._dst_obj.save() + self._src_obj.delete() def _get_settable_if_falsy_attrs(self): """Return a list of attributes that, if falsy in the diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py index 9f1ecf4db..a29bb8879 100644 --- a/coldfront/core/user/utils_/merge_users/runner.py +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -1,5 +1,6 @@ from django.contrib.auth.models import User from django.contrib.admin.utils import NestedObjects +from django.core.exceptions import ObjectDoesNotExist from django.db import DEFAULT_DB_ALIAS from django.db import transaction @@ -124,9 +125,14 @@ def _process_src_user_dependencies(self): class_handler_factory = ClassHandlerFactory() - # Block other threads from retrieving this object until the end of - # the transaction. - obj = obj.__class__.objects.select_for_update().get(pk=obj.pk) + try: + # Block other threads from retrieving this object until the end + # of the transaction. + obj = obj.__class__.objects.select_for_update().get(pk=obj.pk) + except ObjectDoesNotExist: + # The object was deleted in a cascading fashion. Process it + # anyway for reporting purposes. + pass try: handler = class_handler_factory.get_handler( From d2e28a55d5268c2b542e20b201b9104f9bb530cd Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 2 Aug 2023 12:55:14 -0700 Subject: [PATCH 12/13] Revert "Delete objects when processing done so that associated objects are reported correctly" This reverts commit 76467614d50f32c2ec2872fc7a398c5d7a2f4494. --- .../core/user/utils_/merge_users/class_handlers.py | 1 - coldfront/core/user/utils_/merge_users/runner.py | 12 +++--------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/coldfront/core/user/utils_/merge_users/class_handlers.py b/coldfront/core/user/utils_/merge_users/class_handlers.py index f4850d058..294465768 100644 --- a/coldfront/core/user/utils_/merge_users/class_handlers.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -67,7 +67,6 @@ def run(self): self._run_special_handling() if self._dst_obj: self._dst_obj.save() - self._src_obj.delete() def _get_settable_if_falsy_attrs(self): """Return a list of attributes that, if falsy in the diff --git a/coldfront/core/user/utils_/merge_users/runner.py b/coldfront/core/user/utils_/merge_users/runner.py index a29bb8879..9f1ecf4db 100644 --- a/coldfront/core/user/utils_/merge_users/runner.py +++ b/coldfront/core/user/utils_/merge_users/runner.py @@ -1,6 +1,5 @@ from django.contrib.auth.models import User from django.contrib.admin.utils import NestedObjects -from django.core.exceptions import ObjectDoesNotExist from django.db import DEFAULT_DB_ALIAS from django.db import transaction @@ -125,14 +124,9 @@ def _process_src_user_dependencies(self): class_handler_factory = ClassHandlerFactory() - try: - # Block other threads from retrieving this object until the end - # of the transaction. - obj = obj.__class__.objects.select_for_update().get(pk=obj.pk) - except ObjectDoesNotExist: - # The object was deleted in a cascading fashion. Process it - # anyway for reporting purposes. - pass + # Block other threads from retrieving this object until the end of + # the transaction. + obj = obj.__class__.objects.select_for_update().get(pk=obj.pk) try: handler = class_handler_factory.get_handler( From 4f98f044d50e643e1524b4a6e2c81acb0fac2d25 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 2 Aug 2023 13:35:22 -0700 Subject: [PATCH 13/13] Determine whether object was transferred vs. deleted on case-by-case basis --- .../user/utils_/merge_users/class_handlers.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/coldfront/core/user/utils_/merge_users/class_handlers.py b/coldfront/core/user/utils_/merge_users/class_handlers.py index 294465768..6e3e36a0a 100644 --- a/coldfront/core/user/utils_/merge_users/class_handlers.py +++ b/coldfront/core/user/utils_/merge_users/class_handlers.py @@ -74,14 +74,12 @@ def _get_settable_if_falsy_attrs(self): attribute in the source object.""" return [] - def _handle_associated_object(self): + def _handle_associated_object(self, transferred=False): """An object B may be associated with a User through a different object A. When A is deleted, B may be deleted with it. When A is transferred, B is transferred with it. Record that this has occurred.""" - try: - self._src_obj.refresh_from_db() - except ObjectDoesNotExist: + if not transferred: # The object was deleted. message = ( f'{self._class_name}({self._src_obj.pk}): indirectly deleted') @@ -233,7 +231,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - self._handle_associated_object() + transferred = self._src_obj.allocation_user.user == self._dst_user + self._handle_associated_object(transferred=transferred) class AllocationUserAttributeUsageHandler(ClassHandler): @@ -242,7 +241,10 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - self._handle_associated_object() + transferred = ( + self._src_obj.allocation_user_attribute.allocation_user.user == + self._dst_user) + self._handle_associated_object(transferred=transferred) class ClusterAccessRequestHandler(ClassHandler): @@ -251,7 +253,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - self._handle_associated_object() + transferred = self._src_obj.allocation_user.user == self._dst_user + self._handle_associated_object(transferred=transferred) class ProjectUserHandler(ClassHandler): @@ -310,7 +313,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def _run_special_handling(self): - self._handle_associated_object() + transferred = self._src_obj.project_user.user == self._dst_user + self._handle_associated_object(transferred=transferred) class SavioProjectAllocationRequestHandler(ClassHandler):