Backup subject's name in case subject is removed #17871
Conversation
jrafanie
force-pushed
the
backup_notification_subject_in_case_subject_removed
branch
from
e05be1c
to
3f10f63
Compare
I like to be explicit on setters like this |
app/models/notification.rb
Outdated
| @@ -40,6 +42,12 @@ def to_h | |||
| } | |||
| end | |||
|
|
|||
| def backup_subject_name | |||
There was a problem hiding this comment.
Sure, I can make this private.
app/models/notification.rb
Outdated
| def backup_subject_name | ||
| return unless subject | ||
| backup_name = (subject.try(:name) || subject.try(:description)) | ||
| self.options[:subject] = backup_name if backup_name |
There was a problem hiding this comment.
Does options[:subject] get used somewhere?
There was a problem hiding this comment.
yeah, it's quite non-obvious.
The UI calls #to_h. This calls text_bindings which calls text_bindings_dynamic which walks options key/values and will be used when the subject (vm or thing the notification is based on) is no longer existing. See line 90 👇
I was debating rewriting this to make this more clear but the changes were too complicated for this bug fix.
Fixes a bug where notifications look like the following if the subject
vm has been removed:
'Virtual Machine %{subject} has been provisioned.'
https://bugzilla.redhat.com/show_bug.cgi?id=1469372
jrafanie
force-pushed
the
backup_notification_subject_in_case_subject_removed
branch
from
3f10f63
to
e587c5c
Compare
|
Updated based on @bdunne's feedback. Thanks. |
|
Checked commit jrafanie@e587c5c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/notification.rb
|
jrafanie
deleted the
backup_notification_subject_in_case_subject_removed
branch
Fixes a bug where notifications look like the following if the subject
vm has been removed:
'Virtual Machine %{subject} has been provisioned.'
https://bugzilla.redhat.com/show_bug.cgi?id=1469372