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

fix marketplace invitation validation #5058

Merged
merged 1 commit into from Jun 12, 2019
Merged

fix marketplace invitation validation #5058

merged 1 commit into from Jun 12, 2019

Conversation

TilenKreca
Copy link

Resolves #5057
Impact: major
Type: bugfix

Issue

When invitation to new markeplace owner is sent. We are getting error: Order status labels is required.

Solution

Is provided by this pull request.

Breaking changes

No breaking changes or functional changes

Testing

Probably this option need's to be added in tests: Inviting new marketplace owner.

@spencern
Copy link
Contributor

Hi @TilenKreca
Thanks for taking a look at this error and suggesting a fix.
I'm curious why our marketplace owner invitation involves the Order schema at all.
We've been less focused on our current implementation of the marketplace features lately, so it's possible something has slipped in that shouldn't have.

Would you mind investigating why this Schema is used when a marketplace owner invitation is sent?

@TilenKreca
Copy link
Author

Hi @spencern

I see this was added like 1 month ago and according to commit message @kieckhafer "extend Shops schema to allow for orderStatusLabel translations" I assume this has something to do with translations.

I am quite new in Reaction, so maybe it would be much better if someone from core developer's would look at.

@spencern
Copy link
Contributor

Thanks for following up. We'll take a look as time permits :)

@loan-laux
Copy link
Collaborator

loan-laux commented May 8, 2019

@spencern I understand the focus isn't currently on marketplace features, but is there a way to merge this sometime soon? Lots of businesses rely on Reaction for marketplaces, and this is a very simple fix for a major blocking bug.

I've tested it and I couldn't find any breaking changes, although I'm sure my methodology isn't as strict as the core team's.

As for the reason behind the Order status labels error, I was curious too so I did some quick research. orderStatusLabels is actually a field of the Shop schema, and we're not including one in our new Shop object in shop/createShop.

There is no function where orderStatusLabels is mandatory in the codebase. The only place where it's used (/imports/plugins/core/orders/server/no-meteor/resolvers/Order/orderDisplayStatus.js) contains a condition making sure that the Shop object has this orderStatusLabels property. If it doesn't, it falls back on raw data for order status labels.

@mikemurray
Copy link
Member

Confirmed with @kieckhafer that orderStatusLabels is indeed optional, so this change should be fine. I'll do a test of the app, and we should be good to go.

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@mikemurray mikemurray merged commit 77b26ec into reactioncommerce:develop Jun 12, 2019
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.

4 participants