auto-create recipients only for probands matching the massmail department

[rkrenn]

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Improved mass mail recipient management to enforce department validation. Recipients are now correctly filtered and assigned only to probands whose department matches the mass mail's department, preventing cross-department mail assignments.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce department-matching validation across mass mail recipient handling operations. Three methods now conditionally process mass mail recipients only when the mass mail's department matches the proband's department, filtering out recipients outside their respective departments.

Changes

Cohort / File(s)	Summary
Mass Mail Recipient Department Filtering core/src/main/java/org/phoenixctms/ctsms/service/massmail/MassMailServiceImpl.java	In handlePrepareRecipients, added logic to load or derive the Proband instance and conditionally add new mass mail recipients only when massMail.getDepartment() matches proband.getDepartment().
Trial Service Proband List Initialization core/src/main/java/org/phoenixctms/ctsms/service/trial/TrialServiceImpl.java	In addProbandListEntry, filtered mass mail processing to invoke addResetMassMailRecipient only for mass mails whose department matches the proband's department.
Service Utility Recipient Reset Logic core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java	In addProbandListStatusEntry, made mass mail recipient reset conditional on department equality between mass mail and proband, preventing processing for mismatched departments.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops through departments with glee,
Each mail now finds its true family!
Department by department, we sort and we sift,
Making sure each proband gets the perfect gift! ✉️
Three paths aligned, one filtering rule,
Organization's never been so cool!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: adding a department-matching filter for recipient auto-creation across all three modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch discriminate_visitreminder

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.41.1)

core/src/main/java/org/phoenixctms/ctsms/service/trial/TrialServiceImpl.java

core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

🧹 Nitpick comments (2)

core/src/main/java/org/phoenixctms/ctsms/service/massmail/MassMailServiceImpl.java (1)

831-844: Department-matching logic looks correct for the PR objective.

The implementation correctly filters mass mail recipients by department matching before auto-creation. The conditional structure is sound:

• When listEntry is available, it reuses the already-loaded proband
• When listEntry is null, it loads the proband via DAO

One minor defensive coding consideration: while massMail.getDepartment() should never be null (enforced by checkMassMailInput at line 126), using Objects.equals() would be slightly more defensive against future changes or edge cases where validation might be bypassed.

💡 Optional: Use null-safe comparison

+import java.util.Objects;
 ...
-if (massMail.getDepartment().equals(proband.getDepartment())) {
+if (Objects.equals(massMail.getDepartment(), proband.getDepartment())) {

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/org/phoenixctms/ctsms/service/massmail/MassMailServiceImpl.java`
around lines 831 - 844, The department comparison uses
massMail.getDepartment().equals(proband.getDepartment()) which can NPE if
massMail.getDepartment() ever becomes null; change the check to a null-safe
comparison (e.g., Objects.equals(massMail.getDepartment(),
proband.getDepartment())) where the current conditional is performed (around the
block using listEntry, getProbandDao().load(probandId), and calling
ServiceUtil.addMassMailRecipient) to ensure robust behavior without changing the
surrounding logic.

core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java (1)

323-327: Consider moving the department check into addResetMassMailRecipient(...).

Both current call sites (here and in TrialServiceImpl.java) already guard the call with a department check, which is good. However, enforcing this invariant inside the helper method itself would eliminate the risk of future callers accidentally bypassing it and make the function safer and more self-documenting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java` around lines
323 - 327, Move the department equality guard into
addResetMassMailRecipient(...) so the helper enforces the invariant: inside the
method (addResetMassMailRecipient) check if
massMail.getDepartment().equals(proband.getDepartment()) and return early if
not, performing no side effects; update callers (the loop in ServiceUtil and the
call in TrialServiceImpl) to remove their duplicate department checks and simply
call addResetMassMailRecipient(...) unconditionally; ensure the method signature
and behavior remain the same otherwise and that any logging/journal behavior is
skipped when departments differ.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@core/src/main/java/org/phoenixctms/ctsms/service/massmail/MassMailServiceImpl.java`:
- Around line 831-844: The department comparison uses
massMail.getDepartment().equals(proband.getDepartment()) which can NPE if
massMail.getDepartment() ever becomes null; change the check to a null-safe
comparison (e.g., Objects.equals(massMail.getDepartment(),
proband.getDepartment())) where the current conditional is performed (around the
block using listEntry, getProbandDao().load(probandId), and calling
ServiceUtil.addMassMailRecipient) to ensure robust behavior without changing the
surrounding logic.

In `@core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java`:
- Around line 323-327: Move the department equality guard into
addResetMassMailRecipient(...) so the helper enforces the invariant: inside the
method (addResetMassMailRecipient) check if
massMail.getDepartment().equals(proband.getDepartment()) and return early if
not, performing no side effects; update callers (the loop in ServiceUtil and the
call in TrialServiceImpl) to remove their duplicate department checks and simply
call addResetMassMailRecipient(...) unconditionally; ensure the method signature
and behavior remain the same otherwise and that any logging/journal behavior is
skipped when departments differ.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 02c5adac-3e9a-4793-af51-d9bc382dc096

📥 Commits

Reviewing files that changed from the base of the PR and between b7bb082 and caff5d3.

📒 Files selected for processing (3)

• core/src/main/java/org/phoenixctms/ctsms/service/massmail/MassMailServiceImpl.java
• core/src/main/java/org/phoenixctms/ctsms/service/trial/TrialServiceImpl.java
• core/src/main/java/org/phoenixctms/ctsms/util/ServiceUtil.java