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

[REF] minor extraction - getInfoUrl #20421

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] minor extraction - getInfoUrl

Before

variable passed around

After

consistent function used

Technical Details

I did a universe search on checkValidEvent & it wasn't called from anywhere else so I made it protected (perhaps private would have been more explicit since this class is inherited)

Comments

@civibot
Copy link

civibot bot commented May 25, 2021

(Standard links)

@civibot civibot bot added the master label May 25, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the ev_confirm branch 2 times, most recently from 67fabee to 092efbf Compare May 25, 2021 20:58
@totten
Copy link
Member

totten commented May 27, 2021

Looks safe to me. For r-run, I did this:

  • Open two browsers (one logged in as admin; one anonymous/public).
  • (Browser 1/Admin) Go to "Configure Event => Online Registration". Set a registration deadline in the future.
  • (Browser 2/Public) Open the event's info page and proceed to registration page. Page looks good.
  • (Browser 1/Admin) Move the registration end-date. Now it's expired.
  • (Browser 2/Public) Reload registration page. Observe that it redirects and shows an error.

So that seems good. 👍

@totten totten merged commit 0c4dde6 into civicrm:master May 27, 2021
@eileenmcnaughton eileenmcnaughton deleted the ev_confirm branch May 27, 2021 07:33
@eileenmcnaughton
Copy link
Contributor Author

thanks @totten - it's a pretty small baby step in tidying up some pretty scary code sadly - but I appreciate your review

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.

2 participants