Add Notification Providers #2677

joshri · 2022-08-31T21:48:05Z

As always, it was a little more complicated than just adding providers as a resource. Channel is an optional value of the Provider spec, and if it wasn't set, I would expect users to be able to filter by undefined, or -. Allowing for undefined values in the DataTable and filtering by - led me to discover that searching by text was not working as expected in Sources and the new Notifications tables (possibly due to the restructuring of objects in the ListObjects request). When I dove into that code...it didn't click with me - we were checking every field before even checking if the textSearchable boolean was true! I rewrote it in a way that clicks with my brain. The tests for text search also were testing for OR values in filtering rather than AND, as in all text filters didn't have to apply for a row to be included, so I rewrote that test to match as well.

Also - a couple props were removed from the Page component as they were unused

opudrovs · 2022-09-01T20:38:53Z

ui/components/ChipGroup.tsx

@@ -17,6 +17,7 @@ function ChipGroup({ className, chips = [], onChipRemove, onClearAll }: Props) {
  return (
    <Flex className={className} wide align start>
      {_.map(chips, (chip, index) => {
+        if (chip.slice(-1) === ":") chip += " -";


Why is this line required? Does it add - to any chip text ending with :?

opudrovs · 2022-09-01T20:41:16Z

What is required to see notifications data when testing this branch locally? I see no data.
When going to the Notifications page, the first sidebar item (Applications) stays selected, unlike the screenshot. Is it expected?

opudrovs · 2022-09-01T20:55:18Z

@joshri so, we do not have a Settings page at the settings route yet, only only the Notifications page at the notifications route? (I see your comment in the ticket, just want to confirm the route.)

opudrovs · 2022-09-01T21:16:39Z

ui/components/DataTable.tsx

-        if (typeof f.value === "string") {
-          return f.value === colName;
+    fields.forEach((field) => {
+      if (field.textSearchable) {


You could try adding

if (!field.textSearchable) { return; }

at the beginning to remove this large if wrapping the whole code. It should make forEach move to the next element, but please check it. 🙂

opudrovs · 2022-09-01T21:17:44Z

ui/components/DataTable.tsx

-          return f.value === colName;
+    fields.forEach((field) => {
+      if (field.textSearchable) {
+        let value;


An empty line before this line might make the code more readable.

opudrovs · 2022-09-01T21:18:12Z

ui/components/DataTable.tsx

-
-        if (f.sortValue) {
-          return f.sortValue(row) === value;
+        for (let i = 0; i < textFilters.length; i++) {


And an empty line before this line would make reading the code easier.

opudrovs · 2022-09-01T21:21:35Z

ui/components/FilterDialog.tsx

@@ -78,20 +78,19 @@ const FilterSection = ({
          </Text>
        </ListItem>
        {options.sort().map((option: string, index: number) => {


You can use a shorter form here, because you are only returning the ListItem without using any local variables:

{options.sort().map((option: string, index: number) => ( <ListItem key={index}> <ListItemIcon> <FormCheckbox label="" name={`${header}${filterSeparator}${option}`} /> </ListItemIcon> <Text color="neutral40" size="small"> {_.toString(option) || "-"} </Text> </ListItem> ))}

joshri · 2022-09-02T13:28:13Z

@opudrovs try using this yaml to get some providers going. Ripped it from the flux docs here https://fluxcd.io/flux/components/notification/provider

apiVersion: notification.toolkit.fluxcd.io/v1beta1
kind: Provider
metadata:
  name: slack
  namespace: default
spec:
  type: slack
  channel: general
  address: https://hooks.slack.com/services/YOUR/SLACK/WEBHOOK
  proxy: https://proxy.corp:8080
  secretRef:
    name: webhook-url

apiVersion: notification.toolkit.fluxcd.io/v1beta1
kind: Provider
metadata:
  name: msteams
  namespace: flux-system
spec:
  type: msteams
  address: <webhook-url>

joshri · 2022-09-02T13:31:26Z

@opudrovs in regards to the applications tab still being highlighted - the mui tabs component must always have a valid active tab value otherwise you get long console errors. I can look into overriding this more if it's a big issue

opudrovs · 2022-09-02T13:45:41Z

@joshri

try using this yaml

Thanks, will try it.

in regards to the applications tab still being highlighted - the mui tabs component must always have a valid active tab value otherwise you get long console errors. I can look into overriding this more if it's a big issue

I would prefer nothing to be selected/highlighted in the sidebar if there is no Notifications item there, because it might look misleading to the user (like they are still in the Applications page). But if it's hard to do it's not a big issue, we can wait until someone actually complains. @ozamosi do you agree or would you prefer to have all sidebar items deselected while we are at the Notifications page?

ozamosi · 2022-09-02T14:08:22Z

But if it's hard to do it's not a big issue, we can wait until someone actually complains. @ozamosi do you agree

Yeah, sounds good. All else being equal I'd not highlight it, but I can also make up excuses for why this behaviour is technically correct (the notifications relates to your flux resources e.g. applications/sources, so we highlight that section header).

opudrovs · 2022-09-02T19:46:31Z

@joshri clicking on an added chip with - does not remove it. Clicking on any other added chip removes it, as expected. Maybe it does not properly detect which chip to remove? The same issue with the hyphen/undefined values in other tables.

opudrovs · 2022-09-02T19:49:11Z

When filtering by this hard to read channel, the table is empty:

opudrovs · 2022-09-02T19:54:46Z

And when selecting all channels only the general channel is displayed:

opudrovs · 2022-09-02T20:00:05Z

Please consider adding tests for filtering with chips with hyphens/undefined values, if it's technically possible.

opudrovs · 2022-09-02T20:42:42Z

I don't think that it is related to this PR directly but the chips when they appear slightly push content down (no matter if FilterDialog pushes content to the left or is displayed over it). I think some space should be reserved for chips:

chips.mov

joshri · 2022-09-02T21:11:47Z

Inability to remove chips with '-' was bc I changed the value of the actual chip in ChipGroup. Added the filterSeparator.

The long channel string included the filterSeparator which was :. Changed it to something that no one will have in there stuff: ---

I also added the filterSeparator variable to all the tests so we can actually change it without breaking everything.

ozamosi · 2022-09-06T12:10:30Z

Could we please pause and take a step back.

We should be able to filter by empty value. This is a really good change. However, this seems like a separate enhancement, so we can make sure it works well together with e.g. tenants.

Among these filtering fixes there's also an important regression fix. This would also be good to have outside of this PR, so we can call it out in our changelog (i.e. "filtering fixes" as a bug fix PR that calls out the regression - not several bug fix PRs).

As for how to filter by empty value, it seems to me that the quick solution didn't work, and we need to solve the problem by structuring the data, so that the representation doesn't matter - as in, if a chip had a property "field" and a property "value", then whether it's rendered as "field:value" or "value@field" or even "I'm feeling lucky" wouldn't matter, because when determining what filter to remove we'd be able to just read the properties.

…s in the frontend, and allow for undefined values as - in filters

… called filter-undefined

joshri · 2022-09-06T15:14:24Z

As discussed in standup:

Filtering changes removed from this branch - this leaves some bugs with search and filtering remaining (searching does not work due to ListObjects object restructuring in Sources and Notifications tables, filterDialog displays nothing for undefined values but has a working(?) checkbox). I moved the work I had done on these into the branch filter-undefined in case we want to use it later.

The Applications tab being highlighted on Notifications is also something I don't want to address in this PR. The only ways I can see to override the required value prop are either creating a tab in the nav for notifications and overriding all the styling on it to make it invisible (ew), or to not use the Tabs component from Mui, both of which are large changes.

@opudrovs @ozamosi take another look at the code and the notifications table locally for me and make sure I didn't break it with my lovely interactive rebase THANKS

opudrovs · 2022-09-06T16:04:29Z

Can we add delays to the tooltip in UserSettings to prevent tooltip from blinking when the user knows well what they are doing and clicks on the icon fast. Smth. like:

<Tooltip title="Account settings" enterDelay={500} enterNextDelay={500}>

opudrovs

LGTM! 🎉

opudrovs · 2022-09-06T16:06:01Z

Tested it, works fine, I didn't test filtering because it will be added in the another PR.

joshri · 2022-09-06T16:16:26Z

Can we add delays to the tooltip in UserSettings to prevent tooltip from blinking when the user knows well what they are doing and clicks on the icon fast. Smth. like:
<Tooltip title="Account settings" enterDelay={500} enterNextDelay={500}>

Yep that exists as a prop in the mui component as enterDelay

opudrovs · 2022-09-06T16:30:03Z

@joshri I guess enterNextDelay is required too. Otherwise it will appear too fast if it already appeared once.

joshri requested review from ozamosi and opudrovs August 31, 2022 21:48

joshri added the area/ui Issues that require front-end work label Aug 31, 2022

joshri force-pushed the settings-page branch from 9ec0319 to 9dfeb66 Compare September 1, 2022 12:45

opudrovs reviewed Sep 1, 2022

View reviewed changes

joshri force-pushed the settings-page branch 3 times, most recently from 67aa0f0 to f03dee9 Compare September 2, 2022 18:44

joshri force-pushed the settings-page branch from f03dee9 to 062d78e Compare September 2, 2022 21:04

joshri force-pushed the settings-page branch from 0738c2b to ba3a316 Compare September 6, 2022 14:48

joshri added 3 commits September 6, 2022 10:56

empty settings page and route

74b5086

wow it was annoying to make the settings page

331a8e5

did not end up needing to change breadcrumbs

aa92a65

joshri added 7 commits September 6, 2022 11:00

list providers

b965758

debug adding notifications in the backend, debug provider object clas…

832f91f

…s in the frontend, and allow for undefined values as - in filters

snaps

180b333

a nav tab must always be highlighted for mui to be happy

1872c66

sneaky page titles in error pages. are these even used?

647f354

go mod tidy

f4b5311

remove final vestiges of filter bug fixing in this pr - new branch is…

6040aaa

… called filter-undefined

joshri force-pushed the settings-page branch from 3c3cfd1 to 6040aaa Compare September 6, 2022 15:01

joshri requested a review from opudrovs September 6, 2022 15:15

opudrovs approved these changes Sep 6, 2022

View reviewed changes

ozamosi approved these changes Sep 6, 2022

View reviewed changes

tooltip delay

28d9d3c

joshri added 2 commits September 6, 2022 13:53

enter next delay

f815fb8

buf mod update

63f3f51

joshri merged commit f35dbe7 into main Sep 6, 2022

joshri deleted the settings-page branch September 6, 2022 19:38

This was referenced Sep 14, 2022

Updates for 0.9.5-rc.1 #2742

Closed

Updates for 0.9.5-rc.1 #2743

Merged

Updates for 0.9.5 #2747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Notification Providers #2677

Add Notification Providers #2677

joshri commented Aug 31, 2022 •

edited

opudrovs Sep 1, 2022 •

edited

opudrovs commented Sep 1, 2022 •

edited

opudrovs commented Sep 1, 2022 •

edited

opudrovs Sep 1, 2022

opudrovs Sep 1, 2022

opudrovs Sep 1, 2022

opudrovs Sep 1, 2022 •

edited

joshri commented Sep 2, 2022 •

edited by bigkevmcd

joshri commented Sep 2, 2022

opudrovs commented Sep 2, 2022 •

edited

ozamosi commented Sep 2, 2022

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

joshri commented Sep 2, 2022

ozamosi commented Sep 6, 2022

joshri commented Sep 6, 2022 •

edited

opudrovs commented Sep 6, 2022

opudrovs left a comment

opudrovs commented Sep 6, 2022

joshri commented Sep 6, 2022

opudrovs commented Sep 6, 2022

Add Notification Providers #2677

Add Notification Providers #2677

Conversation

joshri commented Aug 31, 2022 • edited

opudrovs Sep 1, 2022 • edited

Choose a reason for hiding this comment

opudrovs commented Sep 1, 2022 • edited

opudrovs commented Sep 1, 2022 • edited

opudrovs Sep 1, 2022

Choose a reason for hiding this comment

opudrovs Sep 1, 2022

Choose a reason for hiding this comment

opudrovs Sep 1, 2022

Choose a reason for hiding this comment

opudrovs Sep 1, 2022 • edited

Choose a reason for hiding this comment

joshri commented Sep 2, 2022 • edited by bigkevmcd

joshri commented Sep 2, 2022

opudrovs commented Sep 2, 2022 • edited

ozamosi commented Sep 2, 2022

opudrovs commented Sep 2, 2022 • edited

opudrovs commented Sep 2, 2022 • edited

opudrovs commented Sep 2, 2022

opudrovs commented Sep 2, 2022 • edited

opudrovs commented Sep 2, 2022 • edited

joshri commented Sep 2, 2022

ozamosi commented Sep 6, 2022

joshri commented Sep 6, 2022 • edited

opudrovs commented Sep 6, 2022

opudrovs left a comment

Choose a reason for hiding this comment

opudrovs commented Sep 6, 2022

joshri commented Sep 6, 2022

opudrovs commented Sep 6, 2022

joshri commented Aug 31, 2022 •

edited

opudrovs Sep 1, 2022 •

edited

opudrovs commented Sep 1, 2022 •

edited

opudrovs commented Sep 1, 2022 •

edited

opudrovs Sep 1, 2022 •

edited

joshri commented Sep 2, 2022 •

edited by bigkevmcd

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

opudrovs commented Sep 2, 2022 •

edited

joshri commented Sep 6, 2022 •

edited