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

Update order status on dispute events #714

Closed
wants to merge 3 commits into from

Conversation

jrodger
Copy link
Contributor

@jrodger jrodger commented Jun 9, 2020

Related to #526.

Changes proposed in this Pull Request

  • Creates 3 new order statuses
  • Updates order status on dispute events:
    • Dispute created - Order is moved to "Disputed" status
    • Dispute accepted - Order is moved to "Disputed - Accepted" status
    • Dispute lost - Order is moved to "Disputed - Lost" status
    • Dispute won - Order is moved back to it's pre-dispute status

Notes

  • As discussed in p2-pc1FvE-1b, ideally we'd break some of this code out into a package so that we can apply the same order statuses in all our payment gateways (that handle disputes). The long term aim would be merging the behaviour into core.
  • The link to the dispute details uses the URL that the WCPay server sent the webhook to. This should be fine in production, but for our dev environment ends up with a broken link because we can't access the internal address used by Docker containers. I don't think there is a better way to get the URL here?
  • Wording of all the status changes and notes needs reviewing
  • There's a lot of duplicate code related to extracting values from the incoming event. I've left this for now because it would be nice to land some refactoring of this webhook controller before we merge this PR. The changes in here definitely push it over the edge into "too big". Ditto for the lack of unit tests.

Dispute statuses on the order screen

Disputed order note

Accepted dispute order note

Lost dispute order note

Won dispute order note (and status moving back to pre-dispute status)

Testing instructions

  • TODO

@thenbrent thenbrent added the needs docs The issue/PR requires documentation to be added. label Jun 10, 2020
@vbelolapotkov vbelolapotkov changed the base branch from master to trunk December 24, 2020 08:36
@jrodger jrodger force-pushed the add/order-status-updates-on-dispute-events branch from bc95683 to ad34cb4 Compare January 14, 2021 18:26
We create 3 new statuses. "Disputed", "Disputed - Accepted", and
"Disputed - Lost".

Note, there is no "Dispute - Won". Won disputes will be signalled with a
note and a switch back to their pre-dispute order status.
We handle the following events:

* Dispute created  - order is moved to "Disputed" status
* Dispute accepted - order is moved to "Disputed - Accepted" status
* Dispute lost     - order is moved to "Disputed - Lost" status
* Dispute won      - order is moved back to it's pre-dispute status
@jrodger jrodger force-pushed the add/order-status-updates-on-dispute-events branch from ad34cb4 to b836bfc Compare February 9, 2021 18:00
@vbelolapotkov vbelolapotkov changed the base branch from trunk to develop March 15, 2021 16:39
@marcinbot
Copy link
Contributor

Other than some code cleanup, is there anything else blocking us from merging this? It seems like it would be really useful given that some people tend to miss the dispute notification emails.

As for the cleanup - we could create something like Order_Service responsible for managing the order metadata, statuses, etc. There are a few functions in the gateway class that could fit in it as well.

And the package idea is definitely worth exploring, but I don't think we should hold off this on that account.

@haszari
Copy link
Contributor

haszari commented Feb 13, 2024

@haszari
Copy link
Contributor

haszari commented Feb 13, 2024

Next steps here might be to:

  • Review the PR and summarise the plan / changes intended in this PR, i.e. the delta from current code (other changes in this PR may have been implemented since).
  • Review the relevant discussions in P2 and elsewhere.
  • P2 summary to decide if this is worth pursuing as a project or maintenance fix.

@marcinbot
Copy link
Contributor

is this PR worth pursuing?

The functionality is definitely worth pursuing. As for the PR - not sure. A lot has changed since 2 years ago, and given the size, fixing it might take more effort than writing this from scratch. We'd probably need to assign a new developer too. But yes, let's work towards shipping this.

@haszari
Copy link
Contributor

haszari commented Feb 13, 2024

But yes, let's work towards shipping this.

@marcinbot gut check - do you think we could ship an disputed order status fix as a maintenance issue, or do you think we'd need a project?

The functionality is definitely worth pursuing. As for the PR - not sure.

I'll add to Helix review queue as a first step.

Helix reviewer – this is an old stale PR, but might be good inspiration for fixing some other issues in the backlog. Feel free to timebox your review, and don't worry about testing, just review the code and highlight any ideas/content that we could reuse or adapt for better handling of order status in dispute flow. i.e. mine for gold 👷🏻⛏️

@haszari haszari requested a review from a team February 13, 2024 20:40
@marcinbot
Copy link
Contributor

gut check - do you think we could ship an disputed order status fix as a maintenance issue, or do you think we'd need a project?

Probably not - we just need to think about it some more, but the actual work shouldn't be too difficult. I think James's original post outlines the 3 statuses pretty well. I wouldn't worry about the potential for reusability across gateways or merging to Core. That discussion is probably out of date. And every time it has been brought up we always ended up with unnecessarily complicated code and never actually merged anything 🤷

Copy link
Contributor

@haszari haszari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarising what's in this PR for posterity:

  • Registers order statuses to match all stages of a dispute.
  • Applies those statuses in webhook handling for disputes – i.e. implement the actual status values.
    • This includes stashing and restoring (from order meta) the previous dispute status, if the merchant wins dispute. E.g. if an order was completed, then disputed & won, it returns to completed; if (for some reason) the order was disputed when pending, flips back to pending. (I'm thinking of this as "stash & pop")

Note this PR implements webhook handling for disputes, which we have since implemented.

This seems good 🤔

Generally I don't see major technical blockers here. I'm curious why we didn't use these custom status values when we implemented dispute handling.

I'm curious if there's any risk or potential problem with core or third party code breaking the flow of our disputed status (e.g. overwriting it) – so there might be a blocker there.

However, I think it needs a wider discussion

As @jrodger noted:

  • The wording & UX needs at least design review.
  • There's a desire to share this behaviour & these status values more widely. E.g. across Woo-owned payment gateways, or even with core.

I think we should work to get these status values shipped as they would make the dispute flow work much more smoothly.

I think it's important to do this as core-first as we can, there are elements here that we should either align on or actually implement in core:

  • The dispute status values - code name and user-facing wording.
  • The rules / state machine for how we transition between status values.
    • Including to/from regular order status - e.g. the "stash and pop" mechanism.

Next steps

  • Find any prior art & discussion (P2/internal/product/design) relating to disputed order status.
  • Ideate what bugs and pain points this could fix/mitigate.
  • P2 this as a proposal, with the status values and how order can transition between. (could be design-led)

Then we can decide whether to proceed, and where (WooPayments vs. WooCommerce core). If we proceed we will need to start afresh in a new PR as the relevant code has changed significantly.

FYI @souravdebnath1986 - we could consider adding explicit order statuses for disputes as a roadmap project.

@haszari
Copy link
Contributor

haszari commented Feb 16, 2024

Closing – this is too stale to proceed. See review for potential next steps – the idea is possibly worth pursuing!

@haszari haszari closed this Feb 16, 2024
@frosso frosso deleted the add/order-status-updates-on-dispute-events branch August 20, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs The issue/PR requires documentation to be added. pr: in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants