Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions app/Http/Controllers/PoliciesController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

namespace App\Http\Controllers;

use App\Http\Resources\PoliciesCollection;
use App\Policy;
use Carbon\CarbonImmutable;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding PHPDoc for this controller

class PoliciesController extends Controller {
public function getCurrentPolicies() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a return type (aka PoliciesCollection in this case)

$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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

->selectRaw('MAX(id) as id')
->groupBy('policy_type')
->pluck('id');

$currentPolicies = Policy::whereIn('id', $latestPolicyIds)->get();

return new PoliciesCollection($currentPolicies);
}
}
19 changes: 19 additions & 0 deletions app/Http/Resources/PoliciesCollection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

namespace App\Http\Resources;

use Illuminate\Http\Request;
use Illuminate\Http\Resources\Json\ResourceCollection;

class PoliciesCollection extends ResourceCollection {
/**
* Transform the resource collection into an array.
*
* @return array<int|string, mixed>
*/
public function toArray(Request $request): array {
return [
'items' => $this->collection,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key items sounds "off" for me. ResourceCollection's responses look like { "data": [ // stuff go here] }.

Not a functionality issue but coding style

https://laravel.com/docs/12.x/collections

];
}
}
4 changes: 4 additions & 0 deletions app/Policy.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Carbon\CarbonImmutable;
use Eloquent;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

/**
Expand All @@ -24,10 +25,13 @@
* @method static Builder<static>|Policy whereId($value)
* @method static Builder<static>|Policy wherePolicyType($value)
* @method static Builder<static>|Policy whereUpdatedAt($value)
* @method static \Database\Factories\PolicyFactory factory(...$parameters)
*
* @mixin Eloquent
*/
class Policy extends Model {
use HasFactory;

// define which attributes are mass assignable
protected $fillable = [
'policy_type',
Expand Down
27 changes: 27 additions & 0 deletions database/factories/PolicyFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

namespace Database\Factories;

use App\Policy;
use Illuminate\Database\Eloquent\Factories\Factory;
use Illuminate\Database\Eloquent\Model;

/**
* @extends Factory<Model>
*/
class PolicyFactory extends Factory {
protected $model = Policy::class;

/**
* Define the model's default state.
*
* @return array<string, mixed>
*/
public function definition(): array {
return [
'policy_type' => $this->faker->randomElement(['terms-of-use', 'hosting-policy']),
'active_from' => now(),
'content_vue_file' => fake()->slug() . '.vue',
];
}
}
1 change: 1 addition & 0 deletions routes/api.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
$router->post('user/resetPassword', ['uses' => 'Auth\ResetPasswordController@reset']);
$router->post('contact/sendMessage', ['uses' => 'ContactController@sendMessage']);
$router->post('complaint/sendMessage', ['uses' => 'ComplaintController@sendMessage']);
$router->get('policies/current', ['uses' => 'PoliciesController@getCurrentPolicies']);

$router->post('auth/login', ['uses' => 'Auth\LoginController@postLogin'])->name('login');
// Authed
Expand Down
27 changes: 27 additions & 0 deletions tests/Http/Controllers/PoliciesControllerTest.php

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few suggestions for extra test coverage:

  1. 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).
  1. 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.
  1. 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).
  1. Current query uses MAX(id) per type (in PoliciesController.php). Add a test that backfills an older active_from with higher id to document expected behavior.

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

namespace Tests\Http\Controllers;

use App\Policy;
use Illuminate\Foundation\Testing\DatabaseTransactions;
use Tests\TestCase;

class PoliciesControllerTest extends TestCase {
use DatabaseTransactions;

public function testGetCurrentPolicies(): void {
// Future policy
Policy::factory()->create([
'active_from' => now()->addDay(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

real-time now() calls can be fragile when used on different timestamps.
Freeze time with Carbon::setTestNow() or just a plain simple $currentTime = now()

]);
// Active policy
Policy::factory()->create([
'active_from' => now()->subMonth(),
]);

$response = $this->getJson('/policies/current');

$response->assertOk();
$response->assertJsonCount(1, 'data.items');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Loading