Skip to content

LG-13233: add sponsor_id to in_person_enrollments model#10759

Merged
KeithNava merged 3 commits intomainfrom
keithw/LG-13233-persist-eipp-status-on-enrollment
Jun 10, 2024
Merged

LG-13233: add sponsor_id to in_person_enrollments model#10759
KeithNava merged 3 commits intomainfrom
keithw/LG-13233-persist-eipp-status-on-enrollment

Conversation

@KeithNava
Copy link
Copy Markdown
Contributor

@KeithNava KeithNava commented Jun 4, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13233

🛠 Summary of changes

As a part of the Enhanced IPP effort, we'll need a way to check whether an enrollment was created during the Enhanced IPP flow during the background jobs. Adding the sponsor_id to the enrollment model will allow us to derive whether the flow - sponsor_id == usps_ipp_sponsor_id means Enhanced IPP

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Run pending migration
  • Verify sponsor_id is an attribute on the in_person_enrollments

@KeithNava KeithNava requested review from a team and gina-yamada June 4, 2024 17:48
Copy link
Copy Markdown
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

I was able to run the migration successfully. 🎉 I do think including a comment in the schema would prove helpful to future developers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with how this column is expected to be used, but would we expect to query such that we'd want to create an index on the column?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! I actually can't think of a requirement to query this field today.... 🤔 I guess their might be a time where we'd like to see how many folks went through the Enhanced IPP flow, but I'm assuming that would be retrieved from the analytics. Let me think on this a bit more. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd probably lean towards no as well.

@n1zyy
Copy link
Copy Markdown
Contributor

n1zyy commented Jun 5, 2024

Glad to see y'all moving EIPP forward! 🥳

My instinct is that what we care about is really "Is this an EIPP enrollment?" vs. "What sponsor_id attribute did we send over the API?" sponsor_id really has no intrinsic meaning other than an opaque identifier with USPS, and what really matters (I think) is whether we were evaluating them as E-IPP or ID-IPP. Would it make sense to store either a type (more descriptive name strongly welcomed) attribute that's an enum of (id_ipp, e_ipp), or an is_eipp? boolean?

This is a weakly-held opinion formulated having not worked on this part of the code in more than a month now. (😢❤️) If you already had this discussion and arrived at the conclusion that storing sponsor_id is the best approach, I'm A-OK with that.

@KeithNava
Copy link
Copy Markdown
Contributor Author

@n1zyy Great points! This is exactly the kind of feedback I was looking for and it's encouraging to see we're on the same page since we just had this same discussion recently.

Agreed with all your points. Essentially the argument for storing the sponsor_id over a flag like is_eipp came down to readability. Since Enhanced IPP is a derived flow based on sponsor_id and assurance_level we thought it would be a level of indirection that wouldn't require some context to understand what the field was. On the other hand, sponsor_id in and of itself doesn't imply what it's used for but it's a more primitive value, hopefully requiring less context to grok. Another consideration was that sponsor_id might be something that's useful outside of EIPP since it seems like USPS is using that field as a flag on their side.

I think we can be persuaded either way but ultimately we thought that sponsor_id was a harmless enough attribute where even if we got it wrong or EIPP isn't a thing, it's still relevant to our current flow.

Glad to see y'all moving EIPP forward! 🥳

My instinct is that what we care about is really "Is this an EIPP enrollment?" vs. "What sponsor_id attribute did we send over the API?" sponsor_id really has no intrinsic meaning other than an opaque identifier with USPS, and what really matters (I think) is whether we were evaluating them as E-IPP or ID-IPP. Would it make sense to store either a type (more descriptive name strongly welcomed) attribute that's an enum of (id_ipp, e_ipp), or an is_eipp? boolean?

This is a weakly-held opinion formulated having not worked on this part of the code in more than a month now. (😢❤️) If you already had this discussion and arrived at the conclusion that storing sponsor_id is the best approach, I'm A-OK with that.

@eileen-nava eileen-nava self-requested a review June 6, 2024 20:17
Copy link
Copy Markdown
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

I locally observed the migration again and observed sponsor_id on the InPersonEnrollment model. Good stuff. I'm approving this PR as is. Keep me posted if I can be helpful in getting more program-wide feedback.
P.S. Also, I hit similar lint errors yesterday with my build, I think they should go away after merging in the latest main.

@KeithNava KeithNava force-pushed the keithw/LG-13233-persist-eipp-status-on-enrollment branch from 1192bad to 4b82de0 Compare June 10, 2024 17:52
@KeithNava KeithNava merged commit e95d466 into main Jun 10, 2024
@KeithNava KeithNava deleted the keithw/LG-13233-persist-eipp-status-on-enrollment branch June 10, 2024 18:37
brandemix pushed a commit to brandemix/18F-identity-idp that referenced this pull request Jun 17, 2024
* feat: add sponsor_id to in_person_enrollments

* changelog: Upcoming Features, In-person proofing, persist sponsor_id on in_person_enrollments

* feat: add comment for new sponsor_id field
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.

5 participants