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

Give all contact infos a user #3131

Merged
merged 8 commits into from
Dec 31, 2020
Merged

Give all contact infos a user #3131

merged 8 commits into from
Dec 31, 2020

Conversation

willgearty
Copy link
Member

I've made the following changes:

  1. All calls to ContactInfo.addOrUpdate now require a user object.
  2. Added a migration to clean up old ContactInfos. This includes deleting any that aren't associated with registration profiles and adding user objects if they are missing them based on their associated registration profiles. This means ContactInfos for emergency contacts and guardians are now associated with the student accounts. Also, we were making blank emergency ContactInfos for every non-student, so those are deleted.
  3. Made the user field required for ContactInfos (including a migration).
  4. Resolved any fallout due to old code that assumed ContactInfos had a 1:1 relationship.

In addition to the benefits of all ContactInfos now having an associated user and deleting a bunch of unneeded/blank ContactInfos, this also makes it possible to perform a user search based on any information within emergency contact or guardian ContactInfos (e.g. find a student from their parent's email address), which has been requested many many times by chapters.

I've tested the migrations multiple times, but it would be good to get another set of eyes on them and their application to make sure no important data is accidentally deleted.

Fixes #1075 and fixes #1903.

Comment on lines +19 to +20
if not (ci.first_name and ci.last_name):
# We want to delete emergency contact info for non-students
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see the relationship between the code and the comment. How is having (or not having) a first name and last name the same as being (or not being) a student?

Copy link
Member Author

@willgearty willgearty Oct 17, 2020

Choose a reason for hiding this comment

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

Ah, so I'm trying to retroactively fix the problem caused by this old line (now moved into the student forloop):

regProf.contact_emergency = ContactInfo.addOrUpdate(regProf, new_data, regProf.contact_emergency, 'emerg_')

We used to add a contact_emergency for all users, not just students. But we only ask for emergency contact info in the student version of the profile form, so it always resulted in a blank contactinfo. So here we are looking to see if a contact info has been used for contact_emergency but does not have a first and last name (indicating it was created by the above line for a non-student).

Comment on lines +81 to +82
# Only get contact infos that are for the actual users (not guardians or emergency contacts)
states_raw = ContactInfo.objects.filter(user__in=users, as_user__isnull=False
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand this as_user condition. How does it tell us that the ContactInfo is for the actual user? I thought as_user was related to registration profiles.

Copy link
Member Author

@willgearty willgearty Oct 17, 2020

Choose a reason for hiding this comment

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

as_user is the set of registration profiles that use the contact info as the contact_user relationship. Otherwise, they use them for contact_emergency or contact_guardian relationship (if the user is a student). If we find that the contact info has been used as the contact_user relationship (as_user__isnull=False), then we know this is the contact info for the actual user.

Copy link
Contributor

@hwatheod hwatheod left a comment

Choose a reason for hiding this comment

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

For item 4, how did you look for all cases where the code assumes that ContactInfos and Users are 1:1 ?

@willgearty
Copy link
Member Author

willgearty commented Oct 17, 2020

I searched GitHub for all the uses of either ContactInfo (for generating an object) or contactinfo (for queries). I also just checked any calls/queries that used contact_user, contact_emergency, or contact_guardian but they are all related to getting those objects for a single registration profile, so they should be fine. I also just checked and as_user, as_guardian, and as_emergency are never used in the codebase before this.

esp/esp/program/modules/handlers/programprintables.py Outdated Show resolved Hide resolved
esp/esp/program/views.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kkbrum kkbrum left a comment

Choose a reason for hiding this comment

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

The migration worked well for me, cleaning up the contact infos without removing anything extra and assigning users as I would have expected it to. I tested creating a student account and the contact infos for the student, parent, and emergency contact all looked good with the affiliated user (and the user search allowed searching by parent & emergency contact info). I also tested that creating a teacher account only creates one contact info for the teacher. If there's anything else I should test, let me know, but otherwise this looks great!

@willgearty willgearty merged commit 3b9f814 into main Dec 31, 2020
@willgearty willgearty deleted the contact-infos branch December 31, 2020 01:41
willgearty added a commit that referenced this pull request Dec 31, 2020
willgearty added a commit that referenced this pull request May 27, 2021
* Initial docs for stable release 13

* Docs for #3116, #3117, and #3118

* Added docs about django upgrade

* Docs for #3128

* Docs for #3129, #3133, #3134, and #3137

* Docs for #3156 and #3153

* Docs for #3174, #3163, and #3184

* Docs for #3139, #3140, and #3141

* Docs for #3143, #3150, #3154, #3160, #3162, and #3168

* Docs for #3171, #3185, #3186, and #3188

* Docs for #3131 and #3189

* Docs for #3149 and #3190

* Docs for #3193, #3194, #3195, #196, and #3197

* Clarification

* Docs for #3192, #3201, #3209, and #2248

* Docs for #3204, #3212, #3214, #3205, 9fd073c, and #3226

* Docs for #3232, de5861c, #3231, #3233, #3234, #3237, #3238, and #3239

* Fix indent

* Docs for #3227 and #3235

* Add missing word

* spelling

* Docs for e57581f, #3255, #3253, #3257, and #3249

* Docs for #3254, #3260, and #3262

* Docs for #3263, #3272, #3240, #3264, #3266, and #3270

* clarifications

* Docs for #3283 and #3252

* Docs for #3288 and misc commits

* Docs for #3292, #3311, #3286, #3289, and #3279

* Docs for a377f0d; move note

* Docs for #3315, #3290, and #3322

* Docs for #3273 and #3317

* Final edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search by parent email ContactInfo Objects with Null User
3 participants