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

Explicitly define backend/promotions routes #2332

Conversation

vladstoick
Copy link
Contributor

@vladstoick vladstoick commented Oct 27, 2017

Currently the show route is also include but it's never defined which leads to a
AbstractController::ActionNotFound being thrown in case someone reaches
that page

@vladstoick vladstoick force-pushed the bug/promotions_controller_missing_action branch from 6f2b481 to 49ff218 Compare October 27, 2017 14:34
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks, left a comment to improve how to exclude a single route from resources.

@@ -9,7 +9,7 @@
end
end

resources :promotions do
resources :promotions, only: [:new, :edit, :update, :create, :destroy,] do
Copy link
Member

Choose a reason for hiding this comment

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

I'd write except: :show instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it 👍

@vladstoick vladstoick force-pushed the bug/promotions_controller_missing_action branch 3 times, most recently from 9d394c8 to 49e4558 Compare October 27, 2017 15:09
@kennyadsl
Copy link
Member

kennyadsl commented Oct 29, 2017

Thinking twice, it's probably better to redirect to the :edit route, as we did in backend products controller.

@mtomov
Copy link
Contributor

mtomov commented Oct 30, 2017

Thinking twice, it's probably better to redirect to the :edit route, as we did in backend products controller.

this has been the best redirect ever, as I can relatively easy add admin to the frontend products page url to get to the admin one. Not as easy the opposite way around.

In the promotions, I don't think it matters much.

@kennyadsl
Copy link
Member

The thing I was thinking at is that by only removing the :show route we still have a 404 error (instead of the current AbstractController::ActionNotFound), which does not solve the case someone reaches that page. With the redirect in controller instead we wont have any error.

@vladstoick vladstoick force-pushed the bug/promotions_controller_missing_action branch from 49e4558 to 0f936fd Compare November 9, 2017 17:10
Copy link
Contributor

@jordan-brough jordan-brough left a comment

Choose a reason for hiding this comment

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

Thank you!

@kennyadsl
Copy link
Member

@vladstoick thanks!

@jordan-brough jordan-brough merged commit b235378 into solidusio:master Nov 10, 2017
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.

4 participants