-
-
Notifications
You must be signed in to change notification settings - Fork 813
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
dev/core#905 Add contribution recur statuses 'Processing' and 'Failing' #14395
Conversation
(Standard links)
|
b92a0bf
to
33ef43c
Compare
test this please |
xml/templates/civicrm_data.tpl
Outdated
@@ -1054,6 +1054,8 @@ VALUES | |||
(@option_group_id_crs, '{ts escape="sql"}Failed{/ts}' , 4, 'Failed' , NULL, 0, NULL, 4, NULL, 0, 1, 1, NULL, NULL, NULL), | |||
(@option_group_id_crs, '{ts escape="sql"}In Progress{/ts}', 5, 'In Progress', NULL, 0, NULL, 5, NULL, 0, 1, 1, NULL, NULL, NULL), | |||
(@option_group_id_crs, '{ts escape="sql"}Overdue{/ts}' , 6, 'Overdue' , NULL, 0, NULL, 6, NULL, 0, 1, 1, NULL, NULL, NULL), | |||
(@option_group_id_crs, '{ts escape="sql"}Processing{/ts}' , 7, 'Processing' , NULL, 0, NULL, 7, NULL, 0, 1, 1, NULL, NULL, NULL), | |||
(@option_group_id_crs, '{ts escape="sql"}Failing{/ts}' , 8, 'Failing' , NULL, 0, NULL, 8, NULL, 0, 1, 1, NULL, NULL, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last time this file was modified it broke new installs. Is the ;
the right thing here - @seamuslee001
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that was part of it because note that the code for the CiviCase Option groups is assuming it is in a big long insert statement
7c8fef0
to
6db09ad
Compare
`option_group_id`, {localize field='label'}`label`{/localize}, `value`, `name`, `weight`, `is_reserved`, `is_active`, `is_default` | ||
) | ||
VALUES( | ||
@option_group_id_ps, 'Failing', @maxValue + 2, 'Processing', @maxWeight + 2, 1 , 1 , 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be Failing instead Processing for civicrm_option_value.name and also {localize} for label field in Values.
Also can this two insert be combined into one sql?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - thanks for pointing these out
@@ -9,3 +9,24 @@ SELECT @option_group_id_ps as option_group_id, {localize field='label'}`label`{/ | |||
FROM civicrm_option_value ov | |||
INNER JOIN civicrm_option_group og | |||
ON og.id = ov.option_group_id AND og.name = 'contribution_status'; | |||
|
|||
SELECT @maxValue := MAX(value) FROM `civicrm_option_value` where option_group_id = @option_group_id_ps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value is of type VARCHAR so MAX won't be accurate unless we convert it.
SELECT @MaxValue := MAX(CAST( valueAS UNSIGNED )) FROMcivicrm_option_value where option_group_id = @ option_group_id_ps;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - good point
Per dev/core#905 there are some statuses that are useful to record for recurrings that are in addition to those currently in core. These are - Processing - when a payment against a contribution recur is actively being processed. This would be used when processing a referring contribution to alter the status prior to the attempt. Then when the attempt has completed it would go back to 'In Progress' or whatever but if it got stuck then it would say 'Processing' - indicating not to 'just try again' - Failing - when one or more payemnts against a recurring contribution has failed but the processor has not yet given up. It would be up to processors to implement these - we are standardising the concepts at this stage - although we might provide a generic cron that processors can 'opt into later Update civicrm_generated and fix civicrm_data.tpl
6db09ad
to
50a0869
Compare
test this please |
Overview
Per dev/core#905 there are some statuses that are useful to record for recurrings that are in addition to those currently in core.
These are
Processing - when a payment against a contribution recur is actively being processed. This would be used when processing
a referring contribution to alter the status prior to the attempt. Then when the attempt has completed
it would go back to 'In Progress' or whatever but if it got stuck then it would say 'Processing' - indicating
not to 'just try again'
Failing - when one or more payemnts against a recurring contribution has failed but the processor has not yet given up.
It would be up to processors to implement these - we are standardising the concepts at this stage - although
we might provide a generic cron that processors can 'opt into
later
Before
Statuses don't exist
After
Statuses exist
Technical Details
Comments
@mattwire @adixon I feel like we have discussed this and pretty much converged on these but as promised I gave them their own PR for clarity
Also I just realised there is one more status I keep seeing a need for - it's something like 'Ended'
What we see is there is a situation where the recurring series finishes & is set to Completed or where the user cancels it or where it fails so much it is ended. But I often get questions about what to change it to when they have paid 50% and then either stop paying or it's agreed they've paid enough for one reason or another. I think people find it unsatisfying to mark a recurring as 'cancelled' when they have paid $2000 over 2 years but don't pay the last chunk. However, they haven't 'completed' it - I feel like an 'Ended' or 'Finished' status would be useful from a UI user POV. (I guess I can split that out as a separate issue)
https://lab.civicrm.org/dev/core/issues/905