Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Breadcrumbs displays invalid emails modifying users #3149

Closed
kennyadsl opened this issue Mar 18, 2019 · 2 comments
Closed

Breadcrumbs displays invalid emails modifying users #3149

kennyadsl opened this issue Mar 18, 2019 · 2 comments
Labels
changelog:solidus_backend Changes to the solidus_backend gem type:bug Error, flaw or fault

Comments

@kennyadsl
Copy link
Member

kennyadsl commented Mar 18, 2019 •
edited
Loading

When changing a user's email in the backend (/admin/users/:id), if there is a validation error on the email, breadcrumbs of the page are updated with the wrong, invalid value.

Solidus Version:

master

To Reproduce

In sandbox:

  1. Go to Admin -> User -> Any user -> Account tab
  2. Change user's email with the email taken by another user and save
  3. Click Update button

Current behavior

Breadcrumbs are updated with the wrong email, even if there's an error on the page

Expected behavior

The old, persisted and valid email is displayed in breadcrumbs

Screenshots

breadcrumbs-bug

👆 I'm actually editing admin@example.com

Additional context

I suppose this happens because breacrumbs is using the ivar (maybe @user, or @object from Spree::Admin::ResourceController) that contains the instance of the User with errors (and previously submitted params) and not the persisted one, which is valid.

I'm afraid this could happen in other pages as well, where the value that the user is trying to change to an invalid one is used to create breadcrumbs of that page.
@kennyadsl kennyadsl added type:bug Error, flaw or fault changelog:solidus_backend Changes to the solidus_backend gem labels Mar 18, 2019
@jtapia jtapia mentioned this issue Mar 21, 2019
Merged
@jtapia
Copy link
Contributor

jtapia commented Mar 21, 2019

@kennyadsl I'm not sure if this is the best approach to tackle this issue, but here is a possible solution #3152

@cavalle cavalle mentioned this issue Apr 22, 2019
Closed
3 tasks
kennyadsl pushed a commit to cavalle/solidus that referenced this issue May 28, 2019
@cavalle @kennyadsl
Set users breadcrumbs from controller to fix bug
bcbcf7b 
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.
kennyadsl pushed a commit to cavalle/solidus that referenced this issue May 28, 2019
@cavalle @kennyadsl
Set all standard breadcrumbs from controllers
94192f0 
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.
kennyadsl pushed a commit to cavalle/solidus that referenced this issue May 28, 2019
@cavalle @kennyadsl
Deprecate support for setting breadcrumbs from views
64230bf 
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.
@chrean
Copy link
Contributor

chrean commented Dec 27, 2022

A PR was accepted to fix an edge case, and we are closing this because we are in the process of building a new version of the Solidus Admin, which will make this issue soon irrelevant.

@chrean chrean closed this as completed Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
changelog:solidus_backend Changes to the solidus_backend gem type:bug Error, flaw or fault
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move breadcrumbs setting from views to controllers cavalle/solidus
3 participants
@kennyadsl @jtapia @chrean