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

dev/core#3463 - CiviMail - prefer frontend_title from group for maili… #23570

Merged

Conversation

ufundo
Copy link
Contributor

@ufundo ufundo commented May 24, 2022

…ng.group token

Overview

Implementation for https://lab.civicrm.org/dev/core/-/issues/3463

Before

{mailing.group} token in CiviMail always uses the title field for each of the groups in the mailing.

After

Uses the frontend_title field for any groups in the mailing where this is set.

Technical Details

I was slightly worried about issues with permissions / bootstrapping. I've test sending scheduled mailings with a cron user with the sysadmin-docs-recommended cron permissions of View all contacts, Access CiviCRM, Access CiviMail and seems to work ok :)

@civibot
Copy link

civibot bot commented May 24, 2022

(Standard links)

@civibot civibot bot added the master label May 24, 2022
@ufundo ufundo force-pushed the mailing-group-token-frontend-title branch from 466ac8d to 90d30f1 Compare May 24, 2022 19:48
@eileenmcnaughton
Copy link
Contributor

@ufundo this makes sense to be but you need to check the style warnings https://test.civicrm.org/job/CiviCRM-Core-PR/49060/checkstyle/

@seamuslee001 @agileware-justin

@ufundo ufundo force-pushed the mailing-group-token-frontend-title branch from 90d30f1 to 1a1be8d Compare May 24, 2022 20:16
->addWhere('mailing_id', '=', $this->id)
->addWhere('entity_table', '=', 'civicrm_group')
->addWhere('group_type', '=', 'Include')
->setLimit(25)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this limit 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it in thinking 'if you have more than 25 groups in your mailing, this token is going to be a mess' but yes that was probably not a good thought! Have removed now.

@agileware-justin
Copy link
Contributor

agileware-justin commented May 24, 2022

@ufundo I agree with this enhancement. The 25 limit looks like it snuck in and suggested this should be removed. I have minor PTSD from this option.

setLimit25 PTSD

@ufundo ufundo force-pushed the mailing-group-token-frontend-title branch from 1a1be8d to b9f2127 Compare May 25, 2022 22:01
@eileenmcnaughton
Copy link
Contributor

@agileware-justin - shall I give this merge-on-pass now?

@agileware-justin
Copy link
Contributor

@eileenmcnaughton let me test this on DEV and confirm OK.

@eileenmcnaughton
Copy link
Contributor

Thanks @agileware-justin !

@agileware-justin
Copy link
Contributor

Existing group token output

Internal group title is output for the {mailing.group} token

image

image

@agileware-justin
Copy link
Contributor

agileware-justin commented May 26, 2022

Patch applied, group token output

With the patch applied, public group title is output, if set for the {mailing.group} token. Otherwise will output the group title.

image

image

@agileware-justin
Copy link
Contributor

@eileenmcnaughton that's a pass from me.

@eileenmcnaughton
Copy link
Contributor

@agileware-justin thanks - I was too mesmerised by your screenshots to be able to figure out if they were right or not :-)

Nice to see some legacy code pattern removed too

@eileenmcnaughton eileenmcnaughton merged commit dd9e108 into civicrm:master May 26, 2022
@agileware-justin
Copy link
Contributor

Screenshot updated.

Agileware ref: CIVICRM-1990

@eileenmcnaughton
Copy link
Contributor

lol!

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.

3 participants