Notifications created by v2v aren't displayed on pages other than v2v

[skateman]

I noticed on the last sprint review that V2V has notifications implemented. As I implemented the notifications in the core ManageIQ, I was curious how it is done here and found two major issues.

1. The notifications are displayed only when the user is on the V2V page
   I assume that most of the tasks are running longer than the time spent on the page by the user, i.e. the user might visit other pages or simply just close the browser and come back later. In both cases the user will not receive any information about the migrations. Also when on the V2V page as the information displayed in the toast notifications is also reflected on the page, this makes the notifications less valuable and more redundant.

2. The notifications are missing after doing a full page refresh (F5)
   This is closely related to the first point, the notification data is not backed by the server and so there is no history of older notifications that happened before visiting/refreshing the page.

We already have a solution for these problems in the core/classic UI using websockets and the notifications endpoint provided by the REST API.

ping @AparnaKarve @Hyperkid123 @martinpovolny

[AllenBW]

Yeaaaaaaaaaaahhhhhhhh I can take a 🔪 at this... though unclear at this moment if any of the v2v generated notifications will be generated if yer not in the plug in 🤔

[martinpovolny]

It would be nice not to implement 2 ways of doing a single thing. Especially if the existing way is sort of ehm, well, superior to the new one ;-)

@AllenBW, @AparnaKarve, can you, please, cooperate with @skateman on fixing this?

Btw: the same mechanism is being used for the (newly added) provider UI extensions.

[skateman]

Yup, the major change would be to emit notifications from the v2v backend instead of doing it from the frontend. This way all the notifications will be persisted in the DB and emitted through websockets, i.e. displayed on every page and kept after a browser refresh/logout.

I'm not saying that the polling should be deleted, but in the future we can use the websockets for pushing the data changes from the backend instead of continuous polling. But for now let's focus on the notifications part, can someone point me to the backend part?

[AllenBW]

There are a few different situations in which v2v creates its own notification, wheeeeeeennnnnnn...

• when interacting with the api, we often times notify of success (not just error)
• when the status of a migration changes, (what we get with the polling)

[skateman]

@AllenBW the first case might stay as it is (not sure) as the response is immediate and we probably don't want to store this in the drawer. However, we're displaying these kind of things across the classic UI a little bit differently:

Maybe we should have this consistently, but that's more like a question for UX.

For the second - that's what we implemented the notifications for 😉

[AllenBW]

@bthurber @fdupont-redhat @blomquisg, can anyone help @skateman with #578 (comment) ?

[AparnaKarve]

@skateman What a co-incidence...
As a matter of fact I'm already looking into it -- check out my comment here - #265 (comment)

(I'm very close to creating a PR for the above issue)

The backend for v2v comprises of the content and the automation-engine repo other than MIQ core of course.

[martinpovolny]

@AparnaKarve : I don't know about much about co-incidence or what co-incidence you are talking about, but we have all seen yesterday's sprint demo and this caught attention of not only @skateman .

It's good to hear that you are aware that there's a problem with the code. I hope it will help us get it fixed faster.

To catch similar issues earlier in the process I suggest pinging people who where previously involved with a particular feature/area of code prior to implementing something hacky.

And if not prior to the actual work, I'd suggest using the PR review as an opportunity to collect feedback from the relevant people. Just ping ;-)

git blame is a great tool if one does not know or remember who was involved with what.

Also feel free to ask me any time. I am trying to keep track of who is involved with what.

[skateman]

@AparnaKarve that's good news, could you please tag this issue in the PRs you're making? As @martinpovolny mentioned, I'm working on enabling notifications in provider extensions. For now I created a set of generic notification types and some mixins to emit them. Can you please take a look and let me know if they're good for your case? If not, I can create some special ones and let you know how to use them.

[AparnaKarve]

@skateman The notification use case that I am currently working on is about Request completion with a success or failure, so I guess it fits under your generic notification types.
Although the way I would have to emit the notification would be different - something along these lines - https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L244
(For some reason Notification.create does not seem to work from this code)

In v2v, when a Request completes with errors, we create notifications with links, as shown below -

Does the current notification implementation support links?

[skateman]

@AparnaKarve call_automate_event_queue sounds just perfect if you're fully relying on automate for the v2v tasks. For now we don't support links in the notification drawer, but you're the second person this week who asked about them, so I see it as a valid reason to finally implement it. Shouldn't take long...

[martinpovolny]

👍 on implementing the links. One of the first questions from people using the provider extension mechanism we worked on recently.

[AparnaKarve]

Fantastic @skateman -- thank you for looking into the link implementation!!

[skateman]

Just for making sure, where the links should point? Is it related to the task somehow? The backend has sort of a link generation, but we would probably need a controller for a final redirection to the given V2V subpage. The main question is, can the subpage somehow determined from the subject field of the notification...if yes, we're already winning.

[AparnaKarve]

Yes, the subject (the miq_request object) would have the required id which the link would use for proper redirection to the v2v subpage.

[skateman]

Perfect...

[skateman]

@AparnaKarve now I'd need some V2V created notifications in the DB to get further, so if you have a dump generated by your future backend PR, please send it over. Thanks 🍻

[AparnaKarve]

@skateman https://drive.google.com/open?id=1kgbc7y92r2mxKnogLpCDFpLVGwnqsyHo is a pg_dump of the DB.
Notification id=34 is the notification for a Request completed with a failed status that has been generated by my planned backend changes.

[skateman]

Thanks, it seems usable, but I have two questions:

• is there a way to determine if the given MiqRequest record is V2V-related?
• where should I redirect the user?

[AparnaKarve]

• v2v related MiqRequest is ServiceTemplateTransformationPlanRequest
• user should be redirected to plan/source_id (source_id can be found here ServiceTemplateTransformationPlanRequest.source_id)

[skateman]

I found the request, but the plan route doesn't exist:

bin/rake routes | grep plan
                                                     planning GET              /planning(.:format)                                                                              planning#index
                                                              POST             /planning/report_data(/:id)(.:format)                                                            planning#report_data
                                                              GET              /planning/index(/:id)(.:format)                                                                  planning#index
                                                              GET              /planning/report_download(/:id)(.:format)                                                        planning#report_download
                                                              POST             /planning/change_tab(/:id)(.:format)                                                             planning#change_tab
                                                              POST             /planning/option_changed(/:id)(.:format)                                                         planning#option_changed
                                                              POST             /planning/plan(/:id)(.:format)                                                                   planning#plan
                                                              POST             /planning/reset(/:id)(.:format)                                                                  planning#reset
                                                              POST             /planning/wait_for_task(/:id)(.:format)                                                          planning#wait_for_task

Are you sure this is the right URL?

[AparnaKarve]

'plan/:id' is not a rails route, it's a react route created by the v2v app.

To view the route in the UI, just click on any of the Completed/Not started Plans

[skateman]

Okay, how can I redirect to a react route, from the rails controller? 😕

[skateman]

/migrations#plan/:id?

[Hyperkid123]

@skateman that should work

[AparnaKarve]

So the View Details in the Notification below -

links to 'plan/:id' (where id = source_id, as explained above)

I think from the backend side, we should just create the appropriate link (plan/id)
and once it's in the migration route realm, I think the route should just work.
Let's try it when you are ready.

EDIT: Either the complete route migration/plan/id route or the relative route plan/id, whichever works.

[skateman]

@AparnaKarve it works! If the model is ServiceTemplateTransformationPlanRequest, is it 100% sure that it's v2v or can it be something else as well?

[AparnaKarve]

@skateman Pretty positive on that!
Anything that is Transformation or TransformationPlan related is v2v

[skateman]

Thanks 👍 🍻 ✨ I'm almost finished!

[AparnaKarve]

Cool, Thanks for the update!

[dclarizio]

@AparnaKarve any new info on this? Would like to get this in sync with the way other notifications in MIQ work. See above, the notifications can now have clickable links in them.

[AparnaKarve]

@dclarizio No new info yet, since other things like OSP have taken priority over this task, which involved both frontend and backend API work on my part.

I have done some testing with local branches and should have PRs ready by sometime next week.

Looking forward to sync up some of the notifications in v2v with the rest of the UI.

@skateman In v2v note that, only certain type of notifications qualify to be global notifications that are visible in the rest of the app. But most others need to be confined in the v2v UI.

[dclarizio]

@AparnaKarve if there are local messages for v2v screens only, then those are actually flash messages and should be shown to the user when they are on the screen. Any other types of messages that will come back asynchronously must be returned the same as all other notifications, via the notifications queue and should be displayed no matter where the user is in the UI as well as be saved in the notification area in case the user is logged off and can see them later.

@bthurber would it be ok for things like "plan created", "plan delete", etc to just be normal notifications? Trying to make notifications appear ONLY when on the v2v screens is conflicting with our notification support. We want to eventually replace flash messages with toast notifications (which these types of messages probably fit best as), but not sure exactly when we'll get that done.

[AparnaKarve]

Examples of PF Toast Notifications

https://www.patternfly.org/pattern-library/communication/toast-notifications/

PF-React Toast Notifications Storybook link

We are displaying Toast Notifications for certain async actions that are relatively immediate like -

• Infrastructure Mapping created
• Plan created
• Plan deleted
• Plan archived

@michaelkro @mturley Can you confirm if those are indeed Toast notifications?

[michaelkro]

Yes, we use https://github.com/patternfly/patternfly-react/tree/master/packages/patternfly-3/patternfly-react/src/components/ToastNotification for our notification system.

While there is a network request involved for these actions, there is no background worker processing required, and so the notifications appear immediately. It should be unnecessary to add these to the global notification system

[AparnaKarve]

Perfect - Thanks @michaelkro !

Other than Migration Plan completed successfully or failed, there are really no other notifications that need to be added to the global notification system.

[AparnaKarve]

@skateman It looks like the links are not working from the Notification drawer.

Here's a video that shows the issue -
https://drive.google.com/open?id=1O4CPCLi3byfLzuRgzRHPmjZitw3dgqmM

Opened an issue for this - ManageIQ/manageiq-ui-classic#4686

[skateman]

Okay, let's be clear with the terminology from our side in the OPS UI:
Flash notifications are displayed after invoking a request-response type of action. Things like provider refresh initiated or record marked for deletion. For now we're using PF inline notifications to display those.

Asynchronous notifications are generated by the backend, stored in the database and sent asynchronously to the available clients via WebSockets. Things like provisioning complete or embedded ansible enabled. For these we're using the PF notification drawer AND PF toast notifications at the same time.

So if I understand it correctly, you implemented both flash and asynchronous notifications and you just used the toasts for both of them. We are almost ready with making the asynchronous ones consistent with the rest of the OPS UI, but the toasts for the flash notifications introduced an inconsistency.

Even though I agree with your approach of the toasts for flash notifications, I don't think we have enough time to make this consistent across the UI by converting all the alert-type notifications to toasts. However, consistency could be reached by "downgrading" the V2V flash notifications to alerts and convert all the alerts to toasts across the whole UI later.

One more thing: could you please link this issue into your PRs that are related to this issue? Thanks.

/cc @dclarizio

[dclarizio]

Flash notifications are displayed after invoking a request-response type of action. Things like provider refresh initiated or record marked for deletion. For now we're using PF inline notifications to display those.

@AparnaKarve do you think you could change the request-response types of messages to use the PF inline notifications so they would look like all of our other flash messages. We will not have time to convert all of our flash messages to use toast notifications in Hammer, so we need v2v to not introduce this at this time as it will look very inconsistent. Thx, Dan

[vconzola]

@dclarizio I understand the desire for UI consistency, but I don't want to distract the v2v UI dev team from what they're working on right now. we're working aggressively to get some key new features and UX improvements into v2v for the 1.1 release. Also, from reading the thread above it sounds like CF will be moving to the notifications model v2v has already adopted, so changing to inline notifications for some of our messages would be a temporary step backward that will need to be changed back to toast notifications in the future. i'd like to keep this issue in our backlog for now and revisit it once we complete the other 1.1 features and UX improvements.

[dclarizio]

@vconzola well, you're kind of right. We will not be moving to the toast notifications in Hammer and who knows how long the "I" release will be, but quite some time. The problem is also that the toast notifications in V2V are not implemented using the standard PF notification component (as it doesn't support toast notifications that don't end up in the drawer).

So, if we leave this is as, we have a new type of "flash message" that is totally different from what has been implemented in MIQ (and the MIQ team will end up having to support it). By changing this to simply use the existing PF inline notifications, there will be no inconsistency in the UI and we will convert it to toast when we convert the other 99% of our flash messages that use the PF inline notifications.

Perhaps we can discuss next week. I'd just like to get this more uniform before Hammer actually goes GA.

[skateman]

Any update on this?