From 4c245899a0bf63c43a703d55aa6e1fafed841e86 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Tue, 21 May 2024 12:12:20 +0200 Subject: [PATCH] chore: refactor merge attributes command for #990 Signed-off-by: David Wallace --- .../management/commands/merge_attributes.py | 270 ++++++++---------- .../management/tests/test_merge_attributes.py | 110 +++---- rdmo/management/utils.py | 13 +- 3 files changed, 163 insertions(+), 230 deletions(-) diff --git a/rdmo/management/management/commands/merge_attributes.py b/rdmo/management/management/commands/merge_attributes.py index 711d4ca63c..b8851c0554 100644 --- a/rdmo/management/management/commands/merge_attributes.py +++ b/rdmo/management/management/commands/merge_attributes.py @@ -1,99 +1,158 @@ import logging -from collections import namedtuple from textwrap import dedent from django.core.management.base import BaseCommand, CommandError -from django.utils.translation import gettext_lazy as _ -from rdmo.conditions.models import Condition from rdmo.domain.models import Attribute from rdmo.management.utils import replace_uri_in_template_string from rdmo.projects.models import Value -from rdmo.questions.models import Page, Question, QuestionSet -from rdmo.tasks.models import Task from rdmo.views.models import View logger = logging.getLogger(__name__) -ModelHelper = namedtuple('ModelHelper', ['model', 'lookup_field', 'field_introspection', 'replacement_function']) - -REPLACE_ATTRIBUTE_MODEL_HELPERS = [ - ModelHelper(Page, 'attribute', False, None), - ModelHelper(QuestionSet, 'attribute', False, None), - ModelHelper(Question, 'attribute', False, None), - ModelHelper(Condition, 'source', False, None), - ModelHelper(Task, 'start_attribute', False, None), - ModelHelper(Task, 'end_attribute', False, None), - ModelHelper(Value, 'attribute', False, None), - ModelHelper(View, 'template', True, replace_uri_in_template_string) -] - +class Command(BaseCommand): + help = 'Replace an attribute with another attribute across multiple models' -def get_queryset(model, lookup_field, field_introspection, *_, attribute=None): - if attribute is None: - raise ValueError("attribute is required") - if field_introspection: - return model.objects.filter(**{f"{lookup_field}__contains": attribute.path}) - return model.objects.filter(**{lookup_field: attribute}) + def add_arguments(self, parser): + parser.add_argument( + '--source', + required=True, + help='The URI of the source attribute that will be replaced by the target and will be deleted' + ) + parser.add_argument( + '--target', + required=True, + help='The URI of the target attribute that will be used to replace the source' + ) + parser.add_argument( + '--save', + action='store_true', + help='''If specified, the changes will be saved and the source attribute will be deleted.\ + If not specified, the command will do a dry run.''' + ) + def handle(self, *args, **options): + source = options['source'] + target = options['target'] + save = options['save'] + verbosity = options.get('verbosity', 1) -def replace_attribute_on_instance(instance, lookup_field, source, target, field_introspection, replacement_function): - instance_field = getattr(instance, lookup_field) + source = get_valid_attribute(source, message_name='Source') + target = get_valid_attribute(target, message_name='Target') + + if source == target: + raise CommandError("Source and Target attribute are the same, nothing to do.") + + results = {} + related_model_fields = [i for i in Attribute._meta.get_fields() \ + if i.is_relation and not i.many_to_many and i.related_model is not Attribute] + for related_field in related_model_fields: + replaced_model_result = replace_attribute_on_related_model_instances( + related_field, + source=source, + target=target, + save=save + ) + results[related_field.related_model._meta.verbose_name_raw] = replaced_model_result + + view_template_result = replace_attribute_in_view_template(source=source, target=target, save=save) + results[View._meta.verbose_name_raw] = view_template_result + + if save and all(a['saved'] for i in results.values() for a in i): + try: + source.delete() + except source.DoesNotExist: + pass + + if verbosity >= 1: + if verbosity >= 2: + affected_instances_msg = make_affected_instances_message(results) + if affected_instances_msg: + self.stdout.write(affected_instances_msg) + affect_elements_msg = make_elements_message(results) + affected_projects_msg = make_affected_projects_message(results) + if save: + self.stdout.write(self.style.SUCCESS( + f"Successfully replaced source attribute {source.uri} with {target.uri}.\n" + f"Source attribute {source.uri} was deleted.\n" + f"{affect_elements_msg}\n" + f"{affected_projects_msg}" + )) + else: + self.stdout.write(self.style.SUCCESS( + f"Source attribute {source.uri} can be replaced with {target.uri}.\n" + f"{affect_elements_msg}\n" + f"{affected_projects_msg}" + )) - if isinstance(instance_field, Attribute) and instance_field != source: - raise ValueError("Instance attribute should be equal to the source attribute") - if not isinstance(instance_field, Attribute) and not field_introspection: - raise ValueError("When instance field is not an attribute, it should use field introspection") - if field_introspection: - replacement_field = replacement_function(instance_field, source, target) - else: - replacement_field = target +def get_valid_attribute(attribute, message_name=''): + if isinstance(attribute, str): + attribute_uri = attribute + try: + attribute = Attribute.objects.get(uri=attribute_uri) + except Attribute.DoesNotExist as e: + raise CommandError(f"{message_name} attribute {attribute_uri} does not exist.") from e + except Attribute.MultipleObjectsReturned as e: + raise CommandError(f"{message_name} attribute {attribute_uri} returns multiple objects.") from e + elif not isinstance(attribute, Attribute): + raise CommandError(f"{message_name} argument should be of type Attribute.") - setattr(instance, lookup_field, replacement_field) - return instance + if attribute.get_descendants(): + raise CommandError(f"{message_name} attribute '{attribute}' with descendants is not supported.") + return attribute -def replace_attribute_on_element_instances(model, - lookup_field, - field_introspection, - replacement_function, +def replace_attribute_on_related_model_instances(related_field, source=None, target=None, save=False ): - qs = get_queryset(model, lookup_field, field_introspection, attribute=source) - replaced_attributes = [] + model = related_field.related_model + lookup_field = related_field.remote_field.name + qs = model.objects.filter(**{lookup_field: source}) + + replacement_results = [] for instance in qs: - instance = replace_attribute_on_instance(instance, lookup_field, source, target, field_introspection, - replacement_function) + instance_source = getattr(instance, lookup_field) + + if isinstance(instance_source, Attribute) and instance_source != source: + raise CommandError("Instance attribute should be equal to the source attribute") + + setattr(instance, lookup_field, target) + if save: instance.save() - replaced_attributes.append({ + replacement_results.append({ 'model_name': model._meta.verbose_name_raw, 'instance': instance, 'source': source, 'target': target, 'saved': save, - 'source_exists': get_queryset(model, lookup_field, field_introspection, attribute=source).exists() }) - return replaced_attributes + return replacement_results -def get_valid_attribute(attribute, message_name=''): - if isinstance(attribute, str): - attribute_uri = attribute - try: - attribute = Attribute.objects.get(uri=attribute_uri) - except Attribute.DoesNotExist as e: - raise ValueError(f"{message_name} attribute {attribute_uri} does not exist.") from e - except Attribute.MultipleObjectsReturned as e: - raise ValueError(f"{message_name} attribute {attribute_uri} returns multiple objects.") from e - elif not isinstance(attribute, Attribute): - raise ValueError(f"{message_name} argument should be of type Attribute.") +def replace_attribute_in_view_template(source=None, target=None, save=False ): + qs = View.objects.filter(**{"template__contains": source.path}) + replacement_results = [] + for instance in qs: + + template_target = replace_uri_in_template_string(instance.template, source, target) + instance.template = template_target + + if save: + instance.save() + replacement_results.append({ + 'model_name': View._meta.verbose_name_raw, + 'instance': instance, + 'source': source, + 'target': target, + 'saved': save, + }) + return replacement_results - return attribute def make_elements_message(results): element_counts = ", ".join([f"{k.capitalize()}({len(v)})" for k, v in results.items() if v]) @@ -126,7 +185,6 @@ def make_affected_instances_message(results): Model={result['model_name']} instance={result['instance']} source={result['source'].uri} - source_exists={result['source_exists']} target={result['target'].uri} saved={result['saved']} ''') @@ -134,95 +192,3 @@ def make_affected_instances_message(results): msg += f"\nproject={result['instance'].project}" msg += '\n' return msg - - -def replace_attribute_on_elements(source, target, model_helpers_with_attributes, save): - - if source.get_descendants(): - raise ValueError(f"Source attribute '{source}' with descendants is not supported.") - if target.get_descendants(): - raise ValueError(f"Target attribute '{target}' with descendants is not supported.") - if source == target: - raise ValueError("Source and Target attribute are the same.") - - results = {} - for model, lookup_field, field_introspection, replacement_function in model_helpers_with_attributes: - replaced_attributes = replace_attribute_on_element_instances( - model, - lookup_field, - field_introspection, - replacement_function, - source=source, - target=target, - save=save - ) - results[model._meta.verbose_name_raw] = replaced_attributes - - if save and all(a['saved'] for i in results.values() for a in i): - try: - source.delete() - except source.DoesNotExist: - pass - return results - - -def main(source, target, save=False): - return replace_attribute_on_elements(source, target, REPLACE_ATTRIBUTE_MODEL_HELPERS, save) - - -class Command(BaseCommand): - help = 'Replace an attribute with another attribute across multiple models' - - def add_arguments(self, parser): - parser.add_argument( - '--source', - required=True, - help='The URI of the source attribute that will be replaced by the target and will be deleted' - ) - parser.add_argument( - '--target', - required=True, - help='The URI of the target attribute that will be used to replace the source' - ) - parser.add_argument( - '--save', - action='store_true', - help='''If specified, the changes will be saved and the source attribute will be deleted.\ - If not specified, the command will do a dry run.''' - ) - - def handle(self, *args, **options): - source = options['source'] - target = options['target'] - save = options['save'] - verbosity = options.get('verbosity', 1) - - try: - source = get_valid_attribute(source, message_name='Source') - target = get_valid_attribute(target, message_name='Target') - results = main(source, target, save) - - if verbosity >= 1: - if verbosity >= 2: - affected_instances_msg = make_affected_instances_message(results) - if affected_instances_msg: - self.stdout.write(affected_instances_msg) - affect_elements_msg = make_elements_message(results) - affected_projects_msg = make_affected_projects_message(results) - if save: - self.stdout.write(self.style.SUCCESS( - f"Successfully replaced source attribute {source.uri} with {target.uri}.\n" - f"Source attribute {source.uri} was deleted.\n" - f"{affect_elements_msg}\n" - f"{affected_projects_msg}" - )) - else: - self.stdout.write(self.style.SUCCESS( - f"Source attribute {source.uri} can be replaced with {target.uri}.\n" - f"{affect_elements_msg}\n" - f"{affected_projects_msg}" - )) - except ValueError as e: - raise CommandError(e) from e - except Exception as e: - raise CommandError(_('There was an unknown error in calling the command. %s') % e) from e diff --git a/rdmo/management/tests/test_merge_attributes.py b/rdmo/management/tests/test_merge_attributes.py index b20f8bbf15..e0e59e56ee 100644 --- a/rdmo/management/tests/test_merge_attributes.py +++ b/rdmo/management/tests/test_merge_attributes.py @@ -8,13 +8,14 @@ from rdmo.conditions.models import Condition from rdmo.domain.models import Attribute -from rdmo.management.management.commands.merge_attributes import REPLACE_ATTRIBUTE_MODEL_HELPERS, get_queryset, main from rdmo.options.models import Option from rdmo.questions.models import Page, Question, QuestionSet, Section from rdmo.views.models import View ElementType = Union[Section, Page, QuestionSet, Question, Option, Condition] +ATTRIBUTE_RELATED_MODELS_FIELDS = [i for i in Attribute._meta.get_fields() \ + if i.is_relation and not i.many_to_many and i.related_model is not Attribute] EXAMPLE_URI_PREFIX = 'http://example.com/terms' foo_merge_uri_prefix = 'http://foo-merge.com/terms' @@ -23,8 +24,6 @@ merge_view_template_addition_uri_path = 'individual/single/textarea' merge_view_template_addition = Template("{% render_value '$new_uri' %}") EXAMPLE_VIEW_URI = Attribute.build_uri(EXAMPLE_URI_PREFIX, merge_view_template_addition_uri_path) - - new_merge_uri_prefixes = [foo_merge_uri_prefix, bar_merge_uri_prefix] def _prepare_instance_for_copy(instance, uri_prefix=None, uri_path=None): @@ -37,35 +36,39 @@ def _prepare_instance_for_copy(instance, uri_prefix=None, uri_path=None): instance.uri_path = uri_path return instance -def _add_new_line_to_view_template(model_helper, instance: View, new_uri_prefix: str) -> View: - current_field_value = getattr(instance, model_helper.lookup_field) +def _get_queryset(related_field, attribute=None): + model = related_field.related_model + if model is View: + return model.objects.filter(**{"template__contains": attribute.path}) + lookup_field = related_field.remote_field.name + return model.objects.filter(**{lookup_field: attribute}) + +def _add_new_line_to_view_template(instance: View, new_uri_prefix: str) -> View: + current_field_value = instance.template new_field_value = current_field_value + "\n" new_uri = Attribute.build_uri(new_uri_prefix, merge_view_template_addition_uri_path) new_field_value += merge_view_template_addition.substitute(new_uri=new_uri) - setattr(instance, model_helper.lookup_field, new_field_value) + instance.template = new_field_value return instance -def _create_copy_of_view_that_uses_new_attribute(model_helper, new_prefixes: List[str]): +def create_copy_of_view_that_uses_new_attribute(new_prefixes: List[str]): # TODO search in View.template for the attribute uri - example_uri = f"{EXAMPLE_URI_PREFIX}/{EXAMPLE_VIEW_URI_PATH}" - instance = model_helper.model.objects.get(uri=example_uri) + # example_uri = f"{EXAMPLE_URI_PREFIX}/{EXAMPLE_VIEW_URI_PATH}" + # source = Attribute.objects.get(uri=example_uri) + qs = View.objects.filter(**{"template__contains": EXAMPLE_VIEW_URI_PATH}) for new_prefix in new_prefixes: - instance = _prepare_instance_for_copy(instance, uri_prefix=new_prefix) - instance.uri_path = EXAMPLE_VIEW_URI_PATH - _add_new_line_to_view_template(model_helper, instance, new_prefix) - instance.save() - -def _create_copies_of_elements_with_new_uri_prefix(new_prefixes): - for model_helper in REPLACE_ATTRIBUTE_MODEL_HELPERS: - if model_helper.field_introspection and model_helper.model == View: - _create_copy_of_view_that_uses_new_attribute(model_helper, new_prefixes) - continue # skip this model_helper - elif model_helper.field_introspection: - continue # skip this model_helper - else: - # create new model instances from example.com objects with the new uri_prefix - filter_kwargs = {f"{model_helper.lookup_field}__uri_prefix": EXAMPLE_URI_PREFIX} - example_objects = model_helper.model.objects.filter(**filter_kwargs) + for instance in qs: + instance = _prepare_instance_for_copy(instance) + instance = _add_new_line_to_view_template(instance, new_prefix) + instance.save() + +def create_copies_of_related_models_with_new_uri_prefix(new_prefixes): + for related_model_field in ATTRIBUTE_RELATED_MODELS_FIELDS: + model = related_model_field.related_model + lookup_field = related_model_field.remote_field.name + # create new model instances from example.com objects with the new uri_prefix + filter_kwargs = {f"{lookup_field}__uri_prefix": EXAMPLE_URI_PREFIX} + example_objects = model.objects.filter(**filter_kwargs) for new_prefix in new_prefixes: @@ -73,15 +76,16 @@ def _create_copies_of_elements_with_new_uri_prefix(new_prefixes): continue for instance in example_objects: instance = _prepare_instance_for_copy(instance, uri_prefix=new_prefix) - current_attribute = getattr(instance, model_helper.lookup_field) + current_attribute = getattr(instance, lookup_field) if not isinstance(current_attribute, Attribute): continue filter_kwargs = {'path': current_attribute.path, 'uri_prefix': new_prefix} new_attribute = Attribute.objects.filter(**filter_kwargs).first() - setattr(instance, model_helper.lookup_field, new_attribute) + setattr(instance, lookup_field, new_attribute) instance.save() -def _create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_prefixes): + +def create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_prefixes): for attribute in example_attributes: for new_prefix in new_prefixes: attribute = _prepare_instance_for_copy(attribute, uri_prefix=new_prefix) @@ -92,8 +96,9 @@ def _create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_pre def create_merge_attributes(db, settings): """ Creates model instances for merge attributes tests """ example_attributes = Attribute.objects.filter(uri_prefix=EXAMPLE_URI_PREFIX).all() - _create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_merge_uri_prefixes) - _create_copies_of_elements_with_new_uri_prefix(new_merge_uri_prefixes) + create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_merge_uri_prefixes) + create_copies_of_related_models_with_new_uri_prefix(new_merge_uri_prefixes) + create_copy_of_view_that_uses_new_attribute(new_merge_uri_prefixes) @pytest.mark.parametrize('uri_prefix', new_merge_uri_prefixes) @@ -106,46 +111,12 @@ def test_that_the_freshly_created_merge_attributes_are_present(create_merge_attr for attribute in merge_attributes: original_attribute = Attribute.objects.get(uri_prefix=EXAMPLE_URI_PREFIX, path=attribute.path) - original_models_qs = [get_queryset(*i, attribute=original_attribute) for i in REPLACE_ATTRIBUTE_MODEL_HELPERS] + original_models_qs = [_get_queryset(i, attribute=original_attribute) for i in ATTRIBUTE_RELATED_MODELS_FIELDS] if not any(len(i) > 0 for i in original_models_qs): continue # skip this attribute - models_qs = [get_queryset(*i, attribute=attribute) for i in REPLACE_ATTRIBUTE_MODEL_HELPERS] + models_qs = [_get_queryset(i, attribute=attribute) for i in ATTRIBUTE_RELATED_MODELS_FIELDS] assert any(len(i) > 0 for i in models_qs) -@pytest.mark.parametrize('source_uri_prefix', new_merge_uri_prefixes) -@pytest.mark.parametrize('save', [False, True]) -def test_replacement_of_attributes_on_elements(create_merge_attributes, source_uri_prefix, save): - source_attributes = Attribute.objects.filter(uri_prefix=source_uri_prefix).all() - assert len(source_attributes) > 2 - unique_uri_prefixes = set(Attribute.objects.values_list("uri_prefix", flat=True)) - # test that the currently selected uri_prefix is in db - assert source_uri_prefix in unique_uri_prefixes - - for source_attribute in source_attributes: - target_attribute = Attribute.objects.get(uri_prefix=EXAMPLE_URI_PREFIX, path=source_attribute.path) - failed, results = False, [] - if source_attribute.get_descendants() or target_attribute.get_descendants(): - with pytest.raises(ValueError): - results = main(source_attribute, target_attribute, save=save) - failed = True - else: - results = main(source_attribute, target_attribute, save=save) - if not results: - continue - - if save and not failed: - # assert that the source attribut was deleted - with pytest.raises(Attribute.DoesNotExist): - Attribute.objects.get(id=source_attribute.id) - - if merge_view_template_addition_uri_path in source_attribute.path: - # assert that the attribute in the view template was replaced as well - view = View.objects.get(uri_prefix=source_uri_prefix, uri_path=EXAMPLE_VIEW_URI_PATH) - assert any(EXAMPLE_VIEW_URI in i for i in view.template.splitlines()) - assert not any(source_attribute.uri in i for i in view.template.splitlines()) - else: - assert Attribute.objects.get(id=source_attribute.id) - @pytest.mark.parametrize('source_uri_prefix', new_merge_uri_prefixes) @pytest.mark.parametrize('save', [False, True]) @@ -185,8 +156,9 @@ def test_command_merge_attributes(create_merge_attributes, source_uri_prefix, sa if source_attribute.path in merge_view_template_addition_uri_path: # assert that the attribute in the view template was replaced as well - view = View.objects.get(uri_prefix=source_uri_prefix, uri_path=EXAMPLE_VIEW_URI_PATH) - assert any(EXAMPLE_VIEW_URI in i for i in view.template.splitlines()) - assert not any(source_attribute.uri in i for i in view.template.splitlines()) + # uri_prefix = source_uri_prefix, uri_path = EXAMPLE_VIEW_URI_PATH + for instance in View.objects.filter(**{"template__contains": EXAMPLE_VIEW_URI_PATH}): + assert any(EXAMPLE_VIEW_URI in i for i in instance.template.splitlines()) + assert not any(source_attribute.uri in i for i in instance.template.splitlines()) else: assert Attribute.objects.get(id=source_attribute.id) diff --git a/rdmo/management/utils.py b/rdmo/management/utils.py index e0a0f1e372..223678cf1b 100644 --- a/rdmo/management/utils.py +++ b/rdmo/management/utils.py @@ -2,20 +2,15 @@ def replace_uri_in_template_string(template: str, source: Attribute, target: Attribute) -> str: - result = [] + new_lines = [] path_changed = source.path != target.path - for n, line in enumerate(template.splitlines()): + for line in template.splitlines(): new_line = line if source.uri in line: new_line = new_line.replace(source.uri, target.uri) if path_changed: if source.path in new_line: new_line = new_line.replace(source.path, target.path) - line_result = { - 'line_no': n, - 'new_line': new_line, - 'original_line': line - } - result.append(line_result) - new_template = "\n".join(i['new_line'] for i in result) + new_lines.append(new_line) + new_template = "\n".join(new_lines) return new_template