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

187384622/new email model - PR3 - Add schema migration to drop old email columns and Adjust Relevant Dependencies #52

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

perryzjc
Copy link
Member

@perryzjc perryzjc commented Apr 15, 2024

Pivotal Tracker Link

What this PR does:

This pull request is the third in a series intended to facilitate easier code review. The first PR can be found
here. The second PR can be found here.

For this PR, it aims to have a schema migration to drop the old email & personal_email column from Teacher model. It also adjust some old code that has dependencies on the old email fields. For example, personal_email || email_addresses.where(primary: false)&.pluck(:email)&.first now becomes email_addresses.find_by(primary: true)&.email

Include screenshots, videos, etc.

Original DB State

image

DB State : After rails db:migrate

image
Frontend works fine as before
image

Who authored this PR?

  1. Perry (Jingchao) Zhong @perryzjc

How should this PR be tested?

  1. Test the app work fine as before after dropping the old email columns
  2. Also would be nice to ensure the code is clean that has no dependencies on the old email column anymore.
  • Is there a deploy we can view?
  • What do the specs/features test?
  • Are there edge cases to watch out for?

Are there any complications to deploying this?

Yes. Run rails db:migrate to apply the migration.

Checklist:

  • Has this been deployed to a staging environment or reviewed by a customer?
  • Tag someone for code review (either a coach / team member)
  • I have renamed the branch to match PivotTracker's suggested one (necessary for BlueJay) (e.g. michael/12345-add-new-feature Any branch name will do as long as the story ID is there. You can use git checkout -b [new-branch-name])

@perryzjc perryzjc force-pushed the 187384622/new-email-model-pr3 branch from 032b754 to 02f32bb Compare April 15, 2024 21:32
@perryzjc perryzjc changed the title 187384622/new email model - PR3 - Change Email and Make Sure original tests didn't break 187384622/new email model - PR3 - Add schema migration to drop old email columns and Adjust Relevant Dependencies Apr 15, 2024
@perryzjc perryzjc marked this pull request as ready for review April 15, 2024 21:44
@perryzjc perryzjc closed this Apr 15, 2024
@perryzjc perryzjc reopened this Apr 15, 2024
@ArushC
Copy link

ArushC commented Apr 17, 2024

This is good.

@ArushC ArushC merged commit 565307d into main Apr 17, 2024
9 checks passed
@ArushC ArushC deleted the 187384622/new-email-model-pr3 branch April 17, 2024 04:06
@perryzjc perryzjc restored the 187384622/new-email-model-pr3 branch April 19, 2024 19:03
@cycomachead cycomachead deleted the 187384622/new-email-model-pr3 branch May 8, 2024 08:58
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