Move breadcrumbs setting from views to controllers #3191
Conversation
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
| @@ -0,0 +1 @@ | |||
| <%= text %> (<%= t(state, scope: 'spree.return_authorization_states')%>) | |||
There was a problem hiding this comment.
Missing a trailing newline at the end of the file.
Use 1 space before %> instead of 0 space.
There was a problem hiding this comment.
Oops! I only checked with Rubocop locally. I'll fix these!
| <%= link_to text, path %> | ||
| <% else %> | ||
| <%= text %> | ||
| <% end %> |
There was a problem hiding this comment.
Missing a trailing newline at the end of the file.
| <%= text %> | ||
| <span class="pill pill-<%= state %>"> | ||
| <%= t(state, scope: 'spree.payment_states') %> | ||
| </span> |
There was a problem hiding this comment.
Missing a trailing newline at the end of the file.
cavalle
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
e26459d to
336fc57
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
cavalle
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
336fc57 to
26d41c6
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Thanks for the accurate research and work on this @cavalle, I think this is the right approach!
Even if I'm not sure about your same doubt on if it's better to put specific breadcrumbs in each controller action or in a single method with some ifs, I'd probably go for the former, probably I'm usually happy when I see ifs disappear. Let see what others think about this, in the meantime, I left some comments, let me know your thoughts!
| add_breadcrumb t('spree.settings') | ||
| add_breadcrumb t('spree.admin.tab.checkout') | ||
| add_breadcrumb plural_resource_name(Spree::AdjustmentReason), spree.admin_adjustment_reasons_path | ||
| add_breadcrumb @adjustment_reason.name if params[:id].present? |
There was a problem hiding this comment.
We are avoiding spaces to align code on different lines, because if one of the other lines changes we have to update all the other lines, degrading git history.
There was a problem hiding this comment.
Thanks for letting me know about this convention. I wasn't aware of it. I'll update everywhere I'm using spaces to align code.
| @@ -0,0 +1 @@ | |||
| <%= text %> (<%= t(state, scope: 'spree.return_authorization_states') %>) | |||
There was a problem hiding this comment.
Do you think we could take advantage of this PR and remove this custom breadcrumb in some way?
There was a problem hiding this comment.
This is an interesting one. This particular breadcrumb could be perfectly specified from the controller itself using simple string interpolation. In fact, I did it like that in an initial version. However, the string interpolated ended up being a bit too long and a bit too complex and, since I had to implement the ability to use custom partials for another breadcrumb, I thought it would make sense to use it in this case for clarity.
Now, one thing I wondered is whether this additional level of complexity (i.e. the ability to specify custom breadcrumbs with partials) was really justified if we only needed for one or two breadcrumbs. Everything would be simpler if all the breadcrumbs would be just text, and didn't need special HTML formatting like in this particular case in the whole app. I'd appreciate your thoughts on this. Is it really needed and valuable to show the status information with colours in the breadcrumbs?
There was a problem hiding this comment.
I think we can remove the HTML from breadcrumbs, even if it probably requires a new PR and an alternative UI method to show the state of the parent object in the page, which I think was the original reason why they have been introduced.
|
@kennyadsl thanks for your very clear feedback! I'll respond and work on it during this morning. |
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
|
Just pushed additional changes to address the points discussed above. I didn't rebase or amend the previous commits, but if you usually work that way let me know and I'll do it. |
|
Thanks @cavalle, I'll talk in the next core team meeting about:
I hope to come back soon with a response. |
|
@cavalle we had a quick chat about this PR in the last Core Team meeting and this is what we decided:
We are not sure but the current implementation is a great starting point that we can revisit later if needed. We also discussed the possibility to create a "DSL" around this, that allows setting specific features only for specific actions, maybe something like: add_breadcrumb @adjustment_reason.name, only: [:show, :edit, :update]That would actually take care of pushing stuff in the right action but for now, this is just an idea, what we have is great.
Yes, we can. We prefer to have this PR simplified than having a So missing TODO items are:
Thanks a lot again for this great contribution! |
|
Awesome! I'll take care of the TODO items as soon as I can. |
cavalle
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
8cf7ee8 to
02a3abe
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
|
I implemented all points in the previous comment and CI passed. This one is ready for review again! |
|
@cavalle Hey Luismi, thanks! I just noticed another thing that we should take care of before merging: if we remove the Thanks again 🙏 |
|
@kennyadsl right! that makes a lot of sense, and I'd love to take care of it, but I'm afraid I no longer have much available time. So, if you don't mind taking care of it in another PR, I'd very much appreciate it! |
|
Sure @cavalle, I'm thinking I could also open a PR on your fork to address the issue so my changes will be part of this one. Does it work for you? |
|
No problem with that! I made you collaborator of my fork. |
This commit makes the `add_breadcrumb` method available in all controllers to allow to define breadcrumbs from there rather than from views. It also moves the definition of the breadcrumbs for all the users' pages from the views to the controller. Setting the breadcrumbs in the `before_action` of the controller, ensures objects haven't been changed by the logic of the action yet, thus avoiding bugs like solidusio#3149. This commit includes a regression test for the aforementioned bug, and tests for the new breadcrumbs rendering logic.
This commit moves the setting of all the breadcrumbs that don't require custom formatting from the views to the controllers, using the new `add_breadcrumb` method. This will avoid bugs like the one reported in solidusio#3149.
This commit removes the pills from those breadcrumbs that were using them. This way there's no need for a custom format and the breadcrumbs can be set from the controllers using the existing helpers.
This commit deletes a template that is not reachable. I found no code referencing to it and no test broke after removing the template.
kennyadsl
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
02a3abe to
b706e07
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
kennyadsl
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
b706e07 to
8878a86
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
Once we have migrated all the breadcrumbs setting logic to controllers, we no longer need the ability to set breadcrumbs from views. Not being able to set breadcrumbs from views will avoid reintroducing bugs like the one reported in solidusio#3149. This commit deprecates using the admin_breadcrumb method but still lets its usage available, so extensions and stores can gradually migrate to the new way of setting breadcrumbs. To avoid breadcrumbs duplication if stores has replaced backend admin views, a check has been added to avoid adding the view breadcrumbs in case a breadcrumb with the same name is already there. strip_tags into admin_page_title is still required since deprecated breadcrumbs coming from that method will probably be html links. Once we remove the method we can probably also avoid stripping tags to set admin page title.
kennyadsl
force-pushed
the
3149-move-breadcrumbs-to-controllers
branch
from
8878a86 to
a4df140
Compare
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
warning: parser/current is loading parser/ruby26, which recognizes
warning: parser/current is loading parser/ruby26, which recognizes warning: 2.6.0-dev-compliant syntax, but you are running 2.6.1. warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. 1 error(s) were found in ERB files Linting 1 files with 12 linters...Avoid using 'javascript_tag do' as it confuses tests that validate html, use inline <script> instead
In file: backend/app/views/spree/admin/adjustments/index.html.erb:19
|
@cavalle I think I've done, the only commit I changed is 64230bf. I've also changed the commit message and description to reflect/explain the changes I've made. Please, let me know your thoughts if you have time. Notes to add the CHANGELOG: Setting breadcrumbs from views has been deprecated and now should do that from the controller. <% admin_breadcrumb(link_to plural_resource_name(Spree::CustomModel), spree.admin_custom_models_path) %>You should remove it and add the following into your before_action :set_breadcrumbs, only: :index
# ...
private
# ...
def set_breadcrumbs
add_breadcrumb plural_resource_name(Spree::CustomModel), spree.admin_custom_models_path
end
|
|
@kennyadsl the changes you made look good to me 👍 |
|
@ericsaupe I'll take care of adding this notes to the CHANGELOG when we'll release Solidus 2.9 |
There was a problem hiding this comment.
First of all thank you very much for the contribution and all the hard work. 🎈
But imo the approach taken here is not what we should do. This is a lot of breaking code for fixing a very edgy problem. I see the advantages this approach might have, but also think that a controller module is not the right place for a UI (views only) feature. Unfortunately Rails does not give us view objects, so an alternative is also not very pleasing, I know.
The approach taken in #3152 to fix the very edgy "bug" in #3149 is a simple fix that I would like to see instead of this approach. But, if all other core team members agree that this apporach is what we want to go with I am not rejecting this change (simple democracy ^_^).
In that case I have some comments I would like see addressed.
Thanks again for you well done contribution.
| module Spree | ||
| module Admin | ||
| module Breadcrumbs | ||
| def add_breadcrumb(name, path = nil) |
There was a problem hiding this comment.
Can we make this methods protected so they don't become actions?
| module Admin | ||
| module Breadcrumbs | ||
| def add_breadcrumb(name, path = nil) | ||
| @admin_breadcrumbs ||= [] |
There was a problem hiding this comment.
Can we make this a Set, so that we do not get potential duplicates and gain some performance?
| @@ -3,6 +3,8 @@ | |||
| module Spree | |||
| module Admin | |||
| class BaseController < Spree::BaseController | |||
| include Spree::Admin::Breadcrumbs | |||
|
|
|||
There was a problem hiding this comment.
Could we add the before_action :set_breadcumbs here and add an empty method that gets called if the child class does not implement set_breadcrumbs? We might even have a "defaut breadcrumb" (build from the resource) for controllers. We could remove a lot of code then.
| @@ -9,6 +9,7 @@ class CustomerReturnsController < ResourceController | |||
| before_action :parent # ensure order gets loaded to support our pseudo parent-child relationship | |||
| before_action :load_form_data, only: [:new, :edit] | |||
| before_action :build_return_items_from_params, only: [:create] | |||
| before_action :set_breadcrumbs | |||
There was a problem hiding this comment.
We could move this before_action into the spree/admin/resource_controller
|
|
||
| def set_breadcrumbs | ||
| set_order_breadcrumbs | ||
| add_breadcrumb t('spree.cart') if action_name == 'cart' |
There was a problem hiding this comment.
We could move the breadcrumb into the cart action instead of this if
| def set_breadcrumbs | ||
| set_order_breadcrumbs | ||
| add_breadcrumb t('spree.cart') if action_name == 'cart' | ||
| add_breadcrumb t('spree.confirm_order') if action_name == 'confirm' |
There was a problem hiding this comment.
Same here, why not add the breadcrumb only in the confirm action?
|
|
||
| layout :get_layout | ||
|
|
||
| before_action :set_user_language | ||
| end | ||
|
|
||
| def plural_resource_name(resource_class) |
There was a problem hiding this comment.
Can we move this method into the admin/resource_controller instead? This module gets included in many (even frontend) controllers.
|
@tvdeyen Thanks for the review. I think that the edge case reported in #3149 is actually present in all admin edit forms, where "the thing" (name, title, whatever) that is rendered in the breadcrumbs has some validation. I see your point that this is a somehow drastic change and potentially could break something, even if I think the only issue will be with extensions The solution should use both approaches, until v2.8 goes EOL (if we merge it now), conditionally switching from a way of setting breadcrumbs to another depending on the version. Eg. in extensions: <% if Solidus.version < '2.9' %>
<% admin_breadcrumb(link_to plural_resource_name(Spree::CustomModel), spree.admin_custom_models_path) %>
<% end %>and in the controller: def set_breadcrumbs
return if Solidus.version < '2.9'
# ... rest of the codewhich is not ideal at all. TL;DR Despite I proposed this path, I'm not sure as well that this is the best solution. Maybe we should go with a third option? At RailsConf @joelhawksley presented this talk about adding a "component" layer into Rails, that is used at GitHub. Here's a demo app. IMO it is a fresh approach that we could consider, even because it will be probably be merged into Rails soon. |
|
Closing, we'll keep this as reference for some future work. Thanks to anyone involved! |
Description
This PR moves the definition of all the breadcrumbs in the admin module from the views to a
before_actionmethod in the controllers. This fixes #3149 and other similar bugs, and will avoid reintroducing them in the future.Since this change affects all the breadcrumbs in the admin engine, the PR is quite large and so I've split it in several cohesive commits. The description of each commit elaborates on the changes implemented in it. I suggest reviewing this PR commit by commit.
These are the key decisions I made:
This PR implements the general approach suggested by @kennyadsl in Fix issue with user breadcrumbs #3152. That is, by setting the breadcrumbs in a
before_actionof the controller, we ensure the logic of the actions haven't changed yet any objects used in the breadcrumb, thus avoiding bugs like Breadcrumbs displays invalid emails modifying users #3149.I took a look at https://github.com/weppos/breadcrumbs_on_rails and (the seemingly better maintained) https://github.com/piotrmurach/loaf for inspiration, but finally decided to go for a simpler solution based on the existing helpers.
After these changes, every controller in the admin module (that renders breadcrumbs) includes a
set_breadcrumbsmethod that is called for all the actions of the controller. Since different actions usually have different breadcrumbs, after the common breadcrumbs are set, this method usually uses a few conditionals to set the action-specific breadcrumbsAn alternative solution would be to set the common breadcrumbs in a common method while any action-specific breadcrumb at the top of each action method. I chose to put everything in the same method because I like better to have everything (both shared and action-specific breadcrumbs) in the same place, rather than scattered around the controller. However, I don't have a very strong opinion about this choice, I understand the merits of the aforementioned alternative.
You can find more details about the changes in each commit's description.
Checklist: