[WMDE-Fundraising] Trigger other data changes when changing donation status
Closed, ResolvedPublic15 Story Points

Description

As can be seen in https://github.com/wmde/fundraising/blob/master/src/DonationProcessing/ZahlungsdienstProzessor.php#L75-L165, setting the status of a donation may trigger a range of side effects. The new application must support these side effects

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 17 2016, 11:43 AM
Lea_WMDE moved this task from Incoming to WMDE Fundraising on the TCB-Team board.May 11 2016, 12:21 PM

I've analyzed the code in the function and have broken it down in into the following components

  • 75-90 Set up Donation. No longer needed, since we're not using Spendenprozessor any more but nice domain objects that are already set up.
  • 97-113 Store fields from request in data field. No longer needed, will be done by the use cases.
  • 115-122 Hide donation when it gets booked and was previously deleted/moderated. I think we should add this code to the Donation::confirmBooked method. See also the new task T136605 which has concerns for this. If T136605 gets scrapped, we don't need this.
  • 124-132 Hide donation when it gets booked and contains words on blacklist. Probably no longer needed, we don't allow donations with words on the blacklist. Question: Is forbidding 'evil' donations the right behavior, consistent with the old application?
  • 136-144 WTF? Looks like the ifstatements contradict each other and dt_del is always set to an empty string when the donation status changes and dt_del is set. That behavior would need to be implemented in the different status change methods of Donation (cancel, confirmBooked and markForModeration)

Somehow related to this task: T136604

@WMDE-Fisch, @WMDE-leszek, @kai.nissen Please also have a look at the original code and comment. Did I overlook anything, are the behavior changes ok?

  • 115-122 Hide donation when it gets booked and was previously deleted/moderated. I think we should add this code to the Donation::confirmBooked method. See also the new task T136605 which has concerns for this. If T136605 gets scrapped, we don't need this.

I think the part you linked to, and the stuff T136605 is following the following logic regarding donation paid using third-party providers and the status "needs moderation": if the payment provider has confirmed the payment, mark donation as booked no matter what its "moderation" status would be. In other words donations of such types actually never end up flagged as "needs moderation".
That makes some sense as reduces amount of "false positives" generated by bad word filter and reduces amount of manual moderation. If a name of donor contain a bad word, but the payment is accepted, than we're all good, that's the logic I believe was supposed to be implemented by the old code.
The only concern I see here is the post-donation comment. Donation accepted does not necessarily mean the related comment should be published. Even if the name fields are not checked for bad words if the external payment has been booked, a person still might have cursed etc. in her comment. In such cases the donation should not be public. I believe the code you linked to is kind of handling this (I mean checking if the donation "ist boese" should be doing this),
I believe similar logic has to be applied for "deleted" donation. The case I could think of is the donation has been mark as deleted in the "backend" app (possibly by accident) but then Paypal books the payment. In such cases the donation is accepted (status booked) but still not publicly visible. In the end, it might have been deleted for a reason. It might also be that a recurring donation has been deleted for whatever reason and then after a year a new payment comes in. This might be also handling such cases. But my guess would be that the case of deleted donation is far less common than the one with "needs moderation".

  • 124-132 Hide donation when it gets booked and contains words on blacklist. Probably no longer needed, we don't allow donations with words on the blacklist. Question: Is forbidding 'evil' donations the right behavior, consistent with the old application?

I am not sure what you mean by "Probably no longer needed, we don't allow donations with words on the blacklist. ". Per what I said above I would expect Paypal donation from Mr. Dickens (not at all sure if this is really a bad word but you get the point) to be accepted and not marked as "needs moderation" when Paypal books a payment. Isn't what the old code does? In such case "forbidding evil donations" does not seem right. Unless I am confusing all completely now.
And as for the code you linked to: this seems to be doing similar things that a bit before but now for already "active" (ie. not deleted, not "needing moderation") donations that have been previously publicly visible. This does not seem to be wrong but then I am not sure if this is actually required - if I get it right, the donation should not be visible due to the comment. As it is not possible to change the comment (in particular, booking of external payment does not change the comment), and the donation has previously been marked as "public", it seems to me there is no need to check is the donation still fine to public after booking a payment.
But there might be some cases I cannot think of where that would actually be required.

  • 136-144 WTF? Looks like the ifstatements contradict each other and dt_del is always set to an empty string when the donation status changes and dt_del is set. That behavior would need to be implemented in the different status change methods of Donation (cancel, confirmBooked and markForModeration)

Clearing a possible deletion timestamp after booking a payment seems to make sense per things said above about booking previously deleted donations. But only in such case. If the old code's method was only used to book external payment (I can't know remember was this update_status_external use in any other cases) that seems fine. Why clear a deletion timestamp in any other cases (if there actually have been any) - I have no idea.
Also I have no clue why this part is written that way. It indeed looks that the deletion timestamp always gets unset (if this method was only used for booking external payment, that makes sense as said above).
This thing seems to have been introduced in the real ancient era: https://github.com/wmde/fundraising/commit/a23152160298a35de3088e3c86c0d875b990c145 Might probably even be that it was meant to be "else if" :)
I think that unless @kai.nissen remembers what was the reason to have it like this and what are all possible scenarios, I'd say lets do it in a way the seems sane. Ie. if the only status change related to handling external payment is booking the payment and accepting the donation - let's clear any possible deletion timestamp. Not necessarily using such a nice structure as the old code did.

Once marked as moderated, the paypal booking process shouldn't change it (it currently does as per my implementation). Status changes triggered by the backend application take certain conditions into account to determine the actual new status of a donation. In case of donations using any external payment provider this is the presence of an ext_payment_id marking the donation as booked (or incomplete if empty).

Currently the old application's comment list does only check the field dt_del and is_public which doesn't seem reasonable to me. Also the field dt_del is set and unset on occasions that don't have anything to do with deleting a donation. I think, the fields is_public and status should work together on deciding whether a donation comment should be published. The first determining the intent of a donor to have their comments published, the latter our (as in: the fundraising team) permission that is needed for publication if our application detected some possibly bad words.

Should this go back to doing?

gabriel-wmde closed this task as Resolved.Jun 8 2016, 5:42 PM
gabriel-wmde claimed this task.

Investigation into this created the following tasks: T130190, T137308, T137307