P20-1796: Add unique constraint on lti_courses #70737
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46e654e1ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...45032_add_unique_index_to_lti_courses_on_lti_integration_id_and_context_id_and_deleted_at.rb
Outdated
Show resolved
Hide resolved
…e-db-unique-constraint
carl-codeorg
left a comment
carl-codeorg
left a comment
There was a problem hiding this comment.
Does this migration need to wait until we manually resolve existing duplicate records?
Yes, otherwise it fails. But we do not have to hard delete duplicate records because soft deleted records are ignored |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3efc7196b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| class AddUniqueIndexToLtiCoursesOnLtiIntegrationIdAndContextIdAndActive < ActiveRecord::Migration[6.1] | ||
| def up | ||
| execute <<~SQL.squish | ||
| CREATE UNIQUE INDEX index_lti_courses_on_context_integration_active ON lti_courses ( |
There was a problem hiding this comment.
CREATE UNIQUE INDEX will abort this migration as soon as lti_courses contains any existing duplicate active pair (context_id, lti_integration_id with deleted_at IS NULL), and there is no cleanup step in this change to make those rows unique first. In environments that already accumulated duplicates, deploy will fail at migration time; add a pre-index data migration (or equivalent dedupe step) before creating the constraint.
Useful? React with 👍 / 👎.
|
Should we provision a clone of our production database to test this change? |
The unit tests should be enough to confirm it works as expected, and since there are only about 10,000 records, creating this index should not be an issue once we remove the existing duplicates |
3 participants
Links