From 3d057f0d655c258c7b237a94de4903ce2a2c5b86 Mon Sep 17 00:00:00 2001 From: Andrew Dickinson Date: Sat, 12 Oct 2024 14:12:54 -0400 Subject: [PATCH 1/2] During imports only dedupe members on primary email --- src/meshdb/utils/spreadsheet_import/parse_member.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/meshdb/utils/spreadsheet_import/parse_member.py b/src/meshdb/utils/spreadsheet_import/parse_member.py index 52b19161..97af9c26 100644 --- a/src/meshdb/utils/spreadsheet_import/parse_member.py +++ b/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: From 0714708c1de4be1ebf4b15aa3096d7c7a0471b2c Mon Sep 17 00:00:00 2001 From: Andrew Dickinson Date: Sat, 12 Oct 2024 14:55:23 -0400 Subject: [PATCH 2/2] During join form only dedupe members on primary email --- src/meshapi/tests/sample_join_form_data.py | 31 ----- src/meshapi/tests/test_join_form.py | 149 +++++++++++---------- src/meshapi/views/forms.py | 40 ++---- 3 files changed, 88 insertions(+), 132 deletions(-) diff --git a/src/meshapi/tests/sample_join_form_data.py b/src/meshapi/tests/sample_join_form_data.py index 14b1c05b..301e197a 100644 --- a/src/meshapi/tests/sample_join_form_data.py +++ b/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", diff --git a/src/meshapi/tests/test_join_form.py b/src/meshapi/tests/test_join_form.py index fbc2864f..b1808669 100644 --- a/src/meshapi/tests/test_join_form.py +++ b/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="jsmith23@yahoo.com", - 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="jsmith@gmail.com", + 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 (jsmith1234@yahoo.com) 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("jsmith1234@yahoo.com") + 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"] = "jsmith1234@yahoo.com" 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, "jsmith@gmail.com") - self.assertEqual(member.additional_email_addresses, ["jsmith1234@yahoo.com"]) 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 = ["jsmith+other@gmail.com"] 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"] = "jsmith+stripe@gmail.com" 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"] = "jsmith+other@gmail.com" 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( diff --git a/src/meshapi/views/forms.py b/src/meshapi/views/forms.py index b9483aa3..241ef876 100644 --- a/src/meshapi/views/forms.py +++ b/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]