fix: send trial emails synchronously so sent flag reflects delivery

[dan2k3k4]

Sends trial emails within the job so the sent flag is only set after a successful send, allowing retries when delivery fails.

Greptile Summary

This PR fixes a bug where trial emails could be marked as sent even when delivery failed, by switching Mail::queue() to Mail::send() so delivery is synchronous within the job and the sent flag is only updated after all owners have been emailed successfully. Three new feature tests are added to verify the happy path and the flag-stays-false-on-failure contract for each job.

• queue() → send() in all three trial email jobs — makes email delivery a blocking operation so any transport exception propagates before the *_email_sent flag is persisted, enabling the queue's built-in retry to re-attempt delivery.
• New feature tests (ProcessMidtrialEmailJobTest, ProcessOneDayLeftEmailJobTest, ProcessTrialCompleteEmailJobTest) — each covers the success path (email dispatched, flag set) and the send-failure path (exception thrown, flag stays false).

Confidence Score: 4/5

Safe to merge for single-owner groups; groups with multiple owners risk duplicate emails to already-contacted owners when a mid-loop send failure triggers a retry.

The synchronous-send fix is correct and the new tests cover the retry contract well. The unresolved concern (already noted in the previous round) is that the owner loop has no per-owner progress tracking: if the second owner's send throws, the job retries from the first owner on the next attempt, so the first owner receives the email twice. Until that is addressed, deployments where userGroup can have more than one owner carry a real duplicate-email risk.

The foreach owner loops in all three job files (ProcessTrialCompleteEmailJob.php lines 44–57, ProcessMidtrialEmailJob.php lines 62–81, ProcessOneDayLeftEmailJob.php lines 50–68) need attention if multi-owner groups are possible in production.

Important Files Changed

Filename	Overview
app/Jobs/ProcessPolydockAppInstanceJobs/Trial/ProcessTrialCompleteEmailJob.php	Changed Mail::queue() to Mail::send() so the sent flag is only set after all owners are emailed; multi-owner retry can still duplicate emails to already-contacted owners.
app/Jobs/ProcessPolydockAppInstanceJobs/Trial/ProcessMidtrialEmailJob.php	Same queue→send change as TrialComplete; identical multi-owner retry duplicate risk in the foreach loop (lines 62–81).
app/Jobs/ProcessPolydockAppInstanceJobs/Trial/ProcessOneDayLeftEmailJob.php	Same queue→send change; same multi-owner retry duplicate risk in foreach loop (lines 50–68).
tests/Feature/Jobs/Trial/ProcessTrialCompleteEmailJobTest.php	New feature test covering happy-path (email sent + flag set) and send-failure (flag stays false); correctly exercises the synchronous send contract.
tests/Feature/Jobs/Trial/ProcessMidtrialEmailJobTest.php	New feature test mirroring the TrialComplete test structure; covers happy path and send-failure path for the midtrial email job.
tests/Feature/Jobs/Trial/ProcessOneDayLeftEmailJobTest.php	New feature test mirroring the TrialComplete test structure; covers happy path and send-failure path for the one-day-left email job.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Q as Queue Worker
    participant J as Trial Email Job
    participant M as Mail::send()
    participant DB as Database

    Q->>J: execute handle()
    J->>J: guard checks (is_trial, flag, schedule)
    alt guards pass
        loop foreach owner
            J->>M: send(Mailable) [synchronous]
            alt send succeeds
                M-->>J: ok
            else send fails
                M-->>J: throws Exception
                J-->>Q: re-throw (flag stays false)
                Q->>J: retry job (starts over from owner 1)
            end
        end
        J->>DB: "update sent flag = true"
    else guards fail
        J-->>Q: return (no-op)
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Q as Queue Worker
    participant J as Trial Email Job
    participant M as Mail::send()
    participant DB as Database

    Q->>J: execute handle()
    J->>J: guard checks (is_trial, flag, schedule)
    alt guards pass
        loop foreach owner
            J->>M: send(Mailable) [synchronous]
            alt send succeeds
                M-->>J: ok
            else send fails
                M-->>J: throws Exception
                J-->>Q: re-throw (flag stays false)
                Q->>J: retry job (starts over from owner 1)
            end
        end
        J->>DB: "update sent flag = true"
    else guards fail
        J-->>Q: return (no-op)
    end

Loading

Comments Outside Diff (1)

1. app/Jobs/ProcessPolydockAppInstanceJobs/Trial/ProcessTrialCompleteEmailJob.php, line 44-60 (link)

   Duplicate emails on retry for multi-owner groups

   When userGroup->owners contains more than one owner and a send() call fails mid-loop (e.g., on the second owner), the exception propagates before trial_complete_email_sent is set to true. On the next retry the job starts over from the first owner, so every owner who was already emailed successfully in the previous attempt receives a duplicate. The same pattern exists in ProcessMidtrialEmailJob (line 62–81) and ProcessOneDayLeftEmailJob (line 50–68).

   A defensive approach is to track sent owners within the job run and skip any that were already emailed — for example, by collecting successfully delivered addresses in a local array before the loop and checking it at the top of each iteration, or by restructuring to catch and re-throw after recording progress. As long as the userGroup typically has exactly one owner this is low-risk, but the code makes no such assumption.

_{Reviews (2): Last reviewed commit: "test: cover midtrial and one-day-left em..." | Re-trigger Greptile}