Skip to content

Add automated birthday greeting emails - #9329

Open
eliassanchez173 wants to merge 4 commits into
ChurchCRM:masterfrom
eliassanchez173:feature/birthday-emails-8979
Open

Add automated birthday greeting emails#9329
eliassanchez173 wants to merge 4 commits into
ChurchCRM:masterfrom
eliassanchez173:feature/birthday-emails-8979

Conversation

@eliassanchez173

Copy link
Copy Markdown

What Changed

Adds automated birthday greeting emails for people whose birthday is today.

This introduces a BirthdayEmail notification, a scheduled birthday email service, an opt-in system setting, duplicate-send protection, and an admin test-send action from the settings UI.

Fixes #8979

Type

  • ✨ Feature
  • 🐛 Bug fix
  • ♻️ Refactor
  • 🏗️ Build/Infrastructure
  • 🔒 Security

Testing

Tested manually with the Docker dev stack and Mailpit:

  • Enabled bEnableBirthdayEmails
  • Created a test person with today’s birthday and an email address
  • Ran the timer job and confirmed the birthday email was delivered in Mailpit
  • Ran the timer job a second time and confirmed no duplicate email was sent
  • Disabled bEnableBirthdayEmails and confirmed no birthday emails were sent
  • Used the admin settings test-send button and confirmed the test birthday email was delivered
  • Confirmed people without an email address are skipped

Note: #8979 asks for unit test coverage, but I could not find an existing PHPUnit setup in the repo. I’m happy to add Cypress coverage, introduce minimal PHPUnit infrastructure, or adjust based on maintainer preference.

Screenshots

Pre-Merge

  • Tested locally
  • No new warnings
  • Build passes
  • Backward compatible (or migration documented)

- BirthdayEmail class extending BaseEmail
- bEnableBirthdayEmails config flag (default off)
- BirthdayEmailService::run() finds today's birthdays and sends greetings
- Idempotency guard via sLastBirthdayEmailRunDate
- Wired into SystemService::runTimerJobs()

Tested locally end-to-end. Unit test coverage and admin preview UI
still pending per discussion on ChurchCRM#8979.
- New POST /admin/api/admin/birthday-emails/test route
- Sends a real BirthdayEmail to the logged-in admin's own address
- Returns a clear error if the admin has no email on file
- Inline 'Send Test Birthday Email to Myself' button under the
  bEnableBirthdayEmails setting in System Settings
- Tested locally: confirmed email lands correctly in Mailpit,
  and the no-email error path triggers correctly

Completes the 'admin can preview/test-send' acceptance criterion
from ChurchCRM#8979. Unit test approach still pending maintainer input.
@eliassanchez173
eliassanchez173 requested a review from a team as a code owner July 28, 2026 20:43
@DawoudIO

Copy link
Copy Markdown
Contributor

🔍 Automated PR review started. Follow the reviewer discussion here: https://agentic-platform.docker.com/shared/83b0e1d5-a1da-4f4b-ae8b-813a2c237c19

@DawoudIO DawoudIO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

Assessment: 🟡 NEEDS ATTENTION

The birthday-email feature is well-structured overall — the service class, email class, admin route, and JS handler all follow existing project conventions. Authorization is correctly inherited from the admin app's global AdminRoleAuthMiddleware. Four issues were found in the new code, two of which need a fix before merge.

Medium (fix before merge)

  1. A spurious </div> in SystemSettings.php will break the System Settings page layout in production.
  2. The deduplication guard (sLastBirthdayEmailRunDate) is only persisted after the full send loop, so a mid-run crash can cause duplicate birthday emails.

Low (clean-up recommended)
3. bEnabledEmail is duplicated in the buildConfigs() array — copy-paste error, no runtime impact.
4. Ordinal suffixes (st, nd, rd, th) in the email body are hardcoded English and cannot be translated for non-English locales.

Comment thread src/SystemSettings.php Outdated
</div>
</div>

</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

[MEDIUM] Spurious </div> prematurely closes the Tabler container-xl layout wrapper

Header.php opens two divs around every page's body — <div class="page-body"> followed by <div class="container-xl"> — and Footer.php closes them in reverse order. The JSON Settings Modal immediately above this line is already self-contained (all four of its open tags have matching close tags). The </div> added here has no matching open tag in SystemSettings.php, so it closes container-xl prematurely.

Impact: the entire settings <form> (and everything inside it) renders outside container-xl, breaking the Tabler grid layout and making the System Settings admin page visually corrupt in production. Footer.php then tries to close container-xl and page-body, but container-xl is already gone, cascading mismatched div closures across the footer region.

Fix: remove this </div> line.

}
}

SystemConfig::setValue('sLastBirthdayEmailRunDate', $todayString);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

[MEDIUM] sLastBirthdayEmailRunDate is written only after the full send loop — a crash mid-loop causes duplicate birthday emails

The deduplication guard checks whether today's date is already stored in sLastBirthdayEmailRunDate. But SystemConfig::setValue('sLastBirthdayEmailRunDate', $todayString) is called only here, after the foreach loop completes. If the PHP process is killed between the loop start and this line (OOM, request timeout, server restart, or a fatal error that escapes the inner catch), the date is never persisted.

Impact: the next timer tick the same day finds no stored date, re-runs the full loop, and every person already emailed receives a duplicate birthday greeting.

Fix: write sLastBirthdayEmailRunDate before the loop begins. The existing guard at the top of the method already prevents a second run on the same day, so moving the setValue call earlier makes the service crash-safe without re-introducing duplicates:

// Persist before the loop so a mid-run crash cannot cause duplicate sends
SystemConfig::setValue('sLastBirthdayEmailRunDate', $todayString);

foreach ($people as $person) {
    // ... send logic unchanged
}

Comment thread src/ChurchCRM/dto/SystemConfig.php Outdated
'bEnabledEvents' => new ConfigItem('bEnabledEvents', 'boolean', '1', gettext('Show or hide the Events section in the main navigation menu')),
'bEnabledFundraiser' => new ConfigItem('bEnabledFundraiser', 'boolean', '1', gettext('Enable Fundraiser menu.')),
'bEnabledEmail' => new ConfigItem('bEnabledEmail', 'boolean', '1', gettext('Enable email sending from ChurchCRM. Required for password reset, notifications, and email links.')),
'bEnabledEmail' => new ConfigItem('bEnabledEmail', 'boolean', '1', gettext('Enable email sending from ChurchCRM. Required for password reset, notifications, and email links.')),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

[LOW] Duplicate bEnabledEmail array key — copy-paste dead code

This line is byte-for-byte identical to the bEnabledEmail entry already present on the line immediately above. In PHP, duplicate associative-array keys silently drop the first entry and keep the last; since both ConfigItem objects are constructed with the same arguments there is no observable runtime difference — but the first entry is unreachable dead code.

This appears to be a copy-paste error: the intent was to add the two new keys (bEnableBirthdayEmails and sLastBirthdayEmailRunDate) after the existing bEnabledEmail line, but the existing line was accidentally duplicated.

Fix: remove this duplicate line.

return array_merge($this->getCommonTokens(), $myTokens);
}

private function getOrdinalSuffix(int $number): string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

[LOW] Ordinal suffixes are hardcoded English — cannot be translated for non-English locales

getOrdinalSuffix() always returns one of 'st', 'nd', 'rd', 'th'. This value is substituted as the %s argument in sprintf(gettext('Wishing you a wonderful %d%s birthday!'), $age, ...). While the surrounding format string is wrapped in gettext() and can be translated, the %s slot is always filled by the hardcoded English suffix — translators cannot supply a locale-appropriate ordinal indicator.

Churches using French ("ème"), German (trailing period), Spanish (gender-dependent), or other non-English locales will receive grammatically wrong or meaningless birthday text.

Suggested fix: remove the ordinal from the translatable string and pass the full age as a plain integer, for example:

// Before (broken for i18n):
$body .= sprintf(gettext('Wishing you a wonderful %d%s birthday!'), $age, $this->getOrdinalSuffix($age));

// After (translators control the full sentence):
$body .= sprintf(gettext('Wishing you a wonderful %d year birthday!'), $age);

Or simply omit the age from this sentence entirely and let gettext cover the whole thing without a numeric placeholder.

@churchcrm-hazel churchcrm-hazel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Welcome to the ChurchCRM project, @eliassanchez173! 🎉 This is a great first contribution — automated birthday emails is a feature people have wanted for a while, and you have tackled it thoughtfully: opt-in setting, duplicate-send protection, the admin test-send action, and solid manual testing with Mailpit are all the right pieces.

On the unit test question — you are right that PHPUnit infrastructure is minimal. Cypress E2E is the preferred path for new features, so if you want to add a spec covering the opt-in toggle and the test-send button that would be a nice addition, but it is not blocking.

A human review from the maintainer is on the way. In the meantime, feel free to join the community on Discord — the #dev-chat channel is the best place to discuss the implementation or ask questions: https://discord.gg/tuWyFzj3Nj

Thanks for taking the time to build this!

@eliassanchez173

Copy link
Copy Markdown
Author

I took care of the issues
Removed the extra from SystemSettings.php.
Moved sLastBirthdayEmailRunDate persistence to before the send loop, preventing duplicate sends if the process stops mid-run.
Removed the duplicate bEnabledEmail config entry.
I’ll leave the ordinal suffix/i18n improvement as a follow-up unless maintainers would prefer it included in this PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automated birthday emails

3 participants