Product Out of Stock Message #1229
Conversation
Fixed magento#1159 [bug]: If user tries to access product page after it goes to out of stock then system enters a infinite loop.
|
This pull request is automatically deployed with Now. Latest deployment for this branch: https://venia-git-fork-sudeep-cedcoss-sudeep-pr-1159-again.magento-research1.now.sh |
|
Hello Stephen, I have just started on Magento PWA a few weeks back and those pull request were among my starting pull requests. So I was not sure how to create a pull request properly and link them properly with original issue. So that's why I created some of the pull requests by mistake. Sorry for that. Now I am getting more habitual with these things, and I will make sure that this does not happen again. Thank you. |
|
No problem! Don't worry about the linking -- we can handle that on our end. I recommend a workflow like this:
Good luck. |
|
Ok Thanks. I'll follow these things while creating PRs in the future. :-) |
| @@ -1,6 +1,8 @@ | |||
| import React, { Component } from 'react'; | |||
| import { string, func } from 'prop-types'; | |||
|
|
|||
| import { isUndefined } from 'util'; | |||
There was a problem hiding this comment.
No need for this guy, we can check if an object is undefined easily without pulling this in.
There was a problem hiding this comment.
Also, just an FYI that isUndefined is deprecated: https://nodejs.org/api/util.html#util_util_isundefined_object.
| description: | ||
| typeof description === 'object' ? description.html : description | ||
| }; | ||
| if (!isUndefined(product)) { |
There was a problem hiding this comment.
Let's do our check for undefined in the render method instead of here.
| addToCart={this.props.addItemToCart} | ||
| /> | ||
| ); | ||
| const productData = this.mapProduct(product); |
There was a problem hiding this comment.
Here is the result of the query for a product that is out of stock:
{
"data": {
"productDetail": {
"items": []
}
}
}Therefore const product = data.productDetail.items[0] will result in product being undefined here and your conditional can just be if (product) { ...
| /> | ||
| ); | ||
| } else { | ||
| return <div>Product Out Of Stock</div>; |
There was a problem hiding this comment.
We have a very basic ErrorView component that would work nicely here. For consistency and ease of update later, let's use that.
Feel free to add an outOfStock (boolean) prop to it that shows this message when the prop is true.
Then you can just render <ErrorView outOfStock={true} /> here 👍
|
It took us a while to give feedback on this so after waiting a week I went ahead and applied the feedback myself. Thanks for your contribution! PR Updated:
|
| const message = loading | ||
| ? messages.get('loading') | ||
| : notFound | ||
| ? messages.get('notFound') | ||
| : outOfStock | ||
| ? messages.get('outOfStock') |
There was a problem hiding this comment.
We may want to allow an error string rather than using this prop. This will allow us to separate the error messages and types from the ErrorView implementation.
export const ERROR_TYPES = {
'LOADING': 'loading',
'OUT_OF_STOCK': 'outOfStock',
...
};
export const errorMap = new Map({
'loading': loadingIndicator,
'outOfStock': 'This Product is currently out of stock. Please try again later.',
'default': 'Something went wrong. Please try again.'
...
});
class ErrorView extends Component {
render() {
const { type } = this.props;
const key = ERROR_TYPES[type] || 'default';
return <h1>{errorMap[key]}</h1>;
}
} There was a problem hiding this comment.
Yeah the ErrorView certainly could use a refactor. I already felt like adding outOfStock was a little scope creep here and I felt like refactoring all of ErrorView would be solidly out of scope for this.
There was a problem hiding this comment.
I can create a follow up ticket to refactor ErrorView for sure
There was a problem hiding this comment.
That's cool -- though the component is super small and I feel like the effort to refactor now is less than the effort to create, groom and work on it later. But w/e you want to do is cool, you're the one doing this work now :D
There was a problem hiding this comment.
A fair point, lol.
I created #1308 just to keep things moving though.
|
PR Updated:
|
|
@supernova-at @sirugh - I think this out-of-stock message should load as a toast, since that is how we are dealing with all of the other alert messages. Thoughts? @sudeep-cecdoss Thanks for the work! The new design for toasts was merged some time ago. Would you be able to make changes to have the message load as a toast? - I will provide a reference design mockup. |
|
Yes! I think this is a perfect example for a toast. In order to do this properly we need to:
We will probably need to do something like navigate back if we emit a toast rather than show the "empty/no product" view. The design/discussion should happen in the issue, #1159, and then we can go from there. Just to prevent all this extra work being done after time has already been spent on coding I would recommend always talking to UX in the issue (use the |
|
@sirugh After a discussion with @awilcoxa - I will open a new issue with a clearly defined user story for this because there is some complexity for us to think through from an experience perspective. For now, I've created a mockup that addresses this bug specifically, and its purpose is to show the toast treatment for the text as well as the position of the toast within the page layout. |
|
@soumya-ashok + @sirugh + @supernova-at — A couple thoughts on inventory that vary across sites I've worked on:
|
supernova-at
removed
the
needs-ux
dpatil-magento
dismissed stale reviews from supernova-at and sirugh
Requested changes were updated.
This is a stop-gap solution for the moment @brendanfalkowski and we still need to define a strategy first on how we deal with out-of-stock items in the shopper flow for Venia.
My preference would be to proactively notify the user that an item is out of stock, and give them the option to save it to a list or be notified when it becomes available.
Right now we have all toasts set to auto-dismiss after a certain amount of time. In addition to the toast, we should probably have on-page text that says the item is out of stock if we block add to cart. I have seen a pattern where "Add to Cart" is disabled but "Add to List" is enabled for an out-of-stock item. Thoughts on this?
Noted.
I think this would be dependent on how a particular merchant deals with out of stock items. For Venia, we haven't decided yet whether or not we would allow out of stock items to be added to the cart.
_Great point, will make a note. As always, thanks for the thoughtful feedback!_ |
|
@soumya-ashok Cool cool, inventory strategy varies a lot between merchants so definitely not expecting a one-size-fits-all solution. The toast feels good for a B2C experience. For B2B, we try to keep the UI actions in-place but disabled, because we may block adding to cart for different contexts (EPA/brand/geo restrictions, discontinued but in stock at other locations, cannot show pricing due to account). We prefer to always show the "add to cart" action in the UI, so it's clear that exists but isn't accessible for XYZ reason. Example:
|
Description
This PR fixes an issue where the application could enter an infinite loop of error notifications when a product becomes out of stock.
Related Issue
Closes #1159 .
Verification Steps
📝 Any product should work but these steps use the
Augusta Earrings.Product In Stock
Product Out of Stock after being In Stock
MAGENTO_SITE_URL+/adminin your browser)Category>ProductsVA12-SI-NA(the SKU for Augusta Earrings)EditQuantityto0andStock StatustoOut of StockSave📝 Sometimes it takes a second for the cache to update and the "out of stock" message to appear.
Product Back in Stock
MAGENTO_SITE_URL+/adminin your browser)Category>ProductsVA12-SI-NA(the SKU for Augusta Earrings)EditQuantityto100andStock StatustoIn StockSave📝 For some reason it definitely takes longer for the cache to flush when the product comes back in stock as opposed to going out of stock. I am honestly unsure the cache timings and where the caching is taking place. If you want to shortcut the process, delete the
apollo-cache-persistentry from local storage on your browser (Application > Local Storage) and refresh Venia usingEmpty Cache and Hard Reload(long press on the refresh button).How Have YOU Tested this?
yarn testScreenshots / Screen Captures (if appropriate):
Infinite Error Loop (before)
Out of Stock Message (after)
Proposed Labels for Change Type/Package
This PR adds a new
propto theErrorViewcomponent. Additions are backwards-compatible.Checklist: