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

Jump to bottom

Alerts: Add more condition comparison options #4134

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Alerts: Add more condition comparison options #4134

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
729ef18
getredash/redash#4132 Add more condition comparison options
kravets-levko Sep 11, 2019
33df306
Add arguments to fallback lambda
kravets-levko Sep 11, 2019
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
23 changes: 17 additions & 6 deletions redash/models/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,14 +791,25 @@ def evaluate(self):
data = json_loads(self.query_rel.latest_query_data.data)

if data['rows'] and self.options['column'] in data['rows'][0]:
operators = {
'>': lambda v, t: v > t,
'>=': lambda v, t: v >= t,
'<': lambda v, t: v < t,
'<=': lambda v, t: v <= t,
'==': lambda v, t: v == t,
'!=': lambda v, t: v != t,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WDYT about replacing that "long" operator names with these? (old ones kept for backward compatibility: already created alerts can use them, but new ones will use new operators)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also consider the dedicated chars like ≥.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

User does not see them; in dropdown we can show whatever we want, but here I'd prefer to use ANSI characters like we do for variable names.

Copy link
Member

Choose a reason for hiding this comment

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

@ranbena why? I don't even know how to type those without looking up some unicode characters map.

Copy link
Contributor

Choose a reason for hiding this comment

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

use ANSI characters like we do for variable names.

Cool.

why?

Cleaner when implementing on the Select component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaner when implementing on the Select component.

Not sure how it will help. Imagine that we'll add some new operation that will not have a dedicated Unicode symbol, e.g. contains or match regexp. IMHO these identifiers are for internal use and shouldn't be used for dropdown option names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I meant sth else. Doesn't matter. Let's land it.


# backward compatibility
'greater than': lambda v, t: v > t,
'less than': lambda v, t: v < t,
'equals': lambda v, t: v == t,
}
should_trigger = operators.get(self.options['op'], lambda v, t: False)

value = data['rows'][0][self.options['column']]
op = self.options['op']
threshold = self.options['value']

if op == 'greater than' and value > self.options['value']:
new_state = self.TRIGGERED_STATE
elif op == 'less than' and value < self.options['value']:
new_state = self.TRIGGERED_STATE
elif op == 'equals' and value == self.options['value']:
if should_trigger(value, threshold):
new_state = self.TRIGGERED_STATE
else:
new_state = self.OK_STATE
Expand Down