Fix failed webhook delivery preventing it from being delivered again

[eri-trabiccolo]

Description

Fixes #210

How has this been tested?

Manually and with existing and new tests.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• [x] My code has been tested.
• [x] My code passes all existing automated tests.
• [x] My code follows the LifterLMS Coding & Documentation Standards.

@thomasplevy as said in #210 we could even totally remove the pending_delivery property and the logic related to it. I didn't do it yet because I wanted to know your opinion on this. I guess that, since we're in beta, we can get rid of public methods without problems, right?

Also, we should remove the related db column, this means that we need an update routine. But you know, I don't know if it's worthwhile to implement all the "bg db update" logic here, or just implement this minor update in core or whatever. Ideas?

[eri-trabiccolo]

I think you mentioned somewhere that we basically don't need the pending_delivery record in the DB. After reading this it seems that it's useless now, right?

It is useless, that column now is useless, see my comment below the PR description (I will put it at the end of this reply as well).

NOTE: I didn't review all the code in the codebase I'm working quickly off my (very poor) memory.

Is this something we should remove while issuing this fix? Or am I misreading/remembering?

You remember well.

---

@thomasplevy as said in #210 we could even totally remove the pending_delivery property and the logic related to it. I didn't do it yet because I wanted to know your opinion on this. I guess that, since we're in beta, we can get rid of public methods without problems, right?

Also, we should remove the related db column, this means that we need an update routine. But you know, I don't know if it's worthwhile to implement all the "bg db update" logic here, or just implement this minor update in core or whatever. Ideas?

[thomasplevy]

I obviously read the PR message and didn't just jump right into the code review...

Anyway,

we could even totally remove the pending_delivery property and the logic related to it. I didn't do it yet because I wanted to know your opinion on this. I guess that, since we're in beta, we can get rid of public methods without problems, right?

Yea, it's "safe" enough since it's beta and the change is recorded in the changelog clearly.

Also, we should remove the related db column, this means that we need an update routine. But you know, I don't know if it's worthwhile to implement all the "bg db update" logic here, or just implement this minor update in core or whatever. Ideas?

I don't truly know either. I suppose we can just leave a column unused and hanging but that feels messy to me and I'd rather clean it up.

Although it is okay with me if you'd rather split that work into a separate issue that can be worked on later. If you'd rather do that please record the issue so we don't forget about it!

[eri-trabiccolo]

@thomasplevy Opened an issue to track the fact that we have to remove that column from the db (old installs).

[thomasplevy]

OH NO CS :-(