Skip to content
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: add alert report timeout limits #12926

Merged
merged 9 commits into from
Feb 22, 2021

Conversation

riahk
Copy link
Contributor

@riahk riahk commented Feb 3, 2021

SUMMARY

  • Working Timeout and Grace Period fields should not be negative
  • Adds min value to modal input fields to prevent user from setting negative values

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Manual Test:

  1. Open alert/report modal
  2. Enter values for Working Timeout/Grace Period (where applicable)
  3. Confirm that value cannot be set below 0

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Feb 3, 2021

Codecov Report

Merging #12926 (67a56d8) into master (2ce7982) will increase coverage by 13.72%.
The diff coverage is 51.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12926       +/-   ##
===========================================
+ Coverage   53.06%   66.78%   +13.72%     
===========================================
  Files         489      493        +4     
  Lines       17314    29084    +11770     
  Branches     4482        0     -4482     
===========================================
+ Hits         9187    19425    +10238     
- Misses       8127     9659     +1532     
Flag Coverage Δ
cypress ?
python 66.78% <51.42%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/constants.py 100.00% <ø> (ø)
superset/examples/birth_names.py 73.19% <ø> (ø)
...43f2fdb_add_granularity_to_charts_where_missing.py 0.00% <0.00%> (ø)
...s/260bf0649a77_migrate_x_dateunit_in_time_range.py 0.00% <0.00%> (ø)
...ons/versions/41ce8799acc3_rename_pie_label_type.py 0.00% <0.00%> (ø)
superset/connectors/sqla/views.py 62.43% <4.34%> (ø)
superset/views/datasource.py 88.70% <16.66%> (ø)
superset/viz.py 58.61% <27.27%> (ø)
superset/views/core.py 72.91% <71.42%> (ø)
superset/charts/commands/exceptions.py 92.85% <77.77%> (ø)
... and 925 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1bb7f4...67a56d8. Read the comment docs.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to add a validation on the backend also:
https://github.com/apache/superset/blob/master/superset/reports/schemas.py#L162

fields.Integer(validate=[Range(min=1, error="Value must be greater than 0")])

@willbarrett
Copy link
Member

Let's also add a test for this change.

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Do we enforce this on the BE as well?

@riahk riahk force-pushed the moriah/alert-report-timeout-limits branch from 3dbdf83 to 9c7a1d2 Compare February 17, 2021 21:11
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 18, 2021
Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment on the python. Let's add some tests for the back-end validations as well.

description=grace_period_description,
example=60 * 60 * 4,
default=60 * 60 * 4,
validate=[Range(min=1, error=_("Value must be greater than 0"))],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is correct, as reports do not require a grace period.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be mistaken, but I'm pretty sure we use this schema for both alerts and reports.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 19, 2021
@riahk riahk requested a review from willbarrett February 19, 2021 22:40
@riahk riahk requested a review from dpgaspar February 22, 2021 19:02
@riahk riahk merged commit fc180ab into apache:master Feb 22, 2021
@riahk riahk deleted the moriah/alert-report-timeout-limits branch February 22, 2021 19:12
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants