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

WIP Alert page redesign #4081

Draft
wants to merge 39 commits into
base: master
from

Conversation

@ranbena
Copy link
Member

commented Aug 19, 2019

  • Refactor
  • Feature

Description

Redesign according to discussion in https://discuss.redash.io/t/alerts-feature-breakdown-aug-19/4312/6.

This PR also migrates to React.

WIP - uploaded just to play around with.
https://deploy-preview-4081--redash-preview.netlify.com/alerts

TODO:

  • Layout not finalized at all.
  • Adding destinations uses SelectItemDialog but should be minimized.
  • Can't create new destination yet.
  • Rearm values that don't adhere to list aren't handled yet.
  • View only mode
  • Refresh schedule link doesn't work.
  • Edit alert name
  • Notification Template
  • Tests
  • Update docs

Issues

Closes #2310.

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Added some new configurations to SelectItemsDialog:

  1. Width (default 80%)
  2. Hide "pending items" pane (default show)
  3. Allow extra footer content (default null)
  4. Override "deselected" icon (default »)

Screen Shot 2019-08-19 at 16 56 09

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@arikfr the pagerduty icon doesn't appear cause it seems to be misconfigured.
The icon should be prefixed with fa- but is also a part of fab, font-awesome-brands package which I think we don't bundle.

def icon(cls):
return 'creative-commons-pd-alt'

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@arikfr the schedule dialog isn't currently built to encapsulate all it needs, therefore cannot be placed outside the Query page without some duplicate code in the Alert page. (query save, refresh intervals, permissions, etc.). Wdyt of redirecting to the Query page instead of opening the dialog?

@arikfr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@arikfr the pagerduty icon doesn't appear cause it seems to be misconfigured.
The icon should be prefixed with fa- but is also a part of fab, font-awesome-brands package which I think we don't bundle.

We shouldn't use the icons anymore. It's an old behavior -- see the destinations list page: we use a proper image like we do for data sources.

@ranbena ranbena self-assigned this Aug 19, 2019

@arikfr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Wdyt of redirecting to the Query page instead of opening the dialog?

Maybe keep only the instructions then.

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Updated destination icons.

Screen Shot 2019-08-20 at 7 41 17

Screen Shot 2019-08-20 at 7 41 32

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Added instructions helptrigger, new alert layout and loading indicator.

Screen Shot 2019-08-20 at 8 56 39

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Took care of rearm values.
Now it allows selecting any rearm value but in a usable way, no need to calculate durations into seconds. So 120 -> 2m, 3600 -> 1h, and 999 -> 999s.

Screen Shot 2019-08-21 at 10 17 09

@arikfr

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

image

The red triangle is to warn about email not enabled or is it something else? If it is, maybe we should disable email destinations?

Additional feedback based on interacting with it:

  • When selecting a query, why do I need to click on continue? Why a full page reload seems to happen?

  • I would show the explanation on why a schedule is needed in a tooltip instead of opening the help page.

  • Rearm:

    • There is no feature of "each time results are refreshes".
    • Please refer to my comment in the forum thread: we need to clarify that regardless the notification will be sent only when it's triggered. Maybe add a note below when picking "At most every" that mentions it will be sent when the alert is triggered and query executed.
@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

@arikfr

The red triangle is to warn about email not enabled or is it something else? If it is, maybe we should disable email destinations?

The hover tooltip says:
Screen Shot 2019-08-21 at 17 14 52
Meaning, mail server configuration issue detected. I would rather not disable it, to allow users to subscribe to email which would work when it's fixed, instead of having to revisit the alert in order to subscribe once the admin fixes the issue.

When selecting a query, why do I need to click on continue? Why a full page reload seems to happen?

That's how it works today. The alert is created after choosing a query and then redirected to alerts/:id.
Screen Shot 2019-08-21 at 17 20 40

I changed the "save" label to "continue" so users would know there awaits a next config step to setting up the alert. If you think this is a crucial UX issue, lmk.

I would show the explanation on why a schedule is needed in a tooltip instead of opening the help page.

Done 👍

Screen Shot 2019-08-21 at 17 33 49

There is no feature of "each time results are refreshes".

I mean rearm=1 second - notification sent each time query executes and alert triggered.
Perhaps this wording works better?:

Screen Shot 2019-08-21 at 17 55 36

Please refer to my comment in the forum thread: we need to clarify that regardless the notification will be sent only when it's triggered. Maybe add a note below when picking "At most every" that mentions it will be sent when the alert is triggered and query executed.

That's a hard one. @susodapop @justinclift can you suggest a clear and brief sentence for this?

@arikfr

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Meaning, mail server configuration issue detected.

It's not about an issue we detect. It's about whether Redash has mail server configuration (even if it's wrong/not working). Usually it's a one way direction: you start with no configuration and then you configure it. Also worth keeping in mind that the people who use Redash might not be aware that further configuration is needed (compared to the person who admins the instance).

I feel that if it's disabled it will reduce confusion.

That's how it works today. The alert is created after choosing a query and then redirected to alerts/:id.

It isn't. When you pick the query it shows the rest of the UI, but the alert is saved only when you click on Save.

Perhaps this wording works better?:

Using the "When triggered send a notification" label makes it much clearer 👌 The options should be:

  • Just once (until back to normal).
  • Each time alert is evaluated (until back to normal).
  • At most every ... when alert is evaluated.

The key thing is the "alert is evaluated" and the new title.

@ranbena

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

I feel that if it's disabled it will reduce confusion.

Instead of disabling (which might be confusing on it's own) I went for showing the toggle only when mail config is ok.

Screen Shot 2019-08-23 at 12 12 15

It isn't. When you pick the query it shows the rest of the UI, but the alert is saved only when you click on Save.

My bad. Fixed.

sing the "When triggered send a notification" label makes it much clearer 👌 The options should be...

Done?

Screen Shot 2019-08-23 at 12 19 25

@ranbena ranbena force-pushed the alert-page branch from d3e1bdf to 3b01c86 Aug 31, 2019

ranbena added 3 commits Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.