-
Notifications
You must be signed in to change notification settings - Fork 4
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
add api endpoint for commitment merging #661
Conversation
486c016
to
9be7db9
Compare
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 will do a more detailed review later today or tomorrow, but for the time being, here are comments on the DB migration and model:
06a7a95
to
a6f38ab
Compare
ed56a1b
to
9a6b216
Compare
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.
FYI, I read a bit more (didn't get to finish again....). To make the feedback cycle shorter, I added some commits for minor stuff. Please review my commits, esp. the one touching the migrations.
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.
We are nearing completion here. I pushed another commit with just whitespace changes (esp. in tr.DBChanges().AssertEqualf()
; that fellow normalizes whitespace on its input, so you can make liberal use of indentation to make the test code look neat).
Finally, since #667 was merged, you will need to do a rebase. There will be a few merge conflicts.
As per our usual convention.
e62b9e5
to
614d361
Compare
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.
When you merge this, please take a look at the qa-de-1 database to see how the DB migration goes.
Checklist: