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.

Sign up for GitHub

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: [alert] allow decimal for alert threshold value #17751

Merged
merged 3 commits into from
Dec 17, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9a84c8
fix: [alert] add tooltip message for alert threshold value
Dec 14, 2021
7da50df
support decimal value for alert condition threshold
Dec 15, 2021
72b9cd1
add integration test
Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
Operator,
Recipient,
} from 'src/views/CRUD/alert/types';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
import { AlertReportCronScheduler } from './components/AlertReportCronScheduler';
import { NotificationMethod } from './components/NotificationMethod';

Expand Down Expand Up @@ -1183,7 +1184,12 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
</StyledInputContainer>
<StyledInputContainer>
<div className="control-label">
{t('Value')}
{t('Value')}{' '}
<InfoTooltipWithTrigger
tooltip={t(
'Threshold value should be double precision number',
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Author

@graceguo-supercat graceguo-supercat Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

)}
/>
<span className="required">*</span>
</div>
<div className="input-container">
Expand Down
2 changes: 1 addition & 1 deletion superset/reports/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class ValidatorConfigJSONSchema(Schema):
description=validator_config_json_op_description,
validate=validate.OneOf(choices=["<", "<=", ">", ">=", "==", "!="]),
)
threshold = fields.Integer()
threshold = fields.Float()


class ReportRecipientConfigJSONSchema(Schema):
Expand Down
3 changes: 3 additions & 0 deletions tests/integration_tests/alerts_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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



@pytest.mark.parametrize(
"description, query, validator_type, config",
Expand Down
16 changes: 15 additions & 1 deletion tests/integration_tests/reports/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,16 @@ def create_alert_slack_chart_grace(request):


@pytest.fixture(
params=["alert1", "alert2", "alert3", "alert4", "alert5", "alert6", "alert7",]
params=[
"alert1",
"alert2",
"alert3",
"alert4",
"alert5",
"alert6",
"alert7",
"alert8",
]
)
def create_alert_email_chart(request):
param_config = {
Expand Down Expand Up @@ -448,6 +457,11 @@ def create_alert_email_chart(request):
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": "!=", "threshold": 11}',
},
"alert8": {
"sql": "SELECT 55 as metric",
"validator_type": ReportScheduleValidatorType.OPERATOR,
"validator_config_json": '{"op": ">", "threshold": 54.999}',
},
}
with app.app_context():
chart = db.session.query(Slice).first()
Expand Down