Fixes #25640 - keep breadcrumb's title after invalid editing #6312
Conversation
app/helpers/layout_helper.rb
Outdated
| return title(_('Edit', page_header)) unless obj | ||
|
|
||
| valid_obj = obj | ||
| label_attr = attrbiture || 'to_s' |
There was a problem hiding this comment.
also why not setting :to_s for attribute argument?
app/helpers/layout_helper.rb
Outdated
| return title(_('Edit', page_header)) unless obj | ||
|
|
||
| valid_obj = obj | ||
| label_attr = attrbiture || 'to_s' |
app/helpers/layout_helper.rb
Outdated
| @@ -63,6 +63,20 @@ def title(page_title, page_header = nil) | |||
| @page_header ||= page_header || page_title.to_s | |||
| end | |||
|
|
|||
| def edit_title(obj = nil, page_header: nil, attrbiture: nil) | |||
There was a problem hiding this comment.
Why it combines keyword args with positional args if all are optional?
There was a problem hiding this comment.
I think the obj parameter shouldn't be optional, after all it's Edit page of a resource, do we have edit pages without any resource to edit?
app/helpers/layout_helper.rb
Outdated
| old_attributes = obj.attributes.merge(obj.changed_attributes) | ||
| valid_obj = obj.class.new(old_attributes) | ||
| end | ||
| title(_('Edit %s') % valid_obj.send(label_attr), page_header) |
There was a problem hiding this comment.
please use public_send instead of send that can also call private methods
|
|
||
| unless obj.valid? | ||
| # we need all attributes and then we need to override changed values to old ones | ||
| old_attributes = obj.attributes.merge(obj.changed_attributes) |
There was a problem hiding this comment.
I might be missing something here but I don't the comment correctly describes the behavior. You take existing (uodated) attributes and merge chages to them. Then you build new object with new attributes. Of course the object is valid since no valid? or save was called on it. But imho it has the same values of attributes. Could maybe a bit better explain what is the cause and why this helps? Thanks
|
@ares can you re-review please? |
| @@ -63,6 +63,17 @@ def title(page_title, page_header = nil) | |||
| @page_header ||= page_header || page_title.to_s | |||
| end | |||
|
|
|||
| def edit_title(obj, page_header: nil, attribute: :to_s) | |||
There was a problem hiding this comment.
do we actually pass in page_header anywhere? i couldn't find any
| # we need to override changed values with previous ones, | ||
| # in order to show the valid name in the page title after an unsuccessful editing | ||
| old_attributes = obj.attributes.merge(obj.changed_attributes) | ||
| valid_obj = obj.class.new(old_attributes) |
There was a problem hiding this comment.
you could simplify this and remove the merge using obj.class.new(attributes_in_database)
| @@ -1,3 +1,3 @@ | |||
| <% title _("Edit %s") % @subnet.to_label %> | |||
| <% edit_title @subnet %> | |||
| @@ -1,4 +1,4 @@ | |||
| <% title _("Edit %s") % @taxonomy.title %> | |||
| <% edit_title @taxonomy, page_header: nil, attrbiture: :title %> | |||
There was a problem hiding this comment.
since you are using kwargs, you don't need to pass all the options - just the ones you don't want the default for.
| @@ -1,3 +1,3 @@ | |||
| <% title _("Edit %s") % @usergroup.to_label %> | |||
| <% edit_title @usergroup, page_header: nil, attrbiture: :to_label %> | |||
|
@amirfefer what is the status here? |
theforeman-bot
added
Waiting on contributor
and removed
Needs testing
Needs re-review
labels
|
@amirfefer, this pull request is currently not mergeable. Please rebase against the develop branch and push again. If you have a remote called 'upstream' that points to this repository, you can do this by running: This message was auto-generated by Foreman's prprocessor |
|
ping @amirfefer what is the status here? do you intend to finish it or shall we close it? |
Before:
After:
