Skip to content

fix(reports): allow assigned curators to edit all report fields #809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vcnainala wants to merge 1 commit into
base: development
Choose a base branch
from
Open

fix(reports): allow assigned curators to edit all report fields #809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 12 additions & 24 deletions app/Filament/Dashboard/Resources/ReportResource/Pages/EditReport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,30 +26,18 @@ protected function mutateFormDataBeforeFill(array $data): array
'references' => $molecule_data['references'] ?? [],
]);
} elseif ($this->record['report_category'] === ReportCategory::UPDATE->value) {
// initiate the flags to show only the fields that need to be shown - overall changes are always from the initial suggestions
if (array_key_exists('geo_location_changes', $this->record['suggested_changes']['overall_changes'])) {
$data['show_geo_location_existing'] = $this->record['suggested_changes']['overall_changes']['geo_location_changes']['delete'] ? true : false;
$data['show_geo_location_new'] = $this->record['suggested_changes']['overall_changes']['geo_location_changes']['add'] ? true : false;
}
if (array_key_exists('synonym_changes', $this->record['suggested_changes']['overall_changes'])) {
$data['show_synonym_existing'] = $this->record['suggested_changes']['overall_changes']['synonym_changes']['delete'] ? true : false;
$data['show_synonym_new'] = $this->record['suggested_changes']['overall_changes']['synonym_changes']['add'] ? true : false;
}
if (array_key_exists('name_change', $this->record['suggested_changes']['overall_changes'])) {
$data['show_name_change'] = $this->record['suggested_changes']['overall_changes']['name_change'] ? true : false;
}
if (array_key_exists('cas_changes', $this->record['suggested_changes']['overall_changes'])) {
$data['show_cas_existing'] = $this->record['suggested_changes']['overall_changes']['cas_changes']['delete'] ? true : false;
$data['show_cas_new'] = $this->record['suggested_changes']['overall_changes']['cas_changes']['add'] ? true : false;
}
if (array_key_exists('organism_changes', $this->record['suggested_changes']['overall_changes'])) {
$data['show_organism_existing'] = $this->record['suggested_changes']['overall_changes']['organism_changes']['delete'] ? true : false;
$data['show_organism_new'] = $this->record['suggested_changes']['overall_changes']['organism_changes']['add'] ? true : false;
}
if (array_key_exists('citation_changes', $this->record['suggested_changes']['overall_changes'])) {
$data['show_citation_existing'] = $this->record['suggested_changes']['overall_changes']['citation_changes']['delete'] ? true : false;
$data['show_citation_new'] = $this->record['suggested_changes']['overall_changes']['citation_changes']['add'] ? true : false;
}
// Curators may edit all change fields, not only those the reporter requested (#738)
$data['show_geo_location_existing'] = true;
$data['show_geo_location_new'] = true;
$data['show_synonym_existing'] = true;
$data['show_synonym_new'] = true;
$data['show_name_change'] = true;
$data['show_cas_existing'] = true;
$data['show_cas_new'] = true;
$data['show_organism_existing'] = true;
$data['show_organism_new'] = true;
$data['show_citation_existing'] = true;
$data['show_citation_new'] = true;

$curators_copy_changes = $this->record['suggested_changes']['curator'];
$data['existing_geo_locations'] = $curators_copy_changes['existing_geo_locations'];
Expand Down
40 changes: 40 additions & 0 deletions app/Models/Report.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Models;

use App\Enums\ReportStatus;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
Expand Down Expand Up @@ -104,4 +105,43 @@ public function transformAudit(array $data): array
{
return changeAudit($data);
}

/**
* Whether the given user may edit this report as an assigned curator
* (preserves the two-curator / four-eyes workflow).
*/
public function assignedCuratorCanEdit(User $user): bool
{
if ($this->status === ReportStatus::SUBMITTED->value) {
if (! $this->curators()->wherePivot('curator_number', 1)->exists()) {
return true;
}

/** @var User|null $curator1 */
$curator1 = $this->curators()->wherePivot('curator_number', 1)->first();

return $curator1?->id === $user->id;
}

if ($this->status === ReportStatus::PENDING_APPROVAL->value
|| $this->status === ReportStatus::PENDING_REJECTION->value) {
/** @var User|null $firstCurator */
$firstCurator = $this->curators()->wherePivot('curator_number', 1)->first();

if ($firstCurator?->id === $user->id) {
return false;
}

if (! $this->curators()->wherePivot('curator_number', 2)->exists()) {
return true;
}

/** @var User|null $curator2 */
$curator2 = $this->curators()->wherePivot('curator_number', 2)->first();

return $curator2?->id === $user->id;
}

return false;
}
}
45 changes: 3 additions & 42 deletions app/Policies/ReportPolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace App\Policies;

use App\Enums\ReportStatus;
use App\Models\Report;
use App\Models\User;
use Illuminate\Auth\Access\HandlesAuthorization;
Expand Down Expand Up @@ -43,47 +42,9 @@ public function create(User $user): bool
*/
public function update(User $user, Report $report): bool
{
// Allow users with update_report permission in these cases:
if ($user->can('update_report')) {
// Case 1: Report is submitted
if ($report->status == ReportStatus::SUBMITTED->value) {
// Allow if no curator 1 is assigned yet
if (! $report->curators()->wherePivot('curator_number', 1)->exists()) {
return true;
}

// Or if user is already assigned as curator 1
/** @var User|null $curator1 */
$curator1 = $report->curators()->wherePivot('curator_number', 1)->first();
if ($curator1?->id == $user->id) {
return true;
}
}

// Case 2: Report is pending approval/rejection
if ($report->status == ReportStatus::PENDING_APPROVAL->value || $report->status == ReportStatus::PENDING_REJECTION->value) {
// Get the first curator ID
/** @var User|null $firstCurator */
$firstCurator = $report->curators()->wherePivot('curator_number', 1)->first();
$firstCuratorId = $firstCurator?->id;

// Don't allow if user was the first curator (enforce four-eyes principle)
if ($firstCuratorId == $user->id) {
return false;
}

// Allow if no curator 2 is assigned yet
if (! $report->curators()->wherePivot('curator_number', 2)->exists()) {
return true;
}

// Or if user is already assigned as curator 2
/** @var User|null $curator2 */
$curator2 = $report->curators()->wherePivot('curator_number', 2)->first();
if ($curator2?->id == $user->id) {
return true;
}
}
if (($user->can('update_report') || $user->hasRole('curator'))
&& $report->assignedCuratorCanEdit($user)) {
return true;
}

// Allow report creator to update only if status is null (report is being created)
Expand Down
2 changes: 1 addition & 1 deletion database/seeders/ShieldSeeder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public function run(): void
{
app()[PermissionRegistrar::class]->forgetCachedPermissions();

$rolesWithPermissions = '[{"name":"super_admin","guard_name":"web","permissions":["view_role","view_any_role","create_role","update_role","delete_role","delete_any_role"]},{"name":"admin","guard_name":"web","permissions":["view_role","view_any_role"]},{"name":"curator","guard_name":"web","permissions":[]}]';
$rolesWithPermissions = '[{"name":"super_admin","guard_name":"web","permissions":["view_role","view_any_role","create_role","update_role","delete_role","delete_any_role"]},{"name":"admin","guard_name":"web","permissions":["view_role","view_any_role"]},{"name":"curator","guard_name":"web","permissions":["view_any_report","view_report","update_report"]}]';
$directPermissions = '[]';

static::makeRolesWithPermissions($rolesWithPermissions);
Expand Down
130 changes: 130 additions & 0 deletions tests/Unit/ReportPolicyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
<?php

namespace Tests\Unit;

use App\Enums\ReportCategory;
use App\Enums\ReportStatus;
use App\Models\Report;
use App\Models\User;
use App\Policies\ReportPolicy;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\DB;
use Spatie\Permission\Models\Role;
use Tests\TestCase;

class ReportPolicyTest extends TestCase
{
use RefreshDatabase;

private ReportPolicy $policy;

protected function setUp(): void
{
parent::setUp();

$this->policy = new ReportPolicy;
Role::create(['name' => 'curator', 'guard_name' => 'web']);

DB::statement('ALTER TABLE reports DROP CONSTRAINT IF EXISTS reports_status_check');
DB::statement('ALTER TABLE reports DROP CONSTRAINT IF EXISTS check_status');
DB::statement(
"ALTER TABLE reports ADD CONSTRAINT check_status CHECK (status IN ('"
.implode("','", ReportStatus::values())
."'))"
);
}

public function test_assigned_curator_one_can_update_submitted_report_without_update_report_permission(): void
{
$reporter = User::factory()->create();
$curator = User::factory()->create();
$curator->assignRole('curator');

$report = $this->createReport($reporter, ReportStatus::SUBMITTED);
$this->assignCurator($report, $curator, 1);

$this->assertTrue($this->policy->update($curator, $report));
}

public function test_curator_one_cannot_update_pending_approval_report_four_eyes(): void
{
$reporter = User::factory()->create();
$curatorOne = User::factory()->create();
$curatorOne->assignRole('curator');

$report = $this->createReport($reporter, ReportStatus::PENDING_APPROVAL);
$this->assignCurator($report, $curatorOne, 1);

$this->assertFalse($this->policy->update($curatorOne, $report));
}

public function test_assigned_curator_two_can_update_pending_approval_report(): void
{
$reporter = User::factory()->create();
$curatorOne = User::factory()->create();
$curatorTwo = User::factory()->create();
$curatorTwo->assignRole('curator');

$report = $this->createReport($reporter, ReportStatus::PENDING_APPROVAL);
$this->assignCurator($report, $curatorOne, 1);
$this->assignCurator($report, $curatorTwo, 2);

$this->assertTrue($this->policy->update($curatorTwo, $report));
}

public function test_curator_cannot_update_approved_or_rejected_report(): void
{
$reporter = User::factory()->create();
$curator = User::factory()->create();
$curator->assignRole('curator');

foreach ([ReportStatus::APPROVED, ReportStatus::REJECTED] as $status) {
$report = $this->createReport($reporter, $status);
$this->assignCurator($report, $curator, 1);

$this->assertFalse(
$this->policy->update($curator, $report),
"Curator should not update {$status->value} reports"
);
}
}

public function test_reporter_cannot_update_submitted_report(): void
{
$reporter = User::factory()->create();
$report = $this->createReport($reporter, ReportStatus::SUBMITTED);

$this->assertFalse($this->policy->update($reporter, $report));
}

public function test_unassigned_curator_can_update_submitted_report_with_no_curator_one(): void
{
$reporter = User::factory()->create();
$curator = User::factory()->create();
$curator->assignRole('curator');

$report = $this->createReport($reporter, ReportStatus::SUBMITTED);

$this->assertTrue($this->policy->update($curator, $report));
}

private function createReport(User $reporter, ReportStatus $status): Report
{
return Report::create([
'title' => 'Test report',
'user_id' => $reporter->id,
'status' => $status->value,
'report_category' => ReportCategory::UPDATE->value,
'suggested_changes' => ['overall_changes' => [], 'curator' => []],
]);
}

private function assignCurator(Report $report, User $curator, int $curatorNumber): void
{
$report->curators()->attach($curator->id, [
'curator_number' => $curatorNumber,
'status' => null,
'comment' => null,
]);
}
}
Loading