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

Limit, cleanup, and secure shop creation #2969

Merged
merged 29 commits into from
Sep 30, 2017

Conversation

spencern
Copy link
Contributor

Resolves #2889

Fixes and Cleanup

  • Fixes issue where new shops would get a new shop name but the same slug as the shop they were cloned from.
  • Remove "Start Accepting Payment" button from profile entirely. Stripe connect integration is accomplished via the shop settings for new merchants now.
  • Fix react proptypes issue in emailUpdate.js
  • Don't show the become a seller button unless marketplace and allowMerchantSignup are enabled
  • When allowMerchantSignup is enabled, don't show the become a seller button if the user already has a shop.
  • Cleanup profile and marketplace client files
  • Remove unused allowGuestSellers settting
  • Remove merchantFulfillment setting which only has one path currently
  • Add newly created shops to merchantShops array on primaryShop

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

I found an issue with shop creation as guest user (which I'm messaging you privately about). Then, a test is failing. Plus the minor comments I added.

return user.username;
} else if (user.profile && user.profile.name) {
return user.profile.name;
const account = Collections.Accounts.findOne(Meteor.userId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to first subscribe here or not?

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 shouldn't need to subscribe here because we've got a global subscription for this, but it's probably worth making sure the subscription is ready.

https://github.com/reactioncommerce/reaction/blob/spencer-2889-cleanup-marketplace-shop-creation/client/modules/core/subscriptions.js#L15

const account = Collections.Accounts.findOne({ _id: Meteor.userId() });
const marketplaceEnabled = Reaction.marketplace && Reaction.marketplace.enabled === true;
const allowMerchantSignup = Reaction.marketplace && Reaction.marketplace.allowMerchantSignup === true;
const userHasShop = account.shopId !== Reaction.getPrimaryShopId();
Copy link
Contributor

Choose a reason for hiding this comment

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

So the user's account.shopId will point to the primary shop except if they own a shop (when it then points to that shop), right?. Can we throw a comment here for anyone seeing it for first time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, anywhere else that you see might need more comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Others look good

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

Re: the issue earlier mentioned: I was able to create a shop as the marketplace owner while logged in as guest. After the shop was created, the guest user can still click on "Become a Seller" and another shop gets created. So, we'll need to guard against non-owners creating a shop for someone else. (and possible multiple shops for user in this case)

@spencern
Copy link
Contributor Author

@impactmass I've fixed the tests (on my local at least) and have resolved the issues that you pointed out. Ready to check again.

@impactmass
Copy link
Contributor

pulling now.

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

Previous issues are fixed.

But there's a case where I can log in as a guest user, then run Meteor.call("shop/createShop") with onwerID, and doing that I can create as many shops as I want. When I then use the "Become a Seller" button to create a shop, and run the createShop manually again, it then brings the Each user may only create one shop error


// Anonymous users should never be permitted to create a shop
if (!hasPrimaryShopOwnerPermission &&
Reaction.hasPermission("anonymous", Meteor.userId(), Reaction.getPrimaryShopId())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A note: Reaction.hasPermission("anonymous", Meteor.userId(), Reaction.getPrimaryShopId() returns true for "owner" as well. per #2895

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that issue is why I also check to make sure the user is not an owner.

@spencern
Copy link
Contributor Author

@impactmass I was able to reproduce the issue you mentioned and have fixed it.

@impactmass
Copy link
Contributor

Pulling to test now

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

All issues earlier mentioned are now fixed.

One other thing I see (outside of the current fixes) is that the owner can invite/create a shop for an email multiple times. I think we'll like to enforce the one shop per user for all creation methods, so worth fixing here.

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

All looks good! 👍

@impactmass
Copy link
Contributor

impactmass commented Sep 29, 2017

hmm... one failing test though. @spencern any idea why that failed? Logs: shop/createShop should create new shop for admin for userId and shopObject

…accounts shopId

This is necessary because this issue restricts creating a new shop to users without a shopId. When a shop is created for a user, we update the `shopId` property in their account to the new shopId
@spencern spencern merged commit e276692 into marketplace Sep 30, 2017
@spencern spencern deleted the spencer-2889-cleanup-marketplace-shop-creation branch September 30, 2017 01:33
@spencern spencern mentioned this pull request Oct 11, 2017
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.

2 participants