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

Remove order merge logic happening on every request #1116

Closed
mtomov opened this issue May 6, 2016 · 2 comments
Closed

Remove order merge logic happening on every request #1116

mtomov opened this issue May 6, 2016 · 2 comments

Comments

@mtomov
Copy link
Contributor

mtomov commented May 6, 2016

Hello,

set_current_order is a method, which is called on every request and results in 3 sql queries for logged in users:

  Spree::Order Load (0.7ms)  SELECT  "spree_orders".* FROM "spree_orders" WHERE "spree_orders"."completed_at" IS NULL AND "spree_orders"."currency" = $1 AND "spree_orders"."guest_token" = $2 AND "spree_orders"."store_id" = $3 AND "spree_orders"."user_id" = $4 LIMIT 1  [["currency", "USD"], ["guest_token", "6YHZ-S_2WypaPxxky4fHVA"], ["store_id", 2], ["user_id", 1]]
  Spree::Order Load (0.4ms)  SELECT  "spree_orders".* FROM "spree_orders" WHERE "spree_orders"."user_id" = $1 AND "spree_orders"."frontend_viewable" = $2 AND "spree_orders"."store_id" = 2  ORDER BY "spree_orders"."created_at" DESC LIMIT 1  [["user_id", 1], ["frontend_viewable", "t"]]
  Spree::Order Load (0.3ms)  SELECT "spree_orders".* FROM "spree_orders" WHERE "spree_orders"."user_id" = $1 AND "spree_orders"."completed_at" IS NULL AND (id != 9)  [["user_id", 1]]

spree orders - 2 queries

The method performs an order merge operation if there's more than one incomplete order for the user.

Unless I miss a use case, this correct behaviour is have this operation be performed only after a user login - exactly in UserSessionsController#create.

It is currently incorrectly executed there in a before_action instead of an after_action with the inclusion of the Order module.

I was wondering if anyone would have a recommendation on how to cleanly move the order merge logic in the create session operation, as stores could be using different gems for authentication.
  • One way is through registering after login hooks, like the self.update_hooks = Set.new on Order.rb, but it seems like too much of a change.

Edit, after having another look, it appears that you have already come up with a brilliant solution to that:

config/initializers/warden.rb

So, do you think it will be a good idea to tuck the Order#merge logic in there as well? I'm only not sure how to get the current_order from warden, but there must be a way.

@mvz - I'm pinging you as you were active on this area of the code recently.

Thank you!

@mtomov mtomov changed the title Remove order merge logic happening on every request. Remove order merge logic happening on every request May 6, 2016
@cbrunsdon
Copy link
Contributor

This actually looks like a pretty reasonable approach to me. We will need to ensure that we have something friendly for non-auth-devise usage as well but I think its pretty low hanging fruit to remove every round trips from every request. Would be a good target for someone during the hack days next week, too...

@jhawthorn
Copy link
Contributor

Fixed by #2185

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

No branches or pull requests

3 participants