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

E-Mail not sent when order consists of reduced positions #294

Closed
9heun9 opened this issue Nov 11, 2021 · 9 comments · Fixed by #297
Closed

E-Mail not sent when order consists of reduced positions #294

9heun9 opened this issue Nov 11, 2021 · 9 comments · Fixed by #297

Comments

@9heun9
Copy link

9heun9 commented Nov 11, 2021

Hi Mollie Team!

We have experienced a problem with the Mollie-Plugin when used in combination with Advanced Promotion Suite.
No order confirmation mail is being sent for all orderes that have reduced positions only.

As soon as we disable the Shopware_Modules_Order_SendMail_Send - Event handling mails are sent again.

Further narrowing down the problem we managed to find out that inside sendConfirmationEmail() function this line

$variables = @json_decode($transaction->getOrdermailVariables(), true);

returns null and therefore email sending is skipped.
Thanks for checking

Alex

@boxblinkracer
Copy link
Collaborator

Hi there
thank you for letting us know.
ok that is...just weird...always all these plugins :D

anyway, doing our best to solve it,
thanks

@boxblinkracer
Copy link
Collaborator

Hi

I've just tested it, and it worked on my end.
can you output the json error that happens?
what is inside the ordermail variables? might be an invalid json

thank you

@snc
Copy link

snc commented Nov 16, 2021

Hi,

I've continued to debug this issue on our side and found the cause of it. It already fails when encoding the JSON in

$transaction->setOrdermailVariables(json_encode($variables));
, so the value in the database is NULL.

I've checked json_last_error() and it returned 7 (JSON_ERROR_INF_OR_NAN).
After adding the JSON_PARTIAL_OUTPUT_ON_ERROR flag to the json_encode call it finally worked again:

$transaction->setOrdermailVariables(json_encode($variables, JSON_PARTIAL_OUTPUT_ON_ERROR));

Maybe the plugin could check if the call to json_encode failed, as no exception is thrown by default.

The part of the variables that is causing JSON_ERROR_INF_OR_NAN can be seen here:

...,"price":"15,71","pseudoprice":"20,95","referenceprice":"NaN","has_pseudoprice":true,"price_numeric":15.712499,...

Let's see if I can find out where the referenceprice is set to NaN...

@boxblinkracer
Copy link
Collaborator

boxblinkracer commented Nov 16, 2021

ah perfect
yes, i would love to improve that in the plugin.
most of the time i try to do the fail-fast approach...otherwise you might not have discovered an invalid price in your json :)

but maybe i can switch over to a softer check and add a json parse log entry instead ;)

what do you think?

@snc
Copy link

snc commented Nov 17, 2021

The following line from the Advanced Promotion Suite plugin is causing the NaN:

$product['additional_details']['referenceprice'] = $price / $purchaseUnit * $referenceUnit;

In our case, the $purchaseUnit and $referenceUnit is null.

I'm unsure about how you should handle NaN and/or Inf case, but you should definitely check if the json_encode result is null. Maybe using serialize and unserialize could be another solution, but this might not be backwards compatible.

@boxblinkracer
Copy link
Collaborator

Hi @snc

thank you for this.
i've created an improvement based on your input, worked great for me when producing a NaN value.

values will now be 0, which yeah...could be misleading in case its even displayed in the mail
but still better i think, because the main values should always be valid i think :)

in addition to this, warning logs are generated
let me know what you think

#297

@snc
Copy link

snc commented Nov 19, 2021

Hi @boxblinkracer, the PR looks good to me! Thanks.

@boxblinkracer
Copy link
Collaborator

perfect, thanks @snc
always happy to help :)

@boxblinkracer
Copy link
Collaborator

Hi @snc
I wanted to reach out and let you know that the pull request has been merged.
The feature will be released with the next update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants