This keeps the `OrderBalance` abstraction but removes the old code now
that the feature is enabled for all users in all instances and there are
no bugs reported. It's become dead code.
This (should) considerably improve traces like
https://app.datadoghq.com/apm/trace/917632173599137280?spanID=3163385094622710144&env=production&sort=time&colorBy=service&spanViewType=metadata&graphType=flamegraph&shouldShowLegend=true
by fixing the following 3 N+1s
```
user: root
GET /admin/customers.json?enterprise_id=4
USE eager loading detected
Customer => [:enterprise]
Add to your query: .includes([:enterprise])
Call stack
/usr/src/app/app/serializers/api/admin/customer_with_calculated_balance_serializer.rb:24:in `balance_value'
/usr/src/app/app/serializers/api/admin/customer_with_calculated_balance_serializer.rb:9:in `balance'
/usr/src/app/app/controllers/admin/customers_controller.rb:20:in `block (2 levels) in index'
/usr/src/app/app/controllers/admin/customers_controller.rb:17:in `index'
user: root
GET /admin/customers.json?enterprise_id=4
USE eager loading detected
Spree::Address => [:state]
Add to your query: .includes([:state])
Call stack
/usr/src/app/app/serializers/api/address_serializer.rb:14:in `state_name'
/usr/src/app/app/controllers/admin/customers_controller.rb:20:in `block (2 levels) in index'
/usr/src/app/app/controllers/admin/customers_controller.rb:17:in `index'
user: root
GET /admin/customers.json?enterprise_id=4
USE eager loading detected
Spree::Address => [:country]
Add to your query: .includes([:country])
Call stack
/usr/src/app/app/serializers/api/address_serializer.rb:10:in `country_name'
/usr/src/app/app/controllers/admin/customers_controller.rb:20:in `block (2 levels) in index'
/usr/src/app/app/controllers/admin/customers_controller.rb:17:in `index'
```
This popped up after improving the balances calculation. Now, that it's
fast, it's clear that are more performance problems on that endpoint.
We'll see if there are any left after this.
Closes#6727.
This avoids the authorization of all the VOs of the hub, which will go
through VOs that may have become invalid due to their underlying product
not belonging to the supplier the hub has permissions with (or any other
data integrity issue).
This is utterly confusing for the user who is only given a generic error
and doesn't understand what's wrong with the particular VO they changed,
while it may be fine after all. What's more, this often results in
a customer support request, which then may end up with a dev finding out
which VO is broken.
Also, there's no point in loading them from DB if the users didn't touch
them.
This removes an N+1 with taggings but doesn't solve the one with tags.
Using `includes(taggings: :base_tags)` based on
47da5036de/lib/acts_as_taggable_on/taggable.rb (L83-L84)
wasn't enough to solve it and I got to stop here. This is scope-creeping
too much.
We get from an initial INNER JOIN with variants and products to fetch
the variant overrides + N queries like:
```sql
SELECT "spree_variants".* FROM "spree_variants" WHERE
"spree_variants"."deleted_at" IS NULL AND "spree_variants"."id" = $1
LIMIT 1 [["id", 1545]]
SELECT "spree_products".* FROM "spree_products" WHERE
"spree_products"."id" = $1 LIMIT 1 [["id", 604]]
```
to the same initial INNER JOIN + just 2 queries like:
```sql
SELECT "spree_variants".* FROM "spree_variants" WHERE
"spree_variants"."deleted_at" IS NULL AND "spree_variants"."id" IN
(1551, 1554)
SELECT "spree_products".* FROM "spree_products" WHERE
"spree_products"."deleted_at" IS NULL AND "spree_products"."id" IN (606,
607)
```
In the line below we filter them out in Ruby so it's a waste of
resources. The fundamental difference is that `#includes` and
`#references` results in LEFT JOINs, whereas `#joins` results in INNER
JOIN, and because there's a default scope on `deleted_at IS NULL`, these
are not included in the result set.
This however, requires us to move away from the current algorithm but
unfortunately we can't refactor it completely yet.
Before:
```sql
SELECT *
FROM "variant_overrides"
LEFT OUTER
JOIN "spree_variants"
ON "spree_variants"."id" = "variant_overrides"."variant_id"
AND "spree_variants"."deleted_at" IS NULL
LEFT OUTER
JOIN "spree_products"
ON "spree_products"."id" = "spree_variants"."product_id"
AND "spree_products"."deleted_at" IS NULL
WHERE "variant_overrides"."permission_revoked_at" IS NULL
AND "variant_overrides"."hub_id" IN (
SELECT "enterprises"."id"
FROM "enterprises"
INNER
JOIN "enterprise_roles"
ON "enterprise_roles"."enterprise_id" = "enterprises"."id"
WHERE (enterprise_roles.user_id = ?)
AND (sells != 'none')
ORDER BY name)
```
After:
```sql
SELECT "variant_overrides".*
FROM "variant_overrides"
INNER
JOIN "spree_variants"
ON "spree_variants"."id" = "variant_overrides"."variant_id"
AND "spree_variants"."deleted_at" IS NULL
INNER
JOIN "spree_products"
ON "spree_products"."id" = "spree_variants"."product_id"
AND "spree_products"."deleted_at" IS NULL
WHERE "variant_overrides"."permission_revoked_at" IS NULL
AND "variant_overrides"."hub_id" IN (
SELECT "enterprises"."id"
FROM "enterprises"
INNER
JOIN "enterprise_roles"
ON "enterprise_roles"."enterprise_id" = "enterprises"."id"
WHERE (enterprise_roles.user_id = ?)
AND (sells != 'none')
ORDER BY name)
```
This is covered in the test suite by
spec/controllers/admin/variant_overrides_controller_spec.rb:72. It keeps
passing so we're good to go.
This method is named "update distribution charge". What this method actually does is delete all of the fee adjustments on an order and all it's line items, then recreate them all from scratch. We call this from lots of different places all the time, and it's incredibly expensive. It even gets called from inside of transactions being run inside callbacks. Renaming it hopefully will add a bit of clarity.
This needs to be a lot more granular!
This makes it possible to deploy it without releasing it to users since
the toggle is not enabled for anyone.
It aims to make the balance calculation consistent across pages.
This query object is meant to be reusable but those includes are
context-specific and will likely not be needed when reusing the query
elsewhere. If we keep them there, chances are next dev might not notice
it and will introduce a performance regression.
It's simpler and many orders of magnitude more efficient to ask the DB
to aggregate the customer balance based on their orders. It removes
a nasty N+1.
The resulting SQL query is:
```sql
SELECT customers.*, SUM(spree_orders.total - spree_orders.payment_total) AS balance
FROM "customers"
INNER JOIN "spree_orders"
ON "spree_orders"."customer_id" = "customers"."id"
WHERE "customers"."enterprise_id" = 1
AND (completed_at IS NOT NULL)
AND (state != 'canceled')
GROUP BY customers.id
ORDER BY email;
```
Sometimes the objects are not paginated. In this case we need to avoid trying to render pagination data, as it will throw an error. This guard clause also means we can remove messy conditionals from several controllers.
