Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4254] Limit Products when in Edit Mode #4256

Merged
merged 1 commit into from May 22, 2018

Conversation

pmn4
Copy link
Collaborator

@pmn4 pmn4 commented May 18, 2018

Resolves #4254
Impact: major/minor
Type: bugfix

Issue

When in Edit Mode, a grid will show all products from all shops**, editable or not.

** - whether it is correct that all products from all shops should be displayed is a different issue, which I will be addressing shortly.

Solution

We fetch a list of shops for which you have the "createProduct" role, but then return the products from all active shops. This PR changes that to only show the products from the shops where you have the role when editMode is true.

Breaking changes

none.

Testing

  1. Add products to the Primary Shop and to a Merchant Shop.
  2. Log in as Merchant Shop Owner
  3. Visit the Product Grid
  4. Turn on Edit Mode
  5. Click on a product which belongs to the Primary Shop

isBackorder: false
});

collector = new PublicationCollector({ userId: Random.id() });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all tests did this, so I moved it to a before block

});
});

it("returns products from only the shops for which an admin has createProduct Role", function (done) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff is a little strange here, but this is a new test.
the user does not have the createProduct role in the "current" shop but does for other.
It is a bit of a contrived example, but I will make it more realistic in the next PR addressing the #4092

@@ -88,14 +108,35 @@ describe("Publication", function () {
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, primaryShopId]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user has access to all shops

@@ -320,14 +320,6 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so
return this.ready();
}

// Get active shop id's to use for filtering
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the fix, I was able to move this code down, saving a query when in editMode

@@ -343,13 +335,17 @@ Meteor.publish("Products", function (productScrollLimit = 24, productFilters, so
$in: [true, false, null, undefined]
};
selector.shopId = {
$in: activeShopsIds
$in: userAdminShopIds
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix!

};

// Get _ids of top-level products
const productIds = Products.find(selector, {
sort,
limit: productScrollLimit
}, {
fields: {
_id: 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant to this PR, but since we only care about the _id I figured I'd save some data transfer and request only the _id field

}).map((product) => product._id);

let newSelector = { ...selector };
let newSelector = _.omit(...selector, ["hashtags", "ancestors"]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both branches of the conditional below perform the omit operation, so I moved it here and combined it with the initial spread/clone

{ _id: primaryShopId }
]
}, {
fields: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a copy/paste from above with the fields added

@pmn4 pmn4 force-pushed the pmn4/product-publication--edit-mode branch 2 times, most recently from 8555ab8 to c4078fb Compare May 18, 2018 14:43
}).map((product) => product._id);

let newSelector = { ...selector };
let newSelector = _.omit(selector, ["hashtags", "ancestors"]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both branches of the conditional below perform the omit operation

@pmn4 pmn4 force-pushed the pmn4/product-publication--edit-mode branch from c4078fb to c758804 Compare May 18, 2018 15:04
@pmn4 pmn4 force-pushed the pmn4/product-publication--edit-mode branch from c758804 to c02255f Compare May 18, 2018 15:05
@pmn4 pmn4 requested a review from zenweasel May 18, 2018 17:30
@aldeed aldeed changed the base branch from master to release-1.12.0 May 22, 2018 19:04
@aldeed aldeed merged commit a83c03a into release-1.12.0 May 22, 2018
@spencern spencern mentioned this pull request May 31, 2018
@aldeed aldeed deleted the pmn4/product-publication--edit-mode branch June 1, 2018 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants