checkitout icon indicating copy to clipboard operation
checkitout copied to clipboard

Shipping method validation message not showing

Open bluec opened this issue 7 years ago • 6 comments

When a user fails to select a shipping method the validation message is not showing. I am seeing this on multiple sites, can anyone confirm or offer a solution?

I can step into the code and see that the isValid method is being called in skin/frontend/base/default/js/ecomdev/checkitout/steps/shipping.js and this in turn is correctly calling the Validation.ajaxError method found in skin/frontend/base/default/js/ecomdev/checkitout/compatibility/global.js. This correctly call the showAdvice method which is supposed to trigger the Appear effect but from what I can see this never happens (i.e. the display stays as none and the opacity never moves to 100).

Can anyone help?

bluec avatar Sep 04 '17 15:09 bluec

Actually seems that something is calling the hideAdvice method at some point after the showAdvice method is called which sets the advice.currentEvent to a Fade.

bluec avatar Sep 04 '17 15:09 bluec

ohh I ran into this bug, and did not find an excellent solution. The problem is, there are two validations for the shipping blocks:

  1. Validate shipping block including nested block
  2. Validate nested block

The problem was one of the validations always return true; the messages are actually shown, but then immediately hidden.

tbh I'm not too sure how to resolve this at the minute. I will ask my teamm

andrewhowdencom avatar Sep 04 '17 16:09 andrewhowdencom

Thanks @andrewhowdencom this does seem to be the behaviour I am seeing. I'm no JS guru so this is quite time consuming for me to figure out but I'll try and get a stack trace on the second validation and see where it is occuring and whether I can find an elegant fix. If you can dig anything out please do let me know.

bluec avatar Sep 04 '17 19:09 bluec

Hola @bluec;

In chatting to my colleagues it appears that we also did not find a solution in the time allotted, but rather side-stepped the issue by selecting a method "by default" (and thus never being invalid).

If memory serves me, the error comes about as a result of special handling of the shipping method; it calls isValid and then $super.isValid (akin to parent::isValid()). To address this, it might be possible to set some sort of state on the parent, and skip revalidation if it has already been marked as invalid. Something like (in psuedo code):

if thisIsValid() {
  parent::isValid(hasFailed = false)
else {
  parent.isValid(hasFailed = true)
}

and add special handling in the parent. I remember this being a difficult issue though, and unfortunately I do not have lots of time to look into it at the minute. If you do find something, I'm sure a PR would be much appreciated. <3

andrewhowdencom avatar Sep 05 '17 07:09 andrewhowdencom

Hi @andrewhowdencom

Thanks for this. I think I may have found the cause: it seems the shipping method step is repeated in the steps hash and is therefore validated twice. The second time it is validated it removes the validation error that was set when the previous step was validated.

I am not entirely sure whether the problem is that the steps hash should not contain a repeated step for shipping method or whether the second validation is broken and should actually also fail.

By setting a breakpoint inside the isValid() function in checkout.js you can see the steps array has two objects both with the same container:

container: <div#checkout-step-shipping_method.checkout-step.checkout-step-shipping_method first>

The isValid() method iterates over each of the steps, and when it hits the first one it correctly validates the shipping method but when it hits the second one it overwrites the validation and removes the validation message.

Digging back further, these two steps are added to the steps hash when the ShippingMethod class is initialised in /skin/[...]/js/ecomdev/checkitout/steps/shipping.js. It appears that the second duplicated step is added via this call on line 55:

new ShippingAdditional(this);

Commenting out that call fixes the problem but I do not understand enough about why this method was even implemented to know what the unintentional consequences of doing this might be.

I'm pretty out of my depth here in terms of JS and Prototype but I hope now that I've dug this far someone else may be able to offer some further insight.

bluec avatar Sep 05 '17 09:09 bluec

If memory serves, it's attempting to validate a block full of arbitrary content that may or may not exist; used to populate additional data such as gift wrapping messages or whatever. So, validating breaks it, but not validating breaks it also.

(It's been a while lol)

andrewhowdencom avatar Sep 05 '17 10:09 andrewhowdencom