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.

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

Fix issue with user breadcrumbs #3152

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Fix issue with user breadcrumbs #3152

merged 1 commit into from
Jan 15, 2020

Conversation

jtapia
Copy link
Contributor

@jtapia jtapia commented Mar 21, 2019

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtapia thanks for making this PR! 👍

I think this works, but do we feel like the real problem is displaying the wrong information or using an instance variable instead of the AR object? Would like your opinion @kennyadsl.

@kennyadsl kennyadsl changed the title Fix issue with user breadcrums Fix issue with user breadcrumbs Mar 22, 2019
@kennyadsl
Copy link
Member

This could be a solution (I'm wondering if using only @user.email_was without any if fixes the issue as well since I think it always returns the right email) anyway I think this issue could be present in several other admin pages and we should come up with a more robust long-term solution.

I'm taking a look at https://github.com/weppos/breadcrumbs_on_rails which is old and unmaintend but it's very few lines of code that we could add to solidus easily maybe. It provides an interface in the controller to set breadcrumbs, this way we could maybe define breadcrumbs before calling the update_attributes that makes the instance dirty? How does it play with ResourceController? Maybe worth exploring a little bit more.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this simple fix for the edge case reported in #3149

Thanks

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@aldesantis
Copy link
Member

@jtapia would like to get this in. Could you rebase?

@jtapia
Copy link
Contributor Author

jtapia commented Jan 10, 2020

@aldesantis done

@aldesantis aldesantis merged commit 6d54d59 into solidusio:master Jan 15, 2020
@aldesantis
Copy link
Member

@jtapia thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants