From 9388f03d1a6769a896666d0a3b5c978df49bc4a1 Mon Sep 17 00:00:00 2001
From: "Jean Cochrane (Lead developer, DataMade)" <jean.cochrane@datamade.us>
Date: Thu, 3 Sep 2020 14:54:15 -0500
Subject: [PATCH] Adjust person.views.get_commanders() to ignore title when
 finding unique commanders

---
 person/views.py            | 13 ++++++++++++-
 tests/conftest.py          | 13 +++++++++++--
 tests/test_organization.py |  3 ++-
 tests/test_person.py       | 14 ++++++++++++++
 4 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/person/views.py b/person/views.py
index f7a5a5457..7d9fab673 100644
--- a/person/views.py
+++ b/person/views.py
@@ -161,7 +161,13 @@ def get_commanders(mem_start, mem_end, compositions, person_id=None, relationshi
         child_id = child.value.uuid
 
         child_commanders_query = '''
-            SELECT DISTINCT(membership.id)
+            SELECT
+                -- Consider two memberships identical if only title is different
+                DISTINCT ON (
+                    person.id, organization.id, rank.value_id, role.value_id,
+                    first_cited.value, last_cited.value
+                )
+                membership.id
             FROM membershipperson_membershipperson AS membership
             JOIN membershipperson_membershippersonmember AS member
               ON membership.id = member.object_ref_id
@@ -171,6 +177,10 @@ def get_commanders(mem_start, mem_end, compositions, person_id=None, relationshi
               ON membership.id = member_org.object_ref_id
             JOIN organization_organization AS organization
               ON member_org.value_id = organization.id
+            JOIN membershipperson_membershippersonrank AS rank
+              ON membership.id = rank.object_ref_id
+            JOIN membershipperson_membershippersonrole AS Role
+              ON membership.id = role.object_ref_id
             JOIN membershipperson_membershippersonfirstciteddate AS first_cited
               ON membership.id = first_cited.object_ref_id
             JOIN membershipperson_membershippersonlastciteddate AS last_cited
@@ -179,6 +189,7 @@ def get_commanders(mem_start, mem_end, compositions, person_id=None, relationshi
             AND (first_cited.value < '{mem_end}' or first_cited is NULL)
             AND (last_cited.value > '{mem_start}' or last_cited is NULL)
             AND person.uuid != '{person_id}'
+            ORDER BY person.id, organization.id, rank.value_id, role.value_id, first_cited.value, last_cited.value
         '''.format(child_id=child_id,
                    mem_end=mem_end,
                    mem_start=mem_start,
diff --git a/tests/conftest.py b/tests/conftest.py
index 99906c3a6..e89097fec 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -565,7 +565,7 @@ def membership_person(access_points, people, organizations):
 
     memberships = []
 
-    for member in people[:2]:
+    for member, organization in zip(people[:2], organizations[:2]):
 
         rank = Rank.objects.create(value='Commander')
         role = Role.objects.create(value='Honcho')
@@ -577,7 +577,7 @@ def membership_person(access_points, people, organizations):
                 'confidence': '1',
             },
             'MembershipPerson_MembershipPersonOrganization': {
-                'value': organizations[0],
+                'value': organization,
                 'sources': access_points,
                 'confidence': '1',
             },
@@ -623,6 +623,15 @@ def membership_person(access_points, people, organizations):
 
         memberships.append(MembershipPerson.create(mem_info))
 
+    # Create an additional membership that is the same as the last one, just
+    # with a different title, for testing apparent duplicates
+    mem_info['MembershipPerson_MembershipPersonTitle'] = {
+        'value': 'Second Title',
+        'sources': access_points,
+        'confidence': '1',
+    }
+    memberships.append(MembershipPerson.create(mem_info))
+
     return memberships
 
 
diff --git a/tests/test_organization.py b/tests/test_organization.py
index c0f6ecafe..3983d58f7 100644
--- a/tests/test_organization.py
+++ b/tests/test_organization.py
@@ -31,7 +31,8 @@ def expected_entity_names(emplacement,
         composition[0].child.get_value().value.name.get_value().value,
         truncatewords(violation.description.get_value(), 10),
         membership_organization.organization.get_value().value.name.get_value().value,
-    ] + [mem.member.get_value().value.name.get_value().value for mem in membership_person]
+        membership_person[0].member.get_value().value.name.get_value().value
+    ]
 
 
 @pytest.mark.django_db
diff --git a/tests/test_person.py b/tests/test_person.py
index 4fcb00a0b..c1eac86ba 100644
--- a/tests/test_person.py
+++ b/tests/test_person.py
@@ -9,6 +9,7 @@
 
 from source.models import AccessPoint
 from person.models import Person
+from person.views import get_commanders
 from organization.models import Organization
 from membershipperson.models import MembershipPerson, Rank, Role
 from tests.conftest import is_tab_active
@@ -688,3 +689,16 @@ def test_person_edit_buttons(setUp, people, membership_person):
             )
         ),
         'Postings')
+
+
+@pytest.mark.django_db
+def test_get_commanders_ignores_title(setUp, access_points, people, membership_person, organizations, composition):
+    """
+    get_commanders() should ignore memberships that are identical except for
+    title. Use the fact that we set up such a membership in the membership_person
+    fixture to test to make sure get_commanders() only returns one commander.
+    """
+    person_id = people[0].uuid
+    child_compositions = composition[0].parent.get_value().value.child_organization.all()
+    commanders = get_commanders(None, None, child_compositions, person_id)
+    assert len(commanders) == 1