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

fix(payments): incoming payment requests aren't supposed to be in 'initiated' state (only outgoing are) #37447

Merged

Conversation

blaggacao
Copy link
Collaborator

Context

As can be seen from the on_submit of payment requests, the "Initiated" status is semantically reserved for outgoing payments.

However, the gateway integration had misunderstood that semnatics and wongly used "Initiated" status to signify "Requested" instead of using "Requested".

This is in further confustion to the "iniation" status of the Integration Request that is triggered from a payment request.

Likely the original author confounded these and wanted to express that the Integration Request may have been initiated. However, this is a question that lies entierly within the domain an lifecycle of the payment gateway integarion and can therefore not be dealt with from the payment request itself.

As a consequence of this error, users would find:

  • Incoming Gateway Requests as well as Outgoing Requests as Initiated
  • All other incoming requests as Requested

While user shoul consistently find:

  • Outgoing requests: Initiated
  • Incoming requests: Requested

Proposed Solution

  • Don't set payment gateway requests to "initiated" when they are already considered "requested" on submit.
  • Leave the Integration Request cycle within the domain of the payment gateway integration.

Impact

This wrong semantic assignation has been a significant source of misunderstanding while working with the codebase and implementing a payment gateway myself.
Just with this error removed, things would become a lot more consistent and clear to the developer and user alike.

…itiated' state (only outgoing are)
… in 'initiated' state (only outgoing are)
@blaggacao blaggacao force-pushed the fix/incoming-payment-request-status branch from ef53a0f to 1d58996 Compare October 10, 2023 22:08
…ment-request-status
@blaggacao
Copy link
Collaborator Author

@s-aga-r I beleive this PR also survives the refactorings unaltered. 🙏

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #37447 (40a1757) into develop (7a3e4a8) will decrease coverage by 0.01%.
Report is 22 commits behind head on develop.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##           develop   #37447      +/-   ##
===========================================
- Coverage    67.33%   67.32%   -0.01%     
===========================================
  Files          757      757              
  Lines        60147    60163      +16     
===========================================
+ Hits         40497    40507      +10     
- Misses       19650    19656       +6     
Files Coverage Δ
...ccounts/doctype/payment_request/payment_request.py 59.72% <ø> (+0.40%) ⬆️
erpnext/controllers/status_updater.py 88.42% <ø> (ø)
erpnext/selling/doctype/sales_order/sales_order.py 79.12% <ø> (ø)
...doctype/landed_cost_voucher/landed_cost_voucher.py 87.59% <ø> (ø)
...tax_withholding_details/tax_withholding_details.py 79.87% <71.42%> (+0.42%) ⬆️

... and 5 files with indirect coverage changes

…ment-request-status
@deepeshgarg007 deepeshgarg007 merged commit 922fffd into frappe:develop Nov 9, 2023
12 checks passed
@blaggacao blaggacao deleted the fix/incoming-payment-request-status branch November 9, 2023 17:20
Status 'Initiated' is reserved for Outward Payment Requests and was a semantic error in previour versions.
"""

if frappe.reload_doc("accounts", "doctype", "Payment Request"):
Copy link
Member

Choose a reason for hiding this comment

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

This patch will never execute because it's in post section where it's already reloaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh damn, sry! Is this the right fix? #38045

@blaggacao blaggacao restored the fix/incoming-payment-request-status branch November 11, 2023 10:45
@blaggacao blaggacao deleted the fix/incoming-payment-request-status branch November 11, 2023 10:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants