[WIP]Prevent duplicate notification creation to to notification queue

[AllenBW]

https://bugzilla.redhat.com/show_bug.cgi?id=1516668
#751 lives again!

When a new notification is identical (by message) to an existing one, the new one is not added to the notification queue, this prevents death by notifications

Death by notifications on the right, this in place on the left

[AllenBW]

cc @ohadlevy @Loicavenel looking for buy in here, if no buyin can i kill the the bz?

[Loicavenel]

@AllenBW Do we have a clear view why this may happening, do we receive the same notification for the same reason from the same object?

[AllenBW]

@Loicavenel 80% of the time its due to polling and yes, that's usually the case.

Given an asset that does not load correctly, the ui will continue to try and load that asset, throwing an error each time loading fails. In the event described in the bz, an order is placed, but fails.

[Loicavenel]

@AllenBW what polling means here? which task?

[AllenBW]

Oh! Apologies, ok so polling in this case is kinda like maro polo, that water game, the ui will ask the server if anything has changed, the server will answer, than on a set interval at some time later the ui will ask again, the server will answer.

This is one alternative to having a "refresh" button on every page... happens in the background

[Loicavenel]

@AllenBW so, we are in a context where SUI fails to connect and keep popping up messages.. I think we should then group them but what will make sense is to have a counter showing how many times this is occurring.

[AllenBW]

@Loicavenel so that represents a level of effort more along the lines of a ref rather than a bz (due to the need for design work), if this is the desired solution, then we should update the bz (I'll be happy as a clam either way)

in the past it was agreeeeeeed that a more thoughtful planned approach to handling duplicate notifications was needed...

[Loicavenel]

@serenamarie125 need your input here.

[Loicavenel]

@AllenBW I did review the previous agreement that I forgot. Looks good for me, let's make sure @serenamarie125 is on board and my other idea is RFE

[serenamarie125]

@AllenBW not sure i understand. if it's in fact preventing the same message with the same timestamp, it makes sense. But if not, as I mentioned previously we should be updating counts, not preventing them. I think my original concern was we were fixing a specific issue by dealing with it generically, and that may cause other problems.

[AllenBW]

So the bz describes a series of events in which the timestamp will be different, but contents of message are identical. Its like checking for a package, 0900, is the package there, nope, 0910 is the package there? nope. etc etc...

So when ya say updating counts... does this mean suppress the toast but update the notification center?

[ohadlevy]

@AllenBW I would rather have this logic server side and not implemented just for the SUI, @skateman whats your take on this?

@serenamarie125 is this already a defined pattern in pf?

[ohadlevy]

i might click too early, @AllenBW if I understand correctly - this only happens because the nature of polling in SUI, so if we have an action that failed, should we have a better way to communicate that we are "disconnected" or "failing" to get data? many apps today handle that kind of situation when roaming across networks and its not really a toast notification kind of message? /cc @serenamarie125

[skateman]

@ohadlevy the architecture of actioncable doesn't allow this to be implemented on the server-side.

[ohadlevy]

@skateman I might have confused you with notification drawer content (which is not the case here) but an off topic question: can't we provide an API to fetch the current notification state over api + new notification as they appear via websockets?

[skateman]

@ohadlevy that's a thing that we're already doing in the OPS UI.

[skateman]

I think we should start to talk about sharing the drawer through UI components to have it consistent.

[Loicavenel]

@skateman YES and YES..

[ohadlevy]

or just migrate both to the pf implemented one, but regardless, this is not the topic for this PR as the notification in question here are toasts notifications of ajax / api loading failures.

[AllenBW]

@ohadlevy yeah I would rather that as well, does that mean we can reassign the bz? It would be great if the sui could only consume rather than process and consume...

My understanding (which may be misinformed) is that both uis use the pfnotification component.
SUI is a little different in that we both listen to websockets and we display responses of api interactions. The issue of death by notifications, is a result of listening to api responses, maybe this should be reconfigured? Only listen to websockets?

[ohadlevy]

@AllenBW just to make sure I fully understand the issue - please correct me if I got something wrong.

1. user click on a service, we try to load that service details over api
2. api call fail (or one of many) and we let the user know that it failed via a toast notification
3. further api requests fail, and we just duplicate the failure message again and again
4. page is still in loading state.

if the above is correct - I would argue that we shouldn't be using the toast to communicate a failure in this case?

[AllenBW]

@ohadlevy yep that about sums up the issue.

So if not a toast notification, what other mechanism? A few ideas...

• Maybe we throw em in the drawer, but don't show a toast?
• Maybe we only show one toast that there has been an error, and then show the rest in the drawer
• Profit?

Open to ideas, but issue we run into, is at which point does this bug become a ref, if any design work is needed, is where I kinda draw the line (rightly? wrongly?) SO for the bz that caused this whole hullabaloo, what should we do? Deffer, say this needs more discussion and it needs to turn into a ref?

[ohadlevy]

@AllenBW I would default to a failed state (just like empty state concept) when displaying the data.
@serenamarie125 what do you think?

[himdel]

Not sure if it helps, but a comparison with UI-classic:

In ui-classic:

• ws-based notifications coming from the server - only these go in the notification area
• reactions to client actions (form save succeeded, validation failed) - these go as a flash message (an inline notification)
• API errors, server errors - we use a popup we call error modal that just shows the error (screenshot in Use the error modal for API errors as well manageiq-ui-classic#1976 )
• JS errors (random exceptions, duplicate DOM id, etc) - nothing in production, a toast message in development mode

In ui-service, all of these go in the notification area, but are still essentially different kinds of notifications.

IMO deduplication should not happen based on the message, but only based on the notification id/href - but that only works for the first kind of notifications, as the rest doesn't have any id/href.

So, I guess what I'm saying is... we may need a different solution for each of these kinds of notifications, not one that goes for all of them.

Unfortunately the BZ doesn't actually seem to mention any specific notification and I don't really know what kind of duplicate notifications SUI tends to emit, so... :)

[ohadlevy]

@serenamarie125 ping? (or can you reassign to someone else?) thanks

[serenamarie125]

@ohadlevy I'm having a hard time following what we are looking for guidance on. This general issue "Prevent duplicate notification creation to to notification queue" was brought up in another PR, and we had a conversation, where from a UX perspective, I suggested that we not generically prevent dups to the drawer.

I think now you may be saying that we want to determine how to handle the case when the user selects an item from the service catalog, and we go to the ordering dialog, put up a loading message, and then get into a case where the there are API failures.

Is that the case @ohadlevy @AllenBW

If so, I need a bit more information - in this case, are we saying that there is a failure, and the user will not be able to order the service? Or are we just saying it's going to take a bit longer to access the dialog ?

[AllenBW]

My understanding from the bz is that there is a failure and that the user will not be able to order the service. For how long? Not able to say, most of the time, when we encounter death by notification, its a failure.

If we take a step back, this pr has brought fourth a few really good questions (and needless to say, I believe warrants switching the bz from a bug to a ref):

1. Should all notifications treated equal? Displayed in the same way to the user?
   • If not, how should lesser notifications be represented?
2. Should the number of toast notifications be limited, or counters implemented once the number of a certain type reaches a predefined number?
3. Are there other means by which we can notify the user that a resource has failed to recurringly load?

[AllenBW]

This is no longer a blocker 😌

[himdel]

So.. this sounds like a larger discussion..

@AllenBW any chance we can just fix the bug without dealing with duplicate notifications in general?

Sounds like if the user gets the same notification again and again while doing nothing, we must have an infinite loop somewhere - and that sounds fixable :).

[miq-bot]

Checked commits AllenBW/manageiq-ui-service@72ce05d~...1c9a1ff with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

[AllenBW]

@himdel We see multiple notifications because of polling, not a bug persey, just the nature of unintelligent polling. The present state of notifications is not architected to handle duplicate notifications, it is built on the assumption everything is unique :-/

[himdel]

@AllenBW Exactly what I mean.... can't we make the polling code fail on the first error and stop trying over and over?

[AllenBW]

@himdel but maybe the first failure was a fluke! and it will work on the second time? of course, that would be one solution, so maybe this would be done for all polling in the SUI though? if ever it fails, STOP polling?

[himdel]

Well, IMO the simplest solution would be to give up on the first error and show a warning about it, with a button to restart. (Or a message to reload.)

Or we could go by error codes, 404 and 403 pretty much always mean this will break again unless you change things.

If the server doesn't respond, maybe keep trying but only show the first error?

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[AllenBW]

gonna close this for now, cuz... well... ya know...

💀 😭 💪