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

Disable shops #3132

Merged
merged 11 commits into from
Oct 30, 2017
Merged

Disable shops #3132

merged 11 commits into from
Oct 30, 2017

Conversation

joykare
Copy link
Contributor

@joykare joykare commented Oct 18, 2017

Resolves #3131

How to test

  1. Create multiple shops
  2. Create a few products in each shop
  3. Disable a shop
  4. Observe you cannot navigate to the shop anymore
  5. Also observe you cannot see products related to disabled shop
  6. Repeat 4 & 5 for non-admin users (ensure you cant navigate to shop or see products associated to shop)

@joykare
Copy link
Contributor Author

joykare commented Oct 18, 2017

@zenweasel review on this

@brent-hoover
Copy link
Collaborator

I think we should probably make this the inverse. That is, look for the "active" status and filter all those that are not active? Or maybe an array of visible states (to be named later, but just "active" for now). I know we are going to add more workflow states to shops really soon.

@@ -166,9 +166,16 @@ function composer(props, onData) {
window.prerenderReady = true;
}

const shops = Shops.find({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could name this something like activeShops to make it clearer what we are filtering for below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we filter for just the _id field we could also avoid using the map below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd still have to use map coz after:

const activeShopIdsArray = Shops.find({
    "workflow.status": {
      $in: ["active", "new"]
    }
  }, { fields: { _id: 1 } }).fetch();

It returns an array of objects that looks like this:

[ { _id: "J8Bhq3uTtdgwZx3rz" }, { _id: "Xw8zPBgTmSF7ZdFZS" } ]

So eventually I'd need to get just the values

Copy link
Collaborator

Choose a reason for hiding this comment

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

True. I forgot that it doesn't just return an array of ids.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Maybe we can make this a little more future proof by just showing shops/products that are in a list of active states

@joykare
Copy link
Contributor Author

joykare commented Oct 18, 2017

another review @zenweasel

@brent-hoover
Copy link
Collaborator

Hold your horses. I'm testing as fast as I can.

@brent-hoover
Copy link
Collaborator

Is "new" a status we want to show products for? That's the default status when they are created so I think not? @mikemurray ?

@joykare
Copy link
Contributor Author

joykare commented Oct 18, 2017

Its also the constant status of the default/primary shop. It doesn't change to "active". So to show products in the primary shop we need it.

@brent-hoover
Copy link
Collaborator

Hmmm, maybe we should change that state then? That seems like it's going to trip us up elsewhere. Is that just set in the Shops.json?

@brent-hoover
Copy link
Collaborator

Except for that question (do we want to display the new state) this seems to be working exactly as advertised.

@brent-hoover
Copy link
Collaborator

Alternately we could change that query to be "shops that are active or equal to the primaryShopId". I don't think we need the option to deactivate the primary shop. Looks like the "new" state is defined by the workflow schema so I don't think we want to just change that by default.

@brent-hoover
Copy link
Collaborator

@spencern Your input would be appreciated

@mikemurray
Copy link
Member

@zenweasel new is the default status because of the workflow schema default value like you pointed out.

If we had onboarding then new could be "not yet onboard", and active be "onboarding complete". But as it stands, only the super admin can make a shop "active".

Status should have no effect on the primary shop.

@joykare
Copy link
Contributor Author

joykare commented Oct 18, 2017

NB. Changed it to use the primaryShop in the meantime

@joykare
Copy link
Contributor Author

joykare commented Oct 18, 2017

More scope on this, just for clarity: @mikemurray and @spencern
To get the active shops I was checking for the workflow statuses; new and active. Coz the default status of a primary shop is new.
Right now I changed the query to search for active status or if the shop is primary.

@spencern
Copy link
Contributor

That sounds correct to me. I think the first solution is to only display products from "Active" shops, in the near term we'll be building out a more complex, extensible workflow for shops, but since a shop shouldn't display products until their payment, address, etc are setup, then I think for now a new shop should not be displayed.

I agree that the exception for the primary shop should exist.

@brent-hoover
Copy link
Collaborator

It just occurred to me that we are doing this all on the client-side and we didn't add any filtering to the publications? We need to do that as well. We shouldn't be leaking unpublished Shops/Products out of the product/shop publications in the first place rather than filtering them on the client.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

We need to filter this data in publications not just on the client side, and maybe not on the client side at all.

@joykare
Copy link
Contributor Author

joykare commented Oct 19, 2017

@zenweasel
The Shop/Products publications had a filter already:

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

// Get disabled shop id's to use for filtering
const disabledShopIds = Shops.find({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we do this the same way we did above? e.g. activeShops rather than disabledShops?

@brent-hoover
Copy link
Collaborator

Data from the Product/Shop publications should be updating when the data changes, that's a pretty core Meteor thing. If the components aren't updating when the data changes that's a different thing (and surely beyond the scope of this PR, but we should open an issue)

@joykare
Copy link
Contributor Author

joykare commented Oct 20, 2017

Yeah the backend is reactive its the components that don't update before a refresh.
Made an issue for this #3146

Its ready now

brent-hoover
brent-hoover previously approved these changes Oct 20, 2017
@aaronjudd aaronjudd self-requested a review October 20, 2017 18:03
Copy link
Contributor

@aaronjudd aaronjudd left a comment

Choose a reason for hiding this comment

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

Investigate / Fix failing tests.

@joykare
Copy link
Contributor Author

joykare commented Oct 24, 2017

Findings, so far:

  • Running the products test suite, by itself === passing tests
  • Shop is deleted at some point by another test file/suite (after trying Shop.find() the shop created in the products test is not part of the shops found in the products pub...)

@spencern spencern dismissed aaronjudd’s stale review October 25, 2017 17:00

Failing tests are passing now

brent-hoover
brent-hoover previously approved these changes Oct 25, 2017
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Good work on the tests

@spencern spencern changed the base branch from master to release-1.5.5 October 30, 2017 15:50
@spencern spencern merged commit 0b4c822 into release-1.5.5 Oct 30, 2017
@spencern spencern deleted the joykare-disable-shops branch October 30, 2017 15:51
@spencern spencern mentioned this pull request Oct 30, 2017
Akarshit pushed a commit that referenced this pull request Jan 7, 2018
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.

5 participants