Skip to content

LG-6708 Add additional columns to in_person_enrollments table #6556

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.

Sign up for GitHub

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

Merged
tomas-nava merged 5 commits into from
Jul 14, 2022
Merged

LG-6708 Add additional columns to in_person_enrollments table #6556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a85a680
completed changes for LG-6708
x341x Jul 5, 2022
29c14c5
updated in_person_enrollment_spec test to match updates to class InPe…
x341x Jul 11, 2022
02a0dc9
changelog: Upcoming Features, In-Person Proofing, Add additional colu…
x341x Jul 11, 2022
801c261
cleanup
Jul 14, 2022
8593c09
Merge remote-tracking branch 'origin/main' into shanice/lg-6708-add-n…
Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions app/models/in_person_enrollment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ class InPersonEnrollment < ApplicationRecord
belongs_to :user
belongs_to :profile
enum status: {
pending: 0,
passed: 1,
failed: 2,
expired: 3,
canceled: 4,
establishing: 0,
Copy link
Copy Markdown
Contributor

@aduth aduth Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Reading the associated ticket, if the new establishing status is meant to allow us to create an enrollment record before we've called out to the USPS API, would it also be expected that the Enrollment's profile could now also be optional? I guess I'm imagining a scenario where we create the enrollment early enough in the process that the profile hasn't been created yet, we'd need to save it without a profile, and update it later to assign the profile (when the profile is created at the password submission step).

Am I understanding correctly, @x341x @tomas-nava ?

Copy link
Copy Markdown
Contributor

@tomas-nava tomas-nava Jul 22, 2022

Choose a reason for hiding this comment

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

Following up on this; at the moment the enrollment is only in establishing for a few moments before it's set to pending, and this all happens when the profile definitely exists. If we end up needing to create the enrollment earlier, we will think about whether the profile should be optional.

pending: 1,
passed: 2,
failed: 3,
expired: 4,
canceled: 5,
Comment on lines +5 to +10
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 7, 2022

Choose a reason for hiding this comment

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

In general I think renumbering enums like this is dangerous.... since we haven't launched yet this is probably fine. I think it would preferable to do establishing: 7 in the future

Copy link
Copy Markdown
Contributor Author

@x341x x341x Jul 7, 2022

Choose a reason for hiding this comment

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

Due to it not being in production we won't break anything also establishing is the phase that comes before pending so logically that order makes sense

}

validate :profile_belongs_to_user
Expand Down
6 changes: 6 additions & 0 deletions db/primary_migrate/20220705152334_add_address_and_location_to_in_person_enrollment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddAddressAndLocationToInPersonEnrollment < ActiveRecord::Migration[6.1]
def change
add_column :in_person_enrollments, :current_address_matches_id, :boolean, comment: "True if the user indicates that their current address matches the address on the ID they're bringing to the Post Office."
add_column :in_person_enrollments, :selected_location_details, :jsonb, comment: "The location details of the Post Office the user selected (including title, address, hours of operation)"
Copy link
Copy Markdown
Contributor

@aduth aduth Jul 7, 2022

Choose a reason for hiding this comment

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

Does USPS give us any location identifier we could use instead of storing a blob of the full details? A concern I'd have is that we'd be storing a lot of duplicate data with this.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 7, 2022

Choose a reason for hiding this comment

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

Unforunately there's no API to look up locations by ID at this point

Copy link
Copy Markdown
Contributor

@aduth aduth Jul 7, 2022

Choose a reason for hiding this comment

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

Do we need to be able to do a look-up with this field data? I'd wonder if we could at least create our own identifier as e.g. a fingerprint/hash of the location data blob.

Copy link
Copy Markdown
Contributor

@tomas-nava tomas-nava Jul 13, 2022

Choose a reason for hiding this comment

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

No, we won't need to do a look-up with this field data, for now.

end
end
13 changes: 13 additions & 0 deletions ...rimary_migrate/20220705164321_change_unique_status_constraint_on_in_person_enrollments.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class ChangeUniqueStatusConstraintOnInPersonEnrollments < ActiveRecord::Migration[6.1]
disable_ddl_transaction!

def up
remove_index :in_person_enrollments, name: 'index_in_person_enrollments_on_user_id_and_status', algorithm: :concurrently if index_exists?(:in_person_enrollments, [:user_id, :status], name: "index_in_person_enrollments_on_user_id_and_status")
add_index :in_person_enrollments, [:user_id, :status], unique: true, where: '(status = 1)', algorithm: :concurrently
end

def down
remove_index :in_person_enrollments, name: 'index_in_person_enrollments_on_user_id_and_status', algorithm: :concurrently if index_exists?(:in_person_enrollments, [:user_id, :status], name: "index_in_person_enrollments_on_user_id_and_status")
add_index :in_person_enrollments, [:user_id, :status], unique: true, where: '(status = 0)', algorithm: :concurrently
end
end
5 changes: 3 additions & 2 deletions db/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#
# It's strongly recommended that you check this file into your version control system.


ActiveRecord::Schema.define(version: 2022_07_05_200821) do

# These are extensions that must be enabled in order to support this database
Expand Down Expand Up @@ -291,8 +290,10 @@
t.integer "status", default: 0, comment: "The status of the enrollment"
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
t.boolean "current_address_matches_id", comment: "True if the user indicates that their current address matches the address on the ID they're bringing to the Post Office."
t.jsonb "selected_location_details", comment: "The location details of the Post Office the user selected (including title, address, hours of operation)"
t.index ["profile_id"], name: "index_in_person_enrollments_on_profile_id"
t.index ["user_id", "status"], name: "index_in_person_enrollments_on_user_id_and_status", unique: true, where: "(status = 0)"
t.index ["user_id", "status"], name: "index_in_person_enrollments_on_user_id_and_status", unique: true, where: "(status = 1)"
t.index ["user_id"], name: "index_in_person_enrollments_on_user_id"
end

Expand Down
6 changes: 3 additions & 3 deletions spec/models/in_person_enrollment_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
describe 'Status' do
it 'defines enum correctly' do
should define_enum_for(:status).
with_values([:pending, :passed, :failed, :expired, :canceled])
with_values([:establishing, :pending, :passed, :failed, :expired, :canceled])
end
end

Expand All @@ -27,9 +27,9 @@
user = create(:user)
profile = create(:profile, :verification_pending, user: user)
profile2 = create(:profile, :verification_pending, user: user)
create(:in_person_enrollment, user: user, profile: profile)
create(:in_person_enrollment, user: user, profile: profile, status: :pending)
expect(InPersonEnrollment.pending.count).to eq 1
expect { create(:in_person_enrollment, user: user, profile: profile2) }.
expect { create(:in_person_enrollment, user: user, profile: profile2, status: :pending) }.
to raise_error ActiveRecord::RecordNotUnique
expect(InPersonEnrollment.pending.count).to eq 1
end
Expand Down