feat: new reports models api #11606
feat: new reports models api #11606
Conversation
…tor-superset into feat/reports-models-api
Codecov Report
@@ Coverage Diff @@
## master #11606 +/- ##
==========================================
+ Coverage 66.41% 67.06% +0.65%
==========================================
Files 878 885 +7
Lines 42272 42860 +588
Branches 3949 3971 +22
==========================================
+ Hits 28074 28744 +670
+ Misses 14095 14018 -77
+ Partials 103 98 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
dpgaspar
changed the title
dpgaspar
changed the title
There was a problem hiding this comment.
This is looking really good! A few small things to take care of, but nothing major. Thanks for all this work @dpgaspar !
superset/reports/commands/delete.py
Outdated
| self._model = ReportScheduleDAO.find_by_id(self._model_id) | ||
| if not self._model: | ||
| raise ReportScheduleNotFoundError() | ||
| # TODO check integrity |
There was a problem hiding this comment.
Let's not leave TODOs lying around. Can we address in this PR?
There was a problem hiding this comment.
It's already addressed, this is a left over, like a note to self. (deleted)
|
|
||
|
|
||
| class ReportScheduleDeleteIntegrityError(CommandException): | ||
| message = _("Report Schedule has associated logs or recipients.") |
There was a problem hiding this comment.
It should be possible to delete a ReportSchedule with associated logs and recipients. Neither of those related objects have meaningful stand-alone delete operations and they make no sense without the related ReportSchedule record. Let's use a cascading delete and remove this error.
There was a problem hiding this comment.
Totally agree, it's exactly how I done it. Deleted
| if report_type == ReportScheduleType.ALERT: | ||
| database_id = self._properties.get("database") | ||
| if not database_id: | ||
| exceptions.append(ReportScheduleAlertRequiredDatabaseValidationError()) |
There was a problem hiding this comment.
While there is objectively nothing wrong with this class name, these are really getting out of hand 🤣
$ echo ReportScheduleAlertRequiredDatabaseValidationError|tr -d '\n' | wc -c
50
* feat: new report schedule models * lint and unique constraint * support sqlite * fix sqlite * add audit mixin and minor fixes * feat(api): alerts and reports REST API * feat: new report schedule models * lint and unique constraint * support sqlite * fix sqlite * add audit mixin and minor fixes * feat(api): alerts and reports REST API * draft working version * add tests * test * black * remove copy pasta * solve dashboard object representation being used on cache * tests and custom filter * fix PUT has PATCH on active field * create feature flag * fix lint * address comments
mistercrunch
added
🏷️ bot
SUMMARY
Implements a REST API for alerts and reports.
Note: This feature is behind a new feature flag
ALERT_REPORTSPR 2 reference for the first PR (#11550).
POST payload OpenAPI spec:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION