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

[2/2][UI] Eng 2196 m2 allow user to configure wf specific notifications #936

Conversation

likawind
Copy link
Contributor

@likawind likawind commented Jan 31, 2023

Describe your changes and why you are making these changes

This PR adds necessary UI changes to configure workflow-specific notification settings, based on backend changes introduced in #935 . We added a new settings section which allow users to add notification settings:

  • this section is shown only if there are active notification integrations
  • the drop-downs shows a list of current + not-yet selected integrations to force user to have one unique selection per integration
  • user can delete / add settings using corresponding buttons
  • we consider the change as an update only if there's a real update (key or value mismatch) comparing to current settings

Related issue number (if any)

ENG-2196

Tests

We tested the following scenarios. We verified all changes are persisted and workflows are successfully run:

  • section not showing up if user has no notification integrations
  • add notifications to a workflow without any settings
  • change around selections
  • delete all settings of a workflow previously with settings
  • ran manual QA and all workflows looks fine

Loom Demo https://www.loom.com/share/b7929818b83743d881db8d712675fdc3

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@@ -226,6 +232,28 @@ type WorkflowSettingsProps = {
onClose: () => void;
};

// Returns whether `updated` is different from `existing`.
function IsNotificationSettingsMapUpdated(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish Typescript had a built in function to do this, but I'm not aware that it does :)

@agiron123
Copy link
Contributor

Nice work! @likawind Code looks good here, would like to see demo to give any design feedback.

…nto eng-2196-m2-allow-user-to-configure-notifications-ui
…nto eng-2196-m2-allow-user-to-configure-notifications-ui
@likawind likawind changed the title Eng 2196 m2 allow user to configure notifications UI [2/2][UI] Eng 2196 m2 allow user to configure wf specific notifications Feb 1, 2023
@likawind
Copy link
Contributor Author

likawind commented Feb 1, 2023

@agiron123 demo is updated!

@agiron123
Copy link
Contributor

Just watched the demo. This is great! Nice work!

@kenxu95
Copy link
Contributor

kenxu95 commented Feb 2, 2023

A couple questions/notes just from watching the demo:

  • I think the cases of the severity level were inconsistent between email and slack? One of them said "Success" and the other said "SUCCESS".
  • I wonder if it would be helpful to also have the slack workspace displayed on the card.
  • Is the "Level" field on the Notification cards meant to be the "Default Level"? Since the user can overwrite them in the workflow settings. Adding the "default" language would have been a lot clearer for me.
  • Can we add some small helper text under the "Notifications" section in the workflow setting sdialog to indicate that these are override configurations? Otherwise, the default levels will be used. It feels a little weird if the Notifications section is blank with no explanation, but the user still gets notified.

@kenxu95 kenxu95 self-requested a review February 2, 2023 20:26
Copy link
Contributor

@kenxu95 kenxu95 left a comment

Choose a reason for hiding this comment

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

Just noting my comments above ^

@likawind
Copy link
Contributor Author

likawind commented Feb 2, 2023

All great comments, thanks @kenxu95 ! I'm going to fix 1 in this PR and learn more feedbacks on 3 in today's demo. For 2 and 4 both requires a bit more work (esp. 2 which requires some backend work), so we will have tasks to address them separately, just to keep the PR in relative small size

@likawind
Copy link
Contributor Author

likawind commented Feb 4, 2023

@vsreekanti , I know there are a few places we would like to improve. But I do want to merge the stack given we are not opening the feature to users, and I'd like to have all tasks in this stack marked as done.

So I will merge this PR together with others, but feel free to keep leaving notes. We can address them separately before we release to users!

* Implement actual code to send slack notifications (#944)
@likawind likawind merged commit ec03048 into eng-2196-m2-allow-user-to-configure-notifications-be Feb 4, 2023
likawind added a commit that referenced this pull request Feb 4, 2023
* [2/2][UI] Eng 2196 m2 allow user to configure wf specific notifications (#936)

* Implement actual code to send email notifications (#943)

* Implement actual code to send slack notifications (#944)
likawind added a commit that referenced this pull request Feb 6, 2023
* [2/2][UI] Eng 2193 Allow connecting to email as integration (#897)

* [1/2][Backend] Eng 2191 add slack integration to backend (#911)

* [2/2][UI] Eng 2191 Allow user to connect to slack integration (#910)

* Eng 2194 m2 implement go interface for (#923)

* [1/2][backend] Eng 2196 m2 allow user to configure notifications (#935)

* [2/2][UI] Eng 2196 m2 allow user to configure wf specific notifications (#936)

* Implement actual code to send email notifications (#943)

* Implement actual code to send slack notifications (#944)
@likawind likawind deleted the eng-2196-m2-allow-user-to-configure-notifications-ui branch February 6, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants