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

unRESTful OrdersController #163

Open
RyanofWoods opened this issue Jul 1, 2021 · 1 comment
Open

unRESTful OrdersController #163

RyanofWoods opened this issue Jul 1, 2021 · 1 comment
Labels

Comments

@RyanofWoods
Copy link
Contributor

Me and @kennyadsl have discussed this a little bit in Solidus PRs 4102 and 4081. So I thought it would be a good idea to consolidate the information and let discussion about it happen here for easier reference in the future.

The OrdersController currently has the following methods that make it unRESTful:

  • populate (can be moved to OrderContentsController like Solidus PR 4102 that would it fix Solidus frontend)
  • populate_redirect (discussed in Solidus PR 4081 that it doesn't give much value. Could be good maybe to remove it)
  • empty (can be moved to OrderContentsController)
  • accurate_title (can become a private method)

The method update also currently has two responsibilities:

  1. update the @order.contents after submitting an edit cart form by clicking "update" or "checkout"
  2. advance the order's state if the "checkout" button gets clicked and it's current state is cart (essentially start the checkout process)

This brings light to point worth discussing. If OrderContentsController was to be added and have the methods create and destroy which would respectively replace OrdersControllers populate and empty, the OrdersController wouldn't be truly RESTful and still would have multiple responsibilities. To achieve this the updating of @orders.content in update would have to be moved to OrdersContentsController. But currently on the cart page, both the "update" and "checkout" buttons act as save buttons for the edit cart form. Moving the update content functionality out would cause the "checkout" to longer be able to save the content changes.

Another point that would be nice to discuss is, if we make it RESTful, what would be the most desirable routes for both of these controllers? Keeping the cart route would be nice, but how should we handle the orders.content?

Here's the different routes paths we would need:

post '/orders/content', to: 'order_contents#create'
get '/cart', to: 'order_contents#edit', as: :cart
patch '/orders/content', to: 'order_contents#update', as: :update_order_content
delete '/orders/content', to: 'order_contents#destroy'

patch '/cart', to: 'orders#update', as: :update_cart
@RyanofWoods RyanofWoods changed the title frontend OrdersController is unRESTful and breaks SRP unRESTful OrdersController Jul 1, 2021
@stale
Copy link

stale bot commented Nov 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 11, 2022
@kennyadsl kennyadsl added the hold label Nov 11, 2022
@stale stale bot removed the stale label Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants