Skip to content

Commit d49b562

Browse files
committed
Changes made during pair review with @tarrow
* Allow `accepted_at` attribute to be mass-assigned. Preventing this doesn't provide that much extra security but does create some slightly confusing behaviour. Potential follow up to make the model set `accepted_at` to `CarbonImmutable::now()` if missing or `null`. * Use `$table->dateTime('accepted_at')` instead of `->timestamp()` in db migration - see comment in the code for rational. * Replaced `testAcceptedAtIgnoresMassAssignment()` with `testCreateFailsIfAcceptedAtIsMissing()` and `testCreateFailsIfAcceptedAtIsNull()` now that `ccepted_at` is mass-assignable. * Add unique constraint on `user_id` and `policy_id` in the `create_policy_acceptances_table` migration and added a test. * Add comment as to why casting to `immutable_datetime` in the models.
1 parent cd92a52 commit d49b562

4 files changed

Lines changed: 40 additions & 16 deletions

File tree

app/Policy.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,10 @@ class Policy extends Model {
4242

4343
protected function casts(): array {
4444
return [
45-
// cast `active_from` to a CarbonImmutable instance rather than a string
45+
// cast `active_from` to a `CarbonImmutable` instance rather than a string
4646
'active_from' => 'immutable_date',
4747

48+
// cast to `CarbonImmutable` until we default to using `CarbonImmutable` globally in T430656
4849
'created_at' => 'immutable_datetime',
4950
'updated_at' => 'immutable_datetime',
5051
];

app/PolicyAcceptance.php

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,6 @@ class PolicyAcceptance extends Model {
3535
protected $fillable = [
3636
'user_id',
3737
'policy_id',
38-
];
39-
40-
protected $guarded = [
41-
// Don't allow `accepted_at` to be mass assigned.
42-
// Most of the time this will be set to the current timestamp by the database.
4338
'accepted_at',
4439
];
4540

@@ -48,6 +43,7 @@ protected function casts(): array {
4843
// cast `accepted_at` to a `CarbonImmutable` instance rather than a string
4944
'accepted_at' => 'immutable_datetime',
5045

46+
// cast to `CarbonImmutable` until we default to using `CarbonImmutable` globally in T430656
5147
'created_at' => 'immutable_datetime',
5248
'updated_at' => 'immutable_datetime',
5349
];

database/migrations/2026_06_22_083910_create_policy_acceptances_table.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,15 @@ public function up(): void {
2121
// Using a separate `accepted_at` column rather than renaming the default `created_at` column because:
2222
// * it reduces confusion by remaining consistent with other tables that use these default columns
2323
// * `accepted_at` will be before `created_at` when backfilling the terms-of-use acceptances
24-
$table->timestamp('accepted_at')->useCurrent();
24+
// Using a DATETIME field as MariaDB has odd behavior around TIMESTAMPs and default values
25+
// `accepted_at` can be NOT NULL if we use a DATETIME which provides us with more data safety
26+
$table->dateTime('accepted_at');
2527

2628
// Use Eloquent built in to create nullable `created_at` and `updated_at` timestamp fields
2729
$table->timestamps();
30+
31+
// Prevent a user from accepting the same policy twice
32+
$table->unique(['user_id', 'policy_id']);
2833
});
2934
}
3035

tests/PolicyAcceptanceTest.php

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use App\User;
88
use Carbon\CarbonImmutable;
99
use Illuminate\Foundation\Testing\RefreshDatabase;
10+
use RuntimeException;
1011

1112
class PolicyAcceptanceTest extends TestCase {
1213
use RefreshDatabase;
@@ -33,6 +34,7 @@ public function testCreatesAndSavesSuccessfully(): void {
3334
[
3435
'user_id' => $this->userId,
3536
'policy_id' => $this->policyId,
37+
'accepted_at' => CarbonImmutable::now(),
3638
]
3739
);
3840
$policyAcceptance->save();
@@ -47,14 +49,34 @@ public function testCreatesAndSavesSuccessfully(): void {
4749
$this->assertInstanceOf(CarbonImmutable::class, $policyAcceptance->accepted_at);
4850
}
4951

50-
public function testAcceptedAtIgnoresMassAssignment(): void {
51-
$policyAcceptance = PolicyAcceptance::create(
52-
[
53-
'user_id' => $this->userId,
54-
'policy_id' => $this->policyId,
55-
'accepted_at' => CarbonImmutable::createFromDate(2026, 1, 1),
56-
]
57-
);
58-
$this->assertNull($policyAcceptance->accepted_at);
52+
public function testCreateFailsIfAcceptedAtIsMissing() {
53+
$this->expectException(RuntimeException::class);
54+
PolicyAcceptance::create([
55+
'user_id' => $this->userId,
56+
'policy_id' => $this->policyId,
57+
]);
58+
}
59+
60+
public function testCreateFailsIfAcceptedAtIsNull() {
61+
$this->expectException(RuntimeException::class);
62+
PolicyAcceptance::create([
63+
'user_id' => $this->userId,
64+
'policy_id' => $this->policyId,
65+
'accepted_at' => null,
66+
]);
67+
}
68+
69+
public function testUserAcceptingSamePolicyTwiceFails() {
70+
PolicyAcceptance::create([
71+
'user_id' => $this->userId,
72+
'policy_id' => $this->policyId,
73+
'accepted_at' => CarbonImmutable::now()->subSeconds(2),
74+
]);
75+
$this->expectException(RuntimeException::class);
76+
PolicyAcceptance::create([
77+
'user_id' => $this->userId,
78+
'policy_id' => $this->policyId,
79+
'accepted_at' => CarbonImmutable::now(),
80+
]);
5981
}
6082
}

0 commit comments

Comments
 (0)