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

Clarify isPrimary, isPaidEvent variables #26283

Merged
merged 1 commit into from
May 22, 2023
Merged

Clarify isPrimary, isPaidEvent variables #26283

merged 1 commit into from
May 22, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Clarify isPrimary, isPaidEvent variables

Before

The event_offline receipt has big conditionals around the mystery $isPrimary variable - the meaning of which is unclear

On digging through the code I concluded this was largely copy & paste from the online template & in the offline template it actually means 'event is monetary' - it is assigned to the template when event.is_monetary is true. It is also assigned when in credit card mode - but that is only the case when event.is_monetary is true.

Preview is very limited
image

After

isPrimary assignment is clearly based on event.is_monetary at the form level & within the template is replaces with the token {event.is_monetary|boolean}

This makes MORE (but still not all) of the template preview-able

image

Technical Details

Comments

@civibot
Copy link

civibot bot commented May 20, 2023

(Standard links)

@civibot civibot bot added the master label May 20, 2023
eileenmcnaughton
eileenmcnaughton commented May 20, 2023
View reviewed changes
xml/templates/message_templates/event_offline_receipt_html.tpl
@@ -152,15 +152,14 @@
{if !empty($lineItem)}
{foreach from=$lineItem item=value key=priceset}
{if $value neq 'skip'}
{if !empty($isPrimary)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is within the same criteria in a loop - so can go

@larssandergreen
Copy link
Contributor

I think we can remove isPrimary here as well, because that also sends an event_offline_receipt.

larssandergreen
larssandergreen reviewed May 20, 2023
View reviewed changes
CRM/Event/Form/Participant.php
$this->assignEventDetailsToTpl($params['event_id'], CRM_Utils_Array::value('role_id', $params), CRM_Utils_Array::value('receipt_text', $params), $this->_isPaidEvent);

$this->assignEventDetailsToTpl($params['event_id'], CRM_Utils_Array::value('role_id', $params), CRM_Utils_Array::value('receipt_text', $params));
$this->assign('isPrimary', (int) $this->_isPaidEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I don't think it is used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it would still be present in people's templates if they haven't updated on upgrade - but I think I could comment to that effect

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, good idea

larssandergreen
larssandergreen reviewed May 20, 2023
View reviewed changes
xml/templates/message_templates/event_offline_receipt_html.tpl Outdated
@@ -28,12 +28,12 @@

{if !empty($isOnWaitlist)}
<p>{ts}You have been added to the WAIT LIST for this event.{/ts}</p>
{if !empty($isPrimary)}
{if {event.is_monetary|boolean}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with how the wait list system works, but do participants get registered automatically if space becomes available in the event if the event is non-monetary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larssandergreen yeah I think that must be the case

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the docs and tested. You do get an email with a link that you have to click to move from waitlist to registered, even for a free event. I think this was probably, as you say, copied from the online template, where it was supposed to prevent additional participants from getting emails (it only emails the primary participant to confirm for all participants in the group). Same for approval. So I think we can remove both these conditionals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the investigation - I have pushed up a fix to remove them! Yay for simplification

Copy link
Contributor

Choose a reason for hiding this comment

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

Any message template simplification is a happy occasion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are so right @larssandergreen

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen thanks for the thoughtful review - and in particular picking up the fee selection form - I have made some updates to that form as well now to ensure it can resolve contribution tokens

@eileenmcnaughton eileenmcnaughton force-pushed the is_primary branch 2 times, most recently from 558a85f to 06a2ea7 Compare May 21, 2023 23:27
@eileenmcnaughton
Copy link
Contributor Author

Error doesn't feel caused by this

Found CiviCRM database version 4.6.36.
Found CiviCRM code version 5.63.alpha1.

In UpgradeDbCommand.php line 64:

[Exception]
CiviCRM versions prior to 4.7.31 cannot be upgraded directly to 5.63.alpha1
. This upgrade will need to be done in stages. First download an intermedia
te version (the LTS may be a good choice) and upgrade to that before procee
ding to this version.

Exception trace:
at phar:///home/homer/buildkit/bin/cv/src/Command/UpgradeDbCommand.php:64
Civi\Cv\Command\UpgradeDbCommand->execute() at phar:///home/homer/buildkit/bin/cv/vendor/symfony/console/Command/Command.php:127
Cvphar\Symfony\Component\Console\Command\Command->run() at phar:///home/homer/buildkit/bin/cv/vendor/symfony/console/Application.php:637
Cvphar\Symfony\Component\Console\Application->doRunCommand() at phar:///home/homer/buildkit/bin/cv/vendor/symfony/console/Application.php:190
Cvphar\Symfony\Component\Console\Application->doRun() at phar:///home/homer/buildkit/bin/cv/src/Application.php:66
Civi\Cv\Application->doRun() at phar:///home/homer/buildkit/bin/cv/vendor/symfony/console/Application.php:101
Cvphar\Symfony\Component\Console\Application->run() at phar:///home/homer/buildkit/bin/cv/src/Application.php:32
Civi\Cv\Application::main() at phar:///home/homer/buildkit/bin/cv/bin/cv:28
require() at /home/homer/buildkit/bin/cv:14

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen this is passing now - is it good to merge in your opinion?

@larssandergreen
Copy link
Contributor

I'm giving this an r-run and running into an error related to your recent #26215. When I'm using Register Participants for Event from search actions, I'm getting an error here when saving. Looks like this is because $this->getParticipantID() is NULL from here.

@larssandergreen
Copy link
Contributor

Otherwise, all good.

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen OK- I think I fixed that - this form overloading is crazy

@larssandergreen
Copy link
Contributor

I know, it's nuts.
Everything seems to be working as intended now, so I think this one is good to go.

@eileenmcnaughton
Copy link
Contributor Author

thanks @larssandergreen - I spotted a few notices in the register flow too which I might look at

@colemanw can you merge this?

@seamuslee001
Copy link
Contributor

merging based on Lars's review

@seamuslee001 seamuslee001 merged commit 9c48eb6 into civicrm:master May 22, 2023
@seamuslee001 seamuslee001 deleted the is_primary branch May 22, 2023 21:44
@eileenmcnaughton
Copy link
Contributor Author

thanks @seamuslee001 & thanks @larssandergreen !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larssandergreen larssandergreen larssandergreen left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

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