Create endpoint to get polices that should be accepted#1198
Conversation
| $now = CarbonImmutable::now(); | ||
|
|
||
| // This works based on the assumption that the latest policy has the highest id given that id is AUTO_INCREMENT | ||
| $latestPolicyIds = Policy::where('active_from', '<', $now) |
There was a problem hiding this comment.
As I understand, query 1 builds a list of IDs with group-by + max and then pluck, query 2 fetches rows by those IDs.
So it means you do 2 round trip to get IDs and fetch rows. I think for a small set of data it's fine, but I recommend avoiding this pattern for large data set
| 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
| $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.
| 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.
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