Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Issue with getOrder method in WebhookController #117

Open
EvgeniyHablak opened this issue Nov 28, 2019 · 30 comments
Open

Issue with getOrder method in WebhookController #117

EvgeniyHablak opened this issue Nov 28, 2019 · 30 comments

Comments

@EvgeniyHablak
Copy link

EvgeniyHablak commented Nov 28, 2019

I've got an error while testing the webhooks

Undefined property: stdClass::$temporary_payment_id {"exception":"[object] (ErrorException(code: 0): Undefined property: stdClass::$temporary_payment_id at /var/www/wordproof/vendor/laravel/cashier-mollie/src/Http/Controllers/WebhookController.php:52)

And it seems like this is because of different properties in this places
Screen Shot 2019-11-28 at 11 46 33

But when i've set the same properties the order is not found by findByPaymentId using uuid.
And thats why order status can't be updated.

This is how fixed version look but it works not as expected.
Screen Shot 2019-11-28 at 11 54 15

@sandervanhooft
Copy link
Collaborator

Thanks @EvgeniyHablak for reporting this. I have fixed the inconsistency in the develop branch.

Can you help me understand what's going on with the second part of your question?

@sandervanhooft
Copy link
Collaborator

ah found it

@sandervanhooft
Copy link
Collaborator

Dropped the 'temp_' prefix in the controller:

dda412f

@EvgeniyHablak
Copy link
Author

EvgeniyHablak commented Nov 28, 2019

We are almost there BUT
now my mollie_payment_status is not updated to 'paid'
I've tried to debug it but unsuccessful

@sandervanhooft
Copy link
Collaborator

Could you check:

  • the Mollie dashboard for the response they're getting when calling your webhookUrl?
  • your logs for any errors?

@EvgeniyHablak
Copy link
Author

EvgeniyHablak commented Dec 2, 2019

Could you check:

  • the Mollie dashboard for the response they're getting when calling your webhookUrl?
  • your logs for any errors?

I've tried to debug library a little bit.
when the lib version was 1.6 mollie made 2 requests and thats because our first response was with status code 500 (because we've tried to get mollie_payment_id from empty order object).
after your fixes we are able to find order record in the database by temp payment id BUT
I've Log that object we have found and it doesn't look like the order in my db.
But how is that possible?
Why for the first request from mollie we can't find the order but for the second one it works?

Here is one of response example
Screen Shot 2019-12-02 at 16 35 05

Here is example of webhooks i received
Screen Shot 2019-12-02 at 16 36 17

@sandervanhooft
Copy link
Collaborator

Mollie's webhook call is crazy fast. It's probably a concurrency issue. We'll have to look into it.

@sandervanhooft sandervanhooft reopened this Dec 2, 2019
@sandervanhooft
Copy link
Collaborator

What cashier-mollie version are you at?

@EvgeniyHablak
Copy link
Author

EvgeniyHablak commented Dec 3, 2019

@sandervanhooft
The debugging above is on version 1.6
And i'm currently on version 1.6 (because at least second webhook from mollie works fine)

@sandervanhooft
Copy link
Collaborator

Can you give 1.7 a try?

@EvgeniyHablak
Copy link
Author

I have just checked it.
I received one webhook from mollie and my response code was 200 BUT
in my db molllie_payment_status is open

Also interesting thing is that i've tried to log the order in webhook
Here is what ive got
Screen Shot 2019-12-04 at 17 43 54
Screen Shot 2019-12-04 at 17 43 16

But even if i die the script and would try to find such order with that temp id I couldn't

@EvgeniyHablak
Copy link
Author

Maybe i'm testing incorrectly
This is how my flow looks like:

  1. changing the date order_items.process_at from (for example) 2020-12-12 to 2019-12-12
  2. running php artisan cashier:run
  3. received the webhook and process the request somehow

@sandervanhooft
Copy link
Collaborator

sandervanhooft commented Dec 4, 2019

Hi @EvgeniyHablak,

  1. It seems the $order is resolved just fine?
  2. In test mode, Mollie does not update payment status for mandated payments. You however can update it manually using your browser. There's a _link embedded on the payment specifically for this reason. I usually grab it using mollie()->payments()->get("the_payment_id")->_links;

@EvgeniyHablak
Copy link
Author

Hi @sandervanhooft ! Thanks! I'll check it out asap

@sandervanhooft
Copy link
Collaborator

Closing this for now, let me know if it should be reopened @EvgeniyHablak .

@Paulsky
Copy link

Paulsky commented Feb 19, 2020

I don't want to hijack this issue, but I also have some issues with the default Webhook callback. The getOrder function returns always null (can't find by mollie_payment_id). Could it be that the Webhook is called before the Order actually exists in the database.....? In this case the webhook is called after swapping a plan.

@sandervanhooft
Copy link
Collaborator

@Paulsky what version are you using?

@Paulsky
Copy link

Paulsky commented Feb 19, 2020

I'm using v1.10.3

@Paulsky
Copy link

Paulsky commented Feb 19, 2020

Just to make things clear (maybe I'm doing something wrong):

I'm using the swap() function on an existing subscription. The webhook is called correctly, and the data in the Mollie dashboard looks also correct. When I manually check the database, the mollie_payments_status is 'open' and the 'mollie_payment_id' is filled. The Order before the swap, created with the newSubscription, is 'paid'. The given payment ID with the request is set and looks correct. But as I said, maybe the Order doesn't exists at the time of the webhook call? Because Order::where('mollie_payment_id', $id)->first(); returns null.

@Paulsky
Copy link

Paulsky commented Feb 20, 2020

So I did a lot of debugging. The Order is found in the webhook function, if I remove the DB::transaction() wrapper in the Subscription restartCycleWithModifications() function.

This way, the handlePaymentPaid() is called in the webhook and the OrderPaymentPaid event is fired. So that's great (for sending a new invoice).

But, somehow, the order status is still 'open', even though $this->update(['mollie_payment_status' => 'paid']); is called.

Hope this helps you understand my issue a little bit!

@Paulsky
Copy link

Paulsky commented Mar 4, 2020

I know you are very busy @sandervanhooft , and I don't want to push you. But maybe you can re-open this issue...?

@sandervanhooft
Copy link
Collaborator

I have been thinking about this...

The transaction is there for a reason. It's good to disable it to understand the problem, but I think we should keep it there.

As I've discussed here, there are essentially two alternatives we can try:

  1. when the webhook is called, return a 404 if the payment is not found. Mollie will retry the webhook a second later, and then again using increasing intervals.

  2. keep on returning a 200 response, but handle the webhook call in a queued Job, effectively enforcing a retry scheme within the app.

For simplicity and ease of debugging, I'm leaning towards option 1. What do you think, @Paulsky et al?

@Paulsky
Copy link

Paulsky commented Mar 20, 2020

Hmm, I'm not sure. I'm also leaning towards option 1.

But, the problem I have is that I don't understand the reason why this is happening. Why is the Order not found (while it really seems to exists in the db)? Maybe if I understand the reason, I can give a better suggestion. Do you know what the problem is and could you explain it to me?

Thank you @sandervanhooft 🙏

Edit: I thought I have read somewhere that you wrote that the Mollie webhook is too fast. But I'm not sure about that myself; why would the Order be found if the DB:transaction wrapper is removed? This shouldn't speed up the webhook code would it? Or am I wrong about this?

@sandervanhooft
Copy link
Collaborator

It's a race condition issue.

Mollie's webhook call is so crazy fast it calls the webhook before the transaction is finished (meaning the Order is not stored yet). This all happens in a fraction of a second. So to the human eye it seems like the Order already is in the database table. But in fact, it's not stored yet.

@Paulsky
Copy link

Paulsky commented Mar 20, 2020

Alright, incredible! If that's the case and if the DB::transaction wrapper can't be removed (I don't know if this is even the problem) I also recommend option 1; return a 404 because the Order really does not exists right? It feels less hacky and a lighter solution (instead of forcing users to use queues right?)

@sandervanhooft
Copy link
Collaborator

Exactly @Paulsky , thanks!

@sandervanhooft
Copy link
Collaborator

@Paulsky is this is issue occurring in test or live mode?

@Paulsky
Copy link

Paulsky commented Mar 20, 2020

This occurs in test mode. But live I'm not sure, because I just saw I have some 'paid' 'mollie_payment_status' in the live database.

@sandervanhooft
Copy link
Collaborator

Ok, any of them not marked as paid that should be?

@Paulsky
Copy link

Paulsky commented Mar 20, 2020

No, it looks correct at live!

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

No branches or pull requests

3 participants