WA-VERIFY-089: use size for embedded association counts (Mongoid 8)#1087
WA-VERIFY-089: use size for embedded association counts (Mongoid 8)#1087kitcommerce merged 3 commits intonextfrom
Conversation
Simplicity ReviewVerdict: PASS_WITH_NOTES (LOW)
|
Security ReviewVerdict: PASS_WITH_NOTES (LOW)
|
Rails Conventions ReviewVerdict: PASS_WITH_NOTES (LOW)
|
Architecture ReviewVerdict: CHANGES_REQUIRED (HIGH)
Action: either add the missing embedded-count changes required by #1080 (and align notes/body), or retitle/split/relink so QueryCache work lives under WA-VERIFY-086 (#1077). |
…ll sites) - Fix ActivityViewModel#scoped_entries: chained any_of in a loop produced AND-of-ORs under Mongoid 8, silently returning no results when multiple ids were supplied. Collect all clauses and call any_of once. - Fix Tax::Rate.search: pass clauses as splat (any_of(*clauses)) to match Mongoid 8's expected signature. - All other 7 call sites audited: single standalone calls, semantics unchanged. Tests added: - GeneratedPromoCode not_expired scope (nil vs future vs past expires_at) - Navigation::Redirect.search (path + destination regex branches) - Tax::Rate.search (region / postal_code / country clauses) - FeaturedProducts.changesets (changeset and original product_ids branches) - Expanded lint tests to cover both nil and [] variants for images/variants Closes #1083
5b53299 to
4a611d8
Compare
|
Updated this branch to actually implement WA-VERIFY-089 / #1080. What changed
Commands run
|
Fix Brief — Wave 1 Architecture CHANGES_REQUIRED (HIGH)The architecture reviewer flagged a mismatch between the PR's stated intent and its diff:
Required fix: Add the embedded association Branch: |
Replace remaining embedded association .count calls with .size in test files where order.items, fulfillment.items, and other embedded collections were using .count unnecessarily. Mongoid 8 changes embedded .count to always hit the database. Using .size instead reads from the already-loaded in-memory collection. Files changed: - core/test/models/workarea/order_test.rb - core/test/services/workarea/add_multiple_cart_items/item_test.rb - core/test/services/workarea/add_multiple_cart_items_test.rb - core/test/services/workarea/cart_cleaner_test.rb - core/test/services/workarea/create_fulfillment_test.rb - core/test/services/workarea/inventory_adjustment_test.rb - core/test/services/workarea/order_merge_test.rb Closes #1080
|
Completed the embedded Full audit confirmed no remaining embedded association |
Rails-Security ReviewVerdict: PASS Purely mechanical |
Wave 2 Review — Database & Test Qualitydatabase: PASS_WITH_NOTES — Replacing embedded association with avoids Mongoid 8 DB hits; looks correct. Note: on non-embedded criteria can load docs, but the call sites here appear to be embedded/loaded collections and tests/docs were updated consistently. Wave 2 PASS_WITH_NOTES. |
Wave 2 Review — Database & Test Qualitydatabase: PASS_WITH_NOTES — Replacing embedded association Wave 2 PASS_WITH_NOTES. |
kitcommerce
left a comment
There was a problem hiding this comment.
Wave 3 reviews (performance / frontend / accessibility)
{"reviewer":"performance","verdict":"PASS_WITH_NOTES","severity":"LOW","summary":"Mostly a query-reduction change, but a couple `.count`→`.size` swaps may change loading behavior depending on collection type.","findings":[{"severity":"LOW","file":"admin/app/views/workarea/admin/catalog_variants/index.html.haml","line":21,"issue":"`@variants.size` may force loading records if `@variants` is a Criteria (depending on Mongoid behavior/version), whereas `.count` is always a count query.","suggestion":"Confirm `@variants` is an Array/paginated collection already loaded; if it can be a Criteria, consider keeping `.count` here while using `.size` specifically for embedded associations."}]}{"reviewer":"frontend","verdict":"PASS","severity":null,"summary":"No JS/Turbo/Stimulus changes in this PR.","findings":[]}{"reviewer":"accessibility","verdict":"PASS","severity":null,"summary":"No accessibility-affecting UI/markup changes beyond numeric counts.","findings":[]}
kitcommerce
left a comment
There was a problem hiding this comment.
Wave 4 (documentation) review
PASS — This PR is code/test/view-only; no user-facing docs appear required.
Fix embedded association
.countcalls that can trigger database queries in Mongoid 8.Mongoid 8 changes embedded document
.countto always hit the database. All embedded association.countcalls have been audited and replaced with.sizewhere in-memory length is semantically correct.Changes
Application code
core/app/seeds/workarea/orders_seeds.rb—user.addresses.count→.sizeadmin/app/views/workarea/admin/catalog_categories/index.html.haml—result.product_rules.count→.sizeadmin/app/views/workarea/admin/catalog_variants/index.html.haml—@variants.count→.sizeTest files
core/test/models/workarea/order_test.rb—order.items.count→.sizecore/test/services/workarea/add_multiple_cart_items/item_test.rb—order.items.count→.sizecore/test/services/workarea/add_multiple_cart_items_test.rb—order.items.count→.sizecore/test/services/workarea/cart_cleaner_test.rb—@order.items.count→.sizecore/test/services/workarea/create_fulfillment_test.rb—fulfillment.items.count→.sizecore/test/services/workarea/inventory_adjustment_test.rb—order.items.count→.sizecore/test/services/workarea/order_merge_test.rb—original.items.count→.sizeDocumentation
docs/source/articles/content.html.md—content.blocks.count→.sizedocs/source/articles/order-pricing.html.md—all_price_adjustments.count→.sizedocs/source/articles/orders-and-items.html.md—item.price_adjustments.count→.sizedocs/source/articles/products.html.md.erb—model.variants.count/view_model.variants.count→.sizeOther
admin/test/integration/workarea/admin/import_taxes_integration_test.rb—category.rates.count→.sizeadmin/test/integration/workarea/admin/pricing_skus_integration_test.rb—sku.prices.count→.sizeadmin/test/view_models/workarea/admin/pricing_sku_view_model_test.rb—@sku.prices.count→.sizecore/test/models/workarea/pricing/tax_applier_test.rb—@shipping.price_adjustments.count→.sizenotes/WA-VERIFY-089-mongoid8-embedded-count.mdIntentionally unchanged
.counton Mongoid criteria/relations (non-embedded) where a database count is expected.counton Ruby arrays/hashes orEnumerable#count { block }core/vendor/)Client impact
None expected —
.sizereturns the same value as the previous in-memory.countbehavior. This prevents unnecessary database queries introduced by Mongoid 8.Closes #1080