Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Change emails foreign key #3893

Merged
merged 2 commits into from
Jan 17, 2016
Merged

Conversation

aandis
Copy link
Contributor

@aandis aandis commented Jan 16, 2016

This pr only introduces a new column in emails table which references participants.id and modifies email writes to include email.participant_id as well. Ideally we would want this to be deployed before we start copying old values from participant.id to email.participant_id. So that would come in a separate migration.

@aandis
Copy link
Contributor Author

aandis commented Jan 16, 2016

References #835

@rohitpaulk
Copy link
Contributor

Naice!

screen shot 2016-01-18 at 12 18 21 am

We'll need to add the unique constraint on participant_id + address too.

@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

Aha! Missed that. Fixing.

@aandis aandis force-pushed the change-emails-foreign-key branch from 771a328 to 7210b23 Compare January 17, 2016 19:20
@aandis aandis force-pushed the change-emails-foreign-key branch from 7210b23 to 6b349ca Compare January 17, 2016 19:32
@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

Ok fixed. So now do I submit the next migration in a separate file in this pr itself or later in a separate pr with the rest of the changes?

@rohitpaulk
Copy link
Contributor

So now do I submit the next migration in a separate file in this pr itself or later in a separate pr with the rest of the changes?

Hmm, might be cleaner to have that as a part of this PR itself. The migration script should just be a few more lines in branch.sql, if I'm not wrong..

@rohitpaulk
Copy link
Contributor

In reference to #3864 (comment) -

  1. MIGRATION - Add a participant_id column to emails, along with the foreign key constraint. Let's start with NULL values by default, we'll fill in values over time.
  2. CODE CHANGE - Modify code such that wherever emails.participant is written to, it writes to emails.participant_id too.
  3. MIGRATION - Here's the interesting part. Now we have all the time in the world to work on filling in the NULL emails.participant_id values. It's not going to affect the app at all, since the app is still relying on emails.participant for reads.
  4. CODE CHANGE - Once we're confident that we've filled in email.participant_id accurately, the application code can be modified to read from email.participant_id, and write to only email.participant_id.
  5. MIGRATION - email.participant isn't being used anymore, safe to remove.

We'd complete steps 1, 2 and 3 with this PR. When deploying, we should ensure that the migration is done before code is deployed

@rohitpaulk
Copy link
Contributor

Oh actually no... We need step 2 before step 3 - so makes sense to deploy this as is.

rohitpaulk added a commit that referenced this pull request Jan 17, 2016
@rohitpaulk rohitpaulk merged commit ab104a3 into gratipay:master Jan 17, 2016
@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

Yeah but I don't think we want to be copying values before deploying 6b349ca, isn't it? I'll be anyway submitting a separate pr when we decide to complete switch over to email.participant_id and remove participant

@rohitpaulk
Copy link
Contributor

Yeah but I don't think we want to be copying values before deploying 6b349ca, isn't it?

Yes, true. Realized that in #3893 (comment)

@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

OK so this needs 2 more prs I guess. One for 3 and 4 and another then for 5.

@aandis aandis deleted the change-emails-foreign-key branch January 17, 2016 20:19
@rohitpaulk
Copy link
Contributor

I think that 3 and 4 can be clubbed together in a single PR (We'll just deploy them separately)

@rohitpaulk
Copy link
Contributor

Deployed, and verified by adding an email address using my account.

@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

I think that 3 and 4 can be clubbed together in a single PR

Yeah that's what I meant. One pr for 3,4 and another for 5.

@aandis
Copy link
Contributor Author

aandis commented Jan 17, 2016

Deployed, and verified by adding an email address using my account.

Yay! 💃

@chadwhitacre
Copy link
Contributor

!m @aandis @rohitpaulk

I love seeing this progress! 💃

@rohitpaulk
Copy link
Contributor

Yeah that's what I meant. One pr for 3,4 and another for 5.

Oops, misread your comment :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants