187384622/new email model - PR1 - Change Email and Make Sure original tests didn't break

[perryzjc]

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.

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]

The backend code looks good for creating the email model and updating the tests and switching everything to use that model. I also like that you are breaking up the task into smaller PRs since this is a big feature that requires a lot of changes to the codebase. I have a couple of things that I would change:

1. I would name the model email instead of email_address for the sake of simplicity and it just looks better in my opinion
2. I would remove the frontend where you actually display the emails of each user in Ruby array format. You broke up the task into two PRs (backend and model creation + frontend/testing/ux). Thus, this PR should just update the backend and shouldn't change what the user sees on the website. I think that this is the best approach since you want the PRs to be fully independent from each other. You want the app to function properly regardless if the frontend of the feature is implemented or not. That being said, for the example below I would just display the primary email ([email protected]) and remove the array of emails from being displayed.
   

[fp456]

Looks good! The code was written concisely and I don't see any major issues except what Razvan mentioned. The testing also seems cohesive and the new lines were properly tested.

Thanks for reducing the PR size. 🙌

[perryzjc]

@razztech @fp456, I just updated this PR based on the feedback from you and Michael.

1. Regarding the naming of the email model, Michael prefers it to remain as email_addresses, so this part is unchanged.
2. Regarding the personal email display issue, I have now removed personal emails from the table. For the views that originally used personal emails, I am currently displaying the first one (.first). In this way, we can ensure that it passes all the existing tests and that customers use it the same way.
3. I also resolved the issue where many primary emails disappeared from the original records. This was because one of my migrations dropped the email column from teacher, causing the loss of data. It should not have been done at this time, but rather later, after my second PR regarding data migration. After the code like email_addresses.find_by(primary: true)&.email || self[:email], email_addresses.where(primary: false)&.pluck(:email)&.first || self.personal_email, it becomes compatible for this PR.

Below are screenshots of the current effect. Customers can see the views the same as the original, and new teachers can be created using the new backend (with new email model).

[cycomachead]

This is good! A lot here.

One high level comment: I think perhaps teacher#create and teacher#update should only ever update the primary email. Updating of personal emails should probably happen with separate controller actions. This will make the params / other logic a lot easier.

That will simplify the forms, controller actions, and params handling.

[cycomachead]

This isn't for this PR, but the creation workflow when signed in seems like it doesn't make sense anymore. We should revisit this...

[perryzjc]

Marked. We will discuss it during next available meeting.

[perryzjc]

@cycomachead, I have just finished updating based on the review you provided as of Apr 11, 2:30 PM. The only uncertain part I have is regarding separating the updating of personal email to a different controller:

I am not sure what is expected for separating that, as updating personal email was already in the original code under #teacher:create. So far, what I have done relevant to this part of the review is refactoring the parameter handling to be something like this, helping to reduce the code complexity: