Create endpoint to get polices that should be accepted#1198
Conversation
| use Carbon\CarbonImmutable; | ||
|
|
||
| class PoliciesController extends Controller { | ||
| public function getCurrentPolicies() { |
There was a problem hiding this comment.
Consider adding a return type (aka PoliciesCollection in this case)
| use App\Http\Resources\PoliciesCollection; | ||
| use App\Policy; | ||
| use Carbon\CarbonImmutable; | ||
|
|
There was a problem hiding this comment.
consider adding PHPDoc for this controller
| */ | ||
| public function toArray(Request $request): array { | ||
| return [ | ||
| 'items' => $this->collection, |
There was a problem hiding this comment.
the key items sounds "off" for me. ResourceCollection's responses look like { "data": [ // stuff go here] }.
Not a functionality issue but coding style
There was a problem hiding this comment.
Here it is following the example from the ticket. but we can also do return [$this->collection] it will yield the output you are suggesting.
| $response = $this->getJson('/policies/current'); | ||
|
|
||
| $response->assertOk(); | ||
| $response->assertJsonCount(1, 'data.items'); |
There was a problem hiding this comment.
this test only asserts the count, so it can pass even if the endpoint returns the wrong policy.
You should add assertions that the returned item is the expected 'active' policy (by id or active_from value) and that the "future" policies are left out.
There was a problem hiding this comment.
yeah, that's a good point
| public function testGetCurrentPolicies(): void { | ||
| // Future policy | ||
| Policy::factory()->create([ | ||
| 'active_from' => now()->addDay(), |
There was a problem hiding this comment.
real-time now() calls can be fragile when used on different timestamps.
Freeze time with Carbon::setTestNow() or just a plain simple $currentTime = now()
There was a problem hiding this comment.
In this case it is not an issue but I understand what you mean.
There was a problem hiding this comment.
a few suggestions for extra test coverage:
- Returns one current policy per policy type
- Create two terms-of-use rows and two hosting-policy rows, with mixed active dates, then assert exactly 2 results (one per type).
- Picks latest active policy for each type
- For the same type, create old active, newer active and future active
- Assert the returned row is the newer active, not old and not future.
- Returns one current policy per policy type
- Create two
terms-of-userows and twohosting-policyrows, with mixed active dates, then assert exactly 2 results (one per type).
- Current query uses MAX(id) per type (in
PoliciesController.php). Add a test that backfills an olderactive_fromwith higheridto document expected behavior.
2 participants
Bug: T429591