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.

Sign up for GitHub

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

Jump to bottom

feat(payment): add more payment channels than just email and phone (think: chat) #40424

Closed
wants to merge 4 commits into from
Closed

feat(payment): add more payment channels than just email and phone (think: chat) #40424

wants to merge 4 commits into from

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Mar 12, 2024 •
edited
Loading

Revival of partially reviewed and then stale-bot terminated: #37448 (see there for context)

no-docs

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Mar 12, 2024
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch 8 times, most recently from ae0cf13 to e1f4c04 Compare March 29, 2024 19:13
@blaggacao

This comment was marked as outdated.

Sign in to view
@blaggacao blaggacao mentioned this pull request Apr 2, 2024
Closed
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch from e1f4c04 to e5fb308 Compare April 2, 2024 17:50
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch 2 times, most recently from 67a6598 to 5fbe42f Compare April 2, 2024 22:30
@blaggacao blaggacao marked this pull request as draft April 2, 2024 22:56
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch 6 times, most recently from 439cbb1 to 3a54e6a Compare April 3, 2024 00:31
@blaggacao blaggacao marked this pull request as ready for review April 3, 2024 00:33
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch from 3a54e6a to d6fb176 Compare April 3, 2024 09:47
@blaggacao blaggacao mentioned this pull request Apr 3, 2024
Closed
@blaggacao blaggacao force-pushed the feat/payments-add-other-payment-channels branch from d6fb176 to 3ba19ef Compare April 3, 2024 10:31
@blaggacao
Copy link
Collaborator Author

@ruthra-kumar I managed to reorganize the commits so that it now implements something akin to TDD: each commit is atomically green on the provided tests (bench run-tests --doctype "Payment Request" --test test_payment_channels). It should now be easier to review.

blaggacao
blaggacao commented Apr 3, 2024
View reviewed changes
erpnext/accounts/doctype/payment_request/test_payment_request.py

self.assertIsNone(pr.payment_url)
self.assertEqual(pr.payment_url, PAYMENT_URL)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no penalty in generating a payment url, the flow starts only when accessing it. Providing a fallback url, even on a Shopping Cart flow is a feature: a customer agent can provide that link from the backend for if the original payment on the website had failed for some reason.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Apr 4, 2024 •
edited
Loading

@ruthra-kumar Is there a way that we could advance on this, as well?

After yesterday's test & typing sprint on frappe/payments (not pushed yet, some cleanup to do) I'm starting to see a reasonable design emerging for a generic payment controller over in my branch and getting this PR here ready to be upstreamed (and to a lesser extend #40842) would help raise my confidence and avoid forcing me to pause the sprint.

It would do so by helping to hedge my risks by branching too far, something that might eventually overstretch my resources and focus.

I'm more than happy to expeditiously put in any extra work necessary for this PR 🤝

And thanks to you, specifically, for the great collab, so far! It has been a pleasure! It's a real privilege to have such expert pairs of eyes on my (user-programmer) code-output.

@stale Stale
Copy link

stale bot commented Apr 28, 2024

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Apr 28, 2024
@blaggacao
Copy link
Collaborator Author

ping

@stale stale bot removed the inactive label Apr 28, 2024
@blaggacao
Copy link
Collaborator Author

blaggacao commented May 2, 2024 •
edited
Loading

@ruthra-kumar @s-aga-r could someone please help out and engage with this PR?

@blaggacao
Copy link
Collaborator Author

blaggacao commented May 2, 2024 •
edited
Loading

Please help to get the complexity and diff of #40845 down, by reviewing and merging this PR or alternatively #40842 which includes this PR.

@blaggacao
Copy link
Collaborator Author

closing this in order to focus on #40845

@blaggacao blaggacao closed this May 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@deepeshgarg007 deepeshgarg007 Awaiting requested review from deepeshgarg007

@ruthra-kumar ruthra-kumar Awaiting requested review from ruthra-kumar ruthra-kumar is a code owner

@rohitwaghchaure rohitwaghchaure Awaiting requested review from rohitwaghchaure

@s-aga-r s-aga-r Awaiting requested review from s-aga-r

Assignees
No one assigned
Labels
needs-tests This PR needs automated unit-tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@blaggacao