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.

Sign up for GitHub

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

Jump to bottom

dev/drupal#72 Ensure front end profile title is used in event confirmation screens & emails #14960

Merged
merged 5 commits into from
Aug 7, 2019
Merged

dev/drupal#72 Ensure front end profile title is used in event confirmation screens & emails #14960

merged 5 commits into from
Aug 7, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 4, 2019 •
edited
Loading

Overview

Fixes a bug whereby the front end profile title is not used in event confirm & thankyou screens & confirmation emails

Before

Register ...
Screen Shot 2019-08-05 at 7 00 29 AM

BUT
Screen Shot 2019-08-05 at 7 00 51 AM

Also in confirmation emails

After

Screen Shot 2019-08-05 at 8 03 46 AM

Screen Shot 2019-08-05 at 8 04 10 AM

Technical Details

Incorporates #14958

Comments

https://lab.civicrm.org/dev/drupal/issues/72

@civibot
Copy link

civibot bot commented Aug 4, 2019

(Standard links)

@civibot civibot bot added the master label Aug 4, 2019
@eileenmcnaughton eileenmcnaughton changed the title dev/core#224 Ensure front end profile title is used in event confirmation emails dev/drupal#72 Ensure front end profile title is used in event confirmation emails Aug 4, 2019
@seamuslee001
Copy link
Contributor

@jusfreeman you got any interest in this?

@jusfreeman
Copy link
Contributor

@seamuslee001 yes, thanks

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Aug 6, 2019 •
edited
Loading

I should note I'm not actually personally vested in this - I was looking at test cover in order to build that up before fixing a more complex issue (which probably will require test build up & code cleanup before I could write a patch that I could reasonably expect someone else to review).

Fixing trivial bugs is usually a good way to add test cover & do relevant function extraction - mostly because it's easier to find a reviewer if people get something they want out of it.

I'm going to chip away at event cleanup for a little bit with a goal to fixing the payment processor flow (at the moment multiple participants only works in a bad way) - generally my chip away process is to put up a PR in an area & then once that is merged do another one - so the speed I work at is moderated by reviewer interest.

seamuslee001
seamuslee001 reviewed Aug 7, 2019
View reviewed changes
CRM/Event/BAO/Event.php Outdated
break;
}
if (!empty($fields)) {
$profile = civicrm_api3('UFGroup', 'getsingle', ['id' => $gid, ['return' => ['title', 'frontend_title']]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton any reason not to use CRM_UF_BAO_UFGroup::getFrontEndTitle() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it didn't exist when I wrote those lines....

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would probably be good to tidy this up before merging but otherwise looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok done

@seamuslee001
Copy link
Contributor

(CiviCRM Review Template WORD-1.2)

  • General standards
    • (r-explain) Pass
    • (r-user) Pass i tested this on an event registration and confirm that this fixes it, also confirmed that it changes it for the confirm step as well which makes it all in line
    • (r-doc) Pass
    • (r-run) Pass as per #r-user
  • Developer standards

@seamuslee001
Copy link
Contributor

Added MOP

@jusfreeman
Copy link
Contributor

jusfreeman commented Aug 7, 2019 •
edited
Loading

Happy to test this, but I need some guidance on how to test. Since I could not find any profile titles being output at all in the event registration confirmation emails.

@eileenmcnaughton
Copy link
Contributor Author

in testing I was seeing the custom pre & post profile blocks had titles. Maybe test the rc since that gets cut tomorroe & this has mop

@eileenmcnaughton eileenmcnaughton mentioned this pull request Aug 7, 2019
Closed
@eileenmcnaughton
Copy link
Contributor Author

note this incorporates #14958

also - my money says that contribution pages have this bug too...

@seamuslee001
Copy link
Contributor

@eileenmcnaughton test fail looks related https://test.civicrm.org/job/CiviCRM-Core-PR/28196/testReport/junit/(root)/CRM_Event_Form_Registration_ConfirmTest/testAssignProfiles/

@eileenmcnaughton
Copy link
Contributor Author

Ah yes...

@eileenmcnaughton eileenmcnaughton merged commit e94e024 into civicrm:master Aug 7, 2019
@eileenmcnaughton eileenmcnaughton deleted the event_test branch August 7, 2019 17:49
@eileenmcnaughton eileenmcnaughton changed the title dev/drupal#72 Ensure front end profile title is used in event confirmation emails dev/drupal#72 Ensure front end profile title is used in event confirmation screens & emails Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

Assignees
No one assigned
Labels
has-test master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @seamuslee001 @jusfreeman