-
Notifications
You must be signed in to change notification settings - Fork 9
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
One login integration migrations #10136
Conversation
5096ad7
to
e07f95b
Compare
These are all the migrations we need for our one login integration, to allow a candidate to login or recover their account.
e07f95b
to
f16e7f8
Compare
class CreateOneLoginAuths < ActiveRecord::Migration[8.0] | ||
def change | ||
create_table :one_login_auths do |t| | ||
t.string :email, null: false |
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.
Pedantry warning: elsewhere in the code we use 'email_address' rather than 'email'. Can we be consistent?
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.
No idea why the pipeline is failing - looks like you've handled the additional fields 🤷♂️
def change | ||
create_table :account_recovery_request_codes do |t| | ||
t.string :hashed_code, null: false | ||
t.index :hashed_code, unique: true |
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 sure this has to be unique 🤔 these would work the same as a password - we wouldn't want passwords to be unique
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.
Yeah, that's true
ActiveRecord::Schema[8.0].define(version: 2024_11_21_144711) do | ||
create_sequence "qualifications_public_id_seq", start: 120000 | ||
ActiveRecord::Schema[8.0].define(version: 2024_12_03_151209) do | ||
create_sequence "qualifications_public_id_seq", start: 30690 |
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.
Does this line normally change? Just a bit concerned as it is actually lower than the original start number for the sequence
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'll put it back how it was
Context
We want to allow our candidates to login into apply with one login. We also want to allow them to recover their account if their one login email is different from the candidate's email.
These are all the migrations we need for our one login integration, to allow a candidate to login or recover their account.
This is based on the one login spike
Changes proposed in this pull request
We need to allow the user to reclaim their account with any code valid in the last 1 hour for example. That's why
AccountRecoveryRequest
has_manyAccountRecoveryRequestCodes
Guidance to review
Does it make sense?
Things to check