Allow inviteShopMember to accept multiple groups

[loan-laux]

Resolves #9
Impact: breaking
Type: feature

Test with api-plugin-email-templates#7

Issue

The inviteShopMember mutation allows a groupId to be specified, but it would be even better to allow multiple groups with a groupIds array.

Solution

Allow inviteShopMember to take a groupIds array instead of a groupId string.

Breaking changes

• The groupId variable won't work anymore for inviteShopMember. To invite a new user to a single account, pass a single-item groupIds array to the mutation.
• The inviteNewShopMember template needs to be at its latest version, as the groupName variable was updated to groupNames.

Testing

1. Call inviteShopMember with one group in the groupIds array.
2. Receive an e-mail referring to this group's name in a singular form.
3. Verify that, for this invite's object in the AccountInvites collection, the groupIds field has the correct value.
4. Sign up as the new account. Verify that the account is part of the expected group.
5. Call inviteShopMember with multiple groups in the groupIds array.
6. Receive an e-mail referring to the group names in a plural form.
7. Verify that, for this invite's object in the AccountInvites collection, the groupIds field has the correct value.
8. Sign up as the new account. Verify that the account is part of the expected groups.

[kieckhafer]

I think the idea behind this makes sense, and the code overall looks good.

As this is really the first breaking change in any of our new plugins, I think we need to at least discuss a process for this.

Some thoughts:

• We could just make a clean break and call this 2.0 right now. If this is the case, we'll need a new commit with the BREAKING CHANGE trigger for semantic-release
• We could keep groupId as a deprecated field for now, and simply add groupIds and encourage everyone to use that instead, until we make a breaking change later on.
• When we update Reaction to include the new version of this plugin, how do we call out there has been a breaking change? Or do we even need to do that anymore?

cc @aldeed

[loan-laux]

@kieckhafer I like the idea of keeping groupId but as a deprecated field for now. I'll let you guys discuss the rest. Happy to make changes as needed.

[aldeed]

@loan-laux @kieckhafer If we pull breaking plugin changes into Reaction's stock API project, we'll need a major release for it. While we don't want to be shy of major releases, we do want to try to keep them to a minimum. The general idea is to always deprecate rather than remove, and every so often, when it's time for a major release, we'll remove everything that is currently deprecated.

So yes, groupId should remain with @deprecated directive added. It should be easy in either the resolver or the internal fn to set groupIds = [groupId]. If they pass in both, it should throw an error.

[kieckhafer]

@aldeed makes sense. should we come up with some standard of tracking breaking releases across all these repos, so that when we do want to make a major Reaction release, we have some standard way of knowing which of the plugins we can update? Perhaps a ticket with at BREAKING tag on it in each repo? Or do we make an EPIC ticket in Reaction that tracks all of the changes across the projects?

[loan-laux]

@aldeed Unfortunately, it looks like the @deprecated directive isn't allowed yet on input fields, but the GraphQL team's working on it. As for the payload, we're simply returning an Account so there's nowhere to use this directive there. Thoughts?

[loan-laux]

Sorry for the mixup here. I actually had PR #8 merged into this branch. I've rebased it so that it's only changes related to this PR.

[aldeed]

@loan-laux Now that you remind me, we came across that issue before and solved it just with DEPRECATED in description. You can follow the pattern here: https://github.com/reactioncommerce/api-plugin-pricing-simple/blob/trunk/src/schemas/schema.graphql#L113-L114

The important thing is that we can run a search for "deprecated" text across all plugins when we're ready to do a breaking release and delete stuff.

[loan-laux]

@aldeed I figured that out and wrote "Deprecated" in the field description but it isn't all-caps. Should I update it to DEPRECATED or is it okay as-is?

[loan-laux]

Nevermind, I realized that the pattern you mentioned also had a use otherField instead part which was missing in my schema. I've replicated it.

[aldeed]

Inline comments

[loan-laux]

@aldeed Implemented all of your requests here. Group IDs in createAccount are now listed in a Set instead of an Array, to natively ensure that there are no duplicates.

[rc-publisher]

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀