Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Jm mb 506 part2 zero down migration #3438
Jm mb 506 part2 zero down migration #3438
Changes from 19 commits
48d8842
5795b06
1cf6ab7
8505af3
4821be6
a82dfa4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #3413, we added the new
mto_service_item_id
column and copied theservice_item_id
value into that new column for everypayment_service_items
record. There were no code changes with that PR. So wouldn't the code have still been writing toservice_item_id
inpayment_service_items
? And if that's the case, then wouldn't we be losing theservice_item_id
/mto_service_item_id
for anypayment_service_items
added between PRs when we drop theservice_item_id
column above? Or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reggieriser I think you are right. Should I do that copy again here?
This did cross my mind and I forgot :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, none of this is actually live so it's not an issue. But I would like to get it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that others may look to this as an example pattern to follow, though. I think the docs state to do it the way you did, too, so we should probably clarify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this migration to do the copy again, but wondering if I should do what I'm doing or maybe even only update if
mto_service_item_id
isNULL
like:instead of what I picked which was:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the where clause at all? In theory, you don't know if theservice_item_id
has changed between the first PR and this one, so you could lose data if you only update a subset of records (although in practice, that's unlikely right now). Could you just update all the records again since you aren't usingmto_service_id
in the code until this PR?Ignore the above in relation to this PR -- I was confused about the ordering of some related PRs. #3401 comes first, then this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way you have the
UPDATE
currently is the right way to do this given the order of the PRs so far.