dev/membership#4 - Admin Membership type is displayed on Public contr…

[jitendrapurohit]

…ibution page

Overview

Admin membership type displayed on public contribution page.

Before

When visibility of membership type is updated to Admin, it is still displayed on front-end contribution pages. Steps to replicate on dmaster -

• Create a membership type.
• Add it to a contribution page.
• Set visibility of the type to Admin.
• Navigate to contribution page as a non-admin user- the membership type is still displayed on the page.

After

Fixed.

Technical Details

Update done to membership type wasn't triggering the change in pricefieldvalue table.

Comments

Added unit test.

---

Gitlab Issue - https://lab.civicrm.org/dev/membership/issues/4

[seamuslee001]

Jenkins re test this please

[totten]

jenkins, test this please

[colemanw]

test this please

[jitendrapurohit]

seems it fails due to the below error - https://test.civicrm.org/job/CiviCRM-Core-PR/20801/console

PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000] [2003] Can't connect to MySQL server on '127.0.0.1' (111)' in phar:///home/jenkins/buildkit/bin/amp/src/Amp/Database/Datasource.php:81

[jitendrapurohit]

jenkins, retest this please

[mattwire]

@jitendrapurohit With your patch applied this is what I'm seeing:

1. Create a membership type.
2. Add it to a contribution page.
3. Set visibility of the type to Admin.
4. Membership type is not displayed on contribution page settings.
5. Navigate to contribution page - the membership type is still displayed on the page.

Is there something I'm missing?

[jitendrapurohit]

@mattwire The steps are correct. Note that, the last step is performed as a non-admin user. Price Field value with Admin visibility is displayed for authenticated users - https://github.com/civicrm/civicrm-core/blob/master/CRM/Price/BAO/PriceSet.php#L1035-L1038

(Agree that I should have mentioned this in the PR description)

Thanks.

[mattwire]

@jitendrapurohit Ok. That's not quite how I expected it to behave, though probably correct. Is it specifically the "Administer CiviCRM" permission that is required or another permission? Could we update the help text on the config page to clarify. (Currently it reads: Is this membership type available for self-service signups ('Public') or assigned by CiviCRM 'staff' users only ('Admin'))

[jitendrapurohit]

From the current flow of the code, it seems edit contributions is required permission to view admin price field values.

[mattwire]

Ok, then I think this message is clearer:
Can this membership type be used for self-service signups ('Public'), or is it only for CiviCRM users with the 'Edit Contributions' permission ('Admin').

[jitendrapurohit]

@mattwire Thanks for the review and suggesstion. The help text is now modified as per the above comment. Can we have this merged now?

[eileenmcnaughton]

I feel like I'd like some more opinions on this - or preferably someone else to kick the UI tires - @agh1 @lcdservices @MegaphoneJon @seanmadsen - any of you up to give a second reviewer eye on this?

[lcdservices]

@jitendrapurohit I ran through some tests but didn't get expected results. I tried a few different scenarios:

• created an admin membership type. when creating a contrib page, that type was not available for selection on the membership tab (behavior present before/after patch applied)
• created a contrib page and added a public mem type. then changed the type to admin. type remained visible on page before/after patch (it should have been hidden)

I think your patch was intended to address the second scenario, but in my testing, doesn't seem to do that. am I missing something?

[jitendrapurohit]

Note the above comment #12178 (comment) for the second scenario, i.e, Admin visibility is actually shown to users having edit contribution permission. This is not something I added to the patch but it was the default behaviour before this changes.

[mattwire]

@lcdservices

created a contrib page and added a public mem type. then changed the type to admin. type remained visible on page before/after patch (it should have been hidden)

This caught me out at first but that's how it's meant to behave apparently and @jitendrapurohit is not changing that behaviour - but it should be clear when configuring the membership type now that the description has been updated to clarify the requirement for edit contributions permission?

[lcdservices]

then it's not clear to me what the purpose of this patch is. when visiting the contrib page I was accessing anonymously, so I did not have the edit contrib perm. I understand that for admin users (i.e. those with that perm) the admin types will still display -- I don't have a problem with that. but for those without that permisson it should be hidden.

[mattwire]

then it's not clear to me what the purpose of this patch is. when visiting the contrib page I was accessing anonymously, so I did not have the edit contrib perm. I understand that for admin users (i.e. those with that perm) the admin types will still display -- I don't have a problem with that. but for those without that permisson it should be hidden.
@lcdservices Your description is correct, but I'm sure that worked for me when I tested it - @jitendrapurohit can you check?

[jitendrapurohit]

@lcdservices @mattwire I think I know what might be the issue with the above testing.

When the visibility of membership type is updated to Admin - the patch aims to also change the visibility of the price field value along with the membership type.

Hence, make sure the membership type is updated after applying the patch. So the complete list of testing the patch would be -

• Create membership type of visibility = public.
• Add it to a contribution page.
• Update visibility of the type to Admin.
• Navigate to contribution page as a non-admin user- the membership type is still displayed on the page.

Apply the patch and repeat the last 2 step. The membership type will not be displayed on the contribution page. Hope that helps.

[lcdservices]

@jitendrapurohit I ran through another round of tests per your update and it worked as expected. And I agree with the behavior.

[eileenmcnaughton]

merging per @lcdservices review