craftcms / commerce-mollie

Mollie payment gateway for Craft Commerce.
https://plugins.craftcms.com/commerce-mollie
MIT License
5 stars 10 forks source link

Bug - When you return from Mollie payments the notification that's added contains a json stirng #15

Closed lenvanessen closed 5 years ago

lenvanessen commented 5 years ago

See screenshot below. How to reproduce:

  1. Initiate payment using mollie
  2. Press return to website
  3. This error below get's shown Schermafbeelding 2019-03-11 om 14 20 46
andris-sevcenko commented 5 years ago

Hi, which plugin version are you using?

lenvanessen commented 5 years ago

Version 2.0.0

andris-sevcenko commented 5 years ago

After looking at this - you probably should not be displaying the full error message returned from the gateway, anyway. Even if the payment failed, you wouldn't want to expose the entire transaction error body.

You can check if the error exists and then display a generic "Payment failed." message.

lenvanessen commented 5 years ago

@andris-sevcenko but how do i check this, the error is just a flash message in the 'errors' category, so i can't differentiate the ones coming from mollie versus for instance other error in the cart

Example: This code is used to show both stock messages, but also handles errors coming from Mollie. How do i check witch is witch. Because the errors generated by Craft Commerce itself are just regular sentences and describing what is happening.

    {% set flashError = craft.app.session.getFlash('error') %}
    {% if flashError %}
       <div data-alert class="alert-box alert radius">
          <span>{{ flashError }}</span>
          <a href="#" class="close">&times;</a>
       </div>
    {% endif %}

So I don't know what is coming from Mollie, and what is coming from craft, so if i just do this:

    {% set flashError = craft.app.session.getFlash('error') %}
    {% if flashError %}
       <div data-alert class="alert-box alert radius">
          <span>*Payment error*</span>
          <a href="#" class="close">&times;</a>
       </div>
    {% endif %}

Half of the time the there could have been a nice error from Craft, like when you left out some required fields, but it's now only showing payment error to fix the Mollie json bug.

andris-sevcenko commented 5 years ago

Okay, that's fair.

What about setting the cancelUrl POST variable on the payment form, so that if a payment fails, you get redirected there and can display a more generic payment failed message? Then you can keep your flash errors in the existing template.

The real underlying problem here is that Mollie doesn't really return an error in the response string if the payment is canceled and Omnipay's solution is to return the entire response as the error message. (https://github.com/thephpleague/omnipay-mollie/blob/3.2/src/Message/AbstractResponse.php#L12-L17)

lenvanessen commented 5 years ago

@andris-sevcenko that could have worked, but if i add a url param the redirect back ends-up in the homepage, instead of the cart. So craft is not handeling the redirect-back properly when it contains the url param.

Couldn't we just ignore the feedback from Mollie and have the plug-in set a generic, translatable, payment-issue message?

lenvanessen commented 5 years ago

Hackis fix for now

 {% if 'mollie' in flashError %}
            <span>{{ 'Er ging iets mis bij de betaling' | t }}</span>
 {% else %}
  <span>{{ flashError }}</span>
          {% endif %}

But ofcourse this isn't a proper solution

andris-sevcenko commented 5 years ago

@andris-sevcenko that could have worked, but if i add a url param the redirect back ends-up in the homepage, instead of the cart. So craft is not handeling the redirect-back properly when it contains the url param.

What value did you set the cancelUrl variable to?

Couldn't we just ignore the feedback from Mollie and have the plug-in set a generic, translatable, payment-issue message?

That also could work. We'll discuss this internally.

andris-sevcenko commented 5 years ago

Just released 2.0.1 which adds generic and translatable messages for when a payment fails or is canceled.

The reason I was reluctant to do so is that Omnipay gateway should have returned a normal error message and while this plugin was in a position to override those messages it shouldn't have had to.

lenvanessen commented 5 years ago

With version 2.0.1 still get json in the error in some cases

Schermafbeelding 2019-03-27 om 15 42 48
andris-sevcenko commented 5 years ago

Not really able to reproduce this, even with forcing Mollie gateway to die an ungraceful death when processing any webhook.

Any chance of getting the transaction row from the craft_commerce_transactions table concerning this order over at support@craftcms.com?

lenvanessen commented 5 years ago

@andris-sevcenko try from a local machine, that hasn't got a url that molly can reach for the webhook. In our case, projectname.test

This will produce the issue on creation of a payment.

I send the transaction row to support@craftcms.com if it's any help.