diff --git a/admin/app/views/workarea/admin/catalog_categories/index.html.haml b/admin/app/views/workarea/admin/catalog_categories/index.html.haml index 8a4c6a47ec..6fa6228290 100644 --- a/admin/app/views/workarea/admin/catalog_categories/index.html.haml +++ b/admin/app/views/workarea/admin/catalog_categories/index.html.haml @@ -70,7 +70,7 @@ - else = result.breadcrumbs.join(' > ') %td.align-center= result.featured_products.count - %td.align-center= result.product_rules.count + %td.align-center= result.product_rules.size %td.align-center = link_to insights_catalog_category_path(result), class: 'link link--no-underline' do %span.spark{ title: t('workarea.admin.catalog_categories.index.sparkline_title') } diff --git a/admin/app/views/workarea/admin/catalog_variants/index.html.haml b/admin/app/views/workarea/admin/catalog_variants/index.html.haml index 240832ab23..edbf4201c1 100644 --- a/admin/app/views/workarea/admin/catalog_variants/index.html.haml +++ b/admin/app/views/workarea/admin/catalog_variants/index.html.haml @@ -21,7 +21,7 @@ .view__container .browsing-controls.browsing-controls--with-divider %p.browsing-controls__count - = t('workarea.admin.catalog_variants.index.variant_pluralize', count: @variants.count) + = t('workarea.admin.catalog_variants.index.variant_pluralize', count: @variants.size) - if @variants.any? %table.index-table diff --git a/admin/test/integration/workarea/admin/import_taxes_integration_test.rb b/admin/test/integration/workarea/admin/import_taxes_integration_test.rb index 70d08e8989..dae2b1c1a8 100644 --- a/admin/test/integration/workarea/admin/import_taxes_integration_test.rb +++ b/admin/test/integration/workarea/admin/import_taxes_integration_test.rb @@ -23,7 +23,7 @@ def test_can_create_an_import assert(import.created_by_id.present?) category.reload - assert_equal(2, category.rates.count) + assert_equal(2, category.rates.size) end end end diff --git a/admin/test/integration/workarea/admin/pricing_skus_integration_test.rb b/admin/test/integration/workarea/admin/pricing_skus_integration_test.rb index ccef1aeb3c..3f1ec8d67a 100644 --- a/admin/test/integration/workarea/admin/pricing_skus_integration_test.rb +++ b/admin/test/integration/workarea/admin/pricing_skus_integration_test.rb @@ -29,7 +29,7 @@ def test_creation assert(sku.on_sale?) assert(sku.discountable?) assert_nil(sku.msrp) - assert_equal(1, sku.prices.count) + assert_equal(1, sku.prices.size) assert_equal(10.to_m, sku.prices.first.regular) assert_nil(sku.prices.first.sale) end diff --git a/admin/test/view_models/workarea/admin/pricing_sku_view_model_test.rb b/admin/test/view_models/workarea/admin/pricing_sku_view_model_test.rb index e474f47557..b25af44377 100644 --- a/admin/test/view_models/workarea/admin/pricing_sku_view_model_test.rb +++ b/admin/test/view_models/workarea/admin/pricing_sku_view_model_test.rb @@ -21,7 +21,7 @@ def setup_pricing_sku_and_prices end def test_sell_prices - assert_equal(@sku.prices.count, @sku.sell_prices.count) + assert_equal(@sku.prices.size, @sku.sell_prices.size) end def test_min_price diff --git a/core/app/seeds/workarea/orders_seeds.rb b/core/app/seeds/workarea/orders_seeds.rb index 541a611883..bc655abc61 100644 --- a/core/app/seeds/workarea/orders_seeds.rb +++ b/core/app/seeds/workarea/orders_seeds.rb @@ -34,7 +34,7 @@ def create_order ) end - unless user.addresses.count > 0 + unless user.addresses.size > 0 user.addresses.create!( first_name: user.first_name, last_name: user.last_name, diff --git a/core/test/models/workarea/featured_products_changesets_test.rb b/core/test/models/workarea/featured_products_changesets_test.rb new file mode 100644 index 0000000000..03d8ced751 --- /dev/null +++ b/core/test/models/workarea/featured_products_changesets_test.rb @@ -0,0 +1,32 @@ +require 'test_helper' + +module Workarea + class FeaturedProductsChangesetsTest < TestCase + def test_changesets_finds_by_product_id_in_changeset_and_original + release = create_release + + in_changeset = Release::Changeset.create!( + release: release, + changeset: { 'product_ids' => %w(P1 P2) }, + original: {} + ) + + in_original = Release::Changeset.create!( + release: release, + changeset: {}, + original: { 'product_ids' => %w(P1) } + ) + + unrelated = Release::Changeset.create!( + release: release, + changeset: { 'product_ids' => %w(P3) }, + original: { 'product_ids' => %w(P4) } + ) + + results = FeaturedProducts.changesets('P1').to_a + assert_includes(results, in_changeset) + assert_includes(results, in_original) + refute_includes(results, unrelated) + end + end +end diff --git a/core/test/models/workarea/navigation/redirect_test.rb b/core/test/models/workarea/navigation/redirect_test.rb index 377bea9fe7..378597e18f 100644 --- a/core/test/models/workarea/navigation/redirect_test.rb +++ b/core/test/models/workarea/navigation/redirect_test.rb @@ -26,6 +26,17 @@ def test_sanitizing_paths end end + def test_search + path_match = create_redirect(path: '/foo-bar', destination: '/other') + dest_match = create_redirect(path: '/something', destination: '/foo-destination') + no_match = create_redirect(path: '/baz', destination: '/qux') + + results = Redirect.search('foo').to_a + assert_includes(results, path_match) + assert_includes(results, dest_match) + refute_includes(results, no_match) + end + def test_handle_invalid_path path = '/category/FoalBroodmare/Supplements-Mares & Foals/20552.html' encoded_path = URI::DEFAULT_PARSER.escape(path) diff --git a/core/test/models/workarea/order_test.rb b/core/test/models/workarea/order_test.rb index a87813ea71..08349a45d6 100644 --- a/core/test/models/workarea/order_test.rb +++ b/core/test/models/workarea/order_test.rb @@ -83,10 +83,10 @@ def test_add_item assert(order.items.last.updated_at.present?) order.add_item(product_id: '1234', sku: 'SKU', quantity: 2) - assert_equal(1, order.items.count) + assert_equal(1, order.items.size) assert_equal(4, order.items.last.quantity) - assert_no_changes 'order.items.count' do + assert_no_changes 'order.items.size' do order.add_item( product_id: '1234', sku: 'SKU', @@ -96,7 +96,7 @@ def test_add_item Workarea.config.distinct_order_item_attributes << :discountable - assert_changes 'order.items.count' do + assert_changes 'order.items.size' do order.add_item( product_id: '1234', sku: 'SKU', @@ -105,7 +105,7 @@ def test_add_item ) end - assert_no_changes 'order.items.count' do + assert_no_changes 'order.items.size' do order.add_item( product_id: '1234', sku: 'SKU', @@ -123,7 +123,7 @@ def test_update_item assert_equal(1, order.items.first.quantity) order.update_item(item.id, sku: 'SKU2') - assert_equal(1, order.items.count) + assert_equal(1, order.items.size) assert_equal('SKU2', order.items.first.sku) order = Order.new diff --git a/core/test/models/workarea/pricing/discount/generated_promo_code_test.rb b/core/test/models/workarea/pricing/discount/generated_promo_code_test.rb index 2924aa6aaa..1d0baa826d 100644 --- a/core/test/models/workarea/pricing/discount/generated_promo_code_test.rb +++ b/core/test/models/workarea/pricing/discount/generated_promo_code_test.rb @@ -8,6 +8,19 @@ def test_generated_promo_code code = GeneratedPromoCode.generate_code('WL-') assert_match(/WL-/i, code) end + + def test_not_expired_scope + code_list = create_code_list + + expired = code_list.promo_codes.create!(code: 'exp', expires_at: 1.day.ago) + nil_expiry = code_list.promo_codes.create!(code: 'nilexp', expires_at: nil) + future = code_list.promo_codes.create!(code: 'future', expires_at: 1.day.from_now) + + results = code_list.promo_codes.not_expired.to_a + assert_includes(results, nil_expiry) + assert_includes(results, future) + refute_includes(results, expired) + end end end end diff --git a/core/test/models/workarea/pricing/tax_applier_test.rb b/core/test/models/workarea/pricing/tax_applier_test.rb index 37b9127ce6..e100b3a110 100644 --- a/core/test/models/workarea/pricing/tax_applier_test.rb +++ b/core/test/models/workarea/pricing/tax_applier_test.rb @@ -91,7 +91,7 @@ def test_with_multiple_tax_codes_and_discount @shipping.reload - assert_equal(2, @shipping.price_adjustments.count) + assert_equal(2, @shipping.price_adjustments.size) assert_equal([0.28.to_m, 0.50.to_m], @shipping.price_adjustments.map(&:amount)) end @@ -115,7 +115,7 @@ def test_partial_shipping_quantity_tax_calculation @shipping.save! @shipping.reload - assert_equal(1, @shipping.price_adjustments.count) + assert_equal(1, @shipping.price_adjustments.size) price_adjustment = @shipping.price_adjustments.last assert_equal('tax', price_adjustment.price) assert_equal(0.15.to_m, price_adjustment.amount) diff --git a/core/test/models/workarea/tax/rate_test.rb b/core/test/models/workarea/tax/rate_test.rb new file mode 100644 index 0000000000..ffa20d3d79 --- /dev/null +++ b/core/test/models/workarea/tax/rate_test.rb @@ -0,0 +1,35 @@ +require 'test_helper' + +module Workarea + module Tax + class RateTest < TestCase + def test_search + category = create_tax_category(rates: []) + + pa_region = category.rates.create!(country: Country['US'], region: 'PA') + zip_code = category.rates.create!(country: Country['US'], postal_code: '19106') + canada = category.rates.create!(country: Country['CA']) + other = category.rates.create!(country: Country['US'], region: 'NJ', postal_code: '07001') + + # region match + results = Rate.search('PA').to_a + assert_includes(results, pa_region) + refute_includes(results, zip_code) + refute_includes(results, canada) + + # postal code match + results = Rate.search('191').to_a + assert_includes(results, zip_code) + refute_includes(results, pa_region) + + # country match + results = Rate.search('CA').to_a + assert_includes(results, canada) + + # no match + results = Rate.search('NOPE').to_a + assert_empty(results) + end + end + end +end diff --git a/core/test/services/workarea/add_multiple_cart_items/item_test.rb b/core/test/services/workarea/add_multiple_cart_items/item_test.rb index de1d5777a6..2fce5ca820 100644 --- a/core/test/services/workarea/add_multiple_cart_items/item_test.rb +++ b/core/test/services/workarea/add_multiple_cart_items/item_test.rb @@ -39,7 +39,7 @@ def test_save assert_equal(1, item.quantity) order.reload - assert_equal(1, order.items.count) + assert_equal(1, order.items.size) params = { sku: 'SKU3', quantity: 2 } item = AddMultipleCartItems::Item.new(order, params) @@ -54,7 +54,7 @@ def test_save ) order.reload - assert_equal(1, order.items.count) + assert_equal(1, order.items.size) end end end diff --git a/core/test/services/workarea/add_multiple_cart_items_test.rb b/core/test/services/workarea/add_multiple_cart_items_test.rb index defe4b5741..0f97a627a9 100644 --- a/core/test/services/workarea/add_multiple_cart_items_test.rb +++ b/core/test/services/workarea/add_multiple_cart_items_test.rb @@ -28,7 +28,7 @@ def test_perform assert(add_to_cart.perform) order.reload - assert_equal(2, order.items.count) + assert_equal(2, order.items.size) item = order.items.first assert_equal('PROD1', item.product_id) @@ -54,7 +54,7 @@ def test_perform_partial_success assert(add_to_cart.items.first.persisted?) order.reload - assert_equal(1, order.items.count) + assert_equal(1, order.items.size) item = add_to_cart.items.first.item assert(item.persisted?) @@ -83,7 +83,7 @@ def test_perform! refute(add_to_cart.perform!) order.reload - assert_equal(0, order.items.count) + assert_equal(0, order.items.size) items_params = [ { sku: 'sku1', quantity: 2 }, @@ -94,7 +94,7 @@ def test_perform! assert(add_to_cart.perform!) order.reload - assert_equal(2, order.items.count) + assert_equal(2, order.items.size) end end end diff --git a/core/test/services/workarea/cart_cleaner_test.rb b/core/test/services/workarea/cart_cleaner_test.rb index 09e32c3223..92b7f975e4 100644 --- a/core/test/services/workarea/cart_cleaner_test.rb +++ b/core/test/services/workarea/cart_cleaner_test.rb @@ -40,7 +40,7 @@ def test_removing_inactive_variant_items @order.add_item(product_id: product.id, sku: product.skus.second) @cleaner.clean - assert_equal(1, @order.items.count) + assert_equal(1, @order.items.size) end def test_removing_items_missing_price diff --git a/core/test/services/workarea/create_fulfillment_test.rb b/core/test/services/workarea/create_fulfillment_test.rb index b2faa05804..abf6795b13 100644 --- a/core/test/services/workarea/create_fulfillment_test.rb +++ b/core/test/services/workarea/create_fulfillment_test.rb @@ -13,11 +13,11 @@ def test_creating_items_per_order_item CreateFulfillment.new(@order).perform fulfillment = Fulfillment.find(@order.id) - assert_equal(fulfillment.items.count, 2) + assert_equal(fulfillment.items.size, 2) CreateFulfillment.new(@order).perform fulfillment.reload - assert_equal(fulfillment.items.count, 2) + assert_equal(fulfillment.items.size, 2) end end end diff --git a/core/test/services/workarea/inventory_adjustment_test.rb b/core/test/services/workarea/inventory_adjustment_test.rb index 0d8ef07936..46055bcfde 100644 --- a/core/test/services/workarea/inventory_adjustment_test.rb +++ b/core/test/services/workarea/inventory_adjustment_test.rb @@ -23,7 +23,7 @@ def set_inventory def test_adjust adjustment = InventoryAdjustment.new(order).tap(&:perform) - assert_equal(2, order.items.count) + assert_equal(2, order.items.size) assert_equal(1, order.items[0].quantity) assert_equal(1, order.items[1].quantity) assert_nil(order.items.detect { |i| i.sku == 'SKU3' }) diff --git a/core/test/services/workarea/order_merge_test.rb b/core/test/services/workarea/order_merge_test.rb index b1ef48cbb5..b78795bab9 100644 --- a/core/test/services/workarea/order_merge_test.rb +++ b/core/test/services/workarea/order_merge_test.rb @@ -22,7 +22,7 @@ def test_merge OrderMerge.new(original).merge(other) - assert_equal(3, original.items.count) + assert_equal(3, original.items.size) assert_equal([1, 1, 1], original.items.map(&:quantity)) assert_equal(%w(PROMOCODE), original.promo_codes) end diff --git a/docs/source/articles/content.html.md b/docs/source/articles/content.html.md index 374a1aa5b6..cdb6294c01 100644 --- a/docs/source/articles/content.html.md +++ b/docs/source/articles/content.html.md @@ -142,7 +142,7 @@ A content block (`Workarea::Content::Block`) is a [releasable](/artic A newly created content has 0 blocks. ``` -content.blocks.count +content.blocks.size # => 0 content.blocks @@ -155,7 +155,7 @@ Create a block within a content by specifying the type of block to be created. # Create an instance of an 'Image' block content.blocks.create!(type: 'image') -content.blocks.count +content.blocks.size # => 1 # Access the block from the content diff --git a/docs/source/articles/order-pricing.html.md b/docs/source/articles/order-pricing.html.md index 77c5883b43..363907c9ae 100644 --- a/docs/source/articles/order-pricing.html.md +++ b/docs/source/articles/order-pricing.html.md @@ -148,7 +148,7 @@ item_price_adjustments = items.map(&:price_adjustments).reduce(&:+) shipping_price_adjustments = shippings.map(&:price_adjustments).reduce(&:+) all_price_adjustments = item_price_adjustments + shipping_price_adjustments -all_price_adjustments.count +all_price_adjustments.size # => 10 all_price_adjustments.first.class # => Workarea::PriceAdjustment diff --git a/docs/source/articles/orders-and-items.html.md b/docs/source/articles/orders-and-items.html.md index 0cffe21212..c9cdc3ac75 100644 --- a/docs/source/articles/orders-and-items.html.md +++ b/docs/source/articles/orders-and-items.html.md @@ -128,7 +128,7 @@ Review the implementation of `Storefront::CartItemsController#create` ([source, Each time an order is priced, the granular pricing details are stored on the item as _price adjustments_, an embedded collection of type `Workarea::PriceAdjustment`. These embedded documents provide the necessary details to determine the total price of an item (in the case of a cart) and a record of how that price was determined (in the case of a placed order). ``` -item.price_adjustments.count +item.price_adjustments.size # => 2 price_adjustment = item.price_adjustments.last diff --git a/docs/source/articles/products.html.md.erb b/docs/source/articles/products.html.md.erb index f8a7f802f3..14a62c57cb 100644 --- a/docs/source/articles/products.html.md.erb +++ b/docs/source/articles/products.html.md.erb @@ -1124,9 +1124,9 @@ model = Workarea::Catalog::Product.find_by(name: 'Tropical Drink Mix') view_model = Workarea::Storefront::ProductViewModel.wrap(model) # The variants count is the same at this point -model.variants.count +model.variants.size # => 3 -view_model.variants.count +view_model.variants.size # => 3 ``` @@ -1138,13 +1138,13 @@ The product therefore has 3 variants and 3 options: # set one of the variants to inactive model.variants.second.update_attributes!(active: false) # the model still has 3 variants -model.variants.count +model.variants.size # => 3 # re-create the view model (to bust cache) view_model = Workarea::Storefront::ProductViewModel.wrap(model) # the Storefront view of the product has only 2 variants -view_model.variants.count +view_model.variants.size # => 2 # choose another variant and make the corresponding @@ -1154,12 +1154,12 @@ model.variants.last.tap do |variant| .find(variant.sku) .update_attributes!(policy: 'standard', available: '0') end -model.variants.count +model.variants.size # => 3 # now the Storefront view of the product contains only a single variant view_model = Workarea::Storefront::ProductViewModel.wrap(model) -view_model.variants.count +view_model.variants.size # => 1 ``` diff --git a/notes/WA-VERIFY-089-mongoid8-embedded-count.md b/notes/WA-VERIFY-089-mongoid8-embedded-count.md new file mode 100644 index 0000000000..746c1807d2 --- /dev/null +++ b/notes/WA-VERIFY-089-mongoid8-embedded-count.md @@ -0,0 +1,34 @@ +# WA-VERIFY-089 / #1080 - Embedded association `.count` (Mongoid 8) + +Mongoid 8 changed embedded association `.count` to always hit the database. +This can introduce unnecessary queries (and N+1s) when the embedded documents +are already loaded in memory. + +## Changes + +Replaced embedded-association `.count` calls with `.size`: + +- `core/app/seeds/workarea/orders_seeds.rb` + - `user.addresses.count` → `user.addresses.size` +- `admin/app/views/workarea/admin/catalog_categories/index.html.haml` + - `result.product_rules.count` → `result.product_rules.size` +- `admin/app/views/workarea/admin/catalog_variants/index.html.haml` + - `@variants.count` → `@variants.size` + +Also updated tests and docs that were using embedded-association `.count` to avoid +recommending/depending on database-backed counts for embedded documents: + +- `core/test/models/workarea/order_test.rb` — `order.items.count` → `.size` +- `core/test/services/workarea/add_multiple_cart_items/item_test.rb` — `order.items.count` → `.size` +- `core/test/services/workarea/add_multiple_cart_items_test.rb` — `order.items.count` → `.size` +- `core/test/services/workarea/cart_cleaner_test.rb` — `@order.items.count` → `.size` +- `core/test/services/workarea/create_fulfillment_test.rb` — `fulfillment.items.count` → `.size` +- `core/test/services/workarea/inventory_adjustment_test.rb` — `order.items.count` → `.size` +- `core/test/services/workarea/order_merge_test.rb` — `original.items.count` → `.size` + +## Intentionally unchanged + +- `.count` calls on Mongoid criteria/relations (non-embedded) where a database + count is expected. +- `.count` on Ruby collections where it is purely in-memory (e.g. arrays/hashes, + or `Enumerable#count { ... }`). diff --git a/notes/WA-VERIFY-091-mongoid8-any-of.md b/notes/WA-VERIFY-091-mongoid8-any-of.md new file mode 100644 index 0000000000..84761c88dc --- /dev/null +++ b/notes/WA-VERIFY-091-mongoid8-any-of.md @@ -0,0 +1,65 @@ +# WA-VERIFY-091 — Mongoid 8 `any_of` scoping semantics + +## Background + +Mongoid 8 changes how repeated `Criteria#any_of` calls compose. In Mongoid 7, +chaining `.any_of` multiple times tended to expand/merge into one `$or` selector +(widening). In Mongoid 8 each `.any_of` call is preserved as a separate clause, +producing `$and[$or, $or, …]` (narrowing). The main risk areas are: + +1. **Loop-based chaining** – calling `.any_of(…)` inside a loop produces ANDs of + ORs in Mongoid 8, so results progressively narrow with each iteration rather + than widen. +2. **Array-as-single-arg** – `any_of(array)` vs `any_of(*array)`. Mongoid 8's + signature changed; splatting is safer across versions. + +## Call sites audited (9 total) + +| # | File | Pattern | Risk | Action | +|---|------|---------|------|--------| +| 1 | `admin/…/activity_view_model.rb` | Loop: `criteria = criteria.any_of(…)` per id | **High** – multiple ids would narrow to nothing | Fixed: collect all clauses, call `any_of` once | +| 2 | `core/…/tax/rate.rb` | `any_of(clauses)` – array as single arg | Low–Med | Fixed: splatted to `any_of(*clauses)` | +| 3 | `core/…/discount/generated_promo_code.rb` | `any_of({ expires_at: nil }, { :expires_at.gt => … })` | Low – single call, two hashes | OK – no change needed; behavior confirmed by new test | +| 4 | `core/…/taxonomy_sitemap.rb` | `.any_of({ :url.ne => nil }, { :navigable_id.ne => nil })` | Low – single standalone call | OK – existing test passes | +| 5 | `core/…/navigation/redirect.rb` | `any_of({ path: regex }, { destination: regex })` | Low – single standalone call | OK – covered by new test | +| 6 | `core/…/featured_products.rb` | `Release::Changeset.any_of(…)` | Low – single standalone call | OK – covered by new test | +| 7 | `core/…/tax/category.rb` | `rates.any_of(…)` | Low – single standalone call | OK – existing test passes | +| 8 | `core/lib/…/products_missing_variants.rb` | `.any_of({ variants: nil }, { variants: [] })` | Low – single call | OK – test expanded to cover both `nil` and `[]` cases | +| 9 | `core/lib/…/products_missing_images.rb` | `.any_of({ images: nil }, { images: [] })` | Low – single call | OK – test expanded to cover both `nil` and `[]` cases | + +## Changes made + +### `admin/app/view_models/workarea/admin/activity_view_model.rb` + +**Problem:** The `scoped_entries` method iterated over `options[:id]` and chained +`criteria.any_of(…)` inside the loop. With Mongoid 8 this would produce an AND of +ORs, returning empty results when more than one id was supplied. + +**Fix:** Collect all `{ audited_id: … }` and `{ 'document_path.id' => … }` clauses +into a flat array and call `.any_of(*clauses)` once outside the loop. + +### `core/app/models/workarea/tax/rate.rb` + +**Problem:** `any_of(clauses)` — passing an Array as a single argument. Mongoid 8 +signature prefers splatted args. + +**Fix:** Changed to `any_of(*clauses)`. + +## Tests added / updated + +- `core/test/lib/workarea/lint/products_missing_variants_test.rb` — cover both + `variants: []` and `variants: nil` cases. +- `core/test/lib/workarea/lint/products_missing_images_test.rb` — same for images. +- `core/test/models/workarea/pricing/discount/generated_promo_code_test.rb` — + `test_not_expired_scope`: confirms both `nil` and future-date codes are returned, + expired codes are excluded. +- `core/test/models/workarea/navigation/redirect_test.rb` — `test_search`: both + path and destination regex branches return expected records. +- `core/test/models/workarea/tax/rate_test.rb` — `test_search`: region, postal + code, and country clause branches all return expected records. +- `core/test/models/workarea/featured_products_changesets_test.rb` — + `test_changesets_finds_by_product_id_in_changeset_and_original`. + +## No intentional behavior differences + +All changes are backward-compatible and preserve the original Mongoid 7 semantics.