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

Backorder on product seems to be working opposite of how it should #2828

Closed
kieckhafer opened this issue Sep 11, 2017 · 13 comments
Closed

Backorder on product seems to be working opposite of how it should #2828

kieckhafer opened this issue Sep 11, 2017 · 13 comments
Labels
bug For issues that describe a defect or regression in the released software

Comments

@kieckhafer
Copy link
Member

Expected behavior

When Allow Backorder is true, products should show the backorder label.
When Allow Backorder is false, products should show a sold-out label`

Actual Behavior

Steps to Reproduce the Behavior

  • Create a product with inventory tracking.
  • Make inventory 0.
  • Toggle Allow Backorder to see badges.

Versions

Node: 4.2.6
NPM: 5.4.0
Meteor Node: 4.8.4
Meteor NPM: 4.6.1
Reaction CLI: 0.11.0
Reaction: 1.5.0
Reaction branch: marketplace
Docker: 17.06.2-ce

basic_reaction_product

basic_reaction_product 2

@kieckhafer kieckhafer added the bug For issues that describe a defect or regression in the released software label Sep 11, 2017
@spencern
Copy link
Contributor

This may be more an issue of the way the i18n labels were implemented:
Allow Backorder is technically "Inventory Policy" on the schema.

If you read through the comments there, https://github.com/reactioncommerce/reaction/blob/marketplace/lib/collections/schemas/products.js#L179-L195

you can see that

inventoryPolicy: true - means enforce that inventory exists before selling
inventoryPolicy: false - means permit order of items that don't necessarily exist

So yes.. this is an issue, but IMO it's more of a terminology problem than a engineering problem.

@kieckhafer
Copy link
Member Author

kieckhafer commented Sep 11, 2017

Yeah, that makes sense, seems like a terminology issue.

I feel like "Allow Backorder" is more clean and more easily decipherable than "Enforce Inventory Policy" though.

Do we "fake" it by displaying !inventoryPolicy as the toggle, or do do go with Enforce Inventory Policy or some other terminology?

We could also change the color of the toggle to red, so it's kind of an "off" switch instead of an "on" switch. When it's off / not active it could say "Backorder Allowed", and when it's on / red it could say "Backorder not allowed". Seems like a bad idea, but just throwing it out there for discussion... @rymorgan tell me how bad of an idea that is.

@spencern
Copy link
Contributor

spencern commented Sep 11, 2017

Could change it to "Disallow Backorder?"

But I agree that's not as clear as "Allow Backorder", displaying the inverse of inventoryPolicy seems like a pretty good solution.

@rymorgan
Copy link
Contributor

@spencern @kieckhafer I think it's weird UI to 'turn on' something that turns something off. I like the first idea better of fake displaying it (and annotating to any future devs what we did). If it worked correctly, what we have now will make the most sense to the user. Also, it should be "Allow backorder" (sentence case).

@spencern
Copy link
Contributor

spencern commented Sep 11, 2017

Not really sure how we got to "Allow Backorder" as the actual "label" field on the inventoryPolicy in the Schema is
label: "Deny when out of stock",
Which is much more clear as to what is going on.

@rymorgan
Copy link
Contributor

rymorgan commented Sep 11, 2017

@spencern I changed that b/c "Deny when out of stock" didn't make sense to me, nor to an end user. I think it got changed in a partial pdp clean up a while ago, but without considering the whole picture of how it was functioning.

@rymorgan
Copy link
Contributor

So, for now, I'd say we fake it and leave "Allow backorder" with the right function. Or "Do not allow backorder." Which, if we are leaving it backorder as the default and you have to toggle it off, might be the clearest way to go.

@rymorgan
Copy link
Contributor

@aaronjudd You think "Deny when out of stock" is going to make sense to a user?

@kieckhafer
Copy link
Member Author

kieckhafer commented Sep 11, 2017

I think PR #2830 makes it pretty clear what is happening without changing much at all... but am happy to update it to whatever the consensus is here.

@spencern
Copy link
Contributor

@aaronjudd any strong thoughts here? I'm tempted to merge #2830 in which leaves the verbiage as it is, but flips the bit on the switch so that when it says "Allow Backorder" is on, it's permitting backorder or not denying when out of stock.

I can also image that if this is the last time we touch this for a while that it could be the implementation for a while.

There is currently an issue because we're mapping "Deny when out of stock" to "Allow Backorder" and those are opposite, so we should change something.

@spencern
Copy link
Contributor

Closing as #2830 got merged. @kieckhafer re-open if I'm missing something

@vigyano
Copy link

vigyano commented Nov 20, 2017

We still see this issue in reaction 1.5.8 in Product grid page.

@kieckhafer
Copy link
Member Author

@vigyano this was an issue for setting the state in the Admin of a product on the PDP page, not related to the Product grid page. Are you saying you are seeing the label the opposite of what it should be on the Grid? Is the label on the grid the same label as the one on the PDP page?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues that describe a defect or regression in the released software
Projects
None yet
Development

No branches or pull requests

4 participants