#23 - collective editing vacation days #26

Merged
Baakoma merged 11 commits from #23-collective-editing-vacation-days into main 2022-01-24 11:28:00 +01:00
13 changed files with 54 additions and 39 deletions
Showing only changes of commit 36b98e1e67 - Show all commits

View File

@@ -4,16 +4,22 @@ declare(strict_types=1);
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
namespace Toby\Helpers;
use Illuminate\Contracts\Session\Session;
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
use Toby\Models\YearPeriod;
class YearPeriodRetriever
{
public const SESSION_KEY = "selected_year_period";
public function __construct(
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
protected Session $session,
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
) {
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
}
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
public function selected(): YearPeriod
{
/** @var YearPeriod $yearPeriod */
$yearPeriod = YearPeriod::query()->find(session()->get(static::SESSION_KEY));
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
$yearPeriod = YearPeriod::query()->find($this->session->get(static::SESSION_KEY));
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
return $yearPeriod !== null ? $yearPeriod : $this->current();
}
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?
krzysztofrewak commented 2022-01-24 10:46:45 +01:00 (Migrated from github.com)
Review

Could you inject session via DI?

Could you inject session via DI?

View File

@@ -15,8 +15,7 @@ class VacationLimitRequest extends FormRequest
return [
"items" => ["required", "array"],
"items.*.id" => ["required", "exists:vacation_limits,id"],
"items.*.hasVacation" => ["required", "boolean"],
"items.*.days" => ["exclude_if:items.*.hasVacation,false", "required", "integer", "min:0"],
"items.*.days" => ["nullable", "integer", "min:0"],
];
}
@@ -25,13 +24,10 @@ class VacationLimitRequest extends FormRequest
return VacationLimit::query()->find($this->collect("items")->pluck("id"));
}
public function data(): Collection
public function data(): array
{
return $this->collect("items")->mapWithKeys(fn(array $item): array => [
$item["id"] => [
"has_vacation" => $item["hasVacation"],
"days" => $item["days"],
],
]);
return $this->collect("items")
->keyBy("id")
->toArray();
}
}

View File

@@ -8,7 +8,7 @@ use Illuminate\Http\Resources\Json\JsonResource;
class UserFormDataResource extends JsonResource
{
public static $wrap = false;
public static $wrap = null;
public function toArray($request): array
{

View File

@@ -8,14 +8,14 @@ use Illuminate\Http\Resources\Json\JsonResource;
class VacationLimitResource extends JsonResource
{
public static $wrap = false;
public static $wrap = null;
public function toArray($request): array
{
return [
"id" => $this->id,
"user" => new UserResource($this->user),
"hasVacation" => $this->has_vacation,
"hasVacation" => $this->hasVacation(),
"days" => $this->days,
];
}

View File

@@ -12,7 +12,6 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo;
* @property int $id
* @property User $user
* @property YearPeriod $yearPeriod
* @property bool $has_vacation
* @property int $days
*/
class VacationLimit extends Model
@@ -21,9 +20,10 @@ class VacationLimit extends Model
protected $guarded = [];
protected $casts = [
"has_vacation" => "boolean",
];
public function hasVacation(): bool
{
return $this->days !== null;
}
public function user(): BelongsTo
{

View File

@@ -6,20 +6,13 @@ namespace Toby\Providers;
krzysztofrewak commented 2022-01-24 10:49:23 +01:00 (Migrated from github.com)
Review

TBH I would move these to some separate ObserverServiceProvider.

TBH I would move these to some separate `ObserverServiceProvider`.
use Illuminate\Support\Carbon;
use Illuminate\Support\ServiceProvider;
use Toby\Models\User;
use Toby\Models\VacationLimit;
use Toby\Models\YearPeriod;
use Toby\Observers\UserObserver;
use Toby\Observers\YearPeriodObserver;
use Toby\Scopes\SelectedYearPeriodScope;
class AppServiceProvider extends ServiceProvider
{
public function boot(): void
{
User::observe(UserObserver::class);
YearPeriod::observe(YearPeriodObserver::class);
Carbon::macro("toDisplayString", fn() => $this->translatedFormat("j F Y"));
VacationLimit::addGlobalScope($this->app->make(SelectedYearPeriodScope::class));

View File

@@ -0,0 +1,20 @@
<?php
declare(strict_types=1);
namespace Toby\Providers;
use Illuminate\Support\ServiceProvider;
use Toby\Models\User;
use Toby\Models\YearPeriod;
use Toby\Observers\UserObserver;
use Toby\Observers\YearPeriodObserver;
class ObserverServiceProvider extends ServiceProvider
{
public function boot(): void
{
User::observe(UserObserver::class);
YearPeriod::observe(YearPeriodObserver::class);
}
}

View File

@@ -42,5 +42,6 @@ return [
Toby\Providers\EventServiceProvider::class,
Toby\Providers\RouteServiceProvider::class,
Toby\Providers\TelescopeServiceProvider::class,
Toby\Providers\ObserverServiceProvider::class,
],
];

View File

@@ -15,7 +15,6 @@ return new class() extends Migration {
krzysztofrewak commented 2022-01-24 10:48:45 +01:00 (Migrated from github.com)
Review

Do we need that field? Nullable days indicate the same, I think.

Do we need that field? Nullable `days` indicate the same, I think.
krzysztofrewak commented 2022-01-24 10:48:45 +01:00 (Migrated from github.com)
Review

Do we need that field? Nullable days indicate the same, I think.

Do we need that field? Nullable `days` indicate the same, I think.
$table->id();
$table->foreignIdFor(User::class)->constrained()->cascadeOnDelete();
$table->foreignIdFor(YearPeriod::class)->constrained()->cascadeOnDelete();
$table->boolean("has_vacation")->default(false);
krzysztofrewak commented 2022-01-24 10:48:45 +01:00 (Migrated from github.com)
Review

Do we need that field? Nullable days indicate the same, I think.

Do we need that field? Nullable `days` indicate the same, I think.
$table->integer("days")->nullable();
$table->timestamps();
});
krzysztofrewak commented 2022-01-24 10:48:45 +01:00 (Migrated from github.com)
Review

Do we need that field? Nullable days indicate the same, I think.

Do we need that field? Nullable `days` indicate the same, I think.
krzysztofrewak commented 2022-01-24 10:48:45 +01:00 (Migrated from github.com)
Review

Do we need that field? Nullable days indicate the same, I think.

Do we need that field? Nullable `days` indicate the same, I think.

View File

@@ -162,7 +162,6 @@ export default {
.transform(data => ({
items: data.items.map(item => ({
id: item.id,
hasVacation: item.hasVacation,
days: item.hasVacation ? item.days : null,
})),
krzysztofrewak commented 2022-01-24 10:46:20 +01:00 (Migrated from github.com)
Review
                    })).sort((a, b) => a.name.toLowerCase().localeCompare(b.name.toLowerCase())),
```suggestion })).sort((a, b) => a.name.toLowerCase().localeCompare(b.name.toLowerCase())), ```
krzysztofrewak commented 2022-01-24 10:47:43 +01:00 (Migrated from github.com)
Review

Or maybe just soirt it on backend?

Or maybe just soirt it on backend?
EwelinaLasowy commented 2022-01-24 11:09:59 +01:00 (Migrated from github.com)
Review

I've created issue for that - link.

I've created issue for that - [link]( https://github.com/blumilksoftware/toby/issues/27).
}))

View File

@@ -14,17 +14,25 @@ class SelectYearPeriodTest extends FeatureTestCase
{
use DatabaseMigrations;
protected YearPeriodRetriever $yearPeriodRetriever;
protected function setUp(): void
{
parent::setUp();
$this->yearPeriodRetriever = $this->app->make(YearPeriodRetriever::class);
}
public function testUserCanSelectNextYearPeriod(): void
{
$nextYearPeriod = $this->createYearPeriod(Carbon::now()->year + 1);
$user = User::factory()->create();
$yearPeriodRetriever = new YearPeriodRetriever();
$this->actingAs($user)
->post("/year-periods/{$nextYearPeriod->id}/select")
->assertRedirect();
$this->assertSame($nextYearPeriod->id, $yearPeriodRetriever->selected()->id);
$this->assertSame($nextYearPeriod->id, $this->yearPeriodRetriever->selected()->id);
}
public function testUserCannotSelectNextYearPeriodIfDoesntExist(): void
@@ -38,9 +46,8 @@ class SelectYearPeriodTest extends FeatureTestCase
public function testIfUserDoesntSelectAnyYearPeriodCurrentActsAsSelected(): void
{
$yearPeriodRetriever = new YearPeriodRetriever();
$currentYearPeriod = $yearPeriodRetriever->current();
$currentYearPeriod = $this->yearPeriodRetriever->current();
$this->assertSame($currentYearPeriod->id, $yearPeriodRetriever->selected()->id);
$this->assertSame($currentYearPeriod->id, $this->yearPeriodRetriever->selected()->id);
}
}

View File

@@ -41,17 +41,14 @@ class VacationLimitTest extends FeatureTestCase
$data = [
[
"id" => $limit1->id,
"hasVacation" => true,
"days" => 25,
],
[
"id" => $limit2->id,
"hasVacation" => false,
"days" => null,
],
[
"id" => $limit3->id,
"hasVacation" => true,
"days" => 20,
],
];
@@ -64,19 +61,16 @@ class VacationLimitTest extends FeatureTestCase
$this->assertDatabaseHas("vacation_limits", [
"id" => $limit1->id,
"has_vacation" => true,
"days" => 25,
]);
$this->assertDatabaseHas("vacation_limits", [
"id" => $limit2->id,
"has_vacation" => false,
"days" => null,
]);
$this->assertDatabaseHas("vacation_limits", [
"id" => $limit3->id,
"has_vacation" => true,
"days" => 20,
]);
}

View File

@@ -31,7 +31,7 @@ class YearPeriodRetrieverTest extends TestCase
$this->current = Carbon::now();
Carbon::setTestNow($this->current);
$this->yearPeriodRetriever = new YearPeriodRetriever();
$this->yearPeriodRetriever = $this->app->make(YearPeriodRetriever::class);
$this->previousYearPeriod = $this->createYearPeriod($this->current->year - 1);
$this->currentYearPeriod = $this->createCurrentYearPeriod();
@@ -43,7 +43,7 @@ class YearPeriodRetrieverTest extends TestCase
$this->assertSame($this->currentYearPeriod->id, $this->yearPeriodRetriever->current()->id);
}
public function testRetrievesCurrentYearPeriodWhenNoSelected(): void
public function testRetrievesCurrentYearPeriodWhenNoneIsSelected(): void
{
$this->clearSelectedYearPeriod();