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

Fet/add toggle display copy info option #1555

Merged
merged 6 commits into from
Sep 11, 2019

Conversation

raphaelkross
Copy link
Contributor

@raphaelkross raphaelkross commented Aug 30, 2019

Problem this Pull Request solves

Solves #1020 adding option to toggle the copy attendee info at Registration Form.

How has this been tested

To test that, toggle the option at Dashboard - EventEspresso - Registrations - Registration Form :: "Allow copy #1 attendee info to extra attendees?"

And check if the copy attendee option is working according to the Yes/No set at option, when trying to register in an event w/ more than 1 attendee (purchasing more than one ticket).

Checklist

@raphaelkross
Copy link
Contributor Author

@tn3rb ok - first try adding and using a new option at the Dashboard - waiting your feedback :)

@tn3rb tn3rb self-requested a review August 30, 2019 16:30
Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GREAT JOB!!!

I could totally approve this if it were not for the fact that there are some changes that I would like you to make that are the result of my failing to notify you prior to your working on this of certain changes in code writing that we now follow.

So basically you've done a great job of the following:

  • adding logic to an admin page
  • building a form using our form system
  • capturing the form data and saving it to the config
  • using a config property to change logic flow in the reg form

and the only problem is that I failed to notify you that for any new code we add to existing classes, we want to follow the more modern PSR formatting and styles as opposed to the older styles that were a cross between WordPress styles and our own preferences.

That's totally my bad, so my apologies for not informing you sooner, although it would be pretty overwhelming if I were to bury you under every possible detail you need to know on your first few days and then expect you to remember everything. So we'll just have to work through all of these little gotchas as they come up.

@tn3rb tn3rb removed their assignment Aug 30, 2019
@tn3rb
Copy link
Member

tn3rb commented Sep 10, 2019

looks good.

one FYI: in the future, when you pass a pull request to someone for re-review, you have to click the little circular arrow icon to the right of their name that shows the "Re-request review" label when you hover over it.

So this is good to go, but you need to merge the latest changes from master into this branch and wait for tests to pass before you can forward this to support for QA testing.

This can be done in either of the following two ways:

  • using GitHub's UI and clicking the "Update branch" button below
  • using your local Git client to pull the latest changes from master, merge them into this branch, and then push the updated branch

the most ideal way to do this is to use your local git client and rebase your branch's commits onto master (after pulling the latest changes) and then force push the changes. This essentially replays your commits onto master and results in a much cleaner git history. Don't do this if you are unfamiliar with rebasing and NEVER do this with a branch that has already been shared (such as master branch) as it rewrites the history for the branch, which is okay to do with a new PR (pull request) like this one. We always use Squash and Merge when bringing PRs into master so ultimately this doesn't really matter.

So here's your next steps with this PR:

  • update the branch as explained above and wait for automated tests to pass
  • verify that you have adequate testing notes written and that they do not require any changes
  • change the pipeline for the PR to "Q/A Testing"
  • remove the status:review label and add the status:needs-testing label
  • assign to Josh for testing triage

@tn3rb tn3rb removed their assignment Sep 10, 2019
@joshfeck
Copy link
Contributor

Works great! This will be much better than telling people to load a blank template or hide the box with CSS.

@joshfeck joshfeck removed their assignment Sep 11, 2019
@joshfeck joshfeck added this to the 4.10.1.p milestone Sep 11, 2019
@joshfeck joshfeck merged commit 8ee5b50 into master Sep 11, 2019
@joshfeck joshfeck deleted the FET/add-toggle-display-copy-info-option branch September 11, 2019 16:54
eeteamcodebase pushed a commit that referenced this pull request Sep 11, 2019
…oggle display copy info option (#1555)

* Add option to admin Reg Form

* Toggle the print copy info according to Reg Config

* Fix extra spacing

* Fix PSR standard
"
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.

3 participants