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

dev/financial#100 Remove 'partially paid' as a contribution status option for 'record p…ayment' #15771

Merged
merged 1 commit into from
Nov 14, 2019
Merged

dev/financial#100 Remove 'partially paid' as a contribution status option for 'record p…ayment' #15771

merged 1 commit into from
Nov 14, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 8, 2019 •
edited
Loading

Overview

Removes the following statuses from the drop down to record payment for a membership

  • Partially Paid
  • Cancelled
  • Failed

And from an event registration

  • Partially Paid
  • Overdue
  • In Progress.

(Cancelled & Failed are both still possible from New Contribution).

Rationale

  1. consistency - we are offering different things in different places
  2. meaningfulness - I'm basically sure the Overdue & In Progress were never intended to be shown. Originally Pledge statuses, Payment Payment statuses & contribution statuses shared the same option group & basically leaked all over the place. I wrongly though we were no longer adding those 2 statuses to new installs & hope to remove them from new installs as an agreed follow up (& perhaps we could remove or disable them on existing installs that have no contributions with those statuses)
  3. Not creating bad data - creating contributions from the Membership form selecting 'Partially Paid' creates invalid data in terms of there being no financial_trxn, financial items etc- as revealed by the unit test when I added the validateAllPayments check to it.
  4. confusion - are we creating a payment or a contribution - these statuses don't create the contribution but they add to it. As @mattwire pointed out on gitlab & as has come up many other times recently - ideally we would not be mixing our concepts ./ forms like this. This change doesn't solve that but it slightly improves it.

Before

Backoffice event
Screen Shot 2019-11-09 at 12 46 25 AM

Backoffice membership
Screen Shot 2019-11-09 at 12 48 04 AM

After

Screen Shot 2019-11-09 at 12 43 24 AM

Screen Shot 2019-11-09 at 12 45 16 AM

Technical Details

This came up because I tried adding a check to our unit tests to ensure they were creating valid payments & many are not. In some cases it's the tests but in this case the test is testing something that users can do too.

This is the test #15706

@JoeMurray @kcristiano @mattwire @magnolia61 @lcdservices @monishdeb @pradpnayak @agh1 @MegaphoneJon

Comments

https://lab.civicrm.org/dev/financial/issues/100

Also Edit Contribution form has dubious statuses - I don't want to block this on resolving 'it all' but happy to agree some follow ups. My current goal is to work through the test fails in #15706 & identify where invalid data is being created.

Screen Shot 2019-11-09 at 12 48 04 AM

@civibot
Copy link

civibot bot commented Nov 8, 2019

(Standard links)

@civibot civibot bot added the master label Nov 8, 2019
mattwire
mattwire reviewed Nov 11, 2019
View reviewed changes
CRM/Contribute/BAO/Contribution/Utils.php
'Overdue',
'Partially paid',
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton I'm a bit puzzled by this bit of code and the comment. Why are we unsetting different statuses for participant/membership?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this they should be the same - before this I think it's historical randomness

Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton Ok. So if you update this comment which will be wrong after this PR I'm happy to merge this: https://github.com/civicrm/civicrm-core/pull/15771/files#diff-99d052aa6a966c5a2cc89d05f057d22dR565-R566

@eileenmcnaughton
Remove 'partially paid' as a contribution status option for 'record p…
eaba6ea 
…ayment'

Fixes a bug where it is possible to select contribution statuses that do not result in valid financial
transactions. Specifically the 'Partially Paid' option creates no payment transaction and any subsequent
financial_trxns get the wrong line item allocations as a result.
@eileenmcnaughton
Copy link
Contributor Author

@mattwire I've updated the comment - maybe it should get merge-ready to give @JoeMurray @lcdservices @kcristiano a few more days to comment

@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Nov 11, 2019
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 11, 2019
@eileenmcnaughton
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
5a33641 
…cenario

I have discovered a lot of tests are creating invalid contributions - civicrm#15706

So far the issues have been in the test + us permitting something that doesn't work on the form - ie civicrm#15771

I'm trying to work through them all & then we can ideally validate payments in general. In this case
it turns out that because 'amount' is currently a 'required' parameter the tests have 'any value' stuck in there.
In a real submission it would be calculated so I'm trying to share the code that would do that with
the path used by the test (& in this case the api) and to move towards getting the tests valid
@eileenmcnaughton eileenmcnaughton mentioned this pull request Nov 11, 2019
Merged
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 11, 2019
@eileenmcnaughton
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
db89cee 
…cenario

I have discovered a lot of tests are creating invalid contributions - civicrm#15706

So far the issues have been in the test + us permitting something that doesn't work on the form - ie civicrm#15771

I'm trying to work through them all & then we can ideally validate payments in general. In this case
it turns out that because 'amount' is currently a 'required' parameter the tests have 'any value' stuck in there.
In a real submission it would be calculated so I'm trying to share the code that would do that with
the path used by the test (& in this case the api) and to move towards getting the tests valid
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 11, 2019
@eileenmcnaughton
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
5771f46 
…cenario

I have discovered a lot of tests are creating invalid contributions - civicrm#15706

So far the issues have been in the test + us permitting something that doesn't work on the form - ie civicrm#15771

I'm trying to work through them all & then we can ideally validate payments in general. In this case
it turns out that because 'amount' is currently a 'required' parameter the tests have 'any value' stuck in there.
In a real submission it would be calculated so I'm trying to share the code that would do that with
the path used by the test (& in this case the api) and to move towards getting the tests valid
@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

test this please

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 12, 2019
@eileenmcnaughton
[REF] calculate 'amount' on ContributionPage in a shared way in one s…
ec57bf8 
…cenario

I have discovered a lot of tests are creating invalid contributions - civicrm#15706

So far the issues have been in the test + us permitting something that doesn't work on the form - ie civicrm#15771

I'm trying to work through them all & then we can ideally validate payments in general. In this case
it turns out that because 'amount' is currently a 'required' parameter the tests have 'any value' stuck in there.
In a real submission it would be calculated so I'm trying to share the code that would do that with
the path used by the test (& in this case the api) and to move towards getting the tests valid
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

In the 6 days this has been merge-ready @JoeMurray has endorsed the principle on gitlab so I'm merging now (merge-ready is a merge-on-pass with a delay)

@eileenmcnaughton eileenmcnaughton merged commit f2d841f into civicrm:master Nov 14, 2019
@eileenmcnaughton eileenmcnaughton deleted the partially branch November 14, 2019 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwire mattwire mattwire left review comments

Assignees
No one assigned
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eileenmcnaughton @mattwire