Register ECT/Mentors - Create TeacherEmail table to store emails #48
Conversation
|
Review app deployed to https://cpd-ec2-review-48-web.test.teacherservices.cloud |
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
4874df1 to
1f5aada
Compare
| @@ -0,0 +1,5 @@ | |||
| class AddEmailToECTAtSchoolPeriods < ActiveRecord::Migration[8.0] | |||
| def change | |||
| add_column :ect_at_school_periods, :email, :string | |||
There was a problem hiding this comment.
Could we add a unique constraint in some form on these? We might need to add an emails table though and make these references to that table as I'm not sure having them in place would allow it? Any thoughts?
There was a problem hiding this comment.
Hey @tonyheadford - as discussed have added constraints on the model and db
There was a problem hiding this comment.
As discussed we are leaving constraints alone for now
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
41a2731 to
d62b850
Compare
| add_index :ect_at_school_periods, :email, unique: true, where: "email IS NOT NULL", name: "unique_email_on_ect_at_school_periods" | ||
| add_index :mentor_at_school_periods, :email, unique: true, where: "email IS NOT NULL", name: "unique_email_on_mentor_at_school_periods" |
There was a problem hiding this comment.
I don't think we need the where: clause here, nulls aren't considered equal.
There was a problem hiding this comment.
Depends possibly - the default behaviour of the unique index is to disregard null values when enforcing uniqueness but if we are potentially going to have lots of emails with 'null' then adding the WHERE clause will make the index smaller.
Happy to take your recommendation on this!
There was a problem hiding this comment.
Will this work for teachers:
- using their personal email and they move school and want to use the same email
- leave a school and then return at some point in the future and wish to keep their original school email
There was a problem hiding this comment.
As discussed we are leaving constraints alone for now
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
10ae35c to
bcb339a
Compare
TobyRet
changed the title
| @@ -12,12 +12,16 @@ def build | |||
| success = true | |||
| school_periods.each do |period| | |||
| school = School.find_by!(urn: period.urn) | |||
| # Check where the email address may come from | |||
| teacher_email = TeacherEmail.find_or_create_by!(teacher_id: teacher.id, email: nil) | |||
There was a problem hiding this comment.
@tonyheadford - can the email be sourced or should we set to nil for now?
| @@ -12,12 +12,16 @@ def build | |||
| success = true | |||
| school_periods.each do |period| | |||
| school = School.find_by!(urn: period.urn) | |||
| # Check where the email address may come from | |||
There was a problem hiding this comment.
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
ce1cc70 to
ede5ad3
Compare
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
ede5ad3 to
f208924
Compare
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
f208924 to
861c2e0
Compare
…dle 'email'. Add email formatting validation
TobyRet
force-pushed
the
1091/migration-email-address-ects-and-mentors
branch
from
861c2e0 to
1ca8dac
Compare
|
Closing in favour of #60 |
|
Review app ECF2 deployed to https://ec2-review-48-web.test.teacherservices.cloud was deleted |
Ticket
https://github.com/DFE-Digital/register-ects-project-board/issues/1091
Note
This 1 of 2 possible solutions up for discussion. Please also see the other proposed solution.
Changes
teacher_idandemail. Theect_at_school_periodandmentor_at_school_periodtables now reference this new table.teacher_idandemailis unique, as well as ensuring thatemailsby themselves are unique (so that other Mentors and ECTs cannot take an existing address)Validation summary (
teacher_emailstable)