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 broken registration link in footer #3174

Merged

Conversation

haseleyi
Copy link
Contributor

@haseleyi haseleyi commented Dec 4, 2018

What? Why?

Closes #3061

We have a broken link: In the account page for a buyer account, the footer features a registration call partial that routes the user to /signup instead of /register.
This fixes things. But I'm not satisfied with it: why would registration_path resolve to /signup on only this page (or at all for that matter)? I'd rather address the root of the problem.

What should we test?

In a buyer account, navigate to My Account > Orders and click "Register here" in the footer.

Release notes

Fixed a broken registration link in the footer of our My Account page.

Changelog Category: Fixed

@luisramos0
Copy link
Contributor

luisramos0 commented Dec 5, 2018

good question: "why would registration_path resolve to /signup on only this page (or at all for that matter)?"
tricky one. Some clues:
this dependency:

gem 'spree_auth_devise', github: 'openfoodfoundation/spree_auth_devise', branch: 'spree-upgrade-intermediate'

takes you to this route:
https://github.com/openfoodfoundation/spree_auth_devise/blob/da9eecefc6fe13dedf4c6f3febec79caad839ec3/config/routes.rb#L18

You can have a look at the registration_path by using byebug in this show method:

Because this controller (account page) is also on this routes file:
https://github.com/openfoodfoundation/spree_auth_devise/blob/da9eecefc6fe13dedf4c6f3febec79caad839ec3/config/routes.rb#L34
I think the registration_path defined on this routes file with /sign_up takes precedence the ofn's one in:

get "/register", to: "registration#index", as: :registration

There's also this devise_scope that I dont really know how it works.

I think we can consider this is a bug in OFN because we should not have re-written this registration named route. I dont think there is a good reason to do this but i am not 100% sure.

The proper fix could be to rename the registration path in the ofn context to separate it from the spree devise registration path.
Or maybe an easier fix would be to overwrite the devise registration path in ofn's routes.

Let's see what reviewers say.

@mkllnk
Copy link
Member

mkllnk commented Dec 6, 2018

Very good detective work @luisramos0. Most front-end pages are our own implementation, but we are still using Spree's UsersController and override the main action. I guess that Spree's controller is referring to Spree routes instead of OFN routes. So I think we could solve this problem by implementing our own account page. It seems out of scope, but is actually a fix we want to do anyway.

So we have some options now:

  • Explicitly link /register.
  • Rename our register path and use that.
  • Overwrite Spree's register path.
  • Add a route to redirect from /signup to /register.
  • Move accounts page into the OFN namespace and abandon Spree's controller.

I'm surprised that there are so many solutions to this simple problem. I'm voting for the last one, but would accept the current pull request until that is implemented. A working link wins over code style. It would be nice to add a comment though. Otherwise someone could have the brilliant idea to change it back to registration_path again, because that's the right way to do it.

@luisramos0
Copy link
Contributor

thanks @mkllnk
I agree. That means we are approving this PR as long as a comment is added :-)

@haseleyi
Copy link
Contributor Author

haseleyi commented Dec 6, 2018

Thank you both for the excellent breakdown of the issue and potential solutions. This is a thoughtful and inspiring community to learn from!

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Excellent comment! Thank you.

-# Please forgive the hard-coded link:
-# The more elegant 'registration_path' resolves to /signup due to spree_auth_device > config > routes.rb
-# This is one of several possible fixes. Long-term, we'd like to bring the accounts page into OFN.
-# View the discussion here: https://github.com/openfoodfoundation/openfoodnetwork/pull/3174
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@RachL
Copy link
Contributor

RachL commented Dec 14, 2018

Deploying in AU staging (yes I jumped one issue, but I'm in a meeting in 15 minutes, so I figured rather I test 1 small one rather than nothing :) )

@RachL RachL self-assigned this Dec 14, 2018
@RachL RachL added the pr-staged-au staging.openfoodnetwork.org.au label Dec 14, 2018
@RachL
Copy link
Contributor

RachL commented Dec 14, 2018

Link redirects correctly to https://staging.openfoodnetwork.org.au/register ! ready to go :)

@RachL RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Dec 14, 2018
@sauloperez sauloperez merged commit fe0c56c into openfoodfoundation:master Dec 14, 2018
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.

Wrong URL in footer leading to error 500
5 participants