Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ask user to confirm info before creating Install #609

Open
wants to merge 58 commits into
base: main
Choose a base branch
from
Open

Conversation

WillNilges
Copy link
Collaborator

@WillNilges WillNilges marked this pull request as draft September 24, 2024 12:26
src/meshapi/views/forms.py Fixed Show resolved Hide resolved
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.23%. Comparing base (28649c1) to head (af4203f).

Files with missing lines Patch % Lines
src/meshapi/views/forms.py 97.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
+ Coverage   94.18%   94.23%   +0.04%     
==========================================
  Files          80       80              
  Lines        3251     3277      +26     
==========================================
+ Hits         3062     3088      +26     
  Misses        189      189              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

State is a dropdown and Zip breaks if it's wrong.
@WillNilges
Copy link
Collaborator Author

Idea: Instead of logging PII, add some marker to the join form log expressing that there was a problem. That would require appending to an existing S3 object in meshforms, since it would save the submission immediately upon entry.

@WillNilges
Copy link
Collaborator Author

Yeah i think i already thought of this but http code

@WillNilges
Copy link
Collaborator Author

I think now all's I gotta do is update Meshforms. Having just spent 30+ hours learning more React, I have i d e a s.

@WillNilges
Copy link
Collaborator Author

Maybe I could include the PII in the Notes of the created user as well...?

src/meshapi/views/query_api.py Outdated Show resolved Hide resolved
src/meshapi/widgets.py Outdated Show resolved Hide resolved
src/meshapi/views/forms.py Outdated Show resolved Hide resolved
src/meshapi/permissions.py Outdated Show resolved Hide resolved
logging.warning(f"Changed street_address: {r.street_address} != {nyc_addr_info.street_address}")
changed_info["street_address"] = nyc_addr_info.street_address

if r.city != nyc_addr_info.city:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the ZIP or state mismatch? Or are we excluding that possibility already above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They totally can, but I think I have a test for it now, (at least with NJ state but NYC address). I think it's acceptable to have this validation in the backend with the rest of our address code, but maybe we should have some explicit check for it so we can return a more specific error. Right now, it just tells the member that Non-NYC registrations are not accepted.

src/meshapi/views/forms.py Outdated Show resolved Hide resolved
@WillNilges
Copy link
Collaborator Author

This is almost done, but I'm seeing some inconsistencies with how the phone number parsing works and how it is stored in the DB. For example, I see every phone number stored like +1 555-555-5555, but that format is "corrected" in the join form (seemingly by the phonenumbers lib were using) to +1555-555-5555 but it shows up as the former in SQL.

@WillNilges
Copy link
Collaborator Author

Maybe Django is formatting it after the fact? IDK I need to do some more testing.

@WillNilges
Copy link
Collaborator Author

>>> print(phonenumbers.format_number(phonenumbers.parse("+1 614-307-4056", "US"), phonenumbers.PhoneNumberFormat.INTERNATIONAL))
+1 614-307-4056
>>> print(phonenumbers.format_number(phonenumbers.parse("+1614-307-4056", "US"), phonenumbers.PhoneNumberFormat.INTERNATIONAL))
+1 614-307-4056

image

src/meshapi/views/forms.py Fixed Show fixed Hide fixed
src/meshapi/views/forms.py Dismissed Show dismissed Hide dismissed
@@ -961,7 +961,7 @@ def test_member_moved_and_used_non_primary_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, expected_member_count=2)
validate_successful_join_form_submission(self, "Valid Join Form", s, response3, expected_member_count=3)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 5AM so I'm making a wild fucking guess, but this should be making 3 instead of 2 now, since phone number is no longer deduped

@WillNilges WillNilges marked this pull request as ready for review October 13, 2024 08:50
@WillNilges WillNilges linked an issue Oct 13, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Join form Silently Mutates Invalid NYC Address Make address (and other) info validation visible to members
3 participants