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.

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

Fix regression whereby membership does not submit #26170

Merged
merged 3 commits into from
May 8, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix regression whereby membership does not submit

Before

Per https://lab.civicrm.org/dev/core/-/issues/4272#note_90443 on 5.61.0 it is not possible to submit a non-quick config membership form as the rule requiring the 'selectMembership' field to be set - however, it is actually never present on the form, having been superceded by the price set fields

After

The code that expects it is removed

Technical Details

I went back to 5.60 & tested with

  • price set with membership
  • normal quick config with membership
  • quick config with membership and contribution
  • quick config with membership and contribution with separate payments

In all these cases $membershipPriceset was TRUE and the lines in question were never hit - hence the fix is to remove them

Comments

There was a further possible issue mentioned in chat that I need to check - as a follow up. Also some follow up cleanup is implied by this patch as it implies more bits of code are unreachable

@civibot
Copy link

civibot bot commented May 8, 2023

(Standard links)

@civibot civibot bot added the 5.62 label May 8, 2023
@eileenmcnaughton
Copy link
Contributor Author

I just pushed in a second commit for the renewal message part

@composerjk
Copy link
Contributor

Preliminary testing properly shows the Renew or Upgrade message and the form was able to submit.

@composerjk
Copy link
Contributor

composerjk commented May 8, 2023

I did just notice in that the automatic CiviDiscount we're applying for renewals did not get applied. The description in the email showed it, but the amount was without the discount. Also, on the member contribution page, it mentioned the discount in the label but the amount is not updated.

Expect to see something like this on the membership contribution form (from CiviCRM 5.60.0):
CiviCRM 5.60.0

But, seeing the following incorrect detail in CiviCRM 5.61.1 + this PR:
CiviCRM 5.61.1 + PR

So, there might still be an issue to fix somewhere.

@composerjk
Copy link
Contributor

The automatic discount with c3f9fce applied does show up when the membership contribution is a renewal.

But, it does not seem to show for an anonymous/guest user for a new membership, though the CiviDiscount automatic has both New and Pending included for membership state.

@composerjk
Copy link
Contributor

Just rolled back to check in 5.60.0 and the CiviDiscount automatic discount does not show for anonymous/guest for a new membership, either. So, perhaps that part is not an issue, here.

It did at least show up properly for the renewal and how we use CiviDiscount.

@totten
Copy link
Member

totten commented May 8, 2023

Thanks for the careful testing, @composerjk!

Merging since this appears to fix the immediate regression and the tests pass.

@totten totten merged commit df78650 into civicrm:5.62 May 8, 2023
@totten totten deleted the 562_member branch May 8, 2023 20:29
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request May 10, 2023
The work for civicrm#26170 concluded that membershipPriceset is always TRUE
at this point. Since we were pushing out a regression fix I left the
variable where it didn't seem to be causing regressions.

However, I think clarifying the variable and
removing the redundant if-else in the rc will set us up well as
1) if there is any further regression we haven't found in
5.61 then people will be able to check on the rc / master
2) if we need to do any further fixes / back-ports the
code will be more synced if we do this in the rc

Note there is some further clean up I feel should be done
folloiwng what we learnt dealing with civicrm#26170 - but
I want to let the dust settle so that I'm not creating a
hard-to-backport situation
@eileenmcnaughton
Copy link
Contributor Author

Note I just put up #26193 because the solution to this was understanding that
'In all these cases $membershipPriceset was TRUE'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants