Add support for links to a details page in the notification drawer

[skateman]

The angular-patternfly has a new notification-body template example for the notification drawer that displays a kebab on the right side of a notification with all the available actions inside.

For now we supported just two actions: marking a notification as read (by clicking on it) and removing the notification (by clicking on the x). However, the plan is to support at least one extra action to redirect the user to a page where the user can see more details.

After the changes it will have the three actions inside the kebab, where the new View Details should redirect to the subject's screen:

For the toasts, it will have an extra View details link that does the same:

Unfortunately, it is not possible to hide the link if the subject has an actual page to redirect to, so if the user clicks on the View Details for a such record, it will redirect to the dashboard with an error message:

Hopefully this could be resolved in the future by omitting the link if it is not available, probably by using decorators or something similar.

@miq-bot add_reviewer @karelhala
@miq-bot add_reviewer @Hyperkid123
@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @AparnaKarve

@miq-bot add_label enhancement

Related issue: ManageIQ/manageiq-v2v#578
Schema PR: ManageIQ/manageiq-schema#263
Backend PR: ManageIQ/manageiq#17913

[skateman]

@mzazrivec is this okay?

[mzazrivec]

Yes, I think so.

[skateman]

Thanks 🍀 🍀

[AparnaKarve]

We do not want all notifications to have the View Details link.

For e.g. in v2v, only the unsuccessful Request notifications need to have the View Details link, whereas the successful Request notifications should be plain messages without links

Is there a way to regulate that?

[AparnaKarve]

Do we need new parameters in notification_types.yml for supplying more information about the link to the frontend ?

Information such as -

• Whether we need a link :link: true or :link: false
• Link text (other areas of the UI may need the text to be something other than "View Details")

[skateman]

Theoretically possible, but there are some questions:

• Do you have control over what kind of notification types are getting emitted from automate?
• Are we okay with schema changes?
• Why do we need a custom link text? I am pretty sure it's not a good idea to specify it in the backend ...

[skateman]

Or after thinking in the tram, it might be doable through decorators. What if we would have a notification_link method into the subject's decorator and move the logic from the techdebt controller to there.

Something like this:

class ServiceTemplateTransformationPlanRequestDecorator < MiqDecorator
  def notification_link
    req = ServiceTemplateTransformationPlanRequest.select(:source_id).find(params[:id])
    if aparna_says_link_is_needed(req)
      { :controller => 'migration', :action => 'index', :anchor => "plan/#{req.source_id}" }
    else
      nil
    end
  end
end

Generating the text binding:

  def text_bindings
    [:initiator, :subject, :cause].each_with_object(text_bindings_dynamic) do |key, result|
      value = public_send(key)
      result[key] = {
        :link => value.decorate.notification_link,
        :text => value.try(:name) || value.try(:description)
      }.compact if value
    end
  end

And finally the controller:

class RestfulRedirectController < ApplicationController
  def index
    redirect_to(params[:link])
  end
end

@AparnaKarve @martinpovolny @himdel what do you think?

[skateman]

After thinking this through, maybe it's not the best idea to expose a redirect_to so publicly. So it would be much safe to test in the text_bindings if the notification has a notnil result from the decorator and call the decorator again in the redirect controller...

[AparnaKarve]

def aparna_says_link_is_needed?(request_id)
  request = ServiceTemplateTransformationPlanRequest.find(request_id)
  request.request_state == "finished" && request.status != "Ok"
end

[skateman]

@AparnaKarve thanks 👍, yes it would work in your case, but on many other screens it won't and there's a request from @Ladas and @martinpovolny to support clickable links for some provider-related tasks that are accessible only through explorer screens. I would not like to maintain two different implementations until its really necessary.

[martinpovolny]

I think that this is getting too complex. The idea with decorators sounds like working around a need to add an attribute.

It's getting too complex.

The notification should be created with all the information needed.

if request_state == "finished" && status != "Ok" then set some attribute telling the details are to be displayed. The information if the link should be displayed needs to be part of the notification.

We should not take the notification and do further lookups to decide whether to display details link.

[AparnaKarve]

@skateman Are we now implementing what I first suggested above? #4492 (comment)

ManageIQ/manageiq-schema#263 looks good to me!

[skateman]

@AparnaKarve as Martin stated I was trying to avoid changing the schema, but it will be better to do so.

[AparnaKarve]

typo

[AparnaKarve]

@skateman So I verified that the View Details link works.
As mentioned above, in v2v we want to display the link for only unsuccessfully completed Request notifications.

[himdel]

@skateman without that ng-if drawerExpanded, doesn't this reintroduce the looong rendering with thousands of notifications problem?

[skateman]

@himdel nice catch, going to fix it

[skateman]

@AparnaKarve could you please provide me the PR (enough if WIP) where you are emitting notifications? I'd need it to proceed...

[AparnaKarve]

@skateman There is no PR yet...
I'm currently testing with these branches -

manageiq - backend_notifications_for_v2v_requests
manageiq-content - yamls_for_request_completion_notifications

[skateman]

I added a backend and a schema PR to set if we want links for the given notification or not.

@miq-bot add_label pending core

[martinpovolny]

I like this approach much better that the idea with the decorators. Thx, @skateman 👍

The "View deatils" typo from the screenshot seems fixed in the code.

[skateman]

@miq-bot rm_label pending core

[skateman]

@miq-bot assign @martinpovolny
This is ready to merge, the typo is indeed fixed and I'm planning to keep the screenshot as a reminder for myself.

[miq-bot]

Checked commits skateman/manageiq-ui-classic@abc67d4~...f9eeeb4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐