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

The modal "add to cart confirmation" is not displayed on error #41

Merged
merged 4 commits into from Jul 18, 2019

Conversation

@jf-viguier
Copy link

commented Apr 10, 2019

I've juste added a test to disable launching of showModal if the add to cart request (event) return an error.

EDIT: to be tested with PrestaShop/PrestaShop#14033

The modal "add to cart confirmation" is not displayed if the ajax req…
…uest return an error

I've juste added a test to disable launching of showModal if the add to cart request (event) return an error.
@matks

This comment has been minimized.

Copy link

commented Apr 11, 2019

Looks good 👍 do you have examples of errors that still open the modal ? Maybe we should notify the customer that something wrong happened, but it depends of what went wrong.

If the server failed to answer the request, I guess in debug mode we should display the error, else only say something like "an error happened, please try again". If however it's a valid reason like "sorry ! someone else got the last item, this product is now out of stock !" we can always display the error message.

What do you think ?

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

  • How to reproduce : load product page, put the product out of stock, and add it to the cart without reloading
  • It a better way to display errors, of course, but I suggest to display the errors in the page (juste like in customer subscription process), not in a modal that has to be closed.
    There is a PR that display errors here in a modal :
    #24
@marionf

This comment has been minimized.

Copy link

commented Apr 11, 2019

@jf-viguier

Thank you for your issue but it's a duplicate of: PrestaShop/PrestaShop#11631 so I close it.
Please, follow this issue and add a comment on it if you have more details to bring :)

@marionf marionf closed this Apr 11, 2019

@marionf

This comment has been minimized.

Copy link

commented Apr 11, 2019

Oops, sorry I was a little too fast and didn't see that is a PR :)

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

OK thanks @marionf to keep it open, it's a major issue for me.
My PR is not so good but it's a first step.

@marionf

This comment has been minimized.

Copy link

commented Apr 11, 2019

@jf-viguier

I just tested your PR

The modal isn't displayed anymore if the product is out of stock and the product page is not refreshed.
So, it's like add to cart button didn't do anything, but the product is anyway added to the cart

Video: https://drive.google.com/file/d/1HF5gts5RwANbmQ4zeTWCr38wao0lsSbg/view?usp=sharing

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

It's not what I have but I'm not in 1.7.6. Have you checked that the ajax cart controller call returns an error ?

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

@marionf I've installed the develop branch and tested my pr :
http://creabilis.com/tmp/addtocartok.jpg
It works for me : I have the modal when success = true and no modal when success = false
Please check with the inspector that the add to cart xhr call return false and that out of stock order is refused.

@marionf

This comment has been minimized.

Copy link

commented Apr 12, 2019

@jf-viguier

I well have the error and no modals when the product is out of stock, but it's still added to the cart

capture d'écran_1370

capture d'écran_1371

We would like, like describe here, display the out of stock message when you click on "add to cart" and the product shouldn't be added to the cart.

Do you think that you could improve your PR this way ?

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

I've posted another issue : the out of stock product shouldn't be added to the cart : PrestaShop/PrestaShop#13374

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

@marionf I'll try to improve it to match @colinegin idea PrestaShop/PrestaShop#11631 (comment)

Display add to cart errors
As suggested by @colinegin the errors are now diplayed under the add to cart button :  PrestaShop/PrestaShop#11631 (comment)
@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

@marionf I've just commited the improvement.
Can you add it in the next release ?
add to cart fix

@marionf

This comment has been minimized.

Copy link

commented Apr 18, 2019

Thank you for this improvement @jf-viguier

Do you think it's possible to display the "label of out of stock products with denied backorders" instead and disable the add to cart button ?

capture d'écran_1439

50278133-82c9dd00-0446-11e9-8eb1-3a64f34c87d5

@jf-viguier

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

It's already like this if you load the page without stock. My PR simply display the error returned by the add to stock ajax request (it was like that in 1.6).
The error is here an out of stock error but it can be many more other errors : disabled product, personalization uncomplete, ...
Modules can add errors too (it's my concern)

@matks

This comment has been minimized.

Copy link

commented Apr 18, 2019

@marionf This PR will display the error message returned by prestashop when the "add to cart" fails. So if we want to display the label of "out of stock products with denied backorders", we need prestashop to return this label in the error message, and this should do the job. I think this needs a modification in the Core 🤔, doesn't it @jf-viguier ?

About disable the add to cart button : it's doable indeed. However this requires to modify the page content using Javascript. I'm wondering if this code is not doing things outside of the module's scope 🤔...

This topic is borderline anyways 😅

});
}
if (event && event.resp && event.resp.hasError) {
$('#product-availability').html('<i class="material-icons product-unavailable">&#xE14B;</i>' + event.resp.errors.join('<br/>'));

This comment has been minimized.

Copy link
@matks

matks Apr 19, 2019

It's not good to inject this HTML here as

  • it mixes JS and HTML, that can make the code hard to debug and maintain
  • if the product page styling is modified, this code will need to be modified accordingly (and I'm sure we will forget)

However I understand that this is the only way because there is not better way available.

What about we provide a better way in the Core ? Maybe a JS function displayErrorMessageUnderAddToCartButton(errorMessage) ? This would be an available function in Classic Theme, and it could be called by modules (here, by ps_shoppingcart).

Any feedback @Quetzacoalt91 @eternoendless ?

This comment has been minimized.

Copy link
@jf-viguier

jf-viguier Apr 19, 2019

Author

I agree, it's the only way without impact on PS core,
The html part just add a material icon, it's not dependant on the product page styling.

Please notice that the add to cart ajax call can return more than one error.

Disabling the add to cart button is releveant in out of stock product but what about adding extra stok quantity ? The button souldn'nt not be desabled if asked quantity is above stock.

@marionf

This comment has been minimized.

Copy link

commented Apr 19, 2019

This error message is too long for the product page, that's why we would like to display the out of stock message and disable the add to cart button.
I tested with disabled product and personalization uncomplete, it's all good in this 2 cases 👍

If a PR on the core is needed, I can validate this one but we should do the other PR before releasing the new version of ps_shoppingcart

Bug fix in delete product in cart
Check that event.resp exists before testing event.resp.hasError.
This fix is required for deletint product in cart
@matks

This comment has been minimized.

Copy link

commented May 15, 2019

Sorry for the wait @jf-viguier @marionf, we got very busy with the 1.7.6 beta 😄

So, in order to sum it up, here's what I suggest:

  1. I'll make a PR on the Core, it will do 2 things:
  • send the right error message using "label of out of stock products with denied backorders" when someone tries to add an out-of-stock product in order to comply with @marionf request
  • provide a new JS function displayErrorMessageUnderAddToCartButton(errorMessage) that modules will be able to use to insert their error message in the right place*

When this PR is done and merged, we can update @jf-viguier PR to use displayErrorMessageUnderAddToCartButton, @marionf can validate the behavior and the bug will be fixed nicely 😄.

*To do so I have to check how current JS functions provided by the theme work. It might require to create a new custom prestashop event to be emitted by module.

I'd like a confirmation that this looks suitable indeed @Quetzacoalt91 (as I have never updated the Core theme before) ?

@jf-viguier

This comment has been minimized.

Copy link
Author

commented May 16, 2019

It's sound good to be, thanks

@matks

This comment has been minimized.

Copy link

commented May 30, 2019

I made PR PrestaShop/PrestaShop#14033 to provide a new FO prestashop event "showErrorNextToAddtoCartButton" that you use to display an error message next to "add to cart" button:

prestashop.emit('showErrorNextToAddtoCartButton', { errorMessage: event.resp.errors.join('<br/>')});

So we can avoid modifying html directly

@marionf I'm wondering if we should really use "label of out of stock products with denied backorders" message here. This message is intended to be displayed to say something like "out of stock, sorry come back later". Right now the message being returned is "The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted."
(used in CartController) and I'm thinking it explains better what happened IMO 🤔

@matks

This comment has been minimized.

Copy link

commented May 30, 2019

Another thing: if we introduce and use this new JS event, then next version of ps_shoppingcart will only work with PS latest versions. People wont be able to use it on an old 1.7 version. Is that an issue ?

ping @Quetzacoalt91 @PierreRambaud

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented May 31, 2019

From my understanding, that would be an improvement available from PS 1.7.y.x

On older versions the module will still work as before, which is fine for me.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I guess we don't even have to set the compliancy from 1.7.x. As I said, the module will work as before on old version of PS 1.7. Only the newest one will be able to display the error properly by catching the JS event.

@marionf

This comment has been minimized.

Copy link

commented Jun 3, 2019

@marionf I'm wondering if we should really use "label of out of stock products with denied backorders" message here. This message is intended to be displayed to say something like "out of stock, sorry come back later". Right now the message being returned is "The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted."
(used in CartController) and I'm thinking it explains better what happened IMO

@matks The problem with this message is the "The item %product% in your cart ", we are on the product page, so it's not necessary to display the product name and we are not in the cart yet. Furthermore, this message seems a little too long.

@matks

This comment has been minimized.

Copy link

commented Jun 3, 2019

@marionf I'm wondering if we should really use "label of out of stock products with denied backorders" message here. This message is intended to be displayed to say something like "out of stock, sorry come back later". Right now the message being returned is "The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted."
(used in CartController) and I'm thinking it explains better what happened IMO

@matks The problem with this message is the "The item %product% in your cart ", we are on the product page, so it's not necessary to display the product name and we are not in the cart yet. Furthermore, this message seems a little too long.

We can modify the message with something better.

Or we can put the message from "label of out of stock products with denied backorders" but as I said before: since this is a message that the merchant can customize for our needs, he might put something totally irrelevant for that case. 😛

On the other hand, right now we can control exactly what is being done and explain what happened.

@marionf

This comment has been minimized.

Copy link

commented Jun 3, 2019

We can modify the message with something better.

@matks Just to be sure, it will be a new message ? That will not changed the message displayed in the cart ?

We can have 2 cases:

  1. Enter 5 in quantity field, change product quantity in BO for 3, return on the product page and click on add to cart. In this case, we can:
  • Add 3 products to the cart and display a message like "Only 3 products have been added to the cart because there isn't enough quantity in stock"
  • Add 0 products to the cart and display a message like "The product is no longer available in this quantity. Please adjust quantity"
  1. Enter 5 in quantity field, change product quantity in BO for 0, return on the product page and click on add to cart. In this case, 0 products should be added to the cart and we can display a message like "This product is out of stock"
@matks

This comment has been minimized.

Copy link

commented Jun 3, 2019

We can modify the message with something better.

@matks Just to be sure, it will be a new message ? That will not changed the message displayed in the cart ?

We can have 2 cases:

  1. Enter 5 in quantity field, change product quantity in BO for 3, return on the product page and click on add to cart. In this case, we can:
  • Add 3 products to the cart and display a message like "Only 3 products have been added to the cart because there isn't enough quantity in stock"
  • Add 0 products to the cart and display a message like "The product is no longer available in this quantity. Please adjust quantity"
  1. Enter 5 in quantity field, change product quantity in BO for 0, return on the product page and click on add to cart. In this case, 0 products should be added to the cart and we can display a message like "This product is out of stock"

According to the code (but it would be better to check), for case 1 message is currently:
"The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted."
while for case 2 message is "The minimum purchase order quantity for the product %product% is %quantity%."

That will not changed the message displayed in the cart

Do you mean in the modal ?

@Quetzacoalt91 Quetzacoalt91 changed the base branch from master to dev Jun 5, 2019

@marionf

This comment has been minimized.

Copy link

commented Jun 5, 2019

@matks

So, we can change "The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted." for "The product is no longer available in this quantity." on the product page.

And keep "The item %product% in your cart is no longer available in this quantity. You cannot proceed with your order until the quantity is adjusted." in the cart like currently.

@matks

This comment has been minimized.

Copy link

commented Jun 6, 2019

I updated both this PR and the one from the Core:

  • this PR was updated to use the new JS event from the Core
  • the PR on the Core was updated to follow @marionf requirements
@matks

matks approved these changes Jun 6, 2019

@marionf

This comment has been minimized.

Copy link

commented Jun 6, 2019

@matks

By testing the 2 PRs at the same time, I have the message below

capture d'écran_1641

@matks

This comment has been minimized.

Copy link

commented Jun 6, 2019

@marionf It's strange, it's the old message o_O
I have the new one
Capture d’écran 2019-06-06 à 15 37 55

@marionf

This comment has been minimized.

Copy link

commented Jun 6, 2019

That's weird, I deleted all the caches and I'm in private navigation

@sarahdib has "The product is no longer available in this quantity." in EN-US and "L'article de votre panier (Mug Today is a good day) n'est plus disponible dans cette quantité. Vous ne pouvez pas continuer votre commande avant d'avoir ajusté la quantité." in FR

@sarahdib

This comment has been minimized.

Copy link

commented Jun 6, 2019

@marionf can you try to upgrade the language ?
Capture d’écran 2019-06-06 à 19 23 15

@sarahdib

This comment has been minimized.

Copy link

commented Jun 7, 2019

@matks we found a wired case with @marionf
if the product with 0 quantity is already in the cart -> new message is displayed
if the product with 0 quantity isn't in the cart -> old message is displayed

@matks

This comment has been minimized.

Copy link

commented Jun 7, 2019

@matks we found a wired case with @marionf
if the product with 0 quantity is already in the cart -> new message is displayed
if the product with 0 quantity isn't in the cart -> old message is displayed

Tricky 😄 I'll handle it

@matks

This comment has been minimized.

Copy link

commented Jun 11, 2019

@sarahdib Cant reproduce 😐 I have the new message in both usecases
Capture d’écran 2019-06-11 à 17 10 40

Are you sure it depends on the product being already in the cart ? I think there's another condition, somehow related to how much there is in the cart, how much is available, an also the "out of stock mode"

Tested:

  • prestashop on develop (because PR on Core has been merged)
  • this module, using this branch
@matks

This comment has been minimized.

Copy link

commented Jun 11, 2019

But I see the old error message on the Cart page
Capture d’écran 2019-06-11 à 17 15 08

@matks

This comment has been minimized.

Copy link

commented Jun 11, 2019

Found the reason behind this behavior with @marionf : I was testing on a product with combinations, she was testing with a product without combinations

@marionf

This comment has been minimized.

Copy link

commented Jun 11, 2019

And there is the same probem with a virtual product

@matks

This comment has been minimized.

Copy link

commented Jun 14, 2019

New PR on the core: PrestaShop/PrestaShop#14214

@matks

This comment has been minimized.

Copy link

commented Jul 18, 2019

@marionf Can we validate this PR now that PrestaShop/PrestaShop#14214 has been approved ?

@marionf marionf added QA approved and removed QA approved labels Jul 18, 2019

@marionf

This comment has been minimized.

Copy link

commented Jul 18, 2019

All good @matks

@matks matks merged commit 6389511 into PrestaShop:dev Jul 18, 2019

@matks

This comment has been minimized.

Copy link

commented Jul 18, 2019

Thanks @jf-viguier that's one more major bug down the road !

@jf-viguier jf-viguier deleted the jf-viguier:patch-1 branch Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.