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

Accept Shop Data when Inviting Shop Owner #3456

Merged
merged 3 commits into from Apr 19, 2018

Conversation

5 participants
@pmn4
Copy link
Collaborator

commented Jan 9, 2018

Feature Request: Accept Shop Data when Inviting a Shop Owner (not just User data)

This PR was prompted by a Shop Importer that I am working on.
I would like to leverage the inviteShopOwner code which creates an account, a new shop, kicks off emails, etc., but currently creates a shop with a copy of the Primary Shop (e.g., the name is "Reaction Shop 1").

As far as testing goes, there should be no impact on current functionality since there are no UI changes. Server code can now call inviteShopOwner with both user data and shop data: Meteor.call("accounts/inviteShopOwner", userData, shopData).
There is a test to show how it might be used.

server/methods/accounts/accounts.app-test.js Outdated
@@ -551,7 +553,7 @@ describe("Account Meteor method ", function () {
email: fakeUser.emails[0].address,
name: fakeUser.profile.addressBook[0].fullName
})
).to.not.throw(Meteor.Error, /Access denied/);
).to.not.throw(Meteor.Error);

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

this is not necessarily related to this change, but I coded an error by mistake and this test was still passing.
I find it good practice to not limit the type of error thrown in a ".to.not.throw" test, so I changed it. lmk if you'd like to revert

server/methods/accounts/accounts.app-test.js Outdated
const primaryShop = getShop();
const shopData = {
name: shopName,
locales: primaryShop.locales

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

(unfortunately locales is required and does not have a defaultValue)

server/methods/accounts/accounts.js Outdated
@@ -592,7 +594,7 @@ export function inviteShopOwner(options) {
});
}

const { shopId } = Meteor.call("shop/createShop", userId) || {};
const { shopId } = Meteor.call("shop/createShop", userId, shopData) || {};

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

this is the fix.

all other changes are in support of this tiny change!

shop/createShop already accepts shop data

server/methods/accounts/accounts.js Outdated
@@ -912,10 +914,19 @@ export function setUserPermissions(userId, permissions, group) {
function getEmailLogo(shop) {
let emailLogo;
if (Array.isArray(shop.brandAssets)) {
let mediaId;

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

the changes from here on add extra null-checking/default value setting.
I changed them to make tests less verbose (which is typically is a bad reason for changing code!), but also figured extra null checks are never a bad thing.

server/methods/core/shop.js Outdated
const count = Collections.Shops.find().count() || "";
seedShop = Reaction.getPrimaryShop();
// generate unique shop name
seedShop.name = seedShop.name + count;

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

Previously, shopData.name would get a number appended regardless of whether the shopData is a clone of PrimaryShop or passed by the calling function. Now, this only happens when Primary Shop is cloned.

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch Jan 9, 2018

server/methods/core/shop.js Outdated
@@ -121,16 +127,16 @@ Meteor.methods({
const newShop = Collections.Shops.findOne({ _id: newShopId });

// we should have created new shop, or errored
Logger.info("Created shop: ", shop._id);
Logger.info("Created shop: ", newShopId);

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

better to use the _id returned from Shops.insert just in case the passed value is ignored (and auto generated... which... it really should be, right?)

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch Jan 9, 2018

server/methods/accounts/accounts.js Outdated
emailLogo = path.join(Meteor.absoluteUrl(), mediaId.url());
} else {
emailLogo = Meteor.absoluteUrl() + "resources/email-templates/shop-logo.png";
let media;

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

the changes from here add extra null-checking.
I changed them to make tests a little easier to write (which is typically not a great reason for changing code, I know!), but also figured extra null checks are never a bad thing.

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch to 5f4a2bf Jan 9, 2018

// if no group or customer permissions retrieved from DB, use the default Reaction customer set
roles[shopId] = group.permissions || Reaction.defaultCustomerRoles;

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 9, 2018

Author Collaborator

new test was failing on this line, where group was null.
While I don't understand how similar tests were passing, an extra null check can't hurt.

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch from 5f4a2bf to de4b339 Jan 9, 2018

@zenweasel zenweasel self-requested a review Jan 17, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

I'm getting an error when create a shop through the dashboard now

Exception while invoking method 'accounts/inviteShopOwner' Error: Match error: Failed Match.OneOf, Match.Maybe or Match.Optional validation
    at exports.check (packages/check.js:55:15)
    at DDPCommon.MethodInvocation.shopCreateShop (server/methods/core/shop.js:30:5)
    at packages/check.js:128:16
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
    at Object._failIfArgumentsAreNotAllChecked (packages/check.js:127:41)
    at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1765:18)
    at DDP._CurrentMethodInvocation.withValue (packages/ddp-server/livedata_server.js:1686:15)
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
    at resolve (packages/ddp-server/livedata_server.js:1684:36)
    at new Promise (<anonymous>)
    at Server.applyAsync (packages/ddp-server/livedata_server.js:1683:12)
    at Server.apply (packages/ddp-server/livedata_server.js:1622:26)
    at Server.call (packages/ddp-server/livedata_server.js:1604:17)
    at DDPCommon.MethodInvocation._inviteShopOwner (server/methods/accounts/accounts.js:597:29)
    at packages/check.js:128:16
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
    at Object._failIfArgumentsAreNotAllChecked (packages/check.js:127:41)
    at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1765:18)
    at DDP._CurrentMethodInvocation.withValue (packages/ddp-server/livedata_server.js:719:19)
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
    at DDPServer._CurrentWriteFence.withValue (packages/ddp-server/livedata_server.js:717:46)
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
 => awaited here:
@zenweasel
Copy link
Member

left a comment

Getting an error when creating a shop via the dashboard. Also some minor code notes

emailLogo = Meteor.absoluteUrl() + "resources/email-templates/shop-logo.png";
let media;
const brandAsset =
_.find(shop.brandAssets, (asset) => asset.type === "navbarBrandImage");

This comment has been minimized.

Copy link
@zenweasel

zenweasel Jan 17, 2018

Member

Is there a reason to use this lodash function over the native find? (I realize that's the existing code but maybe we can reduce the lodash here)

return done();
});

it("creates a shop with the data provided", function (done) {

This comment has been minimized.

Copy link
@zenweasel

zenweasel Jan 17, 2018

Member

You don't actually need the done/return done() here. A lot of test authors don't seem to understand it so I see why you would include it (that's what the other tests in this file do), but it's only needed when testing asynchronous methods and included in the callback.

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2018

@aldeed does the new Simple Schema provider richer error messages when things like Match.OneOf, Match.Maybe, or Match.Optional fail validation? The messages when you check against a schema directly are super useful. I've had to change the validation when these fail to find out what's really going on, then change them back. Not the end of the world, but would prefer not to have to do that!

@@ -66,7 +66,7 @@ describe("Account Meteor method ", function () {
Accounts.remove({});
});

it("should allow user to add new addresses", function (done) {
it("should allow user to add new addresses", function () {

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 17, 2018

Author Collaborator

I removed all of the done callbacks based on @zenweasel's comment (none of these tests are async)

emailLogo = Meteor.absoluteUrl() + "resources/email-templates/shop-logo.png";
let media;
const brandAsset =
shop.brandAssets.find((asset) => asset.type === "navbarBrandImage");

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 17, 2018

Author Collaborator

s/_.find/Array#find

// Get shop logo, if available. If not, use default logo from file-system
let emailLogo;
if (Array.isArray(shop.brandAssets)) {
const brandAsset = _.find(shop.brandAssets, (asset) => asset.type === "navbarBrandImage");

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 17, 2018

Author Collaborator

started to replace this _.find and realized there is a method which already does this

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

@pmn4 Hey Pat, is this ready for re-review?

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 25, 2018

@zenweasel it is, thanks!
do you have a process for re-reviewing PRs, or should I @ you when I am done making changes?


// ensure unique id
// (shouldn't Mongo handle this for us? with lesser risk of collision?)
seedShop._id = Random.id();

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 25, 2018

Member

Note that I have some significant changes to this in #3331, which most likely will be merged before this PR and require some conflict resolution. My changes include letting Mongo/Meteor generate the _id as you mention.

https://github.com/reactioncommerce/reaction/pull/3331/files#diff-a85b4a331f34703f51a72e9728869fe2

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch from 482af9a to b2aa0f1 Jan 30, 2018

@@ -528,8 +516,7 @@ describe("Account Meteor method ", function () {
groupId: Random.id(),
email: fakeUser.emails[0].address,
name: fakeUser.profile.addressBook[0].fullName
})).to.not.throw(Meteor.Error, /Access denied/);
return done();
})).to.not.throw(Meteor.Error);

This comment has been minimized.

Copy link
@pmn4

pmn4 Jan 30, 2018

Author Collaborator

when testing for errors not thrown, it is a good idea to remove the limitation on type of error.

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2018

@zenweasel fixed a merge conflict, but otherwise, nothing has changed in the past few days since I addressed your comments. let me know if you have any other questions.

@pmn4 pmn4 added the community label Jan 31, 2018

issues raised in the review have been addressed, ready for re-review

@zenweasel zenweasel requested a review from jshimko Feb 12, 2018

@aaronjudd

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

@reactioncommerce/community can we move this PR forward for 1.9, shepherds needed..

@zenweasel

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

@pmn4 Hey Pat, thanks for your patience with this. If you can fix up these conflicts (I tried but they didn't seem obvious to me) I will make sure and get this review and hopefully approved today.

@zenweasel zenweasel self-requested a review Feb 27, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@pmn4 If we could get these conflicts resolved that would be great. I tried again today and there's a couple around brandAssets I couldn't make the call. If we could get those resolved we should be able to move forward quickly.

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch from b49c79a to dba42cc Mar 5, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

@pmn4 Sorry Pat, we need to get the conflicts resolved again. i tried (again) and it got too dodgy.

@zenweasel zenweasel self-assigned this Mar 20, 2018

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2018

haha, no problem. I will take a look tomorrow

@zenweasel

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@pmn4 We still have a few conflicts here

@zenweasel

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

@pmn4 Can we get the conflicts on this resolved? I don't see any other thing that's holding us up.

@zenweasel

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Actually, looks like we also have failing tests

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch from dba42cc Apr 10, 2018

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 10, 2018

It's been a few days since I was able to take a look at this @zenweasel, but should be green now

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch to 7c04757 Apr 10, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

When I invite a new shop owner I get this empty error message and no shop is created.

2018-04-11_07-27-31

@zenweasel
Copy link
Member

left a comment

Receive an error when inviting a new shop owner

@pmn4

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2018

ah! sorry for wasting your time @zenweasel. I had been testing on an instance with a "cleaner" primary shop. the "taxes" in Shops.json violates the Shop schema. I will address that in a separate PR.

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch 2 times, most recently Apr 11, 2018

@pmn4 pmn4 force-pushed the pmn4/invite-shop-owner branch to 1a92614 Apr 11, 2018

@zenweasel zenweasel removed the request for review from jshimko Apr 13, 2018

@spencern spencern changed the base branch from master to release-1.11.0 Apr 19, 2018

@spencern spencern merged commit 8c0323c into release-1.11.0 Apr 19, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: test-app Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk No dependency changes
Details

@spencern spencern deleted the pmn4/invite-shop-owner branch Apr 19, 2018

@spencern spencern referenced this pull request Apr 19, 2018

Merged

Release 1.11.0 #4151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.