dev/core#2902 revert apiv3 handling of membership

[eileenmcnaughton]

Overview

This reverts changes that were making api v3 date handling change if skipStatusCalc were set.

Before

APiv3 had stopped calculating dates when the (silly) 'skipStatusCal' parameter was passed in

After

Code in apiv3 layer reverted back to 5.39 code

Technical Details

Comments

@KarinG @agileware-fj @artfulrobot

[civibot]

(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).

[KarinG]

Our newly added Membership Pay Later test -> confirms the regression in the API between 5.39.x and 5.42.x ->

❌ no Join Date for 5.42.x and dev-master
✅ but ok for 5.35.x and 5.39.x

We only test the APIs using Webform CiviCRM. I don't know when the Native Contribution Page UI started no longer adding Membership Dates for Pay Later scenarios [I'm not aware of any of our clients using that] - but if we were to use Membership and Pay Later - I certainly would expect there to be dates.

[eileenmcnaughton]

@KarinG @agileware-justin @agileware-fj is this works we'll put out 5.42.1 tomorrow with it

[agileware-fj]

Test addition and approach looks good to me.

I'm doing some run tests on it at the moment.

[eileenmcnaughton]

thanks @agileware-fj

[agileware-fj]

@eileenmcnaughton All good from the Webform path and clearly works for APIv3 Membership.create per the tests.

Getting some weird results from the Order API, but I highly doubt that's related to this PR.

👍 from me. Redacted, see later comments

[eileenmcnaughton]

thanks @agileware-fj let's get @totten or @colemanw to merge this & we'll port to 5.42.1

Can you please log an issue re 'weird results from the Order API,'

[agileware-fj]

Actually, before merge can we add an assertion for join_date to the tests for completeness? Should be the same as start_date.

[agileware-fj]

Hold up! I'm going to have to put the brakes on this again as on further inspection into the Order API issue I'm reasonably certain it's part of the regression from the same commit.

Essentially, moving skipStatusCal into the BAO class is causing it to have an effect in APIv4, including skipping the dates calculation, which is then not addressed by this PR, because it only applies to APIv3. Overall I think the actual solution is to brand skipStatusCal as a relic of APIv3, move it back there, and never consider it in the BAO actions.

[agileware-fj]

As per dev/core#2904 and #21796 the order API issue is dealt with elsewhere, so I'm again happy for this to go ahead.

[seamuslee001]

ok this seems to have general support merging

[KarinG]

Ok I re-ran our test and can confirm this is now ✅ in wfc + dev-master.

Note to self: update RC in my main.yml 🙂

[eileenmcnaughton]

thanks @KarinG - @agileware-fj found that the core form has been not setting the dates since at least 5.20 - but is of the opinion we should fix (easy fix, remove 'skipStatusCal') - I might ask you to help test if I get round to doing that

[artfulrobot]

I'm trying to catch up on this as I wasn't able to input at the time. I'd like to understand the tests added by this PR better - I'm not sure what cases they're testing for, or why. Both check that a pending membership is created with today as the start end date, and one of them checks the join date too - ? - neither check what happens when a date is passed in, or a status other than pending.

[eileenmcnaughton]

@artfulrobot the tests were just the specific case where passing in 'skipStatusCal' was causing the dates to not calculated. I don't know if there are ohter permutations of weirdness....

[artfulrobot]

@eileenmcnaughton OK, good to know. Ta.

[artfulrobot]

Linking #21892