diff --git a/geniza/entities/admin.py b/geniza/entities/admin.py index 307595445..c08a0f684 100644 --- a/geniza/entities/admin.py +++ b/geniza/entities/admin.py @@ -50,7 +50,7 @@ PlacePlaceRelation, PlacePlaceRelationType, ) -from geniza.entities.views import PersonMerge +from geniza.entities.views import PersonDocumentRelationTypeMerge, PersonMerge from geniza.footnotes.models import Footnote @@ -417,6 +417,43 @@ class PersonDocumentRelationTypeAdmin(TabbedTranslationAdmin, admin.ModelAdmin): search_fields = ("name",) ordering = ("name",) + @admin.display(description="Merge selected Person-Document relationships") + def merge_person_document_relation_types(self, request, queryset=None): + """Admin action to merge selected person-document relation types. This + action redirects to an intermediate page, which displays a form to + review for confirmation and choose the primary type before merging. + """ + selected = request.POST.getlist("_selected_action") + if len(selected) < 2: + messages.error( + request, + "You must select at least two person-document relationships to merge", + ) + return HttpResponseRedirect( + reverse("admin:entities_persondocumentrelationtype_changelist") + ) + return HttpResponseRedirect( + "%s?ids=%s" + % ( + reverse("admin:person-document-relation-type-merge"), + ",".join(selected), + ), + status=303, + ) # status code 303 means "See Other" + + def get_urls(self): + """Return admin urls; adds custom URL for merging""" + urls = [ + path( + "merge/", + PersonDocumentRelationTypeMerge.as_view(), + name="person-document-relation-type-merge", + ), + ] + return urls + super().get_urls() + + actions = (merge_person_document_relation_types,) + @admin.register(PersonPersonRelationType) class PersonPersonRelationTypeAdmin(TabbedTranslationAdmin, admin.ModelAdmin): diff --git a/geniza/entities/forms.py b/geniza/entities/forms.py index 86e7be2f1..c9ac5f253 100644 --- a/geniza/entities/forms.py +++ b/geniza/entities/forms.py @@ -54,6 +54,45 @@ def __init__(self, *args, **kwargs): ) +class PersonDocumentRelationTypeChoiceField(forms.ModelChoiceField): + """Add a summary of each PersonDocumentRelationType to a form (used for merging)""" + + label_template = get_template( + "entities/snippets/persondocumentrelationtype_option_label.html" + ) + + def label_from_instance(self, relation_type): + return self.label_template.render({"relation_type": relation_type}) + + +class PersonDocumentRelationTypeMergeForm(forms.Form): + primary_relation_type = PersonDocumentRelationTypeChoiceField( + label="Select primary person-document relationship", + queryset=None, + help_text=( + "Select the primary person-document relationship, which will be " + "used as the canonical entry. All associated relations and log " + "entries will be combined on the primary relationship." + ), + empty_label=None, + widget=forms.RadioSelect, + ) + + def __init__(self, *args, **kwargs): + ids = kwargs.get("ids", []) + + # Remove the added kwarg so that the super method doesn't error + try: + del kwargs["ids"] + except KeyError: + pass + + super().__init__(*args, **kwargs) + self.fields[ + "primary_relation_type" + ].queryset = PersonDocumentRelationType.objects.filter(id__in=ids) + + class PersonPersonForm(forms.ModelForm): class Meta: model = PersonPersonRelation diff --git a/geniza/entities/models.py b/geniza/entities/models.py index 73cedf4c3..c051a70c7 100644 --- a/geniza/entities/models.py +++ b/geniza/entities/models.py @@ -11,7 +11,7 @@ from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.contenttypes.models import ContentType from django.core.validators import RegexValidator -from django.db import models +from django.db import IntegrityError, models, transaction from django.db.models import F, Q, Value from django.db.models.query import Prefetch from django.forms import ValidationError @@ -1025,6 +1025,9 @@ class PersonDocumentRelationType(models.Model): name = models.CharField(max_length=255, unique=True) objects = PersonDocumentRelationTypeManager() + log_entries = GenericRelation( + LogEntry, related_query_name="persondocumentrelationtype" + ) class Meta: verbose_name = "Person-Document relationship" @@ -1041,6 +1044,67 @@ def objects_by_label(cls): for obj in cls.objects.all() } + def merge_with(self, merge_relation_types, user=None): + """Merge the specified relation types into this one. Combines all + relationships into this relation type and creates a log entry + documenting the merge. + + Closely adapted from :class:`Person` merge.""" + + # if user is not specified, log entry will be associated with script + if user is None: + user = User.objects.get(username=settings.SCRIPT_USERNAME) + + for rel_type in merge_relation_types: + # combine log entries + for log_entry in rel_type.log_entries.all(): + # annotate and reassociate + # - modify change message to type which object this event applied to + log_entry.change_message = "%s [merged type %s (id = %d)]" % ( + log_entry.get_change_message(), + str(rel_type), + rel_type.pk, + ) + + # - associate with the primary relation type + log_entry.object_id = self.id + log_entry.content_type_id = ContentType.objects.get_for_model( + PersonDocumentRelationType + ) + log_entry.save() + + # combine person-document relationships + for relationship in rel_type.persondocumentrelation_set.all(): + # set type of each relationship to primary relation type + relationship.type = self + # handle unique constraint violation (one relationship per type + # between doc and person): only reassign type if it doesn't + # create a duplicate, otherwise delete. + # see https://docs.djangoproject.com/en/3.2/topics/db/transactions/#django.db.transaction.atomic + try: + with transaction.atomic(): + relationship.save() + except IntegrityError: + relationship.delete() + + # save current relation type with changes; delete merged relation types + self.save() + merged_types = ", ".join([str(rel_type) for rel_type in merge_relation_types]) + for rel_type in merge_relation_types: + rel_type.delete() + # create log entry documenting the merge; include rationale + pdrtype_contenttype = ContentType.objects.get_for_model( + PersonDocumentRelationType + ) + LogEntry.objects.log_action( + user_id=user.id, + content_type_id=pdrtype_contenttype.pk, + object_id=self.pk, + object_repr=str(self), + change_message="merged with %s" % (merged_types,), + action_flag=CHANGE, + ) + class PersonDocumentRelation(models.Model): """A relationship between a person and a document.""" diff --git a/geniza/entities/templates/admin/entities/persondocumentrelationtype/merge.html b/geniza/entities/templates/admin/entities/persondocumentrelationtype/merge.html new file mode 100644 index 000000000..e1bd360c8 --- /dev/null +++ b/geniza/entities/templates/admin/entities/persondocumentrelationtype/merge.html @@ -0,0 +1,53 @@ +{% extends 'admin/base_site.html' %} + +{% load admin_urls static %} + +{% block extrastyle %} + {{ block.super }} + + +{% endblock %} + +{% block title %} Merge selected person-document relationships {% endblock %} + +{% block breadcrumbs %} + {% if not is_popup %} +
+ Home + › + Entities + › + Person-Document relationships + › + Merge selected person-document relationships +
+ {% endif %} +{% endblock %} + + +{% block content_title %} +

Merge selected person-document relationships

+

Note: there is no automated way to unmerge! Please review to make sure these relationships should be merged before submitting the form.

+{% endblock %} + +{% block content %} +
+ {% csrf_token %} + {% if form.errors|length > 0 %} +

+ Please correct the error below. +

+ {% endif %} +
+
+ {{ form.primary_relation_type.label_tag }} + {{ form.primary_relation_type }} +

{{ form.primary_relation_type.help_text|safe }}

+
+ +
+ +
+ + +{% endblock %} diff --git a/geniza/entities/templates/entities/snippets/persondocumentrelationtype_option_label.html b/geniza/entities/templates/entities/snippets/persondocumentrelationtype_option_label.html new file mode 100644 index 000000000..c691ea08b --- /dev/null +++ b/geniza/entities/templates/entities/snippets/persondocumentrelationtype_option_label.html @@ -0,0 +1,33 @@ +{# template snippet for displaying person-document relationship label on a form #} +{# used on merge form to provide enough information to merge accurately #} + +
+

{{ relation_type }} + + Change +

+
+ View all {{ relation_type.persondocumentrelation_set.count }} {{ relation_type }} relations +
    + {% for rel in relation_type.persondocumentrelation_set.all %} +
  1. +
    + + +
    +
    2. + {% empty %} +
  2. +
    +
    +
    +
    3. + {% endfor %} +
+
+
diff --git a/geniza/entities/tests/test_entities_forms.py b/geniza/entities/tests/test_entities_forms.py index 44e2895e8..196840730 100644 --- a/geniza/entities/tests/test_entities_forms.py +++ b/geniza/entities/tests/test_entities_forms.py @@ -6,11 +6,12 @@ from geniza.corpus.forms import FacetChoiceField from geniza.entities.forms import ( PersonChoiceField, + PersonDocumentRelationTypeMergeForm, PersonListForm, PersonMergeForm, PlaceListForm, ) -from geniza.entities.models import Name, Person, PersonRole +from geniza.entities.models import Name, Person, PersonDocumentRelationType, PersonRole class TestPersonChoiceField: @@ -51,6 +52,31 @@ def test_init(self): assert people.last() not in mergeform.fields["primary_person"].queryset +class TestPersonDocumentRelationTypeMergeForm: + @pytest.mark.django_db + def test_init(self): + # adapted from TestPersonMergeForm + + # no error if ids not specified + PersonDocumentRelationTypeMergeForm() + + # create test records + PersonDocumentRelationType.objects.bulk_create( + [PersonDocumentRelationType(name=f"test{i}") for i in range(4)] + ) + # initialize with ids for all but the last + types = PersonDocumentRelationType.objects.all().order_by("pk") + ids = list(types.values_list("id", flat=True)) + mergeform = PersonDocumentRelationTypeMergeForm(ids=ids[:-1]) + # total should have all but one type + assert ( + mergeform.fields["primary_relation_type"].queryset.count() + == types.count() - 1 + ) + # last type should not be an available choice + assert types.last() not in mergeform.fields["primary_relation_type"].queryset + + @pytest.mark.django_db class TestPersonListForm: def test_set_choices_from_facets(self, person, person_diacritic): diff --git a/geniza/entities/tests/test_entities_models.py b/geniza/entities/tests/test_entities_models.py index aaed8df00..23ae26eae 100644 --- a/geniza/entities/tests/test_entities_models.py +++ b/geniza/entities/tests/test_entities_models.py @@ -7,7 +7,6 @@ from django.contrib.contenttypes.models import ContentType from django.forms import ValidationError from django.utils import timezone -from modeltranslation.manager import MultilingualQuerySet from parasolr.django.indexing import ModelIndexable from slugify import slugify from unidecode import unidecode @@ -706,6 +705,82 @@ def test_str(self): assert str(relation) == f"{recipient} relation: {goitein} and {doc}" +@pytest.mark.django_db +class TestPersonDocumentRelationType: + def test_merge_with(self, person, person_multiname, document, join): + # create two PersonDocumentRelationTypes and some associations + rel_type = PersonDocumentRelationType.objects.create(name="test") + type_2 = PersonDocumentRelationType.objects.create(name="to be merged") + type_3 = PersonDocumentRelationType.objects.create(name="also merge me") + PersonDocumentRelation.objects.create( + type=rel_type, person=person, document=document + ) + PersonDocumentRelation.objects.create( + type=type_2, person=person, document=document + ) + PersonDocumentRelation.objects.create( + type=type_2, person=person_multiname, document=document + ) + PersonDocumentRelation.objects.create(type=type_3, person=person, document=join) + + # create some log entries + pdrtype_contenttype = ContentType.objects.get_for_model( + PersonDocumentRelationType + ) + creation_date = timezone.make_aware(datetime(2023, 10, 12)) + creator = User.objects.get_or_create(username="editor")[0] + type_2_str = str(type_2) + type_2_pk = type_2.pk + LogEntry.objects.bulk_create( + [ + LogEntry( + user_id=creator.id, + content_type_id=pdrtype_contenttype.pk, + object_id=type_2_pk, + object_repr=type_2_str, + change_message="first input", + action_flag=ADDITION, + action_time=creation_date, + ), + LogEntry( + user_id=creator.id, + content_type_id=pdrtype_contenttype.pk, + object_id=type_2_pk, + object_repr=type_2_str, + change_message="major revision", + action_flag=CHANGE, + action_time=timezone.now(), + ), + ] + ) + assert rel_type.persondocumentrelation_set.count() == 1 + rel_type.merge_with([type_2, type_3]) + # should skip the duplicate and add the others + assert rel_type.persondocumentrelation_set.count() == 3 + # should delete other types and create merge log entry + assert not type_2.pk + assert not type_3.pk + assert LogEntry.objects.filter( + object_id=rel_type.pk, + change_message__contains=f"merged with {type_2}, {type_3}", + ).exists() + assert rel_type.log_entries.count() == 3 + # based on default sorting, most recent log entry will be first + # - should document the merge event + merge_log = rel_type.log_entries.first() + assert merge_log.action_flag == CHANGE + + # reassociated log entries should include merged type's name, id + assert ( + " [merged type %s (id = %s)]" % (type_2_str, type_2_pk) + in rel_type.log_entries.all()[1].change_message + ) + assert ( + " [merged type %s (id = %s)]" % (type_2_str, type_2_pk) + in rel_type.log_entries.all()[2].change_message + ) + + @pytest.mark.django_db class TestPlace: def test_str(self): diff --git a/geniza/entities/tests/test_entities_views.py b/geniza/entities/tests/test_entities_views.py index c4e2e251c..a735f2bd5 100644 --- a/geniza/entities/tests/test_entities_views.py +++ b/geniza/entities/tests/test_entities_views.py @@ -25,6 +25,7 @@ ) from geniza.entities.views import ( PersonAutocompleteView, + PersonDocumentRelationTypeMerge, PersonListView, PersonMerge, PlaceAutocompleteView, @@ -110,6 +111,91 @@ def test_person_merge(self, admin_client, client): assert "test message" in messages +class TestPersonDocumentRelationTypeMergeView: + # adapted from TestPersonMergeView + @pytest.mark.django_db + def test_get_success_url(self): + rel_type = PersonDocumentRelationType.objects.create(name="test") + merge_view = PersonDocumentRelationTypeMerge() + merge_view.primary_relation_type = rel_type + + resolved_url = resolve(merge_view.get_success_url()) + assert "admin" in resolved_url.app_names + assert resolved_url.url_name == "entities_persondocumentrelationtype_change" + assert resolved_url.kwargs["object_id"] == str(rel_type.pk) + + def test_get_initial(self): + merge_view = PersonDocumentRelationTypeMerge() + merge_view.request = Mock(GET={"ids": "12,23,456,7"}) + + initial = merge_view.get_initial() + assert merge_view.ids == [12, 23, 456, 7] + # lowest id selected as default primary type + assert initial["primary_relation_type"] == 7 + + # Test when no ids are provided (a user shouldn't get here, + # but shouldn't raise an error.) + merge_view.request = Mock(GET={"ids": ""}) + initial = merge_view.get_initial() + assert merge_view.ids == [] + merge_view.request = Mock(GET={}) + initial = merge_view.get_initial() + assert merge_view.ids == [] + + def test_get_form_kwargs(self): + merge_view = PersonDocumentRelationTypeMerge() + merge_view.request = Mock(GET={"ids": "12,23,456,7"}) + form_kwargs = merge_view.get_form_kwargs() + assert form_kwargs["ids"] == merge_view.ids + + def test_person_merge(self, admin_client, client): + # Ensure that the merge view is not visible to public + response = client.get(reverse("admin:person-document-relation-type-merge")) + assert response.status_code == 302 + assert response.url.startswith("/accounts/login/") + + # create test records to merge + rel_type = PersonDocumentRelationType.objects.create(name="test") + dupe_rel_type = PersonDocumentRelationType.objects.create(name="test2") + + idstring = ",".join(str(id) for id in [rel_type.id, dupe_rel_type.id]) + + # GET should display choices + response = admin_client.get( + reverse("admin:person-document-relation-type-merge"), {"ids": idstring} + ) + assert response.status_code == 200 + + # POST should merge + merge_url = "%s?ids=%s" % ( + reverse("admin:person-document-relation-type-merge"), + idstring, + ) + response = admin_client.post( + merge_url, {"primary_relation_type": rel_type.id}, follow=True + ) + TestCase().assertRedirects( + response, + reverse( + "admin:entities_persondocumentrelationtype_change", args=[rel_type.id] + ), + ) + message = list(response.context.get("messages"))[0] + assert message.tags == "success" + assert "Successfully merged" in message.message + assert f"with {str(rel_type)} (id = {rel_type.pk})" in message.message + + with patch.object(PersonDocumentRelationType, "merge_with") as mock_merge_with: + # should catch ValidationError and send back to form with error msg + mock_merge_with.side_effect = ValidationError("test message") + response = admin_client.post( + merge_url, {"primary_relation_type": rel_type.id}, follow=True + ) + TestCase().assertRedirects(response, merge_url) + messages = [str(msg) for msg in list(response.context["messages"])] + assert "test message" in messages + + class TestPersonAutocompleteView: @pytest.mark.django_db def test_get_queryset(self): diff --git a/geniza/entities/views.py b/geniza/entities/views.py index 38813dcc5..12432aba7 100644 --- a/geniza/entities/views.py +++ b/geniza/entities/views.py @@ -19,11 +19,17 @@ from geniza.corpus.dates import PartialDate from geniza.corpus.views import SolrDateRangeMixin -from geniza.entities.forms import PersonListForm, PersonMergeForm, PlaceListForm +from geniza.entities.forms import ( + PersonDocumentRelationTypeMergeForm, + PersonListForm, + PersonMergeForm, + PlaceListForm, +) from geniza.entities.models import ( PastPersonSlug, PastPlaceSlug, Person, + PersonDocumentRelationType, PersonPersonRelationType, PersonSolrQuerySet, Place, @@ -95,6 +101,81 @@ def form_valid(self, form): return super().form_valid(form) +class PersonDocumentRelationTypeMerge(PermissionRequiredMixin, FormView): + permission_required = ( + "entities.change_persondocumentrelationtype", + "entities.delete_persondocumentrelationtype", + ) + form_class = PersonDocumentRelationTypeMergeForm + template_name = "admin/entities/persondocumentrelationtype/merge.html" + + def get_success_url(self): + return reverse( + "admin:entities_persondocumentrelationtype_change", + args=[self.primary_relation_type.pk], + ) + + def get_form_kwargs(self): + form_kwargs = super().get_form_kwargs() + form_kwargs["ids"] = self.ids + return form_kwargs + + def get_initial(self): + # Default to first relation type selected + ids = self.request.GET.get("ids", None) + if ids: + self.ids = [int(id) for id in ids.split(",")] + # by default, prefer the first record created + return {"primary_relation_type": sorted(self.ids)[0]} + else: + self.ids = [] + + def form_valid(self, form): + """Merge the selected person-document relation types into the primary one.""" + primary_relation_type = form.cleaned_data["primary_relation_type"] + self.primary_relation_type = primary_relation_type + + secondary_ids = [id for id in self.ids if id != primary_relation_type.pk] + secondary_relation_types = PersonDocumentRelationType.objects.filter( + pk__in=secondary_ids + ) + + # Get string representations before they are merged + primary_relation_str = ( + f"{str(primary_relation_type)} (id = {primary_relation_type.pk})" + ) + secondary_relation_str = ", ".join( + [f"{str(rel)} (id = {rel.pk})" for rel in secondary_relation_types] + ) + + # Merge secondary relation types into the selected primary relation type + user = getattr(self.request, "user", None) + + try: + primary_relation_type.merge_with(secondary_relation_types, user=user) + except ValidationError as err: + # in case the merge resulted in an error, display error to user + messages.error(self.request, err.message) + # redirect to this form page instead of one of the items + return HttpResponseRedirect( + "%s?ids=%s" + % ( + reverse("admin:person-document-relation-type-merge"), + self.request.GET.get("ids", ""), + ), + ) + + # Display info about the merge to the user + messages.success( + self.request, + mark_safe( + f"Successfully merged {secondary_relation_str} with {primary_relation_str}." + ), + ) + + return super().form_valid(form) + + class UnaccentedNameAutocompleteView(autocomplete.Select2QuerySetView): def get_queryset(self): """entities filtered by entered query, or all entities, ordered by name""" diff --git a/sitemedia/css/admin-local.css b/sitemedia/css/admin-local.css index c38dff87e..2c5132a6a 100644 --- a/sitemedia/css/admin-local.css +++ b/sitemedia/css/admin-local.css @@ -380,3 +380,13 @@ div.form-row.field-doc_date_original .inline-group { grid-template-columns: 1fr 1fr; } } + +.merge-relationtype input[type="radio"] { + margin-top: 0.5rem; + margin-bottom: auto; + vertical-align: top; +} +.merge-relationtype details summary { + cursor: pointer; + margin-top: 0.5rem; +}