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

GL-44 admin price field options on event info page #11923

Merged
merged 1 commit into from
Aug 13, 2018

Conversation

lcdservices
Copy link
Contributor

Overview

Suppress admin visibility price field options in event info page.

Before

Price set field option set to admin visibility were exposed on event info page to unauthenticated users.

After

Admin price field options are suppressed.

@lcdservices
Copy link
Contributor Author

Not sure how we're linking gitlab issues to PRs, so here is the issue: https://lab.civicrm.org/dev/core/issues/44

@seamuslee001
Copy link
Contributor

@lcdservices Brian i notice the code is almost the same as https://github.com/civicrm/civicrm-core/pull/11923/files#diff-ca9ad334affe5bb2bf94f5bc1c9b78f5R133 is this being duplicated or is this not working as its too far up the code line? (sorry Brian wrong link before)

@lcdservices
Copy link
Contributor Author

@seamuslee001 visibility can be set at both the field and option level. The code you linked to addresses it at the field level. It was not being handled at the option level. The code added is similar but addresses the option value loop.

@seamuslee001
Copy link
Contributor

@lcdservices ah ok, have you checked if contribution pages are correctly dealing with both elements the option and the field level?

@lcdservices
Copy link
Contributor Author

@seamuslee001 This issue only addresses events. I did not investigate contribution pages. The event registration form was already being handled properly -- only the event info page was incorrect.

@eileenmcnaughton
Copy link
Contributor

@yashodha any interest in reviewing this (or @mattwire )

@magnolia61
Copy link
Contributor

Since disclosing info we don't want to disclose (discounts for special groups, etc), adding a test would be very wise, would you be able to integrate that in the current tests? (Thinking of this also regarding non-public custom fields, will open up an issue for that)

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit seems a little related to one you raised - perhaps you could review this? I think it would be a hard one to write a test for so despite agreeing the benefit I think we could let that slip

@mattwire
Copy link
Contributor

  • (r-explain) Pass
  • (r-test) N/A (Pass) - only affects an event page and resolves an inconsistency in display
  • (r-code) Pass
  • (r-doc) Pass
  • (r-maint) Pass - UI is consistent with options in backend.
  • (r-run) Pass - works as expected
  • (r-user) Pass - this removes an inconsistency in the UI
  • (r-tech) Pass

This seems pretty trivial and only affects the display of a page. @seamuslee001 @eileenmcnaughton I think we should merge this.

@eileenmcnaughton
Copy link
Contributor

Thanks @mattwire

@eileenmcnaughton eileenmcnaughton merged commit 7432a41 into civicrm:master Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants