[Membership-backoffice] Reduce reliance on multiple specific but confusing class variables #14919
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
civicrm#14888 seeks to add auto_renew as a parameter on buildMembershipTypeValues but on digging the results of that are not used much & could be used less. It's only because we store them far away from where we use them that the relative silliness is kinda hidden. This PR removes one of the reliances. Note that testSubmitRecurCompleteInstant is a good one for stepping through this
eileenmcnaughton
force-pushed
the
mem_review
branch
from
66c764b to
ec7af55
Compare
eileenmcnaughton
changed the title
|
@mattwire did you look at this ? Good to merge? |
|
@mattwire should we merge or close this - you have changed your PR so it no longer touches this |
|
@mattwire can you check this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
#14888 seeks to add auto_renew as a parameter on buildMembershipTypeValues
but on digging the results of that are not used much & could be used less.
It's only because we store them far away from where we use them that the relative silliness is kinda
hidden.
This PR removes one of the reliances.
Note that testSubmitRecurCompleteInstant is a good one for stepping through this
Before
Use of second class var just for this purpose
After
Re-use of generic var
Technical Details
@mattwire I'm reluctant to add $recurOnly to the function signature here https://github.com/civicrm/civicrm-core/pull/14888/files#diff-f43c8498e32f5b2d68ab27bcd243ca36L1555 beca
because I think we are adding complexity to a function we'd rather deprecate & by-pass. I propose this & a similar one to remove the use of $this->membershipTypeRenewalStatus (which is used once) instead
Comments