Send link with the text_bindings in notifications when link_to is set #17913
Conversation
| result[key] = { | ||
| :link => { |
There was a problem hiding this comment.
I think this is okay, but I just wanted to call out what looks like a slight change in behavior.
Because you are moving which key to link to to the notification type, the :link key is getting moved up as a sibling to the relation keys.
Prior to this change it was possible to set a link for each of :initiator, :subject and, :cause, but now we can only set it to one as determined by the notification type.
There was a problem hiding this comment.
Yup, the design here was for up to 3 links for a notification with the links in the notification text itself, however, UX designed the visual part differently so only one link is allowed.
app/models/notification_type.rb
Outdated
| @@ -9,6 +9,7 @@ class NotificationType < ApplicationRecord | |||
| has_many :notifications | |||
| validates :message, :presence => true | |||
| validates :level, :inclusion => { :in => %w(success error warning info) } | |||
| validates :link_to, :inclusion => { :in => %w(subject initiator cause) } | |||
There was a problem hiding this comment.
Should we add a value to :link_to for each type to the notification_type fixture? Also does this need to be set in a migration for existing types?
There was a problem hiding this comment.
@carbonin only if you need links for those notifications, it's perfectly fine if the link_to is not set, we were never using links in notifications before.
There was a problem hiding this comment.
Okay, I think this needs :allow_blank => true then.
There was a problem hiding this comment.
Specs should be fixed after adding the allow_blank from the previous comment.
skateman
force-pushed
the
notification-links
branch
from
4aeb15e
to
e8f30a2
Compare
|
@skateman looks like the spec is failing on the old link behavior, can you update it to describe how things should be working after your change? |
|
@carbonin I'm on it! |
skateman
force-pushed
the
notification-links
branch
2 times, most recently
from
1380a3a
to
aebaa1b
Compare
spec/models/notification_spec.rb
Outdated
| end | ||
|
|
||
| it 'contains the link to the initiator' do | ||
| expect(notification.to_h).to include(:bindings => a_hash_including(:link => a_hash_including(:model, :id))) |
There was a problem hiding this comment.
Couldn't this match the user more specifically? I figure it makes the expectation that the user object originally passed in the initiator field should match the one in the link.
Maybe something like this:
expect(notification.to_h).to include(:bindings => a_hash_including(:link => {:model => "user", :id => user.id}))
skateman
force-pushed
the
notification-links
branch
from
aebaa1b
to
c28d11c
Compare
|
Checked commit skateman@c28d11c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/notification.rb
|
Each notification can have a
subject,initiatorand acauserelation. Each of these are polymorphic and can point to a given entity. When displaying a notification, we would like to have a View details link that could redirect to one of these entities. This can be specified at the notification type declaration by setting thelink_toparameter.Based on this parameter I adjusted the
text_bindingsto have at most a singlelinkattribute if thelink_tois set properly to one of the possible values.Related issue: ManageIQ/manageiq-v2v#578
Depends on: ManageIQ/manageiq-schema#263
UI part: ManageIQ/manageiq-ui-classic#4492
@miq-bot add_label gaprindashvili/no