Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(management,merge_attributes): allow target attributes with descendants #1127

Merged
merged 2 commits into from
Aug 20, 2024
Merged

fix(management,merge_attributes): allow target attributes with descendants #1127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c65b7c0
fix(management,merge_attributes): allow target attributes with descen…
MyPyDavid Aug 20, 2024
da935ec
style(management,merge_attributes): update style
MyPyDavid Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 33 additions & 21 deletions rdmo/management/management/commands/merge_attributes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,15 @@ def handle(self, *args, **options):
verbosity = options.get('verbosity', 1)

source = get_valid_attribute(source, message_name='Source')
target = get_valid_attribute(target, message_name='Target')
target = get_valid_attribute(target, message_name='Target', allow_descendants=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is main part of the change


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]
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,
Expand All @@ -87,7 +88,7 @@ def handle(self, *args, **options):
affected_projects_msg = make_affected_projects_message(results)
affected_views_msg = make_affected_views_message(results)
if save_changes:
info_msg = f"Successfully replaced source attribute {source.uri} with {target.uri} on affected elements." # noqa: E501
info_msg = f"Successfully replaced source attribute {source.uri} with {target.uri} on affected elements." # noqa: E501
if update_views:
info_msg += f"\nSuccessfully replaced source attribute {source.uri} in affected View templates."
self.stdout.write(self.style.SUCCESS(info_msg))
Expand All @@ -98,14 +99,13 @@ def handle(self, *args, **options):
))
if not all_instances_were_updated:
self.stdout.write(
self.style.NOTICE(f"Source attribute {source.uri} was deleted without moving affected elements to the target attribute.") # noqa: E501
self.style.NOTICE(f"Source attribute {source.uri} was deleted without moving affected elements to the target attribute.") # noqa: E501
)
if not save_changes and not delete_source:
self.stdout.write(self.style.SUCCESS(
f"Nothing was changed. Displaying affected elements for replacement of source attribute {source.uri} with target {target.uri}." # noqa: E501
f"Nothing was changed. Displaying affected elements for replacement of source attribute {source.uri} with target {target.uri}." # noqa: E501
))


self.stdout.write(self.style.SUCCESS(
f"{affect_elements_msg}\n"
f"{affected_projects_msg}\n"
Expand All @@ -118,23 +118,33 @@ def handle(self, *args, **options):
self.stdout.write(affected_instances_msg)


def get_valid_attribute(attribute, message_name=''):
def get_attribute_from_uri(attribute_uri, message_name=''):
try:
attribute = Attribute.objects.get(uri=attribute_uri)
return attribute
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


def validate_attribute(attribute, message_name='', allow_descendants=None):
if not isinstance(attribute, Attribute):
raise CommandError(f"{message_name} attribute argument should be of type Attribute.")

if not allow_descendants and attribute.get_descendants().exists():
raise CommandError(f"{message_name} attribute '{attribute.uri}' with descendants is not supported.")


def get_valid_attribute(attribute, message_name='', allow_descendants=None):
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.")

if attribute.get_descendants():
raise CommandError(f"{message_name} attribute '{attribute}' with descendants is not supported.")
attribute = get_attribute_from_uri(attribute, message_name=message_name)

validate_attribute(attribute, message_name=message_name, allow_descendants=allow_descendants)

return attribute


def replace_attribute_on_related_model_instances(related_field, source=None, target=None, save_changes=False):
model = related_field.related_model
lookup_field = related_field.remote_field.name
Expand All @@ -156,7 +166,6 @@ def replace_attribute_on_related_model_instances(related_field, source=None, tar
source.uri, model._meta.verbose_name_raw, instance.id, target.uri
)


replacement_results.append({
'model_name': model._meta.verbose_name_raw,
'instance': instance,
Expand Down Expand Up @@ -191,10 +200,12 @@ def replace_attribute_in_view_template(source=None, target=None, save_changes=Fa
})
return replacement_results


def get_affected_projects_from_results(results):
value_results = results.get(Value._meta.verbose_name_raw, [])
return list({i['instance'].project for i in value_results})


def make_affected_elements_counts_message(results):
element_counts = ", ".join([f"{k.capitalize()}({len(v)})" for k, v in results.items() if v])
if not element_counts or not any(results.values()):
Expand All @@ -214,6 +225,7 @@ def make_affected_projects_message(results):
msg += f"\n\t{project.id}\t{project}"
return msg


def make_affected_views_message(results):
view_results = results.get(View._meta.verbose_name_raw, [])
if not view_results:
Expand Down
122 changes: 85 additions & 37 deletions rdmo/management/tests/test_merge_attributes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@

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]
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'
Expand All @@ -27,6 +28,7 @@
EXAMPLE_VIEW_URI = Attribute.build_uri(EXAMPLE_URI_PREFIX, VIEW_TEMPLATE_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):
instance.pk = None
instance.id = None
Expand All @@ -37,13 +39,15 @@ def _prepare_instance_for_copy(instance, uri_prefix=None, uri_path=None):
instance.uri_path = uri_path
return instance


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 create_new_uris_with_uri_prefix_for_template(new_uri_prefix: str) -> List[str]:
new_uris = []
for extra_path in VIEW_TEMPLATE_URI_PATH_ADDITIONS:
Expand All @@ -52,10 +56,8 @@ def create_new_uris_with_uri_prefix_for_template(new_uri_prefix: str) -> List[st
new_uris.append(new_uri)
return new_uris


def create_copy_of_view_that_uses_new_attribute(db, new_prefixes: List[str]):
# TODO search in View.template for the attribute uri
# example_uri = f"{EXAMPLE_URI_PREFIX}/{EXAMPLE_VIEW_URI_PATH}"
# source = Attribute.objects.get(uri=example_uri)
qs = View.objects.filter(**{"uri__contains": EXAMPLE_VIEW_URI_PATH}).all()
if not qs.exists():
raise ValueError("Views for tests should exist here.")
Expand All @@ -72,6 +74,7 @@ def create_copy_of_view_that_uses_new_attribute(db, new_prefixes: List[str]):
instance.template = new_template
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
Expand Down Expand Up @@ -101,8 +104,8 @@ def create_copies_of_attributes_with_new_uri_prefix(example_attributes, new_pref
attribute = _prepare_instance_for_copy(attribute, uri_prefix=new_prefix)
attribute.save()

def get_related_affected_instances(attribute) -> list:

def get_related_affected_instances(attribute) -> list:
related_qs = []
for related_field in ATTRIBUTE_RELATED_MODELS_FIELDS:
model = related_field.related_model
Expand All @@ -121,16 +124,65 @@ def _create_new_merge_attributes_and_views(db, settings):
create_copy_of_view_that_uses_new_attribute(db, NEW_MERGE_URI_PREFIXES)


@pytest.mark.usefixtures("_create_new_merge_attributes_and_views")
def test_command_merge_attributes_fails_correctly(db, settings):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some test for when it should fail because the input source or target was wrong

first_parent_attribute = Attribute.objects.exclude(parent=None).first().parent
first_leaf_attribute = None
for attribute in Attribute.objects.all():
if not attribute.get_descendants().exists():
first_leaf_attribute = attribute
break

source_and_target_are_the_same = {
'source': first_parent_attribute,
'target': first_parent_attribute
}
stdout, stderr = io.StringIO(), io.StringIO()
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **source_and_target_are_the_same)

source_has_descendants = {
'source': first_parent_attribute,
'target': first_leaf_attribute
}
stdout, stderr = io.StringIO(), io.StringIO()
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **source_has_descendants)

source_does_not_exist = {
'source': 'http://uri-does-not-exist-1.com',
'target': first_leaf_attribute
}
stdout, stderr = io.StringIO(), io.StringIO()
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **source_does_not_exist)

target_does_not_exist = {
'source': first_leaf_attribute,
'target': 'http://uri-does-not-exist-1.com',
}
stdout, stderr = io.StringIO(), io.StringIO()
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **target_does_not_exist)


@pytest.mark.parametrize('uri_prefix', NEW_MERGE_URI_PREFIXES)
@pytest.mark.usefixtures("_create_new_merge_attributes_and_views")
def test_that_the_freshly_created_merge_attributes_are_present(db, uri_prefix):
merge_attributes = Attribute.objects.filter(uri_prefix=uri_prefix).all()
assert len(merge_attributes) > 2
merge_attributes_uris = Attribute.objects.filter(
uri_prefix=uri_prefix).all().values_list(
'uri', flat=True).distinct()
assert len(merge_attributes_uris) > 2
unique_uri_prefixes = set(Attribute.objects.values_list("uri_prefix", flat=True))
# test that the currently selected uri_prefix is in db
assert uri_prefix in unique_uri_prefixes

for attribute in merge_attributes:
for attribute_uri in merge_attributes_uris:
attribute = Attribute.objects.get(uri=attribute_uri)
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 ATTRIBUTE_RELATED_MODELS_FIELDS]
if not any(len(i) > 0 for i in original_models_qs):
Expand All @@ -157,45 +209,43 @@ def test_that_the_freshly_created_merge_attributes_are_present(db, uri_prefix):
@pytest.mark.parametrize('view', [False, True])
@pytest.mark.usefixtures("_create_new_merge_attributes_and_views")
def test_command_merge_attributes(db, settings, source_uri_prefix, save, delete, view):
source_attributes = Attribute.objects.filter(uri_prefix=source_uri_prefix).all()
assert len(source_attributes) > 2
source_attribute_uris = Attribute.objects.filter(
uri_prefix=source_uri_prefix).all().values_list(
'uri', flat=True).distinct()
assert len(source_attribute_uris) > 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:
for source_attribute_uri in source_attribute_uris:
source_attribute = Attribute.objects.get(uri=source_attribute_uri)
target_attribute = Attribute.objects.get(uri_prefix=EXAMPLE_URI_PREFIX, path=source_attribute.path)
stdout, stderr = io.StringIO(), io.StringIO()
before_source_related_qs = get_related_affected_instances(source_attribute)
# before_target_related_qs = get_related_affected_instances(target_attribute)

command_kwargs = {'source': source_attribute.uri,
'target': target_attribute.uri,
'save': save, 'delete': delete, 'view': view}
failed = False

if source_attribute.get_descendants():
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **command_kwargs)
failed = True
elif target_attribute.get_descendants():
stdout, stderr = io.StringIO(), io.StringIO()
with pytest.raises(CommandError):
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **command_kwargs)
failed = True
else:
stdout, stderr = io.StringIO(), io.StringIO()
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **command_kwargs)


if delete and not failed:
# assert that the source attribute was deleted
with pytest.raises(Attribute.DoesNotExist):
Attribute.objects.get(id=source_attribute.id)
else:
assert Attribute.objects.get(id=source_attribute.id)


after_source_related_qs = get_related_affected_instances(source_attribute)
after_target_related_qs = get_related_affected_instances(target_attribute)

Expand Down Expand Up @@ -233,7 +283,7 @@ def test_command_merge_attributes_for_views(db, settings, source_uri_prefix, sav
'save': save, 'delete': delete, 'view': view}
failed = False
call_command('merge_attributes',
stdout=stdout, stderr=stderr, **command_kwargs)
stdout=stdout, stderr=stderr, **command_kwargs)

if delete and not failed:
# assert that the source attribute was deleted
Expand All @@ -252,21 +302,19 @@ def test_command_merge_attributes_for_views(db, settings, source_uri_prefix, sav
if save and not failed:
pass


if (save and not failed and view
and VIEW_TEMPLATE_URI_PATH in source_attribute_uri):
# assert that the attribute in the view template was replaced as well
# the EXAMPLE_VIEW_URI is from the target attribute
# uri_prefix = source_uri_prefix, uri_path = EXAMPLE_VIEW_URI_PATH
assert not after_source_related_view_uri_qs.exists()
assert after_target_related_view_uri_qs.exists()

if (before_source_related_view_uri_templates and
source_attribute_uri != target_attribute_uri):

after_occurrences = list(filter(lambda x:target_attribute_uri in x,
after_target_related_view_uri_qs.first().template.splitlines()))
assert len(after_occurrences) == 1
before_occurrences = list(filter(lambda x: target_attribute_uri in x,
before_source_related_view_uri_qs.first().template.splitlines()))
assert len(before_occurrences) == 0
# assert that the attribute in the view template was replaced as well
# the EXAMPLE_VIEW_URI is from the target attribute
# uri_prefix = source_uri_prefix, uri_path = EXAMPLE_VIEW_URI_PATH
assert not after_source_related_view_uri_qs.exists()
assert after_target_related_view_uri_qs.exists()

if (before_source_related_view_uri_templates and
source_attribute_uri != target_attribute_uri):
after_occurrences = list(filter(lambda x: target_attribute_uri in x,
after_target_related_view_uri_qs.first().template.splitlines()))
assert len(after_occurrences) == 1
before_occurrences = list(filter(lambda x: target_attribute_uri in x,
before_source_related_view_uri_qs.first().template.splitlines()))
assert len(before_occurrences) == 0