Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[REF] Simplify code calculating the number of membership terms #19801

Merged
merged 1 commit into from
Mar 24, 2021
Merged

[REF] Simplify code calculating the number of membership terms #19801

merged 1 commit into from
Mar 24, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 14, 2021 •
edited
Loading

Overview

[REF] Simplify code calculating the number of membership terms

Before

Yeah what IS going on in the code

After

Thash better

Technical Details

We have 2 possibilities

  1. there is a price set in use - the number of line items comes from the calculated line items, the field is not present on the form
  2. default priceset in use - it is a submitted value and the field is present on the form

The option to submit is only present in 2 so we know that if it is
submitted it should take precedence, otherwise it comes from the line item

This simplifies the code to do that clearly

Comments

@civibot
Copy link

civibot bot commented Mar 14, 2021

(Standard links)

@civibot civibot bot added the master label Mar 14, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the mem_terms branch 4 times, most recently from 08dcdb9 to 8534621 Compare March 14, 2021 21:06
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
[REF] Simplify code calculating the number of membership terms
e84c5dc 
We have 2 possibilities
1) there is a price set in use - the number of line items comes from the calculated line items
2) default priceset in use - it is a submitted value

The option to submit is only present in 2 so we know that if it is
submitted it should take precedence, otherwise it comes from the line item

This simplifies the code to do that clearly
@eileenmcnaughton
Copy link
Contributor Author

@colemanw this is one I'm keen to get merged (it's on the path to exposing Membership as a v4 api)

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 22, 2021
@eileenmcnaughton
[REF] Simplify a couple of variables
eb5a5da 
Rather than rely on these being passed around retrieve them with
a consistent function.

Note tax_amount is retrieved one other place but that is fixed in
civicrm#19757

Line item will be used less once
civicrm#19801 is merged
@eileenmcnaughton eileenmcnaughton mentioned this pull request Mar 22, 2021
Merged
@eileenmcnaughton
Copy link
Contributor Author

@colemanw @demeritcowboy this one is also touching on the membership mess ....

@demeritcowboy
Copy link
Contributor

I don't really know how to review this properly so I just ran some backend membership forms and step-debugged these spots to see if they were doing anything obviously crazy.

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
  • Developer standards
    • [r-tech] ?
      • I didn't even really try.
    • [r-code] PASS
    • [r-maint] ?
      • I'm not sure I understand the removal of the preprocess calls from the tests? Wouldn't you want to be testing that too? It could ALSO test it without preprocess in a duplicate test to get coverage of both with and without, if there's some reason to test it without.
    • [r-test] PASS

@colemanw colemanw merged commit 218f91c into civicrm:master Mar 24, 2021
@colemanw colemanw deleted the mem_terms branch March 24, 2021 15:17
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy getForm() should run preProcess so it's a duplication

@demeritcowboy
Copy link
Contributor

Ah I see - local getForm vs the "usual" getFormObject. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @demeritcowboy @colemanw