-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
fix(reports): make name unique between alerts and reports #12196
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12196 +/- ##
==========================================
- Coverage 63.06% 62.84% -0.23%
==========================================
Files 994 995 +1
Lines 49075 49106 +31
Branches 4984 4983 -1
==========================================
- Hits 30950 30861 -89
- Misses 17925 18045 +120
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
else: | ||
with op.batch_alter_table( | ||
"report_schedule", copy_from=report_schedule |
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.
This was required to remove UNIQUE (name)
that is not an actual constraint from SQLite
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.
LGTM. SQLite constraints are such a pain to deal with, we should consider dropping official support for it soon.
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.
LGTM!
with op.batch_alter_table( | ||
"report_schedule", copy_from=report_schedule | ||
) as batch_op: | ||
batch_op.create_unique_constraint( | ||
"uq_report_schedule_name_type", ["name", "type"] | ||
) |
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.
This took a loong while for me to wrap my head around 😆 A small comment here could be really helpful for the next person trying to understand what's going on.
SUMMARY
Uniqueness was being imposed on Alerts/Report names, preventing user's from creating an Alert name "X" and a Report with the same name "X". This allows it while still enforcing name uniqueness on Alerts and Reports separately
Also guarantees that a database reference is not sent when creating a report
ADDITIONAL INFORMATION