get membership type ID and number of terms from price_x fields

[MegaphoneJon]

https://lab.civicrm.org/dev/membership/-/issues/41
Guest starring: https://issues.civicrm.org/jira/browse/CRM-21177

Overview

When multiple auto-renew memberships are available for selection on a contribution page, the recurring contribution will take the frequency unit and frequency interval from the first membership option, regardless of which is chosen.

Before

Wrong recurring contribution settings.

After

Correct recurring contribution settings.

Technical Details

I started a more limited fix, but saw the @todo to stop using the selectMembership param, so I incorporated that. This actually reduced LOC.

getRecurDetails is removed because it's I removed the only reference to it. It also can't return the correct value except by accident. It tries to deduce the values with a price set ID, which is impossible - you need the price field value ID.

Comments

Once I fixed membership#41, I realized that CRM-21177 was an almost identical issue (frequency interval vs. frequency unit). This also meant a unit test existed, though I needed to extend it to handle frequency unit testing.

This will break the unit tests in CRM_Contribute_Form_Contribution_MainTest. They pass in the SelectMembership the @todo said to not rely on. If we agree that this is the correct approach, I'll modify those tests to pass a price_x field and a price_field_id instead.

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

[seamuslee001]

I think this is correct @agh1 @mattwire @monishdeb throughs?

[eileenmcnaughton]

Quite a few fails - eg

api_v3_ContributionPageTest::testSubmit
Failure in api call for ContributionPage submit: Undefined array key "membership_type_id"
#0 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/CRM/Contribute/Form/ContributionBase.php(1151): PHPUnit\Util\ErrorHandler->__invoke(2, 'Undefined array...', '/home/jenkins/b...', 1151)
#1 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/CRM/Contribute/Form/Contribution/Confirm.php(2048): CRM_Contribute_Form_ContributionBase->setRecurringMembershipParams()
#2 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/api/v3/ContributionPage.php(87): CRM_Contribute_Form_Contribution_Confirm::submit(Array)
#3 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/Civi/API/Provider/MagicFunctionProvider.php(89): civicrm_api3_contribution_page_submit(Array)
#4 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/Civi/API/Kernel.php(149): Civi\API\Provider\MagicFunctionProvider->invoke(Array)
#5 /home/jenkins/bknix-dfl/build/core-22825-7kcea/web/sites/all/modules/civicrm/Civi/API/Kernel.php(81): Civi\API\Kernel->runRequest(Array)

[MegaphoneJon]

OK - the test failures in api_v3_ContributionPageTest were fixed by returning earlier when there's no membership to evaluate.

The failures in CRM_Contribute_Form_Contribution_MainTest are because those tests weren't realistic. We needed to be submitting params to testSubmit() that included a priceSetId, since that value will always be set on a real submission. And, as noted above, we needed to pass the price_x instead of selectMembership because we just eliminated our use of that var.

[BettyDolfing]

Test this please

[jaapjansma]

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
  • User impact (r-user)
    • PASS: The change would be unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We followed the reproduction steps written in the issue and we did it on dmaster and the test environment of this PR. So we could reproduce the problem and we could see that the fix works as expected.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage
  • Test results (r-test)
    • PASS: The test results are all-clear.

@eileenmcnaughton or @seamuslee001 can one of you merge this PR?

[monishdeb]

Done