Merge pull request #1724 from Princeton-CDH/feature/1500-personperson…

…-merge

Merge tool for person-person relationship types (#1500)

geniza/entities/admin.py

@@ -50,7 +50,11 @@
     PlacePlaceRelation,
     PlacePlaceRelationType,
 )
-from geniza.entities.views import PersonDocumentRelationTypeMerge, PersonMerge
+from geniza.entities.views import (
+    PersonDocumentRelationTypeMerge,
+    PersonMerge,
+    PersonPersonRelationTypeMerge,
+)
 from geniza.footnotes.models import Footnote
 
 
@@ -409,33 +413,26 @@ class RoleAdmin(TabbedTranslationAdmin, admin.ModelAdmin):
     ordering = ("display_label", "name")
 
 
-@admin.register(PersonDocumentRelationType)
-class PersonDocumentRelationTypeAdmin(TabbedTranslationAdmin, admin.ModelAdmin):
-    """Admin for managing the controlled vocabulary of people's relationships to documents"""
-
-    fields = ("name",)
-    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
+class RelationTypeMergeAdminMixin:
+    @admin.display(description="Merge selected %(verbose_name_plural)s")
+    def merge_relation_types(self, request, queryset=None):
+        """Admin action to merge selected entity-entity 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",
+                "You must select at least two person-person relationships to merge",
             )
             return HttpResponseRedirect(
-                reverse("admin:entities_persondocumentrelationtype_changelist")
+                reverse("admin:entities_%s_changelist" % self.model._meta.model_name)
             )
         return HttpResponseRedirect(
             "%s?ids=%s"
             % (
-                reverse("admin:person-document-relation-type-merge"),
+                reverse(f"admin:{self.merge_path_name}"),
                 ",".join(selected),
             ),
             status=303,
@@ -446,22 +443,39 @@ def get_urls(self):
         urls = [
             path(
                 "merge/",
-                PersonDocumentRelationTypeMerge.as_view(),
-                name="person-document-relation-type-merge",
+                self.view_class.as_view(),
+                name=self.merge_path_name,
             ),
         ]
         return urls + super().get_urls()
 
-    actions = (merge_person_document_relation_types,)
+    actions = (merge_relation_types,)
+
+
+@admin.register(PersonDocumentRelationType)
+class PersonDocumentRelationTypeAdmin(
+    RelationTypeMergeAdminMixin, TabbedTranslationAdmin, admin.ModelAdmin
+):
+    """Admin for managing the controlled vocabulary of people's relationships to documents"""
+
+    fields = ("name",)
+    search_fields = ("name",)
+    ordering = ("name",)
+    merge_path_name = "person-document-relation-type-merge"
+    view_class = PersonDocumentRelationTypeMerge
 
 
 @admin.register(PersonPersonRelationType)
-class PersonPersonRelationTypeAdmin(TabbedTranslationAdmin, admin.ModelAdmin):
+class PersonPersonRelationTypeAdmin(
+    RelationTypeMergeAdminMixin, TabbedTranslationAdmin, admin.ModelAdmin
+):
     """Admin for managing the controlled vocabulary of people's relationships to other people"""
 
     fields = ("name", "converse_name", "category")
     search_fields = ("name",)
     ordering = ("name",)
+    merge_path_name = "person-person-relation-type-merge"
+    view_class = PersonPersonRelationTypeMerge
 
 
 @admin.register(PersonPlaceRelationType)

geniza/entities/forms.py

@@ -10,6 +10,7 @@
     PersonDocumentRelationType,
     PersonEventRelation,
     PersonPersonRelation,
+    PersonPersonRelationType,
     PersonPlaceRelation,
     PersonRole,
     PlaceEventRelation,
@@ -54,6 +55,22 @@ def __init__(self, *args, **kwargs):
         )
 
 
+class RelationTypeMergeFormMixin:
+    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 = self.reltype_model.objects.filter(id__in=ids)
+
+
 class PersonDocumentRelationTypeChoiceField(forms.ModelChoiceField):
     """Add a summary of each PersonDocumentRelationType to a form (used for merging)"""
 
@@ -65,7 +82,7 @@ def label_from_instance(self, relation_type):
         return self.label_template.render({"relation_type": relation_type})
 
 
-class PersonDocumentRelationTypeMergeForm(forms.Form):
+class PersonDocumentRelationTypeMergeForm(RelationTypeMergeFormMixin, forms.Form):
     primary_relation_type = PersonDocumentRelationTypeChoiceField(
         label="Select primary person-document relationship",
         queryset=None,
@@ -77,20 +94,33 @@ class PersonDocumentRelationTypeMergeForm(forms.Form):
         empty_label=None,
         widget=forms.RadioSelect,
     )
+    reltype_model = PersonDocumentRelationType
 
-    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
+class PersonPersonRelationTypeChoiceField(forms.ModelChoiceField):
+    """Add a summary of each PersonPersonRelationType to a form (used for merging)"""
 
-        super().__init__(*args, **kwargs)
-        self.fields[
-            "primary_relation_type"
-        ].queryset = PersonDocumentRelationType.objects.filter(id__in=ids)
+    label_template = get_template(
+        "entities/snippets/personpersonrelationtype_option_label.html"
+    )
+
+    def label_from_instance(self, relation_type):
+        return self.label_template.render({"relation_type": relation_type})
+
+
+class PersonPersonRelationTypeMergeForm(RelationTypeMergeFormMixin, forms.Form):
+    primary_relation_type = PersonPersonRelationTypeChoiceField(
+        label="Select primary person-person relationship",
+        queryset=None,
+        help_text=(
+            "Select the primary person-person 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,
+    )
+    reltype_model = PersonPersonRelationType
 
 
 class PersonPersonForm(forms.ModelForm):

geniza/entities/models.py

@@ -1020,29 +1020,10 @@ def get_by_natural_key(self, name):
         return self.get(name_en=name)
 
 
-class PersonDocumentRelationType(models.Model):
-    """Controlled vocabulary of people's relationships to documents."""
-
-    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"
-        verbose_name_plural = "Person-Document relationships"
-
-    def __str__(self):
-        return self.name
-
-    @cached_class_property
-    def objects_by_label(cls):
-        return {
-            # lookup on name_en since solr should always index in English
-            obj.name_en: obj
-            for obj in cls.objects.all()
-        }
+class MergeRelationTypesMixin:
+    """Mixin to include shared merge logic for relation types.
+    Requires inheriting relation type model to make its relationships
+    queryset available generically by the method name :meth:`relation_set`"""
 
     def merge_with(self, merge_relation_types, user=None):
         """Merge the specified relation types into this one. Combines all
@@ -1069,12 +1050,12 @@ def merge_with(self, merge_relation_types, user=None):
                 # - associate with the primary relation type
                 log_entry.object_id = self.id
                 log_entry.content_type_id = ContentType.objects.get_for_model(
-                    PersonDocumentRelationType
+                    self.__class__
                 )
                 log_entry.save()
 
-            # combine person-document relationships
-            for relationship in rel_type.persondocumentrelation_set.all():
+            # combine relationships
+            for relationship in rel_type.relation_set():
                 # set type of each relationship to primary relation type
                 relationship.type = self
                 # handle unique constraint violation (one relationship per type
@@ -1093,19 +1074,46 @@ def merge_with(self, merge_relation_types, user=None):
         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
-        )
+        rtype_contenttype = ContentType.objects.get_for_model(self.__class__)
         LogEntry.objects.log_action(
             user_id=user.id,
-            content_type_id=pdrtype_contenttype.pk,
+            content_type_id=rtype_contenttype.pk,
             object_id=self.pk,
             object_repr=str(self),
             change_message="merged with %s" % (merged_types,),
             action_flag=CHANGE,
         )
 
 
+class PersonDocumentRelationType(MergeRelationTypesMixin, models.Model):
+    """Controlled vocabulary of people's relationships to documents."""
+
+    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"
+        verbose_name_plural = "Person-Document relationships"
+
+    def __str__(self):
+        return self.name
+
+    @cached_class_property
+    def objects_by_label(cls):
+        return {
+            # lookup on name_en since solr should always index in English
+            obj.name_en: obj
+            for obj in cls.objects.all()
+        }
+
+    def relation_set(self):
+        # own relationships QuerySet as required by MergeRelationTypesMixin
+        return self.persondocumentrelation_set.all()
+
+
 class PersonDocumentRelation(models.Model):
     """A relationship between a person and a document."""
 
@@ -1140,7 +1148,7 @@ def get_by_natural_key(self, name):
         return self.get(name_en=name)
 
 
-class PersonPersonRelationType(models.Model):
+class PersonPersonRelationType(MergeRelationTypesMixin, models.Model):
     """Controlled vocabulary of people's relationships to other people."""
 
     # name of the relationship
@@ -1171,6 +1179,9 @@ class PersonPersonRelationType(models.Model):
         choices=CATEGORY_CHOICES,
     )
     objects = PersonPersonRelationTypeManager()
+    log_entries = GenericRelation(
+        LogEntry, related_query_name="personpersonrelationtype"
+    )
 
     class Meta:
         verbose_name = "Person-Person relationship"
@@ -1179,6 +1190,10 @@ class Meta:
     def __str__(self):
         return self.name
 
+    def relation_set(self):
+        # own relationships QuerySet as required by MergeRelationTypesMixin
+        return self.personpersonrelation_set.all()
+
 
 class PersonPersonRelation(models.Model):
     """A relationship between two people."""

geniza/entities/templates/admin/entities/persondocumentrelationtype/merge.html

@@ -31,7 +31,7 @@ <h2>Note: there is no automated way to unmerge! Please review to make sure these
 {% endblock %}
 
 {% block content %}
-    <form method="post" class="merge-document merge-relationtype">
+    <form method="post" class="merge-relationtype">
         {% csrf_token %}
         {% if form.errors|length > 0 %}
             <p class="errornote">

geniza/entities/templates/admin/entities/personpersonrelationtype/merge.html

@@ -0,0 +1,53 @@
+{% extends 'admin/base_site.html' %}
+
+{% load admin_urls static %}
+
+{% block extrastyle %}
+    {{ block.super }}
+    <link rel="stylesheet" href="{% static "admin/css/forms.css" %}">
+    <link rel="stylesheet" href="{% static "css/admin-local.css" %}">
+{% endblock %}
+
+{% block title %} Merge selected person-person relationships {% endblock %}
+
+{% block breadcrumbs %}
+    {% if not is_popup %}
+        <div class="breadcrumbs">
+            <a href="{% url 'admin:index' %}">Home</a>
+            &rsaquo;
+            <a href="{% url 'admin:app_list' app_label='entities' %}">Entities</a>
+            &rsaquo;
+            <a href="{% url 'admin:entities_personpersonrelationtype_changelist'%}">Person-Person relationships</a>
+            &rsaquo;
+            Merge selected person-person relationships
+        </div>
+    {% endif %}
+{% endblock %}
+
+
+{% block content_title %}
+    <h1>Merge selected person-person relationships</h1>
+    <h2>Note: there is no automated way to unmerge! Please review to make sure these relationships should be merged before submitting the form.</h2>
+{% endblock %}
+
+{% block content %}
+    <form method="post" class="merge-relationtype">
+        {% csrf_token %}
+        {% if form.errors|length > 0 %}
+            <p class="errornote">
+                Please correct the error below.
+            </p>
+        {% endif %}
+        <fieldset class="module aligned">
+            <div class="form-row">
+                {{ form.primary_relation_type.label_tag }}
+                {{ form.primary_relation_type }}
+                <p class="help">{{ form.primary_relation_type.help_text|safe }}</p>
+            </div>
+
+            <div class="submit-row">
+                <input type="submit" value="Submit">
+            </div>
+        </field>
+    </form>
+{% endblock %}