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 - PR1 - Add new EmailAddress Model without Schema Change on Original Models #308

Closed
wants to merge 77 commits into from

Conversation

perryzjc
Copy link
Contributor

Some review have already been made on the Original PR

Pivotal Tracker Link1

What This PR Does

This pull request is the first in a series intended to facilitate easier code review. It restructures the teacher model to enable a teacher to have multiple email_addresses. The primary goals of this feature are:

  • To allow administrators to manually link several email addresses to a teacher, while a teacher can only modify their primary email (typically a school email).
  • To split the implementation across three PRs for clarity and manageability. This initial PR focuses on backend model changes, updates to value access in the frontend template and controller, modifications to existing tests, and adjustments to some seed data and fixtures.

Notably, this PR does not introduce new tests. Its primary objective is to ensure that existing tests continue to pass without issues. Subsequent PRs will include:

  • A library task script for migrating current data (not schema) to align with the updated data model, accompanied by an RSpec test.
  • Frontend enhancements allowing administrators to add multiple personal emails for a teacher and displaying these emails for both administrators and teachers, complete with comprehensive testing.

Screenshots and Visuals

After running rails db:migrate and rails db:seed, the following changes are observable:

  1. Teacher names and other associated attributes remain visible, except for the email field for original entries in the database. This is due to the migration of the database schema without corresponding data migration, leaving the new email model without the original email information. A forthcoming PR will address this with a data migration script.
  2. The current frontend does not yet support displaying multiple email addresses and instead shows a primary email alongside an array of non-primary emails for each teacher.
image image

Who authored this PR?

  1. Perry (Jingchao) Zhong (@perryzjc)
  2. Michael Tao (@realmichaeltao )

How should this PR be tested?

At least make sure every original feature doesn't break.

  • Is there a deploy we can view?
  • What do the specs/features test?
    Specs & Features are basically same as the original tests, with changes only relevant to some keywords changing & the way mocking the data (such as originally we can directly create a teacher with a email, now we need to associate them together)
  • Are there edge cases to watch out for?
    For this PR, at least make sure it didn't break any existing test.

Are there any complications to deploying this?

  1. rails db:migrate
  2. rails db:seed

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])

razztech and others added 30 commits March 7, 2024 19:41
Country field added for international schools
@perryzjc perryzjc changed the title 187384622/new email model 187384622/new email model - PR1 - Add new EmailAddress Model without Schema Change on Original Models Apr 19, 2024
@perryzjc perryzjc mentioned this pull request Apr 19, 2024
3 tasks
@ArushC ArushC mentioned this pull request Apr 22, 2024
3 tasks
@perryzjc
Copy link
Contributor Author

@cycomachead To avoid merge conflict, it's easier to just review Arush's PR for merging teacher, which already merges PR1,2,3&4 from New Email Model and handled merge conflicts.

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.

3 participants