Send each scheduled payment's due notice exactly once
CI / Coding Standards (pull_request) Successful in 28s
CI / Tests (PHP 8.1) (pull_request) Successful in 35s
CI / No Debug Code (pull_request) Successful in 9s
CI / Tests (PHP 8.3) (pull_request) Successful in 44s
CI / Tests (PHP 8.2) (pull_request) Successful in 47s
CI / Tests (PHP 8.5) (pull_request) Successful in 48s
CI / Static Analysis (pull_request) Successful in 52s
CI / Build Plugin Zip (pull_request) Skipped
CI / Coding Standards (pull_request) Successful in 28s
CI / Tests (PHP 8.1) (pull_request) Successful in 35s
CI / No Debug Code (pull_request) Successful in 9s
CI / Tests (PHP 8.3) (pull_request) Successful in 44s
CI / Tests (PHP 8.2) (pull_request) Successful in 47s
CI / Tests (PHP 8.5) (pull_request) Successful in 48s
CI / Static Analysis (pull_request) Successful in 52s
CI / Build Plugin Zip (pull_request) Skipped
The daily billing scan runs on request via WP-Cron and can overlap itself under concurrent traffic. Each run emailed the payments it created with no record that a notice had gone out, so two overlapping runs could send a payer two identical "Payment due" emails for one charge — read by families as being billed twice, though only one row exists. Stamp us_payments.notice_sent_at atomically before emailing: the scan now claims each payment with a conditional UPDATE ... WHERE notice_sent_at IS NULL and only notices, credits and batches the rows it won. A competing run finds them claimed and stays quiet, so exactly one notice is sent regardless of how the scan is triggered. A one-time backfill stamps existing scheduled rows on upgrade so already-noticed charges are not re-emailed. Co-authored-by: anthropic/claude-opus-4-8
This commit is contained in:
co-authored by
anthropic/claude-opus-4-8
parent
6c92fc35fd
commit
e389e40843
@@ -11,6 +11,11 @@ When a `v*` tag is pushed, `.gitea/workflows/release.yml` publishes the matching
|
||||
the plugin to the next patch version and adds a fresh section here for it. Record
|
||||
each change under the current top section as you work.
|
||||
|
||||
## [1.5.8]
|
||||
|
||||
### Fixed
|
||||
- **A student is no longer emailed the same "Payment due" notice twice.** The daily billing scan runs whenever the site gets traffic, and on a busy day two copies of it could end up running at the same time. Neither knew about the other, so each would send its own notice for the same charge — one payment on the books, but the family saw two identical requests to pay and reasonably read it as being billed twice. Each payment is now stamped the moment its notice goes out, and a second run that reaches the same payment sees the stamp and stays quiet, so exactly one notice is sent no matter how the scan is triggered. Payments already noticed before this update are marked as such on upgrade, so nobody gets a fresh round of reminders for charges they were already told about.
|
||||
|
||||
## [1.5.7]
|
||||
|
||||
## [1.5.6]
|
||||
|
||||
@@ -59,6 +59,13 @@ class Payment {
|
||||
public readonly ?string $stripePaymentIntentId = null,
|
||||
public readonly ?string $receiptNumber = null,
|
||||
public readonly ?string $receiptSentAt = null,
|
||||
/**
|
||||
* When the daily billing scan emailed this payment's due notice, or null
|
||||
* if it has not been noticed yet. Gates the notice so a payment is emailed
|
||||
* exactly once even if the scan runs more than once (WP-Cron fires on
|
||||
* request and can overlap under concurrent traffic).
|
||||
*/
|
||||
public readonly ?string $noticeSentAt = null,
|
||||
public readonly ?string $paidAt = null,
|
||||
public readonly ?string $createdAt = null,
|
||||
public readonly ?int $id = null,
|
||||
@@ -85,6 +92,7 @@ class Payment {
|
||||
stripePaymentIntentId: Val::stringOrNull( $row->stripe_payment_intent_id ),
|
||||
receiptNumber: Val::stringOrNull( $row->receipt_number ),
|
||||
receiptSentAt: Val::stringOrNull( $row->receipt_sent_at ),
|
||||
noticeSentAt: Val::stringOrNull( $row->notice_sent_at ?? null ),
|
||||
paidAt: Val::stringOrNull( $row->paid_at ),
|
||||
createdAt: Val::stringOrNull( $row->created_at ),
|
||||
id: Val::int( $row->id ),
|
||||
|
||||
@@ -34,10 +34,11 @@ class PaymentRepository {
|
||||
'stripe_payment_intent_id' => $payment->stripePaymentIntentId,
|
||||
'receipt_number' => $payment->receiptNumber,
|
||||
'receipt_sent_at' => $payment->receiptSentAt,
|
||||
'notice_sent_at' => $payment->noticeSentAt,
|
||||
'paid_at' => $payment->paidAt,
|
||||
'created_at' => current_time( 'mysql' ),
|
||||
],
|
||||
[ '%d', '%d', '%d', '%s', '%d', '%f', '%s', '%s', '%s', '%f', '%f', '%f', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s' ]
|
||||
[ '%d', '%d', '%d', '%s', '%d', '%f', '%s', '%s', '%s', '%f', '%f', '%f', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s' ]
|
||||
);
|
||||
|
||||
return $this->db->insert_id;
|
||||
@@ -271,6 +272,66 @@ class PaymentRepository {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically claim a payment for its one due-payment notice. Stamps
|
||||
* `notice_sent_at` only if it is still null, and returns whether *this* call
|
||||
* won the claim (one row updated). The billing scan calls this before
|
||||
* emailing so a payment's notice is sent exactly once: WP-Cron fires on
|
||||
* request and can overlap under concurrent traffic, so two scans may both
|
||||
* reach the send step for the same payment — the loser here updates zero rows
|
||||
* and skips the email. The conditional `WHERE ... IS NULL` is the guard, not a
|
||||
* prior read, so there is no check-then-act race.
|
||||
*/
|
||||
public function markNoticed( int $id ): bool {
|
||||
$sql = $this->db->prepare(
|
||||
'UPDATE %i SET notice_sent_at = %s WHERE id = %d AND notice_sent_at IS NULL',
|
||||
$this->table,
|
||||
current_time( 'mysql' ),
|
||||
$id
|
||||
);
|
||||
|
||||
if ( null === $sql ) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return (int) $this->db->query( $sql ) === 1;
|
||||
}
|
||||
|
||||
/**
|
||||
* One-time repair for sites upgraded before `notice_sent_at` existed: dbDelta
|
||||
* adds the column, and this backfills it so the historical payments those
|
||||
* sites already emailed notices for are not re-noticed on the next scan.
|
||||
* Every pre-existing pending scheduled row is treated as already noticed.
|
||||
* Guarded by its own option flag in {@see \Unsupervised\Schedular\Plugin},
|
||||
* not the version gate, since affected sites may already be on the current
|
||||
* version. Returns false when the column is absent so the caller does not set
|
||||
* its flag before dbDelta has run.
|
||||
*/
|
||||
public function backfillNoticeSent(): bool {
|
||||
$column = $this->db->get_var(
|
||||
$this->db->prepare(
|
||||
'SHOW COLUMNS FROM %i LIKE %s',
|
||||
$this->table,
|
||||
'notice_sent_at'
|
||||
)
|
||||
);
|
||||
|
||||
if ( null === $column ) {
|
||||
return false;
|
||||
}
|
||||
|
||||
$sql = $this->db->prepare(
|
||||
'UPDATE %i SET notice_sent_at = created_at WHERE notice_sent_at IS NULL AND period_key IS NOT NULL',
|
||||
$this->table
|
||||
);
|
||||
|
||||
if ( null !== $sql ) {
|
||||
$this->db->query( $sql );
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
public function updateStatus( int $id, string $status ): bool {
|
||||
if ( ! in_array( $status, Payment::VALID_STATUSES, true ) ) {
|
||||
return false;
|
||||
|
||||
@@ -106,6 +106,16 @@ class PaymentService {
|
||||
$this->payments->assignNoticeBatch( $ids, $batch );
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically claim a payment for its single due-payment notice, returning
|
||||
* whether this call won the claim. The daily scan calls this before emailing
|
||||
* so a payment is noticed exactly once even when WP-Cron overlaps. Delegates
|
||||
* to the ledger.
|
||||
*/
|
||||
public function markNoticed( int $paymentId ): bool {
|
||||
return $this->payments->markNoticed( $paymentId );
|
||||
}
|
||||
|
||||
/**
|
||||
* Studio-admin confirmation that a pending payment (e-transfer) was received.
|
||||
* Marks it paid, confirms the registration, and emails the receipt.
|
||||
|
||||
@@ -352,6 +352,24 @@ class ScheduledBillingRunner {
|
||||
*/
|
||||
private function sendNotices( array $buckets ): void {
|
||||
foreach ( $buckets as $payerId => $entries ) {
|
||||
// Claim each payment's one-and-only notice up front. markNoticed stamps
|
||||
// notice_sent_at only if still null and reports whether this run won —
|
||||
// so an overlapping scan that also created/collected these payments
|
||||
// finds them already claimed and drops them here, and no payer is
|
||||
// emailed the same charge twice. Only the claimed entries go on to be
|
||||
// credited, batched and listed.
|
||||
$entries = array_values(
|
||||
array_filter(
|
||||
$entries,
|
||||
fn( array $entry ): bool => null !== $entry['payment']->id
|
||||
&& $this->payments->markNoticed( (int) $entry['payment']->id )
|
||||
)
|
||||
);
|
||||
|
||||
if ( [] === $entries ) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$payments = array_map( static fn( array $entry ): Payment => $entry['payment'], $entries );
|
||||
$applied = $this->payments->applyCredits( $payerId, $payments );
|
||||
|
||||
|
||||
+13
-1
@@ -95,7 +95,19 @@ class Plugin {
|
||||
$guardianRepo = new GuardianRepository( $wpdb );
|
||||
$guardians = new GuardianService( $guardianRepo, $bookings, $enrollments );
|
||||
|
||||
$paymentRepo = new PaymentRepository( $wpdb );
|
||||
$paymentRepo = new PaymentRepository( $wpdb );
|
||||
|
||||
// One-time backfill of us_payments.notice_sent_at, which dbDelta adds
|
||||
// defaulting to NULL — leaving every already-noticed scheduled payment
|
||||
// looking un-noticed, which the billing scan would email again. Backfills
|
||||
// existing scheduled rows to their created_at so only genuinely new
|
||||
// payments get a notice from here on. Guarded by its own flag rather than
|
||||
// the version gate, since affected sites may already be on the current
|
||||
// version; set only once the column exists and the update runs.
|
||||
if ( '1' !== get_option( 'us_payments_notice_sent_backfilled', '' ) && $paymentRepo->backfillNoticeSent() ) {
|
||||
update_option( 'us_payments_notice_sent_backfilled', '1' );
|
||||
}
|
||||
|
||||
$creditRepo = new CreditRepository( $wpdb );
|
||||
$settings = new StudioSettings();
|
||||
$resolver = new BillingMethodResolver( $settings );
|
||||
|
||||
@@ -207,6 +207,7 @@ class Schema {
|
||||
stripe_payment_intent_id VARCHAR(255) DEFAULT NULL,
|
||||
receipt_number VARCHAR(50) DEFAULT NULL,
|
||||
receipt_sent_at DATETIME DEFAULT NULL,
|
||||
notice_sent_at DATETIME DEFAULT NULL,
|
||||
created_at DATETIME NOT NULL,
|
||||
paid_at DATETIME DEFAULT NULL,
|
||||
PRIMARY KEY (id),
|
||||
|
||||
@@ -124,6 +124,61 @@ class PaymentRepositoryTest extends TestCase
|
||||
self::assertTrue($this->repo->markPaid(50, 'USC-50'));
|
||||
}
|
||||
|
||||
public function testMarkNoticedStampsOnlyUnnoticedRowAndReportsTheWin(): void
|
||||
{
|
||||
Functions\expect('current_time')->with('mysql')->andReturn('2026-06-08 12:00:00');
|
||||
|
||||
$this->db->shouldReceive('prepare')
|
||||
->once()
|
||||
->with(Mockery::pattern('/SET notice_sent_at = %s WHERE id = %d AND notice_sent_at IS NULL/'), 'wp_us_payments', '2026-06-08 12:00:00', 50)
|
||||
->andReturn('UPDATE ...');
|
||||
|
||||
// One row updated -> this call won the claim.
|
||||
$this->db->shouldReceive('query')->once()->with('UPDATE ...')->andReturn(1);
|
||||
|
||||
self::assertTrue($this->repo->markNoticed(50));
|
||||
}
|
||||
|
||||
public function testMarkNoticedReturnsFalseWhenAlreadyClaimed(): void
|
||||
{
|
||||
Functions\expect('current_time')->with('mysql')->andReturn('2026-06-08 12:00:00');
|
||||
|
||||
$this->db->shouldReceive('prepare')->once()->andReturn('UPDATE ...');
|
||||
// Zero rows updated -> another run already stamped notice_sent_at.
|
||||
$this->db->shouldReceive('query')->once()->with('UPDATE ...')->andReturn(0);
|
||||
|
||||
self::assertFalse($this->repo->markNoticed(50));
|
||||
}
|
||||
|
||||
public function testBackfillNoticeSentStampsExistingScheduledRowsWhenColumnPresent(): void
|
||||
{
|
||||
$this->db->shouldReceive('prepare')
|
||||
->once()
|
||||
->with(Mockery::pattern('/SHOW COLUMNS FROM %i LIKE %s/'), 'wp_us_payments', 'notice_sent_at')
|
||||
->andReturn('SHOW ...');
|
||||
$this->db->shouldReceive('get_var')->once()->with('SHOW ...')->andReturn('notice_sent_at');
|
||||
|
||||
$this->db->shouldReceive('prepare')
|
||||
->once()
|
||||
->with(Mockery::pattern('/SET notice_sent_at = created_at WHERE notice_sent_at IS NULL AND period_key IS NOT NULL/'), 'wp_us_payments')
|
||||
->andReturn('UPDATE ...');
|
||||
$this->db->shouldReceive('query')->once()->with('UPDATE ...')->andReturn(3);
|
||||
|
||||
self::assertTrue($this->repo->backfillNoticeSent());
|
||||
}
|
||||
|
||||
public function testBackfillNoticeSentIsNoopWhenColumnAbsent(): void
|
||||
{
|
||||
$this->db->shouldReceive('prepare')->once()->andReturn('SHOW ...');
|
||||
$this->db->shouldReceive('get_var')->once()->with('SHOW ...')->andReturn(null);
|
||||
|
||||
// Column not there yet: do not attempt the UPDATE, and report not-done so
|
||||
// the caller does not set its one-time flag before dbDelta has run.
|
||||
$this->db->shouldNotReceive('query');
|
||||
|
||||
self::assertFalse($this->repo->backfillNoticeSent());
|
||||
}
|
||||
|
||||
public function testUpdateTaxRecomputesAmountFromRate(): void
|
||||
{
|
||||
$this->db->shouldReceive('prepare')
|
||||
|
||||
@@ -45,6 +45,7 @@ class PaymentTest extends TestCase
|
||||
'stripe_payment_intent_id' => null,
|
||||
'receipt_number' => 'USC-7',
|
||||
'receipt_sent_at' => null,
|
||||
'notice_sent_at' => '2026-06-07 08:00:00',
|
||||
'paid_at' => '2026-06-08 10:00:00',
|
||||
'created_at' => '2026-06-08 09:00:00',
|
||||
]);
|
||||
@@ -56,6 +57,7 @@ class PaymentTest extends TestCase
|
||||
self::assertSame(Payment::METHOD_COMP, $payment->method);
|
||||
self::assertTrue($payment->isPaid());
|
||||
self::assertSame('USC-7', $payment->receiptNumber);
|
||||
self::assertSame('2026-06-07 08:00:00', $payment->noticeSentAt);
|
||||
self::assertSame('2026-06-08 09:00:00', $payment->createdAt);
|
||||
}
|
||||
|
||||
|
||||
@@ -44,6 +44,9 @@ class ScheduledBillingRunnerTest extends TestCase
|
||||
$this->payments->shouldReceive('assignNoticeBatch')->byDefault();
|
||||
// No account credit unless a test says otherwise.
|
||||
$this->payments->shouldReceive('applyCredits')->andReturn([])->byDefault();
|
||||
// Every payment wins its notice claim unless a test simulates an
|
||||
// overlapping run that already claimed it.
|
||||
$this->payments->shouldReceive('markNoticed')->andReturn(true)->byDefault();
|
||||
|
||||
Functions\when('wp_generate_uuid4')->justReturn('abcdef12-3456-7890-abcd-ef1234567890');
|
||||
|
||||
@@ -324,6 +327,62 @@ class ScheduledBillingRunnerTest extends TestCase
|
||||
$this->runner->run();
|
||||
}
|
||||
|
||||
/**
|
||||
* A payment is emailed exactly once. WP-Cron fires on request and can run the
|
||||
* scan twice concurrently; the second run reaching the send step for a
|
||||
* payment already claimed by the first (markNoticed returns false) must not
|
||||
* email it again, nor re-batch it. This is the double-notice regression.
|
||||
*/
|
||||
public function testDoesNotEmailAPaymentWhoseNoticeWasAlreadyClaimed(): void
|
||||
{
|
||||
$this->now('2026-07-15 09:00:00');
|
||||
$this->bookings->shouldReceive('findUnbilledScheduledLessons')
|
||||
->andReturn([ $this->lessonRow(101, Offering::BILLING_WEEKLY, '2026-07-15 18:00:00', 35.0) ]);
|
||||
|
||||
$this->payments->shouldReceive('createForRegistration')->once()->andReturn($this->pending(500, '2026-07-14'));
|
||||
|
||||
// The competing run already stamped notice_sent_at, so the claim loses.
|
||||
$this->payments->shouldReceive('markNoticed')->with(500)->once()->andReturn(false);
|
||||
|
||||
// No credit, no batch, no email for an already-noticed payment.
|
||||
$this->payments->shouldNotReceive('applyCredits');
|
||||
$this->payments->shouldNotReceive('assignNoticeBatch');
|
||||
$this->mailer->shouldNotReceive('send');
|
||||
|
||||
$this->runner->run();
|
||||
}
|
||||
|
||||
/**
|
||||
* When only some of a payer's charges are already claimed, the run notices
|
||||
* the rest — the still-unclaimed payment is emailed and batched on its own.
|
||||
*/
|
||||
public function testEmailsOnlyTheStillUnclaimedPaymentsInABucket(): void
|
||||
{
|
||||
$this->now('2026-07-15 09:00:00');
|
||||
|
||||
$one = $this->lessonRow(101, Offering::BILLING_WEEKLY, '2026-07-15 18:00:00', 35.0);
|
||||
$two = $this->lessonRow(102, Offering::BILLING_WEEKLY, '2026-07-15 19:00:00', 35.0);
|
||||
$this->bookings->shouldReceive('findUnbilledScheduledLessons')->andReturn([$one, $two]);
|
||||
|
||||
$this->payments->shouldReceive('createForRegistration')
|
||||
->andReturn($this->pending(500, '2026-07-14'), $this->pending(501, '2026-07-14'));
|
||||
|
||||
// 500 already claimed by an overlapping run; 501 is this run's to send.
|
||||
$this->payments->shouldReceive('markNoticed')->with(500)->once()->andReturn(false);
|
||||
$this->payments->shouldReceive('markNoticed')->with(501)->once()->andReturn(true);
|
||||
|
||||
$this->payments->shouldReceive('applyCredits')
|
||||
->once()
|
||||
->with(5, Mockery::on(static fn (array $p): bool => count($p) === 1 && (int) $p[0]->id === 501))
|
||||
->andReturn([]);
|
||||
$this->payments->shouldReceive('assignNoticeBatch')->once()->with([501], Mockery::type('string'));
|
||||
$this->mailer->shouldReceive('send')
|
||||
->once()
|
||||
->with(Mockery::type(\WP_User::class), Mockery::on(static fn (array $items): bool => count($items) === 1), Mockery::type('string'), 0.0);
|
||||
|
||||
$this->runner->run();
|
||||
}
|
||||
|
||||
private function groupOffering(string $mode, string $termStart, string $termEnd): Offering
|
||||
{
|
||||
return new Offering(
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
* Plugin Name: Unsupervised Scheduler
|
||||
* Plugin URI: https://git.unsupervised.ca/Unsupervised/unsupervised-scheduler
|
||||
* Description: Instructor/student lesson scheduling for WordPress.
|
||||
* Version: 1.5.7
|
||||
* Version: 1.5.8
|
||||
* Requires at least: 6.2
|
||||
* Requires PHP: 8.1
|
||||
* Author: Unsupervised
|
||||
@@ -21,7 +21,7 @@ if (! defined('ABSPATH')) {
|
||||
exit;
|
||||
}
|
||||
|
||||
define('USC_VERSION', '1.5.7');
|
||||
define('USC_VERSION', '1.5.8');
|
||||
define('USC_PLUGIN_FILE', __FILE__);
|
||||
define('USC_PLUGIN_DIR', plugin_dir_path(__FILE__));
|
||||
define('USC_PLUGIN_URL', plugin_dir_url(__FILE__));
|
||||
|
||||
Reference in New Issue
Block a user