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

LG-13233: add sponsor_id to in_person_enrollments model #10759

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

KeithNava
Copy link
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
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.

db/schema.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
class AddSponsorIdToInPersonEnrollment < ActiveRecord::Migration[7.1]
def change
add_column :in_person_enrollments, :sponsor_id, :string
Copy link
Member

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
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
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
Member

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
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
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
2 checks passed
@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