dev/membership#18 Enhance parameters for Job.process_membership #16298

mattwire · 2020-01-15T14:51:52Z

Overview

This adds some parameters to control the Job.process_membership API/scheduled job per gitlab issues.

https://lab.civicrm.org/dev/membership/issues/18

Before

Cannot control behaviour of Job.process_membership.

After

Can control Job.process_membership:

exclude_test_memberships: Exclude test memberships from calculations (default = TRUE)
only_active_membership_types: Exclude disabled membership types from calculations (default = TRUE)
exclude_membership_status_ids: Default: Exclude Pending, Cancelled, Expired. Deceased will always be excluded

Technical Details

Comments

@michaelmcandrew Can you write a unit test to validate the new parameters? There are a number of tests on Job.process_membership in tests/phpunit/api/v3/JobTest.php which could be extended/copied.

civibot · 2020-01-15T14:51:54Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

mattwire · 2020-01-15T15:49:03Z

@michaelmcandrew @alifrumin

alifrumin · 2020-01-15T16:30:24Z

Great @mattwire I am happy to test when you are ready.

michaelmcandrew · 2020-01-24T08:22:08Z

api/v3/Job.php

@@ -470,7 +470,14 @@ function civicrm_api3_job_process_membership($params) {
    return civicrm_api3_create_error('Could not acquire lock, another Membership Processing process is running');
  }

-  $result = CRM_Member_BAO_Membership::updateAllMembershipStatus();
+  // We need to pass this through as a simple array of membership status IDs as values.


If the purpose of this code is to accept variations in peoples preferences for passing parameters, I'm not sure I would be so forgiving. Instead I would expect people to pass in parameters in the correct format.

@mattwire I agree with @michaelmcandrew on this - but the presence of many tests was pushing me to merge it, even though this chuck of code doesn't really make sense to me

However, I am concerned that we don't declare a type on exclude_membership_status_ids in the metadata and we are not validating it at the api layer & the validation at the next layer is a bit unclear too.

I think that if it were declare as a INT with a pseudoconstant then an array would be accepted but it would be validated

@eileenmcnaughton @michaelmcandrew I've added the metadata for the exclude_membership_status_ids parameter - it's now validating as an INT but I don't get a pseudoconstant selector in the API3 UI.

CRM/Member/BAO/Membership.php

michaelmcandrew · 2020-01-24T08:44:49Z

Have added a few comments. Happy to comment some more. Would be easier to review if the status and types stuff was separated.

michaelmcandrew · 2020-03-24T12:54:57Z

@elcapo - want to take a look and comment?

elcapo · 2020-03-24T13:15:07Z

@elcapo - want to take a look and comment?

Nothing to add. Looks great to me.

Currently having a problem with loads of "Expired" memberships that MembershipStatus.calc properly identifies as "Current" but where I can't use the process_membership job to fix them. With this draft, that would be solved, as I'd be able to specify the membership statuses to be excluded.

Thanks @mattwire!

michaelmcandrew · 2020-04-22T08:53:21Z

bump @mattwire - do you know what else we need to do to get this merged?

mattwire · 2020-04-22T10:08:07Z

@michaelmcandrew thanks - I've just rebased and moved the other bit (disabled memberships) to #17143. Are you / @elcapo able to write some unit tests to cover the new parameters? Then this should be ready to be merged. I've just added a note on which tests could be extended to the description of this PR.

michaelmcandrew · 2020-04-22T10:55:16Z

thanks @mattwire. yes we can do that - @elcapo could you write some tests please? :)

elcapo · 2020-04-22T12:02:09Z

thanks @mattwire. yes we can do that - @elcapo could you write some tests please? :)

Will do! Just let me find some time these days between upgrades.

elcapo · 2020-04-23T16:34:52Z

@mattwire I've prepared a test suite for the new parameters. See commit 0205a6cb in my membership_disabled branch. I couldn't see how to make a pull request to your branch, so let me know how you want me to send you the changes.

Also, some tests were failing and I wasn't sure if it was because of the tests, or because of the parameter handling, so I created a second branch, membership_disabled_fixes, that incorporates both the tests and the fixes that made the code pass all the new tests.

elcapo · 2020-04-23T16:46:22Z

@mattwire I've prepared a test suite for the new parameters. See commit 0205a6cb in my membership_disabled branch. I couldn't see how to make a pull request to your branch, so let me know how you want me to send you the changes.

Right after sending my message, I've realized I had a few asserts commented in the last test, so please consider also this additional commit 20d11c61.

elcapo · 2020-04-25T00:02:57Z

Thanks @elcapo I've pulled your commits into this PR and fixed the style warnings. It looks like two of the tests are failing - any ideas?

Did you check out my fixes to make the tests pass? I changed a few things like checking if variables were defined in a different way, as the way they were passed as FALSE, sometimes they were ignored and assumed TRUE. Check this https://github.com/elcapo/civicrm-core/commit/7b10697389e0e5b0439eff2e9e89b228308b9b1f and let me know your thoughts about it.

mattwire · 2020-04-29T12:40:39Z

Did you check out my fixes to make the tests pass?

@elcapo I didn't - but now I have and it's still failing on 3 tests - any ideas?

elcapo · 2020-04-30T12:10:35Z

@elcapo I didn't - but now I have and it's still failing on 3 tests - any ideas?

I've just updated the tests so they are a bit easier to use. See https://github.com/elcapo/civicrm-core/commit/68dd0d85d2ecc8d8dfc88dfea5ce550ddbf2b8ad.

I was making several asserts per test and now I'm only making one, so we've now 20 tests that are showing 2 failures.

Include Test Memberships

When using this call:

civicrm_api3('job', 'process_membership', [
  'exclude_test_memberships' => FALSE,
]);

the membership created in createTestMembershipThatShouldBeCurrent should be included in the review process (therefore updated to current) but it is still being excluded.

See JobProcessMembershipTest.php#L314.

Including Memberships of Inactive Membership Types

When using this call:

$this->callAPISuccess('job', 'process_membership', [
  'only_active_membership_types' => FALSE,
]);

the membership created in createOldMembershipThatShouldBeCurrent should be included in the review process (therefore updated to current) but it is still being excluded.

See JobProcessMembershipTest.php#L373.

elcapo · 2020-05-12T13:13:53Z

Hi @mattwire,

Is there any problem with the changes I can still help with? I've checked Jenkins and apparently there are some minor style issues:

Let me know if you want me to fix them.

mattwire · 2020-05-12T14:09:17Z

@elcapo Yes please can you fix the style issues

elcapo · 2020-05-15T18:25:34Z

@mattwire Please, test again with the file as here: JobProcessMembershipTest.php.

There are this two changes:

I tested the file with civilint and it appears to be fine now.

mattwire · 2020-06-03T09:11:48Z

@elcapo I'm still getting test failures on this:

api_v3_JobProcessMembershipTest.testIncludingTestMembershipsActuallyIncludesThem
api_v3_JobProcessMembershipTest.testIncludingInactiveMembershipTypesConsidersInactive

Any ideas? I squashed the commits but should have everything from your branch?

elcapo · 2020-06-16T11:35:35Z

Any ideas? I squashed the commits but should have everything from your branch?

@mattwire Your last commit fixes the issue, right? At least the one of our two tests failing. I've just executed the new tests with your branch and they all worked fine.

Apparently the issue is now with standard membership tests...

mattwire · 2020-06-16T11:46:44Z

@elcapo Let's see if the tests pass now

eileenmcnaughton · 2020-07-14T04:18:32Z

This has good test cover & most review has been responded to - there is just one comment by @michaelmcandrew that I think might need more response.

If you were opening this today I think we would insist it was apiv4 - @colemanw that raises the same 'how would it look question' as another similar job api one today

mattwire · 2020-07-15T11:16:47Z

@eileenmcnaughton @colemanw The issue with scheduled jobs and API4 is that we can't run API4 scheduled jobs yet because the scheduled jobs manager only works with API3 - if we could remove that blocker then I'd start writing a lot more in API4!

mattwire · 2020-07-15T11:32:26Z

@eileenmcnaughton Have responded - pseudoconstant does not load in API3 UI but I've added it anyway. And now validating as INT

eileenmcnaughton · 2020-07-17T01:17:39Z

test this please

eileenmcnaughton · 2020-07-17T01:20:39Z

OK sounds good - I've removed the needs-documentation flag as I see that as optional now as I really want to pivot to a v4 focus. To be clear I don't want to discourage you - but it's less compulsory now as we try to get away from apiv3 I think

freeform-sg · 2022-09-30T18:33:47Z

Hi there, thank you so much for this. I came upon this wondering why my membership statuses weren't updating.

After testing this out, I have a couple of comments:

the documentation does not mention that parameters are now available for this job
the parameter "exclude_membership_status_ids" implies by the name that it accepts an array (plural ids) but only accepts a single integer

I see you wanted to focus on api4 - is there somewhere else I can monitor the progression of this? I was having trouble finding anything related to this job.

Thanks!

civibot bot added the master label Jan 15, 2020

mattwire added needs-documentation needs-test labels Jan 15, 2020

mattwire force-pushed the membership_disabled branch 2 times, most recently from b5b281f to 044b4af Compare January 15, 2020 15:43

michaelmcandrew reviewed Jan 24, 2020

View reviewed changes

CRM/Member/BAO/Membership.php Show resolved Hide resolved

michaelmcandrew reviewed Jan 24, 2020

View reviewed changes

CRM/Member/BAO/Membership.php Show resolved Hide resolved

mattwire force-pushed the membership_disabled branch 2 times, most recently from 5408dad to 31c3b81 Compare February 2, 2020 17:52

mattwire changed the title ~~Membership disabled~~ Enhance parameters for Job.process_membership Apr 5, 2020

mattwire marked this pull request as ready for review April 5, 2020 18:47

mattwire force-pushed the membership_disabled branch from 31c3b81 to be03a7f Compare April 5, 2020 20:55

mattwire force-pushed the membership_disabled branch from be03a7f to 7dfc5c4 Compare April 22, 2020 09:57

mattwire changed the title ~~Enhance parameters for Job.process_membership~~ dev/membership#18 Enhance parameters for Job.process_membership Apr 22, 2020

mattwire force-pushed the membership_disabled branch 3 times, most recently from aa56310 to b8a3bb6 Compare April 24, 2020 11:55

mattwire force-pushed the membership_disabled branch from 5822f24 to 68cef92 Compare May 7, 2020 09:15

eileenmcnaughton added has-test and removed needs-test labels May 14, 2020

mattwire force-pushed the membership_disabled branch from 505c7c0 to 666a686 Compare June 1, 2020 17:16

mattwire force-pushed the membership_disabled branch from e463485 to 6513c2e Compare June 26, 2020 16:11

civicrm deleted a comment from seamuslee001 Jul 6, 2020

mattwire and others added 4 commits July 15, 2020 12:18

Add parameters to process_membership job

ac4d4d8

Test the new Job.process.membership parameters

57aa6fb

Tests should call API not updateAllMembershipStatus() function directly.

90292a0

Add api3 metadata to validate exclude_membership_status_ids

8a9500e

mattwire force-pushed the membership_disabled branch from 6513c2e to 8a9500e Compare July 15, 2020 11:27

eileenmcnaughton added merge on pass and removed needs-documentation labels Jul 17, 2020

eileenmcnaughton merged commit 7151204 into civicrm:master Jul 17, 2020

mattwire deleted the membership_disabled branch August 19, 2020 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/membership#18 Enhance parameters for Job.process_membership #16298

dev/membership#18 Enhance parameters for Job.process_membership #16298

mattwire commented Jan 15, 2020 •

edited

Loading

civibot bot commented Jan 15, 2020

mattwire commented Jan 15, 2020

alifrumin commented Jan 15, 2020

michaelmcandrew Jan 24, 2020

eileenmcnaughton Jul 14, 2020

mattwire Jul 15, 2020

michaelmcandrew commented Jan 24, 2020

michaelmcandrew commented Mar 24, 2020

elcapo commented Mar 24, 2020

michaelmcandrew commented Apr 22, 2020

mattwire commented Apr 22, 2020

michaelmcandrew commented Apr 22, 2020 •

edited

Loading

elcapo commented Apr 22, 2020

elcapo commented Apr 23, 2020 •

edited

Loading

elcapo commented Apr 23, 2020

elcapo commented Apr 25, 2020

mattwire commented Apr 29, 2020 •

edited

Loading

elcapo commented Apr 30, 2020

elcapo commented May 12, 2020

mattwire commented May 12, 2020

elcapo commented May 15, 2020

mattwire commented Jun 3, 2020

elcapo commented Jun 16, 2020

mattwire commented Jun 16, 2020

eileenmcnaughton commented Jul 14, 2020

mattwire commented Jul 15, 2020

mattwire commented Jul 15, 2020

eileenmcnaughton commented Jul 17, 2020

eileenmcnaughton commented Jul 17, 2020

freeform-sg commented Sep 30, 2022

dev/membership#18 Enhance parameters for Job.process_membership #16298

dev/membership#18 Enhance parameters for Job.process_membership #16298

Conversation

mattwire commented Jan 15, 2020 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jan 15, 2020

mattwire commented Jan 15, 2020

alifrumin commented Jan 15, 2020

michaelmcandrew Jan 24, 2020

Choose a reason for hiding this comment

eileenmcnaughton Jul 14, 2020

Choose a reason for hiding this comment

mattwire Jul 15, 2020

Choose a reason for hiding this comment

michaelmcandrew commented Jan 24, 2020

michaelmcandrew commented Mar 24, 2020

elcapo commented Mar 24, 2020

michaelmcandrew commented Apr 22, 2020

mattwire commented Apr 22, 2020

michaelmcandrew commented Apr 22, 2020 • edited Loading

elcapo commented Apr 22, 2020

elcapo commented Apr 23, 2020 • edited Loading

elcapo commented Apr 23, 2020

elcapo commented Apr 25, 2020

mattwire commented Apr 29, 2020 • edited Loading

elcapo commented Apr 30, 2020

Include Test Memberships

Including Memberships of Inactive Membership Types

elcapo commented May 12, 2020

mattwire commented May 12, 2020

elcapo commented May 15, 2020

mattwire commented Jun 3, 2020

elcapo commented Jun 16, 2020

mattwire commented Jun 16, 2020

eileenmcnaughton commented Jul 14, 2020

mattwire commented Jul 15, 2020

mattwire commented Jul 15, 2020

eileenmcnaughton commented Jul 17, 2020

eileenmcnaughton commented Jul 17, 2020

freeform-sg commented Sep 30, 2022

mattwire commented Jan 15, 2020 •

edited

Loading

michaelmcandrew commented Apr 22, 2020 •

edited

Loading

elcapo commented Apr 23, 2020 •

edited

Loading

mattwire commented Apr 29, 2020 •

edited

Loading