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

One login Account Recovery #10215

Merged
merged 4 commits into from
Jan 13, 2025
Merged

One login Account Recovery #10215

merged 4 commits into from
Jan 13, 2025

Conversation

CatalinVoineag
Copy link
Contributor

@CatalinVoineag CatalinVoineag commented Jan 6, 2025

Context

When going live with one login, a candidate can sign up with a different
email address, not the magic link email address.

For this, we need to allow our candidates to recover their 'old'
account. This commit adds this feature.

We show a banner which the candidate can dismiss or they can click to
recover their old account. They will be asked to input their old email
and a code will be sent to their email. The code is encrypted with
bcrypt.

Changes proposed in this pull request

Account recovery forms
button_to style to look like a link
specs

Guidance to review

Go on QA and try to recover an account. The account that you recover needs to exist in QA DB.

I have created candidates in the QA DB for you to recover. [email protected] Created 3 accounts so you can go +apply up to 3.

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist, if included inform data insights team of the changes
  • If this code adds a column that may include PII, the sanitise.sql script and 0025-protecting-personal-data-in-production-dump.md ADR have been updated.
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Attach the PR to the Trello card

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Database-level enum changes detected

Please include a data migration for these attributes and values:

33a34
> <enum> Candidate.account_recovery_status: ["not_started", "recovered", "dismissed"]

.erb_lint.yml Outdated Show resolved Hide resolved
scope :not_expired, -> { where('created_at >= ?', 15.minutes.ago) }

def self.generate_code
Array.new(6) { rand(0..9) }.join
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't care about other candidates having the same code because the code is scoped to a specific candidate

valid_request_code = current_candidate.account_recovery_request.codes.not_expired.find do |requested_code|

@CatalinVoineag
Copy link
Contributor Author

Database-level enum changes detected

Please include a data migration for these attributes and values:

33a34
> <enum> Candidate.account_recovery_status: ["not_started", "recovered", "dismissed"]

I think we can ignore this. All the candidates have 'not_started' as their account_recovery_status

Copy link
Contributor

@elceebee elceebee left a comment

Choose a reason for hiding this comment

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

Nice work! A few comments from me before I test on QA.

app/frontend/styles/application.scss Outdated Show resolved Hide resolved
I18n.t('page_titles.account_recovery', email: current_candidate.previous_account_email_address)
end %>

<% content_for :title, page_title %>
Copy link
Contributor

@elceebee elceebee Jan 10, 2025

Choose a reason for hiding this comment

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

Suggested change
<% content_for :title, page_title %>
<% content_for :title, title_with_error_prefix(page_title, @account_recovery.errors.any?) %>

If there is a form error, the page title should change to be Error: page_title

@@ -0,0 +1,27 @@
<% content_for :title, t('page_titles.account_recovery_request') %>
Copy link
Contributor

@elceebee elceebee Jan 10, 2025

Choose a reason for hiding this comment

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

Suggested change
<% content_for :title, t('page_titles.account_recovery_request') %>
<% content_for :title, title_with_error_prefix(t('page_titles.account_recovery_request'), @account_recovery_request.errors.any?) %>

config/locales/candidate_interface/account_recovery.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@elceebee elceebee left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments! This looks great. I haven't tested it on QA, but I know Rebecca and Pete have and we'll do more at the bug party on Thursday, so happy for the code to go in! :shipit:

When going live with one login, a candidate can sign up with a different
email address, not the magic link email address.

For this, we need to allow our candidates to recover their 'old'
account. This commit adds this feature.

We show a banner which the candidate can dismiss or they can click to
recover their old account. They will be asked to input their old email
and a code will be sent to their email. The code is encrypted with
bcrypt.
@CatalinVoineag CatalinVoineag force-pushed the cv/one-login-account-recovery branch from fd9eabf to 8e9360f Compare January 13, 2025 10:11
@CatalinVoineag CatalinVoineag merged commit 60fa716 into main Jan 13, 2025
25 checks passed
@CatalinVoineag CatalinVoineag deleted the cv/one-login-account-recovery branch January 13, 2025 13:01
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.

2 participants