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

Replace use of spree.root_path and root_url with main_app.root_path and main_app.root_url #4635

Merged
merged 6 commits into from
Feb 4, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Dec 28, 2019

What? Why?

Continues #4595

This change will enable us to stop depending on spree_backend and some of the base spree routes that come with it.

Here I also adapt an unnecessary call to a devise method related to routes in after_sign_in_path_for. This was breaking the build without spree_backend.

What should we test?

I dont think we need to test all the changes to main_app.root_path.
I suggest we validate manually the change in after_sign_in_path_for by testing the redirection process after login:
1 - type domain/admin directly on the browser, like https://staging.openfoodnetwork.org.au/admin
2 - logout if you are logged in and repeat step 1
3 - after step 2 you will see the login box
4 - login
5 - expect to land in the backoffice at /admin

Release notes

Changelog Category: Changed
Switched some routes from spree base routes to ofn (main_app) routes to enable removing spree_backend dependency.

@luisramos0 luisramos0 self-assigned this Dec 28, 2019
@luisramos0 luisramos0 changed the title Main app routes Replace use of spree.root_path and root_url with main_app.root_path and main_app.root_url Dec 28, 2019
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.

Looking good but Code Climate is still not happy.

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

LGTM.

The build is failing after you added those minor changes for CodeClimate. The added newlines are altering the test's expectation of what the string should be:

expected: "The hub you have selected is temporarily closed for orders. Please try again later."
     got: "The hub you have selected is temporarily closed for orders.\n        Please try again later."
     

@luisramos0
Copy link
Contributor Author

sorry, I forgot to check the build after pushing.
It's fixed now.

@lin-d-hop lin-d-hop added the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 4, 2020
@lin-d-hop
Copy link
Contributor

Ping @luisramos0 Fancy unpicking the code climate issue?

@luisramos0
Copy link
Contributor Author

the codeclinate issue will be ignored. it's application_controller being too long... not easy at all.

this is ready for testing.

@lin-d-hop
Copy link
Contributor

Tested:
Admin URL when not logged in - showed login panel
Logging in then checking the admin URL - works
Logging out and refreshing admin URL - shows login panel
Logging back in and refreshing the admin URL - works
Logging out then logging in to visit an unauthorised admin URL - shows 'unauthorised' message

All behaved as expected.
Ready to go :-)

@lin-d-hop lin-d-hop added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Feb 4, 2020
@luisramos0 luisramos0 merged commit ea75714 into openfoodfoundation:master Feb 4, 2020
@luisramos0 luisramos0 deleted the main_app_routes branch February 4, 2020 16:02
@RachL RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 4, 2020
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.

5 participants