Only de-duplicate on primary email address

src/meshapi/tests/sample_join_form_data.py

@@ -60,37 +60,6 @@
     },
 }
 
-valid_join_form_submission_no_email = {
-    "first_name": "John",
-    "last_name": "Smith",
-    "email": None,
-    "phone": "+1585-758-3425",  # CSH's phone number :P
-    "street_address": "151 Broome St",  # Also covers New York County Test Case
-    "parsed_street_address": "151 Broome Street",
-    "city": "New York",
-    "state": "NY",
-    "zip": 10002,
-    "apartment": "",
-    "roof_access": True,
-    "referral": "Googled it or something IDK",
-    "ncl": True,
-    "dob_addr_response": {
-        "features": [
-            {
-                "properties": {
-                    "housenumber": "151",
-                    "street": "Broome Street",
-                    "borough": "New York",
-                    "region_a": "NY",
-                    "postalcode": "10002",
-                    "addendum": {"pad": {"bin": 1077609}},
-                },
-                "geometry": {"coordinates": [0, 0]},
-            }
-        ]
-    },
-}
-
 richmond_join_form_submission = {
     "first_name": "Maya",
     "last_name": "Viernes",

src/meshapi/tests/test_join_form.py

@@ -24,7 +24,6 @@
     queens_join_form_submission,
     richmond_join_form_submission,
     valid_join_form_submission,
-    valid_join_form_submission_no_email,
     valid_join_form_submission_with_apartment_in_address,
 )
 from .util import TestThread
@@ -120,7 +119,6 @@ def tearDown(self):
     @parameterized.expand(
         [
             [valid_join_form_submission],
-            [valid_join_form_submission_no_email],
             [richmond_join_form_submission],
             [kings_join_form_submission],
             [queens_join_form_submission],
@@ -427,15 +425,6 @@ def test_member_moved_join_form(self):
 
     @mock.patch("meshapi.views.forms.notify_administrators_of_data_issue")
     def test_member_moved_join_form_but_somehow_duplicate_objects_already_exist_for_them(self, mock_admin_notif_func):
-        # Create a pre-exsiting duplicate member object,
-        # that won't be matched until the second join form submission
-        pre_existing_member = Member(
-            name="John Smith",
-            primary_email_address="[email protected]",
-            phone_number="+1-555-555-5555",
-        )
-        pre_existing_member.save()
-
         # Name, email, phone, location, apt, rooftop, referral
         form, s = pull_apart_join_form_submission(valid_join_form_submission)
         response1 = self.c.post("/api/v1/join/", form, content_type="application/json")
@@ -449,10 +438,16 @@ def test_member_moved_join_form_but_somehow_duplicate_objects_already_exist_for_
 
         validate_successful_join_form_submission(self, "Valid Join Form", s, response1)
 
+        pre_existing_duplicate_member = Member(
+            name="John Smith",
+            primary_email_address="[email protected]",
+            phone_number="+1-555-555-5555",
+        )
+        pre_existing_duplicate_member.save()
+
         # Now test that the member can "move" and still access the join form
         v_sub_2 = valid_join_form_submission.copy()
         v_sub_2["street_address"] = "152 Broome Street"
-        v_sub_2["phone"] = "+1 555-555-5555"
 
         form, s = pull_apart_join_form_submission(v_sub_2)
 
@@ -479,7 +474,7 @@ def test_member_moved_join_form_but_somehow_duplicate_objects_already_exist_for_
         self.assertEqual(
             set(mock_admin_notif_func.call_args.args[0]),
             {
-                pre_existing_member,
+                pre_existing_duplicate_member,
                 Member.objects.get(
                     id=json.loads(
                         response1.content.decode("utf-8"),
@@ -560,7 +555,7 @@ def test_member_moved_and_changed_names_join_form(self, mock_admin_notif_func):
             ANY,
         )
 
-    def test_member_moved_and_used_a_new_email_join_form(self):
+    def test_member_moved_and_used_additional_email_join_form(self):
         # Name, email, phone, location, apt, rooftop, referral
         form, s = pull_apart_join_form_submission(valid_join_form_submission)
         response1 = self.c.post("/api/v1/join/", form, content_type="application/json")
@@ -574,8 +569,18 @@ def test_member_moved_and_used_a_new_email_join_form(self):
 
         validate_successful_join_form_submission(self, "Valid Join Form", s, response1)
 
-        # Now test that the member can "move" and still access the join form
-        # (even with a new email, provided they use the same phone number)
+        # Add the email we're going to use below ([email protected]) as an additional email
+        # to confirm that we don't de-duplicate on these additional addresses
+        join_form_1_member = Member.objects.get(
+            id=json.loads(
+                response1.content.decode("utf-8"),
+            )["member_id"]
+        )
+        join_form_1_member.additional_email_addresses.append("[email protected]")
+        join_form_1_member.save()
+
+        # Now test that the member can "move" and still access the join form, getting a new install
+        # number and member object when this happens
         v_sub_2 = valid_join_form_submission.copy()
         v_sub_2["email"] = "[email protected]"
         v_sub_2["street_address"] = "152 Broome Street"
@@ -592,9 +597,11 @@ def test_member_moved_and_used_a_new_email_join_form(self):
             f"status code incorrect for Valid Join Form. Should be {code}, but got {response2.status_code}.\n Response is: {response2.content.decode('utf-8')}",
         )
 
-        validate_successful_join_form_submission(self, "Valid Join Form", s, response2)
+        validate_successful_join_form_submission(self, "Valid Join Form", s, response2, expected_member_count=2)
 
-        self.assertEqual(
+        # Ensure we created a new member ID for the second request, since the primary email address
+        # doesn't match
+        self.assertNotEqual(
             json.loads(
                 response1.content.decode("utf-8"),
             )["member_id"],
@@ -603,15 +610,13 @@ def test_member_moved_and_used_a_new_email_join_form(self):
             )["member_id"],
         )
 
-        # Make sure the member's primary email wasn't changed (prevents join form griefing)
-        # but also confirm we noted the new email in additional emails
+        # Also ensure the original member's primary email wasn't changed (prevents join form griefing)
         member = Member.objects.get(
             id=json.loads(
                 response1.content.decode("utf-8"),
             )["member_id"]
         )
         self.assertEqual(member.primary_email_address, "[email protected]")
-        self.assertEqual(member.additional_email_addresses, ["[email protected]"])
 
     def test_member_moved_and_used_a_new_phone_number_join_form(self):
         # Name, email, phone, location, apt, rooftop, referral
@@ -666,59 +671,21 @@ def test_member_moved_and_used_a_new_phone_number_join_form(self):
         self.assertEqual(member.phone_number, "+1 585-758-3425")
         self.assertEqual(member.additional_phone_numbers, ["+1 212-555-5555"])
 
-    def test_member_moved_and_used_only_a_badly_formatted_phone_number_join_form(self):
+    def test_no_email_join_form(self):
+        no_email_submission = valid_join_form_submission.copy()
+        no_email_submission["email"] = None
+
         # Name, email, phone, location, apt, rooftop, referral
-        form, s = pull_apart_join_form_submission(valid_join_form_submission)
+        form, s = pull_apart_join_form_submission(no_email_submission)
         response1 = self.c.post("/api/v1/join/", form, content_type="application/json")
 
-        code = 201
+        code = 400
         self.assertEqual(
             code,
             response1.status_code,
             f"status code incorrect for Valid Join Form. Should be {code}, but got {response1.status_code}.\n Response is: {response1.content.decode('utf-8')}",
         )
 
-        validate_successful_join_form_submission(self, "Valid Join Form", s, response1)
-
-        # Now test that the member can "move" and still access the join form
-        # (even if they don't provide an email, and give a badly formatted phone number)
-        v_sub_2 = valid_join_form_submission.copy()
-        v_sub_2["street_address"] = "152 Broome Street"
-        v_sub_2["phone"] = "+1 5 8 5 75 8-3 425  "
-        v_sub_2["email"] = None
-
-        form, s = pull_apart_join_form_submission(v_sub_2)
-
-        # Name, email, phone, location, apt, rooftop, referral
-        response2 = self.c.post("/api/v1/join/", form, content_type="application/json")
-
-        code = 201
-        self.assertEqual(
-            code,
-            response2.status_code,
-            f"status code incorrect for Valid Join Form. Should be {code}, but got {response2.status_code}.\n Response is: {response2.content.decode('utf-8')}",
-        )
-
-        validate_successful_join_form_submission(self, "Valid Join Form", s, response2)
-
-        # Make sure the member's primary phone number wasn't changed,
-        # and that the member matches up to the original submission
-        self.assertEqual(
-            json.loads(
-                response1.content.decode("utf-8"),
-            )["member_id"],
-            json.loads(
-                response2.content.decode("utf-8"),
-            )["member_id"],
-        )
-        member = Member.objects.get(
-            id=json.loads(
-                response1.content.decode("utf-8"),
-            )["member_id"]
-        )
-        self.assertEqual(member.phone_number, "+1 585-758-3425")
-        self.assertEqual(member.additional_phone_numbers, [])
-
     def test_different_street_addr_same_bin_multi_node(self):
         """ "
         This test case simulates a new building joining the Jefferson structure to
@@ -813,7 +780,7 @@ def test_different_street_addr_same_bin_multi_node(self):
             set(building2.nodes.all().values_list("network_number", flat=True)),
         )
 
-    def test_member_moved_and_used_stripe_email_join_form(self):
+    def test_member_moved_and_used_non_primary_email_join_form(self):
         self.requests_mocker.get(
             NYC_PLANNING_LABS_GEOCODE_URL,
             json=valid_join_form_submission["dob_addr_response"],
@@ -837,8 +804,8 @@ def test_member_moved_and_used_stripe_email_join_form(self):
         member_object.additional_email_addresses = ["[email protected]"]
         member_object.save()
 
-        # Now test that the member can "move", use the stripe email address,
-        # and we will connect it to their old registration
+        # Now test that the member can move, use the stripe email address,
+        # and we will NOT connect it to their old registration
         v_sub_2 = valid_join_form_submission.copy()
         v_sub_2["email"] = "[email protected]"
         v_sub_2["street_address"] = "152 Broome Street"
@@ -863,17 +830,26 @@ def test_member_moved_and_used_stripe_email_join_form(self):
             f"but got {response2.status_code}.\n Response is: {response2.content.decode('utf-8')}",
         )
 
-        validate_successful_join_form_submission(self, "", s, response2)
+        validate_successful_join_form_submission(self, "", s, response2, expected_member_count=2)
 
-        self.assertEqual(
+        self.assertNotEqual(
             str(member_object.id),
             json.loads(
                 response2.content.decode("utf-8"),
             )["member_id"],
         )
 
-        # Now test that the member can "move" again, use an additional email address,
-        # and we will connect it to their old registration
+        self.assertNotEqual(
+            json.loads(
+                response1.content.decode("utf-8"),
+            )["install_number"],
+            json.loads(
+                response2.content.decode("utf-8"),
+            )["install_number"],
+        )
+
+        # Now test that the member can move again, use an additional email address,
+        # and we will still not connect it to their old registration
         v_sub_3 = valid_join_form_submission.copy()
         v_sub_3["email"] = "[email protected]"
         v_sub_3["street_address"] = "178 Broome Street"
@@ -898,14 +874,39 @@ def test_member_moved_and_used_stripe_email_join_form(self):
             f"but got {response3.status_code}.\n Response is: {response3.content.decode('utf-8')}",
         )
 
-        validate_successful_join_form_submission(self, "Valid Join Form", s, response3)
+        validate_successful_join_form_submission(self, "Valid Join Form", s, response3, expected_member_count=2)
 
-        self.assertEqual(
+        self.assertNotEqual(
             str(member_object.id),
             json.loads(
                 response3.content.decode("utf-8"),
             )["member_id"],
         )
+        self.assertNotEqual(
+            json.loads(
+                response1.content.decode("utf-8"),
+            )["install_number"],
+            json.loads(
+                response3.content.decode("utf-8"),
+            )["install_number"],
+        )
+
+        self.assertNotEqual(
+            json.loads(
+                response2.content.decode("utf-8"),
+            )["member_id"],
+            json.loads(
+                response3.content.decode("utf-8"),
+            )["member_id"],
+        )
+        self.assertNotEqual(
+            json.loads(
+                response2.content.decode("utf-8"),
+            )["install_number"],
+            json.loads(
+                response3.content.decode("utf-8"),
+            )["install_number"],
+        )
 
     def test_pre_existing_building_and_node(self):
         self.requests_mocker.get(

src/meshapi/views/forms.py

@@ -1,9 +1,7 @@
 import json
 import logging
-import operator
 from dataclasses import dataclass
 from datetime import date
-from functools import reduce
 from json.decoder import JSONDecodeError
 
 from django.db import IntegrityError, transaction
@@ -104,8 +102,8 @@ def join_form(request: Request) -> Response:
 
     join_form_full_name = f"{r.first_name} {r.last_name}"
 
-    if not r.email and not r.phone:
-        return Response({"detail": "Must provide an email or phone number"}, status=status.HTTP_400_BAD_REQUEST)
+    if not r.email:
+        return Response({"detail": "Must provide an email"}, status=status.HTTP_400_BAD_REQUEST)
 
     if r.email and not validate_email_address(r.email):
         return Response({"detail": f"{r.email} is not a valid email"}, status=status.HTTP_400_BAD_REQUEST)
@@ -134,29 +132,17 @@ def join_form(request: Request) -> Response:
             {"detail": "Your address could not be validated."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR
         )
 
-    # Check if there's an existing member. Group members by matching on both email and phone
-    # A member can have multiple install requests, if they move apartments for example
-    existing_member_filter_criteria = []
-    if r.email:
-        existing_member_filter_criteria.append(
-            Q(primary_email_address=r.email)
-            | Q(stripe_email_address=r.email)
-            | Q(additional_email_addresses__contains=[r.email])
-        )
-
-    if formatted_phone_number:
-        existing_member_filter_criteria.append(
-            Q(phone_number=formatted_phone_number) | Q(additional_phone_numbers=[formatted_phone_number])
-        )
-
-    existing_members = list(
-        Member.objects.filter(
-            reduce(
-                operator.or_,
-                existing_member_filter_criteria,
-            )
-        )
-    )
+    # A member can have multiple install requests, if they move apartments for example, so we
+    # check if there's an existing member. Group members by matching only on primary email address
+    # This is sublte but important. We do NOT want to dedupe on phone number, or even on additional
+    # email addresses at this time because this could lead to a situation where the email address
+    # entered in the join form does not match the email address we send the OSTicket to.
+    #
+    # That seems like a minor problem, but is actually a potential safety risk. Consider the case
+    # where a couple uses one person's email address to fill out the join form, but signs up for
+    # stripe payments with the other person's email address. If they then break up, and one person
+    # moves out, we definitely do not want to send an email with their new home address to their ex
+    existing_members = list(Member.objects.filter(Q(primary_email_address=r.email)))
 
     join_form_member = (
         existing_members[0]

src/meshdb/utils/spreadsheet_import/parse_member.py

@@ -291,16 +291,8 @@ def nop(*args, **kwargs):
     )
 
     existing_member_filter_criteria = []
-    if len(other_emails) > 0:
-        existing_member_filter_criteria.extend(
-            Q(primary_email_address=email)
-            | Q(stripe_email_address=email)
-            | Q(additional_email_addresses__contains=[email])
-            for email in other_emails + ([stripe_email] if stripe_email else [])
-        )
-
-    if formatted_phone_number:
-        existing_member_filter_criteria.append(Q(phone_number=formatted_phone_number))
+    if len(primary_emails) > 0:
+        existing_member_filter_criteria.append(Q(primary_email_address=primary_emails[0]))
 
     existing_members = []
     if existing_member_filter_criteria: