Inconsistency in product component

[PubliAlex]

Hello,

By trying to understand core concepts of Mall, I noticed a weird thing in the product component.

When you have a product with variants and custom fields, we can see the following :

• variant property fields name start with "props[]"
• customfields name start with "fields[]"

But :

• on the onChangeProperty() method, you try to read the variant properties by looking at the post('values') array.
• on the stockCheckResponse() method, you try to read the custom fields values by looking at post('props') array.

It works cause, when you change property you manually initialize datas in you product/scripts.htm partial with that code :

            $.request('onChangeProperty', {
                data: {values: values, props: props, initial: isInitial},
                loading: isInitial ? null : $.oc.stripeLoadIndicator
            })

By getting the right post() variables in the onChangeProperty() method and on the stockCheckResponse method, you could avoid to manipulate data before sending the request, you could just reference the form object as data source exactly like you do it with your request to the onChangeConfiguration.

I tried some changes :

• in the onChangeProperty, load $values with the following code : $values = post('props', []);

• in the stockCheckResponse, load $fields with the following code : $fields = $this->mapToCustomFields(post('fields', []));

• in the script, reference form instead of manually send data :

            $.request('onChangeProperty', {
                form: $form,
                // data: {values: values, props: props, initial: isInitial}, not needed anymore
                loading: isInitial ? null : $.oc.stripeLoadIndicator
            })
  

It seems to work the same way and you don't have to build the values and props array yourself. What do you think about this ?

[tobias-kuendig]

Good find! This looks like an oversight. The initial property is not in use server-side?

We can definitely clean this up, but have to make sure we don't break backwards compatibility for existing installations.

[PubliAlex]

Thank you tobias for your feedback.

Good find! This looks like an oversight. The initial property is not in use server-side ?

Yes it is, didn't not investigate deeply into that variable, but it's used (at least) to not display the loader for the first stock refresh call.

We can definitely clean this up, but have to make sure we don't break backwards compatibility for existing installations.

We could add some extra condition for backward compatibility that check the existence of post('values') in onChangeProperty method. Something like that :

if(!empty(post('values))) {
    $values = post('values', []);
} else {
    $values = post('props', []);
}

and in the stockCheckResponse method :

if(!empty(post('fields')) {
    $fields = $this->mapToCustomFields(post('fields', []));
} else {
    $fields = $this->mapToCustomFields(post('props', []));
}

A little dirty but I should do the job. The important thing IMO is to make the change quite fast to prevent people from working with the wrong variables.

Best regards,

[xyz1123581321]

TypeError: (target || document).dispatchEvent is not a function. (In '(target || document).dispatchEvent(event)', '(target || document).dispatchEvent' is undefined)

When passing form: $form I do get this error, what could cause it?