-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
financial#173: Show 'Pay Now' on user dashboard for partially paid contributions #20376
Conversation
(Standard links)
|
I recognize that despite being form-level, it is and should be testable. If I get "Concept Approved" I'll add the test. |
Yep - I agree with this change & @magnolia61 is the right person to test! |
Hello @MegaphoneJon here: #20382 is a slightly updated PR that was based on previous work by #12319 by @jitendrapurohit This PR also outputs correct table layout display and a balance field. I will also try to test the functionality itself the coming days |
@MegaphoneJon from ^^ - this would definitely be mergeable with a test - potentially you could pull in some of @magnolia61's changes as he is losing the jenkins battle at the moment :-) |
Thanks to both of you for the attention on this. @magnolia61 - unless you tell me otherwise, I'm going to merge your changes into this PR and get to the bottom of the test failure in the next day or two. |
hint @magnolia61 you don't want to tell him not to :-) |
@eileenmcnaughton I created a second PR #20388 (comment) which was ok in itself but that got stuck on the test. Would be glad if @MegaphoneJon integrates the code. Thanks for working on this. |
99d78bc
to
fcd395a
Compare
test this |
test this please |
@magnolia61 This PR is now #20388 exactly, plus some test fixes. If you look at the tests, they should speak for themselves - but they ensure that the correct headers appear in the correct order, and all the cells in the row are present. Since we added headers and cells, the tests were failing, so I corrected the tests to reflect the new headers/cell. |
@MegaphoneJon jenkins is given an error - maybe do a pull --rebase & force push? We seem to have been seeing this a lot lately - not sure why |
fcd395a
to
cb492da
Compare
It's a green light from jenkins - were you going to add in a test @MegaphoneJon ? |
Oh, right! Tomorrow. I should be able to extend the current tests to cover this. |
cb492da
to
dbfe939
Compare
test this please |
Fail looks unrelated - @magnolia61 did you test the final version? |
Houston... When I tested I looked at the appearance of the buttons, and also if the contribution would be ok (the remaining amount). Unfortunately I noticed the payment amount is not recorded well. During the payment itself the proper remaining amount is charged, but the payment is recorded in civicrm as being the full amount of the contribution, disrupting the total balance. I opend up an issue for this. Possibly a very easy needle somewhere, but in which haystack? https://lab.civicrm.org/dev/financial/-/issues/174 |
My take on this - also on the issue - is that the url should point to the add payment form civicrm/contact/view/contribution?reset=1&action=add&cid=202&context=contribution&mode=live not the contribution page |
https://lab.civicrm.org/dev/financial/-/issues/173
Overview
The user dashboard has a "Pay Now" button next to contributions of type "Pending" but not those of type "Partially Paid".
Before
After
Comments
@magnolia61 Am I forgetting something important about this?