From e40f7926ddf934280d403c618c4e12b853f6da08 Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Mon, 21 May 2018 17:30:11 -0500 Subject: [PATCH 1/9] fix: no hang on publishing many "Products" for admins --- lib/collections/schemas/products.js | 6 +- server/publications/collections/products.js | 143 ++++++-------------- 2 files changed, 47 insertions(+), 102 deletions(-) diff --git a/lib/collections/schemas/products.js b/lib/collections/schemas/products.js index e588ae20056..04eb30471a9 100644 --- a/lib/collections/schemas/products.js +++ b/lib/collections/schemas/products.js @@ -437,7 +437,8 @@ export const Product = new SimpleSchema({ }, "ancestors": { type: Array, - defaultValue: [] + defaultValue: [], + index: 1 }, "ancestors.$": { type: String @@ -595,7 +596,8 @@ export const Product = new SimpleSchema({ }, "createdAt": { type: Date, - autoValue: createdAtAutoValue + autoValue: createdAtAutoValue, + index: 1 }, "updatedAt": { type: Date, diff --git a/server/publications/collections/products.js b/server/publications/collections/products.js index db8662f20a9..1fdd0dbb138 100644 --- a/server/publications/collections/products.js +++ b/server/publications/collections/products.js @@ -267,27 +267,17 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so check(sort, Match.OneOf(undefined, Object)); check(editMode, Match.Maybe(Boolean)); - // TODO: Consider publishing the non-admin publication if a user is not in "edit mode" to see what is published - // Active shop const shopId = Reaction.getShopId(); const primaryShopId = Reaction.getPrimaryShopId(); - // Get a list of shopIds that this user has "createProduct" permissions for (owner permission is checked by default) - const userAdminShopIds = Reaction.getShopsWithRoles(["createProduct"], this.userId); - // Don't publish if we're missing an active or primary shopId if (!shopId || !primaryShopId) { return this.ready(); } - // Get active shop id's to use for filtering - const activeShopsIds = Shops.find({ - $or: [ - { "workflow.status": "active" }, - { _id: Reaction.getPrimaryShopId() } - ] - }).fetch().map((activeShop) => activeShop._id); + // Get a list of shopIds that this user has "createProduct" permissions for (owner permission is checked by default) + const userAdminShopIds = Reaction.getShopsWithRoles(["createProduct"], this.userId); const selector = filterProducts(productFilters); @@ -296,106 +286,59 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so } // We publish an admin version of this publication to admins of products who are in "Edit Mode" - // userAdminShopIds is a list of shopIds that the user has createProduct or owner access for - if (editMode && userAdminShopIds && Array.isArray(userAdminShopIds) && userAdminShopIds.length > 0) { - selector.isVisible = { - $in: [true, false, null, undefined] + if (editMode) { + // userAdminShopIds is a list of shopIds that the user has createProduct or owner access for + if (!Array.isArray(userAdminShopIds) || userAdminShopIds.length === 0) { + return this.ready(); + } + + delete selector.isVisible; + selector.shopId = { + $in: userAdminShopIds }; + } else { + // Get active shop IDs to use for filtering + const activeShopsIds = Shops.find({ + $or: [ + { "workflow.status": "active" }, + { _id: Reaction.getPrimaryShopId() } + ] + }, { + fields: { + _id: 1 + } + }).map((activeShop) => activeShop._id); + selector.shopId = { $in: activeShopsIds }; } - // This is where the publication begins for non-admin users - // Get _ids of top-level products + // Get the IDs of the first N (limit) top-level products that match the query const productIds = Products.find(selector, { sort, limit: productScrollLimit - }).map((product) => product._id); - - let newSelector = { ...selector }; - - // Remove hashtag filter from selector (hashtags are not applied to variants, we need to get variants) - if (productFilters && Object.keys(productFilters).length === 0 && productFilters.constructor === Object) { - newSelector = _.omit(selector, ["hashtags", "ancestors"]); - - if (productFilters.tags) { - // Re-configure selector to pick either Variants of one of the top-level products, - // or the top-level products in the filter - _.extend(newSelector, { - $or: [{ - ancestors: { - $in: productIds - } - }, { - $and: [{ - hashtags: { - $in: productFilters.tags - } - }, { - _id: { - $in: productIds - } - }] - }] - }); - } - // filter by query - if (productFilters.query) { - const cond = { - $regex: productFilters.query, - $options: "i" - }; - _.extend(newSelector, { - $or: [{ - title: cond - }, { - pageTitle: cond - }, { - description: cond - }, { - ancestors: { - $in: productIds - } - }, - { - _id: { - $in: productIds - } - }] - }); - } - } else { - newSelector = _.omit(selector, ["hashtags", "ancestors"]); - - _.extend(newSelector, { - $or: [{ - ancestors: { - $in: productIds - } - }, { - _id: { - $in: productIds - } - }] - }); - } - - // Adjust the selector to include only active shops - newSelector = { - ...newSelector, - shopId: { - $in: activeShopsIds + }, { + fields: { + _id: 1 } - }; + }).map((product) => product._id); - // Returning Complete product tree for top level products to avoid sold out warning. - return Products.find(newSelector, { + // Return a cursor for the matching products plus all their variants + return Products.find({ + $or: [{ + ancestors: { + $in: productIds + } + }, { + _id: { + $in: productIds + } + }] + }, { sort - // TODO: REVIEW Limiting final products publication for non-admins - // I think we shouldn't limit here, otherwise we are limited to 24 total products which - // could be far less than 24 top-level products - // limit: productScrollLimit + // We shouldn't limit here. Otherwise we are limited to 24 total products which + // could be far less than 24 top-level products. }); }); From 02cc44fced07381ac6e1757ac2db2e6dc5b90071 Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Wed, 30 May 2018 18:07:13 -0500 Subject: [PATCH 2/9] fix: incorporate fix and refactor from #4259 @pmn4 --- .../product-publications.app-test.js | 401 +++++++++++------- server/publications/collections/products.js | 165 ++++--- 2 files changed, 320 insertions(+), 246 deletions(-) diff --git a/server/publications/collections/product-publications.app-test.js b/server/publications/collections/product-publications.app-test.js index 3a6780e0130..83dbbd6f8a8 100644 --- a/server/publications/collections/product-publications.app-test.js +++ b/server/publications/collections/product-publications.app-test.js @@ -3,23 +3,52 @@ import Random from "@reactioncommerce/random"; import { expect } from "meteor/practicalmeteor:chai"; import { sinon } from "meteor/practicalmeteor:sinon"; +import { Factory } from "meteor/dburles:factory"; import { Roles } from "meteor/alanning:roles"; +import { PublicationCollector } from "meteor/johanbrook:publication-collector"; import { createActiveShop } from "/server/imports/fixtures/shops"; import { Reaction } from "/server/api"; import * as Collections from "/lib/collections"; import Fixtures from "/server/imports/fixtures"; -import { PublicationCollector } from "meteor/johanbrook:publication-collector"; +import publishProductsToCatalog from "/imports/plugins/core/catalog/server/no-meteor/utils/publishProductsToCatalog"; +import collections from "/imports/collections/rawCollections"; Fixtures(); describe("Publication", function () { - const shopId = Random.id(); + let shopId; + let merchantShopId; + let primaryShopId; + let inactiveMerchantShopId; let sandbox; + let merchantShop1ProductIds; + let merchantShop1VisibleProductIds; + let activeShopProductIds; + let activeShopVisibleProductIds; + let activeMerchantProductIds; + + const productScrollLimit = 24; + beforeEach(function () { - Collections.Shops.remove({}); - createActiveShop({ _id: shopId }); + shopId = Random.id(); + merchantShopId = Random.id(); + primaryShopId = Random.id(); + + sandbox = sinon.sandbox.create(); + sandbox.stub(Reaction, "getPrimaryShopId", () => primaryShopId); + + Collections.Shops.remove({}); + + // muting some shop creation hook behavior (to keep output clean) + sandbox.stub(Reaction, "setShopName"); + sandbox.stub(Reaction, "setDomain"); + + createActiveShop({ _id: shopId, shopType: "merchant" }); + createActiveShop({ _id: merchantShopId, shopType: "merchant" }); + createActiveShop({ _id: primaryShopId, shopType: "primary" }); + Factory.create("shop", { _id: inactiveMerchantShopId, shopType: "merchant" }); }); afterEach(function () { @@ -27,6 +56,8 @@ describe("Publication", function () { }); describe("with products", function () { + let collector; + const priceRangeA = { range: "1.00 - 12.99", min: 1.00, @@ -43,7 +74,7 @@ describe("Publication", function () { Collections.Products.remove({}); // a product with price range A, and not visible - Collections.Products.insert({ + const productId1 = Collections.Products.insert({ ancestors: [], title: "My Little Pony", shopId, @@ -55,7 +86,7 @@ describe("Publication", function () { isBackorder: false }); // a product with price range B, and visible - Collections.Products.insert({ + const productId2 = Collections.Products.insert({ ancestors: [], title: "Shopkins - Peachy", shopId, @@ -67,7 +98,7 @@ describe("Publication", function () { isBackorder: false }); // a product with price range A, and visible - Collections.Products.insert({ + const productId3 = Collections.Products.insert({ ancestors: [], title: "Fresh Tomatoes", shopId, @@ -78,28 +109,95 @@ describe("Publication", function () { isSoldOut: false, isBackorder: false }); + // a product for an unrelated marketplace shop + const productId4 = Collections.Products.insert({ + ancestors: [], + title: "Teddy Ruxpin", + shopId: merchantShopId, + type: "simple", + price: priceRangeA, + isVisible: true, + isLowQuantity: false, + isSoldOut: false, + isBackorder: false + }); + // a product for the Primary Shop + const productId5 = Collections.Products.insert({ + ancestors: [], + title: "Garbage Pail Kids", + shopId: primaryShopId, + type: "simple", + price: priceRangeA, + isVisible: true, + isLowQuantity: false, + isSoldOut: false, + isBackorder: false + }); + // a product for an inactive Merchant Shop + // this product is here to guard against false-positive test results + Collections.Products.insert({ + ancestors: [], + title: "Lite Bright", + shopId: inactiveMerchantShopId, + type: "simple", + price: priceRangeA, + isVisible: true, + isLowQuantity: false, + isSoldOut: false, + isBackorder: false + }); + + // helper arrays for writing expecations in tests + merchantShop1ProductIds = [productId1, productId2, productId3]; + merchantShop1VisibleProductIds = [productId2, productId3]; + activeShopProductIds = [productId1, productId2, productId3, productId4, productId5]; + activeShopVisibleProductIds = [productId2, productId3, productId4, productId5]; + activeMerchantProductIds = [productId2, productId3, productId4]; + + collector = new PublicationCollector({ userId: Random.id() }); }); describe("Products", function () { - it("should return all products to admins", function (done) { + it("should return all products from active shops to admins in the Primary Shop", function (done) { + // setup + sandbox.stub(Reaction, "getShopId", () => primaryShopId); + sandbox.stub(Roles, "userIsInRole", () => true); + sandbox.stub(Reaction, "hasPermission", () => true); + sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); + + collector.collect("Products", 24, undefined, {}, ({ Products }) => { + const productIds = Products.map((p) => p._id); + + expect(productIds).to.have.members(activeShopProductIds); + }).then(() => done(/* empty */), done); + }); + + it("should return all products from the current shop to admins in a Merchant Shop", function (done) { // setup sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => true); sandbox.stub(Reaction, "hasPermission", () => true); - sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); + sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; + collector.collect("Products", 24, undefined, {}, ({ Products }) => { + const productIds = Products.map((p) => p._id); - collector.collect("Products", 24, undefined, {}, (collections) => { - const products = collections.Products; - expect(products.length).to.equal(3); + expect(productIds).to.have.members(merchantShop1ProductIds); + }).then(() => done(/* empty */), done); + }); - if (!isDone) { - isDone = true; - done(); - } - }); + it("returns products from only the shops for which an admin has createProduct Role", function (done) { + // setup + sandbox.stub(Reaction, "getShopId", () => primaryShopId); + sandbox.stub(Roles, "userIsInRole", () => true); + sandbox.stub(Reaction, "hasPermission", () => true); + sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); + + collector.collect("Products", 24, undefined, {}, ({ Products }) => { + const productIds = Products.map((p) => p._id); + + expect(productIds).to.have.members(merchantShop1ProductIds); + }).then(() => done(/* empty */), done); }); it("should have an expected product title", function (done) { @@ -109,172 +207,182 @@ describe("Publication", function () { sandbox.stub(Reaction, "hasPermission", () => true); sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; - - collector.collect("Products", 24, undefined, {}, (collections) => { - const products = collections.Products; - const data = products[1]; + collector.collect("Products", 24, undefined, {}, ({ Products }) => { + const data = Products[1]; const expectedTitles = ["My Little Pony", "Shopkins - Peachy"]; expect(expectedTitles.some((title) => title === data.title)).to.be.ok; - - if (!isDone) { - isDone = true; - done(); - } - }); + }).then(() => done(/* empty */), done); }); it("should return only visible products to visitors", function (done) { sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; - - collector.collect("Products", 24, undefined, {}, (collections) => { - const products = collections.Products; - const data = products[0]; + collector.collect("Products", 24, undefined, {}, ({ Products }) => { + const data = Products[0]; const expectedTitles = ["Fresh Tomatoes", "Shopkins - Peachy"]; - expect(products.length).to.equal(2); + expect(Products.length).to.equal(2); expect(expectedTitles.some((title) => title === data.title)).to.be.ok; - - if (isDone === false) { - isDone = true; - done(); - } - }); + }).then(() => done(/* empty */), done); }); it("should return only products matching query", function (done) { - const productScrollLimit = 24; const filters = { query: "Shopkins" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - const data = products[0]; + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + const data = Products[0]; expect(data.title).to.equal("Shopkins - Peachy"); - - done(); - }); + }).then(() => done(/* empty */), done); }); it("should not return products not matching query", function (done) { - const productScrollLimit = 24; const filters = { query: "random search" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - - expect(products.length).to.equal(0); - - done(); - }); + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + expect(Products.length).to.equal(0); + }).then(() => done(/* empty */), done); }); it("should return products in price.min query", function (done) { - const productScrollLimit = 24; const filters = { "price.min": "2.00" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - - expect(products.length).to.equal(1); - - done(); - }); + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + expect(Products.length).to.equal(1); + }).then(() => done(/* empty */), done); }); it("should return products in price.max query", function (done) { - const productScrollLimit = 24; const filters = { "price.max": "24.00" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - - expect(products.length).to.equal(2); - - done(); - }); + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + expect(Products.length).to.equal(2); + }).then(() => done(/* empty */), done); }); it("should return products in price.min - price.max range query", function (done) { - const productScrollLimit = 24; const filters = { "price.min": "12.00", "price.max": "19.98" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - - expect(products.length).to.equal(2); - - done(); - }); + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + expect(Products.length).to.equal(2); + }).then(() => done(/* empty */), done); }); it("should return products where value is in price set query", function (done) { - const productScrollLimit = 24; const filters = { "price.min": "13.00", "price.max": "24.00" }; + sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + expect(Products.length).to.equal(1); + }).then(() => done(/* empty */), done); + }); + + it("should return products from all shops when multiple shops are provided", function (done) { + const filters = { shops: [shopId, merchantShopId] }; + + sandbox.stub(Reaction, "getShopId", () => primaryShopId); + sandbox.stub(Roles, "userIsInRole", () => false); + + collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { + const productIds = Products.map((p) => p._id); + + expect(productIds).to.have.members(activeMerchantProductIds); + }).then(() => done(/* empty */), done); + }); + }); + + describe("Products/grid", function () { + beforeEach(function () { + Collections.Catalog.remove({}); + }); + + describe("Catalog conditions", function () { + it("returns nothing when the Catalog is empty", function (done) { + sandbox.stub(Reaction, "getShopId", () => shopId); + + collector.collect("Products/grid", ({ Catalog }) => { + const productIds = Catalog.map((p) => p._id); + + expect(productIds).to.be.empty; + }).then(() => done(/* empty */), done); + }); + + it("returns products from the Catalog", function (done) { + sandbox.stub(Reaction, "getShopId", () => shopId); - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; + publishProducts(); - expect(products.length).to.equal(1); + collector.collect("Products/grid", ({ Catalog }) => { + const productIds = Catalog.map((p) => p._id); - done(); + expect(productIds).to.not.be.empty; + }).then(() => done(/* empty */), done); }); }); - it("should return products from all shops when multiple shops are provided", function (done) { - const filters = { shops: [shopId] }; - const productScrollLimit = 24; - sandbox.stub(Reaction, "getCurrentShop", function () { return { _id: "123" }; }); - sandbox.stub(Roles, "userIsInRole", () => true); - sandbox.stub(Reaction, "hasPermission", () => true); - sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); + describe("Shop conditions", function () { + beforeEach(function () { + publishProducts(); + }); + + it("returns products from the active shop", function (done) { + sandbox.stub(Reaction, "getShopId", () => shopId); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; + collector.collect("Products/grid", ({ Catalog }) => { + const productIds = Catalog.map((c) => c.product._id); - collector.collect("Products", productScrollLimit, filters, {}, (collections) => { - const products = collections.Products; - expect(products.length).to.equal(3); + expect(productIds).to.have.members(merchantShop1VisibleProductIds); + }).then(() => done(/* empty */), done); + }); - const data = products[1]; - expect(["My Little Pony", "Shopkins - Peachy"].some((title) => title === data.title)).to.be.ok; + it("returns all visible products from all active shops when the Primary Shop is active", function (done) { + sandbox.stub(Reaction, "getShopId", () => primaryShopId); - if (!isDone) { - isDone = true; - done(); - } + collector.collect("Products/grid", ({ Catalog }) => { + const productIds = Catalog.map((c) => c.product._id); + + expect(productIds).to.have.members(activeShopVisibleProductIds); + }).then(() => done(/* empty */), done); + }); + + it("returns products from all shops when the Primary Shop is active, filtered by shop id", function (done) { + const filters = { shopIdsOrSlugs: [shopId, merchantShopId] }; + + sandbox.stub(Reaction, "getShopId", () => primaryShopId); + + collector.collect("Products/grid", 24, filters, ({ Catalog }) => { + const productIds = Catalog.map((c) => c.product._id); + + expect(productIds).to.have.members(activeMerchantProductIds); + }).then(() => done(/* empty */), done); }); }); + + function publishProducts() { + const productIds = + Collections.Products.find({}).fetch().map((p) => p._id); + + return Promise.await(publishProductsToCatalog(productIds, collections)); + } }); describe("Product", function () { @@ -284,54 +392,36 @@ describe("Publication", function () { }); sandbox.stub(Reaction, "getShopId", () => shopId); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Product", product._id, (collections) => { - const products = collections.Products; - const data = products[0]; + collector.collect("Product", product._id, ({ Products }) => { + const data = Products[0]; expect(data.title).to.equal(product.title); - - done(); - }); + }).then(() => done(/* empty */), done); }); it("should not return a product if handle does not match exactly", function (done) { sandbox.stub(Reaction, "getShopId", () => shopId); - const collector = new PublicationCollector({ userId: Random.id() }); - - collector.collect("Product", "shopkins", (collections) => { - const products = collections.Products; - if (products) { - expect(products.length).to.equal(0); + collector.collect("Product", "shopkins", ({ Products }) => { + if (Products) { + expect(Products.length).to.equal(0); } else { - expect(products).to.be.undefined; + expect(Products).to.be.undefined; } - done(); - }); + }).then(() => done(/* empty */), done); }); it("should not return a product based on exact handle match if it isn't visible", function (done) { sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; - - collector.collect("Product", "my-little-pony", (collections) => { - const products = collections.Products; - if (products) { - expect(products.length).to.equal(0); + collector.collect("Product", "my-little-pony", ({ Products }) => { + if (Products) { + expect(Products.length).to.equal(0); } else { - expect(products).to.be.undefined; - } - - if (!isDone) { - isDone = true; - done(); + expect(Products).to.be.undefined; } - }); + }).then(() => done(/* empty */), done); }); it("should return a product to admin based on a exact handle match even if it isn't visible", function (done) { @@ -339,20 +429,11 @@ describe("Publication", function () { sandbox.stub(Roles, "userIsInRole", () => true); sandbox.stub(Reaction, "hasPermission", () => true); - const collector = new PublicationCollector({ userId: Random.id() }); - let isDone = false; - - collector.collect("Product", "my-little-pony", (collections) => { - const products = collections.Products; - const data = products[0]; + collector.collect("Product", "my-little-pony", ({ Products }) => { + const data = Products[0]; expect(data.title).to.equal("My Little Pony"); - - if (!isDone) { - isDone = true; - done(); - } - }); + }).then(() => done(/* empty */), done); }); }); }); diff --git a/server/publications/collections/products.js b/server/publications/collections/products.js index 1fdd0dbb138..eb8e038fa65 100644 --- a/server/publications/collections/products.js +++ b/server/publications/collections/products.js @@ -87,17 +87,37 @@ const catalogProductFiltersSchema = new SimpleSchema({ } }); -function filterProducts(productFilters) { - // if there are filter/params that don't match the schema - // validate, catch except but return no results - try { - if (productFilters) filters.validate(productFilters); - } catch (e) { - Logger.debug(e, "Invalid Product Filters"); +function applyShopsFilter(selector, shopIdsOrSlugs) { + // Active shop + const shopId = Reaction.getShopId(); + const primaryShopId = Reaction.getPrimaryShopId(); + + // Don't publish if we're missing an active or primary shopId + if (!shopId || !primaryShopId) { return false; } - const shopIdsOrSlugs = productFilters && productFilters.shops; + let activeShopIds; + // if the current shop is the primary shop, get products from all shops + // otherwise, only list products from _this_ shop. + if (shopId === primaryShopId) { + activeShopIds = Shops.find({ + $or: [ + { "workflow.status": "active" }, + { _id: primaryShopId } + ] + }, { + fields: { + _id: 1 + } + }).fetch().map((activeShop) => activeShop._id); + } else { + activeShopIds = [shopId]; + } + + if (!activeShopIds.length) { + return false; + } if (shopIdsOrSlugs) { // Get all shopIds associated with the slug or Id @@ -108,33 +128,48 @@ function filterProducts(productFilters) { }, { slug: { $in: shopIdsOrSlugs } }] + }, { + fields: { + _id: 1 + } }).map((shop) => shop._id); - // If we found shops, update the productFilters - if (shopIds) { - productFilters.shops = shopIds; - } else { - return false; - } + activeShopIds = _.intersection(activeShopIds, shopIds); + } + + if (activeShopIds.length) { + return { + ...selector, + shopId: { $in: activeShopIds } + }; + } + + return selector; +} + +function filterProducts(productFilters) { + // if there are filter/params that don't match the schema + // validate, catch except but return no results + try { + if (productFilters) filters.validate(productFilters); + } catch (e) { + Logger.warn(e, "Invalid Product Filters"); + return false; } // Init default selector - Everyone can see products that fit this selector - const selector = { + const baseSelector = { ancestors: [], // Lookup top-level products isDeleted: { $ne: true }, // by default, we don't publish deleted products isVisible: true // by default, only lookup visible products }; - if (productFilters) { - // handle multiple shops - if (productFilters.shops) { - _.extend(selector, { - shopId: { - $in: productFilters.shops - } - }); - } + const shopIdsOrSlugs = productFilters && productFilters.shops; + const selector = applyShopsFilter(baseSelector, shopIdsOrSlugs); + + if (!selector) return false; + if (productFilters) { // filter by tags if (productFilters.tags) { _.extend(selector, { @@ -267,17 +302,14 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so check(sort, Match.OneOf(undefined, Object)); check(editMode, Match.Maybe(Boolean)); - // Active shop - const shopId = Reaction.getShopId(); - const primaryShopId = Reaction.getPrimaryShopId(); - - // Don't publish if we're missing an active or primary shopId - if (!shopId || !primaryShopId) { - return this.ready(); - } + // // Active shop + // const shopId = Reaction.getShopId(); + // const primaryShopId = Reaction.getPrimaryShopId(); - // Get a list of shopIds that this user has "createProduct" permissions for (owner permission is checked by default) - const userAdminShopIds = Reaction.getShopsWithRoles(["createProduct"], this.userId); + // // Don't publish if we're missing an active or primary shopId + // if (!shopId || !primaryShopId) { + // return this.ready(); + // } const selector = filterProducts(productFilters); @@ -285,33 +317,18 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so return this.ready(); } + // Get a list of shopIds that this user has "createProduct" permissions for (owner permission is checked by default) + const userAdminShopIds = Reaction.getShopsWithRoles(["createProduct"], this.userId) || []; + // We publish an admin version of this publication to admins of products who are in "Edit Mode" if (editMode) { - // userAdminShopIds is a list of shopIds that the user has createProduct or owner access for - if (!Array.isArray(userAdminShopIds) || userAdminShopIds.length === 0) { + // Limit to only shops we have "createProduct" role for + selector.shopId.$in = _.intersection(selector.shopId.$in, userAdminShopIds); + if (selector.shopId.$in.length === 0) { return this.ready(); } - delete selector.isVisible; - selector.shopId = { - $in: userAdminShopIds - }; - } else { - // Get active shop IDs to use for filtering - const activeShopsIds = Shops.find({ - $or: [ - { "workflow.status": "active" }, - { _id: Reaction.getPrimaryShopId() } - ] - }, { - fields: { - _id: 1 - } - }).map((activeShop) => activeShop._id); - - selector.shopId = { - $in: activeShopsIds - }; + delete selector.isVisible; // in edit mode, you should see all products } // Get the IDs of the first N (limit) top-level products that match the query @@ -348,45 +365,21 @@ function filterCatalogItems(catalogFilters) { try { if (catalogFilters) catalogProductFiltersSchema.validate(catalogFilters); } catch (e) { - Logger.debug(e, "Invalid Catalog Product Filters"); + Logger.warn(e, "Invalid Catalog Product Filters"); return false; } - const shopIdsOrSlugs = catalogFilters && catalogFilters.shopIdsOrSlugs; - - if (shopIdsOrSlugs) { - // Get all shopIds associated with the slug or Id - const shopIds = Shops.find({ - "workflow.status": "active", - "$or": [{ - _id: { $in: shopIdsOrSlugs } - }, { - slug: { $in: shopIdsOrSlugs } - }] - }).map((shop) => shop._id); - - // If we found shops, update the productFilters - if (shopIds) { - catalogFilters.shopIds = shopIds; - } else { - return false; - } - } - // Init default selector - Everyone can see products that fit this selector - const selector = { + const baseSelector = { "product.isDeleted": { $ne: true }, // by default, we don't publish deleted products "product.isVisible": true // by default, only lookup visible products }; - if (!catalogFilters) return selector; + const { shopIdsOrSlugs } = catalogFilters || {}; + const selector = applyShopsFilter(baseSelector, shopIdsOrSlugs); - // handle multiple shops - if (catalogFilters.shopIds) { - selector.shopId = { - $in: catalogFilters.shopIds - }; - } + if (!selector) return false; + if (!catalogFilters) return selector; // filter by tags if (catalogFilters.tagIds) { From ddfb1c933a76fa2d201b4b2cbfd0696e2f6489e4 Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Wed, 30 May 2018 18:52:15 -0500 Subject: [PATCH 3/9] tests: fix merge issue --- .../publications/collections/product-publications.app-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/publications/collections/product-publications.app-test.js b/server/publications/collections/product-publications.app-test.js index 83dbbd6f8a8..e8235dda082 100644 --- a/server/publications/collections/product-publications.app-test.js +++ b/server/publications/collections/product-publications.app-test.js @@ -34,7 +34,7 @@ describe("Publication", function () { shopId = Random.id(); merchantShopId = Random.id(); primaryShopId = Random.id(); - + inactiveMerchantShopId = Random.id(); sandbox = sinon.sandbox.create(); sandbox.stub(Reaction, "getPrimaryShopId", () => primaryShopId); From 28b95fe77fc313534198605d2ed2572f9df239db Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Fri, 1 Jun 2018 13:16:10 -0500 Subject: [PATCH 4/9] tests: add timeouts to stop random test failures --- .../lib/jobCollection.app-test.js | 43 ++++++++++++++++--- server/methods/core/shops.app-test.js | 3 ++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/imports/plugins/core/job-collection/lib/jobCollection.app-test.js b/imports/plugins/core/job-collection/lib/jobCollection.app-test.js index adb146696de..e4610481b62 100644 --- a/imports/plugins/core/job-collection/lib/jobCollection.app-test.js +++ b/imports/plugins/core/job-collection/lib/jobCollection.app-test.js @@ -19,6 +19,7 @@ const validJobDoc = (d) => Match.test(d, defaultColl.jobDocPattern); describe("JobCollection default constructor", function () { it("should be an instance of JobCollection", function () { + this.timeout(15000); expect(defaultColl, "JobCollection constructor failed").to.be.an.instanceOf(JobCollection); expect(defaultColl.root, "default root isn't 'queue'").to.equal("queue"); @@ -61,10 +62,12 @@ describe("JobCollection", function () { }); it("should set permissions to allow admin on ClientTest", function () { + this.timeout(15000); expect(clientTestColl.allows.admin[0]()).to.equal(true); }); it("should set polling interval", function () { + this.timeout(15000); let { interval } = clientTestColl; clientTestColl.promote(250); expect(interval, "clientTestColl interval not updated").to.not.equal(clientTestColl.interval); @@ -75,6 +78,7 @@ describe("JobCollection", function () { }); it("should run startJobServer on new job collection", function (done) { + this.timeout(15000); testColl.startJobServer(function (err, res) { if (err) { done(err); } expect(res, "startJobServer failed in callback result").to.equal(true); @@ -87,6 +91,7 @@ describe("JobCollection", function () { if (Meteor.isServer) { it("should create a server-side job and see that it is added to the collection and runs", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }); assert.ok(validJobDoc(job.doc)); @@ -110,6 +115,7 @@ describe("JobCollection", function () { } it("should create a job and see that it is added to the collection and runs", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }); assert.ok(validJobDoc(job.doc)); @@ -128,6 +134,7 @@ describe("JobCollection", function () { }); it("should create an invalid job and see that errors correctly propagate", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }); @@ -164,6 +171,7 @@ describe("JobCollection", function () { }); it("should create a job and then make a new doc with its document", function (done) { + this.timeout(15000); let job; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job2 = new Job(testColl, jobType, { some: "data" }); @@ -191,6 +199,7 @@ describe("JobCollection", function () { }); it("should should create a repeating job that returns the _id of the next job", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }).repeat({ repeats: 1, wait: 250 }); @@ -229,6 +238,7 @@ describe("JobCollection", function () { }); it("should have dependent jobs run in the correct order", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -256,6 +266,7 @@ describe("JobCollection", function () { if (Meteor.isServer) { it("should dry run of dependency check returns status object", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -290,6 +301,7 @@ describe("JobCollection", function () { } it("should have dependent job saved after completion of antecedent still runs", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -316,6 +328,7 @@ describe("JobCollection", function () { }); it("should have dependent job saved after failure of antecedent is cancelled", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -339,6 +352,7 @@ describe("JobCollection", function () { }); it("should have dependent job saved after cancelled antecedent is also cancelled", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -359,6 +373,7 @@ describe("JobCollection", function () { }); it("should have dependent job saved after removed antecedent is cancelled", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -383,6 +398,7 @@ describe("JobCollection", function () { }); it("should cancel succeeds for job without deps, with using option dependents: false", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, {}); job.save(function (err2, res2) { @@ -397,6 +413,7 @@ describe("JobCollection", function () { }); it("should have dependent job with delayDeps is delayed", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -423,9 +440,10 @@ describe("JobCollection", function () { }); }); }); - }).timeout(4000); + }); it("should be dependent job with delayDeps is delayed", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { order: 1 }); const job2 = new Job(testColl, jobType, { order: 2 }); @@ -452,9 +470,10 @@ describe("JobCollection", function () { }); }); }); - }).timeout(4000); + }); it("Job priority is respected", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const jobs = []; @@ -486,6 +505,7 @@ describe("JobCollection", function () { }); it("Job priority is respected", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const jobs = []; @@ -517,6 +537,7 @@ describe("JobCollection", function () { }); it("A forever retrying job can be scheduled and run", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: testColl.forever, wait: 0 }); @@ -539,6 +560,7 @@ describe("JobCollection", function () { }); it("Retrying job with exponential backoff", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: 2, wait: 200, backoff: "exponential" }); @@ -561,6 +583,7 @@ describe("JobCollection", function () { }); it("should have a forever retrying job with 'until'", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }).retry({ until: new Date(new Date().valueOf() + 1500), wait: 500 }); job.save(function (err, res) { @@ -580,9 +603,10 @@ describe("JobCollection", function () { 2500 ); }); - }).timeout(5000); + }); it("should autofail and retry a job", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }).retry({ retries: 2, wait: 0 }); @@ -608,10 +632,11 @@ describe("JobCollection", function () { 2500 ); }); - }).timeout(5000); + }); if (Meteor.isServer) { it("should save, cancel, restart, refresh: retries are correct.", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const j = new Job(testColl, jobType, { foo: "bar" }); j.save(); @@ -623,6 +648,7 @@ describe("JobCollection", function () { }); it("should add, cancel and remove a large number of jobs", function (done) { + this.timeout(15000); const count = 500; let c = count; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; @@ -656,9 +682,10 @@ describe("JobCollection", function () { } return result; })(); - }).timeout(5000); + }); it("should have a forever repeating job with 'schedule' and 'until'", function (done) { + this.timeout(15000); let counter = 0; const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(testColl, jobType, { some: "data" }) @@ -695,10 +722,11 @@ describe("JobCollection", function () { ev = testColl.events.on("jobDone", listener); return ev; }); - }).timeout(4000); + }); } it("should run shutdownJobServer on the job collection", function (done) { + this.timeout(15000); testColl.shutdownJobServer({ timeout: 1 }, function (err, res) { if (err) { done(err); } expect(res, true, "shutdownJobServer failed in callback result"); @@ -711,6 +739,7 @@ describe("JobCollection", function () { if (Meteor.isClient) { it("should run startJobServer on remote job collection", function (done) { + this.timeout(15000); remoteServerTestColl.startJobServer(function (err, res) { if (err) { done(err); } expect(res, "startJobServer failed in callback result").to.equal(true); @@ -719,6 +748,7 @@ describe("JobCollection", function () { }); it("should create a job and see that it is added to a remote server collection and runs", function (done) { + this.timeout(15000); const jobType = `TestJob_${Math.round(Math.random() * 1000000000)}`; const job = new Job(remoteServerTestColl, jobType, { some: "data" }); assert.ok(validJobDoc(job.doc)); @@ -735,6 +765,7 @@ describe("JobCollection", function () { }); it("should run shutdownJobServer on remote job collection", function (done) { + this.timeout(15000); remoteServerTestColl.shutdownJobServer({ timeout: 1 }, function (err, res) { if (err) { done(err); } expect(res, "shutdownJobServer failed in callback result").to.equal(true); diff --git a/server/methods/core/shops.app-test.js b/server/methods/core/shops.app-test.js index a70b3edd298..665d2f7523b 100644 --- a/server/methods/core/shops.app-test.js +++ b/server/methods/core/shops.app-test.js @@ -98,12 +98,14 @@ describe("core shop methods", function () { }); it("creates a new shop for admin for userId and a partial shopObject", function () { + this.timeout(15000); const partialShop = { name }; Meteor.call("shop/createShop", userId, partialShop); }); it("creates a new shop for admin for userId and a partial shopObject ignoring extraneous data", function () { + this.timeout(15000); const extraneousData = Random.id(); const partialShop = { name, extraneousData }; @@ -120,6 +122,7 @@ describe("core shop methods", function () { describe("shop/changeLayouts", function () { it("should replace every layout with the new layout", function () { + this.timeout(15000); const shop = Factory.create("shop"); Meteor.call("shop/changeLayouts", shop._id, "myNewLayout"); const myShop = Shops.findOne(shop._id); From bbd52524c9b5b4b542f71cde9ff7f5a1ca792d4b Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Fri, 1 Jun 2018 13:17:41 -0500 Subject: [PATCH 5/9] tests: fix the default shop fixture --- server/imports/fixtures/shops.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/imports/fixtures/shops.js b/server/imports/fixtures/shops.js index 73c7ec647ef..5737632bcbf 100755 --- a/server/imports/fixtures/shops.js +++ b/server/imports/fixtures/shops.js @@ -160,7 +160,7 @@ const shop = { enabled: true }], workflow: { - status: "active" + status: "new" }, public: true, brandAssets: [ From b869e656c3d7847be1de6365930e9e9f60be33db Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Fri, 1 Jun 2018 13:18:47 -0500 Subject: [PATCH 6/9] fix: editMode default is now false for Products This should impact tests only since our subscribe passes in a value. --- server/publications/collections/products.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/server/publications/collections/products.js b/server/publications/collections/products.js index 962dc048ce4..e86366b380a 100644 --- a/server/publications/collections/products.js +++ b/server/publications/collections/products.js @@ -291,18 +291,20 @@ function filterProducts(productFilters) { } /** - * products publication - * @param {Number} [productScrollLimit] - optional, defaults to 24 - * @param {Array} shops - array of shopId to retrieve product from. - * @return {Object} return product cursor + * @summary Products publication + * @param {Number} [productScrollLimit] - Top-level product limit. Optional, defaults to 24 + * @param {Object} [productFilters] - Optional filters to apply + * @param {Object} [sort] - Optional MongoDB sort object + * @param {Boolean} [editMode] - If true, will add a shopId filter limiting the results to shops + * for which the logged in user has "createProduct" permission. Default is false. + * @return {MongoCursor|undefined} Products collection cursor, or undefined if none to publish */ -Meteor.publish("Products", function (productScrollLimit = 24, productFilters, sort = {}, editMode = true) { +Meteor.publish("Products", function (productScrollLimit = 24, productFilters, sort = {}, editMode = false) { check(productScrollLimit, Number); check(productFilters, Match.OneOf(undefined, Object)); check(sort, Match.OneOf(undefined, Object)); check(editMode, Match.Maybe(Boolean)); - const selector = filterProducts(productFilters); if (selector === false) { From 618cb76664a8acafcc6dfc18dfdb47a78f6e0869 Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Fri, 1 Jun 2018 13:18:59 -0500 Subject: [PATCH 7/9] tests: fix product publication tests --- .../product-publications.app-test.js | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/server/publications/collections/product-publications.app-test.js b/server/publications/collections/product-publications.app-test.js index e8235dda082..d1301cffbdd 100644 --- a/server/publications/collections/product-publications.app-test.js +++ b/server/publications/collections/product-publications.app-test.js @@ -71,12 +71,14 @@ describe("Publication", function () { }; beforeEach(function () { + this.timeout(15000); Collections.Products.remove({}); // a product with price range A, and not visible const productId1 = Collections.Products.insert({ ancestors: [], title: "My Little Pony", + handle: "my-little-pony", shopId, type: "simple", price: priceRangeA, @@ -89,6 +91,7 @@ describe("Publication", function () { const productId2 = Collections.Products.insert({ ancestors: [], title: "Shopkins - Peachy", + handle: "shopkins-peachy", shopId, price: priceRangeB, type: "simple", @@ -101,6 +104,7 @@ describe("Publication", function () { const productId3 = Collections.Products.insert({ ancestors: [], title: "Fresh Tomatoes", + handle: "fresh-tomatoes", shopId, price: priceRangeA, type: "simple", @@ -113,6 +117,7 @@ describe("Publication", function () { const productId4 = Collections.Products.insert({ ancestors: [], title: "Teddy Ruxpin", + handle: "teddy-ruxpin", shopId: merchantShopId, type: "simple", price: priceRangeA, @@ -125,6 +130,7 @@ describe("Publication", function () { const productId5 = Collections.Products.insert({ ancestors: [], title: "Garbage Pail Kids", + handle: "garbage-pail-kids", shopId: primaryShopId, type: "simple", price: priceRangeA, @@ -138,6 +144,7 @@ describe("Publication", function () { Collections.Products.insert({ ancestors: [], title: "Lite Bright", + handle: "lite-bright", shopId: inactiveMerchantShopId, type: "simple", price: priceRangeA, @@ -147,7 +154,7 @@ describe("Publication", function () { isBackorder: false }); - // helper arrays for writing expecations in tests + // helper arrays for writing expectations in tests merchantShop1ProductIds = [productId1, productId2, productId3]; merchantShop1VisibleProductIds = [productId2, productId3]; activeShopProductIds = [productId1, productId2, productId3, productId4, productId5]; @@ -165,7 +172,7 @@ describe("Publication", function () { sandbox.stub(Reaction, "hasPermission", () => true); sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); - collector.collect("Products", 24, undefined, {}, ({ Products }) => { + collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { const productIds = Products.map((p) => p._id); expect(productIds).to.have.members(activeShopProductIds); @@ -179,7 +186,7 @@ describe("Publication", function () { sandbox.stub(Reaction, "hasPermission", () => true); sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); - collector.collect("Products", 24, undefined, {}, ({ Products }) => { + collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { const productIds = Products.map((p) => p._id); expect(productIds).to.have.members(merchantShop1ProductIds); @@ -193,28 +200,13 @@ describe("Publication", function () { sandbox.stub(Reaction, "hasPermission", () => true); sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); - collector.collect("Products", 24, undefined, {}, ({ Products }) => { + collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { const productIds = Products.map((p) => p._id); expect(productIds).to.have.members(merchantShop1ProductIds); }).then(() => done(/* empty */), done); }); - it("should have an expected product title", function (done) { - // setup - sandbox.stub(Reaction, "getShopId", () => shopId); - sandbox.stub(Roles, "userIsInRole", () => true); - sandbox.stub(Reaction, "hasPermission", () => true); - sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); - - collector.collect("Products", 24, undefined, {}, ({ Products }) => { - const data = Products[1]; - const expectedTitles = ["My Little Pony", "Shopkins - Peachy"]; - - expect(expectedTitles.some((title) => title === data.title)).to.be.ok; - }).then(() => done(/* empty */), done); - }); - it("should return only visible products to visitors", function (done) { sandbox.stub(Reaction, "getShopId", () => shopId); sandbox.stub(Roles, "userIsInRole", () => false); @@ -341,7 +333,7 @@ describe("Publication", function () { describe("Shop conditions", function () { beforeEach(function () { - publishProducts(); + return publishProducts(); }); it("returns products from the active shop", function (done) { @@ -360,6 +352,8 @@ describe("Publication", function () { collector.collect("Products/grid", ({ Catalog }) => { const productIds = Catalog.map((c) => c.product._id); + console.log("TEST productIds", JSON.stringify(productIds)); + console.log("TEST activeShopVisibleProductIds", JSON.stringify(activeShopVisibleProductIds)); expect(productIds).to.have.members(activeShopVisibleProductIds); }).then(() => done(/* empty */), done); }); From 064b7f882beb33effa6694ba818338ab1d183996 Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Fri, 1 Jun 2018 14:07:14 -0500 Subject: [PATCH 8/9] tests: remove console log --- .../publications/collections/product-publications.app-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/publications/collections/product-publications.app-test.js b/server/publications/collections/product-publications.app-test.js index d1301cffbdd..9d8a2587859 100644 --- a/server/publications/collections/product-publications.app-test.js +++ b/server/publications/collections/product-publications.app-test.js @@ -352,8 +352,6 @@ describe("Publication", function () { collector.collect("Products/grid", ({ Catalog }) => { const productIds = Catalog.map((c) => c.product._id); - console.log("TEST productIds", JSON.stringify(productIds)); - console.log("TEST activeShopVisibleProductIds", JSON.stringify(activeShopVisibleProductIds)); expect(productIds).to.have.members(activeShopVisibleProductIds); }).then(() => done(/* empty */), done); }); From b8af239d1c505da17ddf5a6c372c09ce0655c58a Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Mon, 4 Jun 2018 15:14:51 -0500 Subject: [PATCH 9/9] chore: fix eslint issues in test file --- .../product-publications.app-test.js | 66 ++++++++++--------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/server/publications/collections/product-publications.app-test.js b/server/publications/collections/product-publications.app-test.js index 9d8a2587859..9bd8e43dace 100644 --- a/server/publications/collections/product-publications.app-test.js +++ b/server/publications/collections/product-publications.app-test.js @@ -1,5 +1,6 @@ /* eslint dot-notation: 0 */ /* eslint prefer-arrow-callback:0 */ +/* eslint promise/no-callback-in-promise:0 */ import Random from "@reactioncommerce/random"; import { expect } from "meteor/practicalmeteor:chai"; import { sinon } from "meteor/practicalmeteor:sinon"; @@ -173,10 +174,10 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { - const productIds = Products.map((p) => p._id); + const productIds = Products.map((product) => product._id); expect(productIds).to.have.members(activeShopProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return all products from the current shop to admins in a Merchant Shop", function (done) { @@ -187,10 +188,10 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId, merchantShopId, primaryShopId]); collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { - const productIds = Products.map((p) => p._id); + const productIds = Products.map((product) => product._id); expect(productIds).to.have.members(merchantShop1ProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("returns products from only the shops for which an admin has createProduct Role", function (done) { @@ -201,10 +202,10 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopsWithRoles", () => [shopId]); collector.collect("Products", 24, undefined, {}, true, ({ Products }) => { - const productIds = Products.map((p) => p._id); + const productIds = Products.map((product) => product._id); expect(productIds).to.have.members(merchantShop1ProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return only visible products to visitors", function (done) { @@ -217,7 +218,7 @@ describe("Publication", function () { expect(Products.length).to.equal(2); expect(expectedTitles.some((title) => title === data.title)).to.be.ok; - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return only products matching query", function (done) { @@ -230,7 +231,7 @@ describe("Publication", function () { const data = Products[0]; expect(data.title).to.equal("Shopkins - Peachy"); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should not return products not matching query", function (done) { @@ -241,7 +242,7 @@ describe("Publication", function () { collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { expect(Products.length).to.equal(0); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return products in price.min query", function (done) { @@ -252,7 +253,7 @@ describe("Publication", function () { collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { expect(Products.length).to.equal(1); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return products in price.max query", function (done) { @@ -263,7 +264,7 @@ describe("Publication", function () { collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { expect(Products.length).to.equal(2); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return products in price.min - price.max range query", function (done) { @@ -274,7 +275,7 @@ describe("Publication", function () { collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { expect(Products.length).to.equal(2); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return products where value is in price set query", function (done) { @@ -285,7 +286,7 @@ describe("Publication", function () { collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { expect(Products.length).to.equal(1); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return products from all shops when multiple shops are provided", function (done) { @@ -295,10 +296,10 @@ describe("Publication", function () { sandbox.stub(Roles, "userIsInRole", () => false); collector.collect("Products", productScrollLimit, filters, {}, ({ Products }) => { - const productIds = Products.map((p) => p._id); + const productIds = Products.map((product) => product._id); expect(productIds).to.have.members(activeMerchantProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); }); @@ -312,10 +313,10 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopId", () => shopId); collector.collect("Products/grid", ({ Catalog }) => { - const productIds = Catalog.map((p) => p._id); + const productIds = Catalog.map((product) => product._id); expect(productIds).to.be.empty; - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("returns products from the Catalog", function (done) { @@ -324,10 +325,10 @@ describe("Publication", function () { publishProducts(); collector.collect("Products/grid", ({ Catalog }) => { - const productIds = Catalog.map((p) => p._id); + const productIds = Catalog.map((product) => product._id); expect(productIds).to.not.be.empty; - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); }); @@ -340,20 +341,20 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopId", () => shopId); collector.collect("Products/grid", ({ Catalog }) => { - const productIds = Catalog.map((c) => c.product._id); + const productIds = Catalog.map((item) => item.product._id); expect(productIds).to.have.members(merchantShop1VisibleProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("returns all visible products from all active shops when the Primary Shop is active", function (done) { sandbox.stub(Reaction, "getShopId", () => primaryShopId); collector.collect("Products/grid", ({ Catalog }) => { - const productIds = Catalog.map((c) => c.product._id); + const productIds = Catalog.map((item) => item.product._id); expect(productIds).to.have.members(activeShopVisibleProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("returns products from all shops when the Primary Shop is active, filtered by shop id", function (done) { @@ -362,16 +363,19 @@ describe("Publication", function () { sandbox.stub(Reaction, "getShopId", () => primaryShopId); collector.collect("Products/grid", 24, filters, ({ Catalog }) => { - const productIds = Catalog.map((c) => c.product._id); + const productIds = Catalog.map((item) => item.product._id); expect(productIds).to.have.members(activeMerchantProductIds); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); }); + /** + * @summary Publishes all products in the database + * @returns {Promise} true on successful publish for all documents, false if one ore more fail to publish + */ function publishProducts() { - const productIds = - Collections.Products.find({}).fetch().map((p) => p._id); + const productIds = Collections.Products.find({}).fetch().map((product) => product._id); return Promise.await(publishProductsToCatalog(productIds, collections)); } @@ -388,7 +392,7 @@ describe("Publication", function () { const data = Products[0]; expect(data.title).to.equal(product.title); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should not return a product if handle does not match exactly", function (done) { @@ -400,7 +404,7 @@ describe("Publication", function () { } else { expect(Products).to.be.undefined; } - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should not return a product based on exact handle match if it isn't visible", function (done) { @@ -413,7 +417,7 @@ describe("Publication", function () { } else { expect(Products).to.be.undefined; } - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); it("should return a product to admin based on a exact handle match even if it isn't visible", function (done) { @@ -425,7 +429,7 @@ describe("Publication", function () { const data = Products[0]; expect(data.title).to.equal("My Little Pony"); - }).then(() => done(/* empty */), done); + }).then(() => done(/* empty */)).catch(done); }); }); });