fix: [alert] allow decimal for alert threshold value #17751

graceguo-supercat · 2021-12-14T21:54:37Z

SUMMARY

Our users typing in decimal value in alert condition threshold, and then Superset returns error message from server-side.

This PR is to allow backend accept decimal number, and add a simple tooltip for this input field.

cc @dpgaspar

TESTING INSTRUCTIONS

CI and manual test.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

etr2460

any reason why not to adjust the onThresholdChange function as well to make inputting a non integer impossible?

graceguo-supercat · 2021-12-14T22:04:24Z

onThresholdChange

server-side has value validation. I think add front-end validation is not necessary.

etr2460 · 2021-12-14T22:06:03Z

I guess why let the user type in something invalid at all? We're adding the tooltip, but that would only be seen if you hover over it. You wouldn't let a user type letters into a credit card number input (for example), so why not filter out/disallow decimal points from the text here?

graceguo-supercat · 2021-12-14T22:40:31Z

Server-side has validation, front-end doesn't really need to do extra validation, no matter its decimal or letter.

why let the user type in something invalid at all? why not? I think it is ok to let user type in whatever they want, server-side will prevent bad data get into database. As a Superset user come to Alert setting page, i assume he /she will type in values with his/her common sense. When error / exception showing up, he/she will check tooltip and adjust his/her behavior after that.

codecov · 2021-12-14T22:58:47Z

Codecov Report

Merging #17751 (72b9cd1) into master (215ee08) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #17751      +/-   ##
==========================================
- Coverage   67.78%   67.78%   -0.01%     
==========================================
  Files        1602     1605       +3     
  Lines       64146    64208      +62     
  Branches     6773     6790      +17     
==========================================
+ Hits        43484    43525      +41     
- Misses      18809    18827      +18     
- Partials     1853     1856       +3

Flag	Coverage Δ
hive	`81.79% <100.00%> (+0.01%)`	⬆️
javascript	`54.82% <100.00%> (-0.02%)`	⬇️
mysql	`82.17% <100.00%> (+0.01%)`	⬆️
postgres	`82.22% <100.00%> (+0.01%)`	⬆️
presto	`82.08% <100.00%> (+0.01%)`	⬆️
python	`82.70% <100.00%> (+0.01%)`	⬆️
sqlite	`81.90% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`53.54% <100.00%> (+0.12%)`	⬆️
superset/reports/schemas.py	`98.78% <100.00%> (ø)`
...src/dashboard/components/PropertiesModal/index.tsx	`66.03% <0.00%> (-0.85%)`	⬇️
superset/dashboards/dao.py	`95.55% <0.00%> (-0.72%)`	⬇️
superset/datasets/commands/importers/v1/utils.py	`58.33% <0.00%> (-0.50%)`	⬇️
superset/models/core.py	`90.00% <0.00%> (ø)`
superset/migrations/env.py	`0.00% <0.00%> (ø)`
superset/datasets/schemas.py	`96.61% <0.00%> (ø)`
superset-frontend/src/featureFlags.ts	`100.00% <0.00%> (ø)`
superset-frontend/src/components/Icons/index.tsx	`100.00% <0.00%> (ø)`
... and 33 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 215ee08...72b9cd1. Read the comment docs.

etr2460 · 2021-12-15T02:43:13Z

idk, i still think a change like this is worthwhile (and pretty easy):

maybe someone else from frontend can comment/tiebreak? @ktmud or @rusackas

ktmud · 2021-12-15T17:05:53Z

Why do we have such a server side restriction in the first place? It feels like here at least floats should be allowed, too. It's very common to set up alerts on percent/ratio metrics, etc.

Can we maybe just change the server side validation to fields.Float()? It's a JSON field so no db migration is needed.

cc @dpgaspar

ktmud · 2021-12-15T17:11:43Z

I'm also surprised we don't have inline form validation on the client side. It feels like we should at least introduce some common pattern of how to do form validation for forms outside of the Explore controls.

It might be worth starting a front-end infra project to tackle things like this.
cc @rusackas @zhaoyongjie @villebro

graceguo-supercat · 2021-12-15T21:18:54Z

to @etr2460: your suggestion is, user type in 3.5, Superset secretly truncate to 3 and save into backend without user notice? User still believe Superset accept decimal but actually not? 😳 I think this behavior is even worse than throw exception.

to @ktmud: fields.Float() was part of my plan, but it raises another question is what is precision? what if user type in something like 3.1415926535 8979323846 2643383279 5028841971 6939937510 5820974944 5923078164 0628620899 8628034825 3421170679. We still need to let user know the limit (we can add it in the tooltip). If the extra precision is needed, user still need to handle it like user handles integer. But handling short float is a pretty common use case.

I think add front-end input validation just for this input field is very random and not necessary. For Superset, site-wide consistent front-end validation behavior is probably more desirable for chart controls, etc. If in the near future, Superset has plan for explore view, like consistent chart controls or similar features, this validation framework is a good candidate.

etr2460 · 2021-12-15T21:26:56Z

your suggestion is, user type in 3.5, Superset secretly truncate to 3 and save into backend without user notice?

nope, that handler sets the value in state and then renders it to the input (because it's a controlled component). The effect of this change would be that pressing the period key would put nothing in the input, and pasting a decimal string would truncate to everything before the decimal:

                    <input
                      type="number"
                      name="threshold"
                      disabled={conditionNotNull}
                      value={
                        currentAlert &&
                        currentAlert.validator_config_json &&
                        currentAlert.validator_config_json.threshold !==
                          undefined
                          ? currentAlert.validator_config_json.threshold
                          : ''
                      }
                      placeholder={t('Value')}
                      onChange={onThresholdChange}
                    />

just like how making the input a number type should mean that typing letters doesn't work

graceguo-supercat · 2021-12-15T22:07:52Z

Besides changes you added, to make your solution work, you also need to draft a UI to display the error message, right? And did your solution handle 123 456 or 😀 😃 😄 😁?

that's what i mean the simple front-end validation is not necessary. It's not completed, not well designed, not consistent with the rest of Superset input. So i think it's random and not necessary.

ktmud · 2021-12-15T22:12:11Z

I think running into the precision problem is probably less frustrating than not being able to type in float. It's unlikely users would need the alert threshold to be highly sensitive with many decimals.

Also, with input[type=number], we can probably just skip the frontend validation.

graceguo-supercat · 2021-12-15T22:16:37Z

Also, with input[type=number], we can probably just skip the frontend validation.

~~oh thanks! i think this is good.~~ Currently it already had number type constraint. It looks to me that avoid front-end validation is probably the original plan.

So far I think support decimal is a general acceptable solution. I will update this PR.

etr2460 · 2021-12-16T00:03:10Z

Agree that supporting decimal values is the better solution, thanks for making that change! Can you also change the PR title to be accurate?

And did your solution handle 123 456 or 😀 😃 😄 😁?

yup, because the input was already of type="number". unfortunately html inputs didn't have an "integer" type :/

etr2460

stamping, but can you update the screenshots and the test plan to reflect the validation you did that decimals work? thanks!

etr2460 · 2021-12-16T00:04:10Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

+                    {t('Value')}{' '}
+                    <InfoTooltipWithTrigger
+                      tooltip={t(
+                        'Threshold value should be double precision number',


it doesn't need to be a double right? an integer is still fine. do we even need the tooltip now? Or if you want to add it, you could make it "Threshold can be an integer or a float"

double precision number could be integer or float right?
And, if you type in 3.14159265358979323846264338327950288419716939937510 it will be truncated to 3.141592653589793 by backend. I think it is necessary to let users know there is a limit.

ah, i got confused by the fact that the schema called it a float, but python calls its doubles floats 🤦 carry on

dpgaspar

Looks good, we need to make sure this is not a breaking change. left a comment regarding tests

dpgaspar · 2021-12-16T15:34:10Z

tests/integration_tests/alerts_tests.py

@@ -266,6 +266,9 @@ def test_operator_validator(setup_database):
    # Test passing with result that equals threshold
    assert operator_validator(alert2, '{"op": "==", "threshold": 55}') is True

+    # Test passing with result that equals decimal threshold
+    assert operator_validator(alert2, '{"op": ">", "threshold": 54.999}') is True


This test is for the old alerts code base. This test can be added here: https://github.com/apache/superset/blob/master/tests/integration_tests/reports/commands_tests.py#L414

Optional (but would be great). Adding integration API tests to assert bad and good validator_config_json

graceguo-supercat · 2021-12-17T06:51:15Z

added integration test to commands_tests.

* fix: [alert] add tooltip message for alert threshold value * support decimal value for alert condition threshold * add integration test

fix: [alert] add tooltip message for alert threshold value

f9a84c8

pull-request-size bot added the size/XS label Dec 14, 2021

graceguo-supercat requested a review from etr2460 December 14, 2021 21:58

etr2460 reviewed Dec 14, 2021

View reviewed changes

graceguo-supercat requested a review from etr2460 December 14, 2021 23:00

support decimal value for alert condition threshold

7da50df

pull-request-size bot added size/S and removed size/XS labels Dec 15, 2021

etr2460 approved these changes Dec 16, 2021

View reviewed changes

graceguo-supercat changed the title ~~fix: [alert] add tooltip message for alert threshold value~~ fix: [alert] allow decimal for alert threshold value Dec 16, 2021

dpgaspar reviewed Dec 16, 2021

View reviewed changes

graceguo-supercat force-pushed the gg-AlertConditionDecimal branch 2 times, most recently from 7385ea4 to 3e45e15 Compare December 16, 2021 23:04

pull-request-size bot added size/M and removed size/S labels Dec 16, 2021

graceguo-supercat force-pushed the gg-AlertConditionDecimal branch from 3e45e15 to bbdecea Compare December 16, 2021 23:08

add integration test

72b9cd1

graceguo-supercat force-pushed the gg-AlertConditionDecimal branch from bbdecea to 72b9cd1 Compare December 16, 2021 23:35

pull-request-size bot added size/S and removed size/M labels Dec 16, 2021

graceguo-supercat merged commit c5af7a4 into apache:master Dec 17, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [alert] allow decimal for alert threshold value #17751

fix: [alert] allow decimal for alert threshold value #17751

graceguo-supercat commented Dec 14, 2021 •

edited

Loading

etr2460 left a comment

graceguo-supercat commented Dec 14, 2021

etr2460 commented Dec 14, 2021 •

edited

Loading

graceguo-supercat commented Dec 14, 2021 •

edited

Loading

codecov bot commented Dec 14, 2021 •

edited

Loading

etr2460 commented Dec 15, 2021

ktmud commented Dec 15, 2021

ktmud commented Dec 15, 2021

graceguo-supercat commented Dec 15, 2021 •

edited

Loading

etr2460 commented Dec 15, 2021 •

edited

Loading

graceguo-supercat commented Dec 15, 2021

ktmud commented Dec 15, 2021

graceguo-supercat commented Dec 15, 2021 •

edited

Loading

etr2460 commented Dec 16, 2021

etr2460 left a comment

etr2460 Dec 16, 2021

graceguo-supercat Dec 16, 2021 •

edited

Loading

etr2460 Dec 16, 2021

dpgaspar left a comment

dpgaspar Dec 16, 2021

graceguo-supercat commented Dec 17, 2021

fix: [alert] allow decimal for alert threshold value #17751

fix: [alert] allow decimal for alert threshold value #17751

Conversation

graceguo-supercat commented Dec 14, 2021 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

etr2460 left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Dec 14, 2021

etr2460 commented Dec 14, 2021 • edited Loading

graceguo-supercat commented Dec 14, 2021 • edited Loading

codecov bot commented Dec 14, 2021 • edited Loading

Codecov Report

etr2460 commented Dec 15, 2021

ktmud commented Dec 15, 2021

ktmud commented Dec 15, 2021

graceguo-supercat commented Dec 15, 2021 • edited Loading

etr2460 commented Dec 15, 2021 • edited Loading

graceguo-supercat commented Dec 15, 2021

ktmud commented Dec 15, 2021

graceguo-supercat commented Dec 15, 2021 • edited Loading

etr2460 commented Dec 16, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Dec 16, 2021

Choose a reason for hiding this comment

graceguo-supercat Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

etr2460 Dec 16, 2021

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Dec 16, 2021

Choose a reason for hiding this comment

graceguo-supercat commented Dec 17, 2021

graceguo-supercat commented Dec 14, 2021 •

edited

Loading

etr2460 commented Dec 14, 2021 •

edited

Loading

graceguo-supercat commented Dec 14, 2021 •

edited

Loading

codecov bot commented Dec 14, 2021 •

edited

Loading

graceguo-supercat commented Dec 15, 2021 •

edited

Loading

etr2460 commented Dec 15, 2021 •

edited

Loading

graceguo-supercat commented Dec 15, 2021 •

edited

Loading

graceguo-supercat Dec 16, 2021 •

edited

Loading