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

Added inventoryPolicy check and min removed from Schema #2825

Merged
merged 2 commits into from
Sep 20, 2017

Conversation

Akarshit
Copy link
Contributor

Fix for #2743

  1. Removed that min: 0 check from Product schema.
  2. Added a condition to check for the inventoryPolicy before rejecting a order.

But this would cause the inventoryQuantity to be as negative as the no. of backorders on that variant. Is this the required behaviour?

@aaronjudd
Copy link
Contributor

@Akarshit this looks good, but can you resubmit to marketplace (or we can release if there is a 1.4.3, but I'd rather just bring it into marketplace now as we're doing a fixes to the PDP). @spencern let's keep an eye and cherry-pick if needed to move this along.

@Akarshit Akarshit changed the base branch from master to marketplace September 20, 2017 17:02
@Akarshit
Copy link
Contributor Author

@spencern Changed base and updated the PR.

Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

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

👍 Works well, thanks for the PR.

@spencern
Copy link
Contributor

This PR raises a few issues that are related but not caused by this PR. I'll create separate issues for these, but also list them here.

  1. Toggling inventoryPolicy / "Allow Backorder" for a variant does not cascade that choice into it's options
  2. Options do not have a switch that will permit toggling the inventory policy. I had to edit the inventoryPolicy for the options in the database directly in order to permit backordering on the default product.

image

@spencern spencern merged commit 286d1e6 into reactioncommerce:marketplace Sep 20, 2017
@spencern
Copy link
Contributor

#2911 is the new issue that I created to address the additional issues noticed here.

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.

3 participants