-
Notifications
You must be signed in to change notification settings - Fork 112
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
LG-13233: add sponsor_id to in_person_enrollments model #10759
Conversation
There was a problem hiding this 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.
@@ -0,0 +1,5 @@ | |||
class AddSponsorIdToInPersonEnrollment < ActiveRecord::Migration[7.1] | |||
def change | |||
add_column :in_person_enrollments, :sponsor_id, :string |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
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?" 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 |
@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 I think we can be persuaded either way but ultimately we thought that
|
There was a problem hiding this 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.
1192bad
to
4b82de0
Compare
* 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
🎫 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.
in_person_enrollments