-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Fixed accessories and consumables not being logged correctly during bulk check-in #15221
Merged
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
7161b64
Add failing test for accessories and consumables checkin
marcusmoore 44dbbeb
Add accessory and consumable specific checkin methods
marcusmoore 78a0417
Add another user into the mix
marcusmoore 480e4f3
Improve readability
marcusmoore bfebcdc
Improve variable name
marcusmoore 364775d
Improve readability
marcusmoore 4ed3347
Add permission check
marcusmoore 5ecdb7e
Add validation test
marcusmoore e3049ff
Remove comment
marcusmoore b06501d
Add assertions
marcusmoore 67b3ab8
Add todo comments
marcusmoore 8a206a6
Add assertion
marcusmoore 5786ff5
Add assertion for success message
marcusmoore fc405d9
Assert redirected to correct place
marcusmoore 5cd9dd4
Add assertion to ensure user cannot perform bulk actions on self
marcusmoore d1bb3ef
Scaffold two tests
marcusmoore 16aa475
Implement test for assets
marcusmoore 59b7d5d
Remove comment
marcusmoore 1acd24f
Re-order helpers
marcusmoore a556932
Move test class
marcusmoore 96fafa6
Improve readability
marcusmoore 35e7a31
Implement test case
marcusmoore 392d344
Remove code handled by ConsumableAssignment:: call above
marcusmoore 1c664af
Remove todo outside of scope
marcusmoore 01e4382
Formatting
marcusmoore 17eccfc
Formatting
marcusmoore f2b78d1
Merge branch 'develop' into fixes/bulk-checkin-logging
marcusmoore 94e00b8
Use new accessory checkout relationship
marcusmoore 2a8ac81
Fix namespace
marcusmoore a220986
Switch to snake case
marcusmoore 41437e4
Merge branch 'develop' into fixes/bulk-checkin-logging
marcusmoore d03baa4
Make diff cleaner
marcusmoore 4a2d2f7
Improve variable name
marcusmoore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
260 changes: 260 additions & 0 deletions
260
tests/Feature/Users/Ui/BulkActions/BulkDeleteUsersTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,260 @@ | ||
<?php | ||
|
||
namespace Tests\Feature\Users\Ui\BulkActions; | ||
|
||
use App\Models\Accessory; | ||
use App\Models\Asset; | ||
use App\Models\Consumable; | ||
use App\Models\License; | ||
use App\Models\LicenseSeat; | ||
use App\Models\Statuslabel; | ||
use App\Models\User; | ||
use Illuminate\Database\Eloquent\Model; | ||
use Tests\TestCase; | ||
|
||
class BulkDeleteUsersTest extends TestCase | ||
{ | ||
public function test_requires_correct_permission() | ||
{ | ||
$this->actingAs(User::factory()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
User::factory()->create()->id, | ||
], | ||
'status_id' => Statuslabel::factory()->create()->id, | ||
]) | ||
->assertForbidden(); | ||
} | ||
|
||
public function test_validation() | ||
{ | ||
$user = User::factory()->create(); | ||
Asset::factory()->assignedToUser($user)->create(); | ||
|
||
$actor = $this->actingAs(User::factory()->editUsers()->create()); | ||
|
||
// "ids" required | ||
$actor->post(route('users/bulksave'), [ | ||
// 'ids' => [ | ||
// $user->id, | ||
// ], | ||
'status_id' => Statuslabel::factory()->create()->id, | ||
])->assertSessionHas('error')->assertRedirect(); | ||
|
||
// "status_id" needed when provided users have assets associated | ||
$actor->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$user->id, | ||
], | ||
// 'status_id' => Statuslabel::factory()->create()->id, | ||
])->assertSessionHas('error')->assertRedirect(); | ||
} | ||
|
||
public function test_cannot_perform_bulk_actions_on_self() | ||
{ | ||
$actor = User::factory()->editUsers()->create(); | ||
|
||
$this->actingAs($actor) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$actor->id, | ||
], | ||
'delete_user' => '1', | ||
]) | ||
->assertRedirect(route('users.index')) | ||
->assertSessionHas('success', trans('general.bulk_checkin_delete_success')); | ||
|
||
$this->assertNotSoftDeleted($actor); | ||
} | ||
|
||
public function test_accessories_can_be_bulk_checked_in() | ||
{ | ||
[$accessoryA, $accessoryB] = Accessory::factory()->count(2)->create(); | ||
[$userA, $userB, $userC] = User::factory()->count(3)->create(); | ||
|
||
// Add checkouts for multiple accessories to multiple users to get different ids in the mix | ||
$this->attachAccessoryToUsers($accessoryA, [$userA, $userB, $userC]); | ||
$this->attachAccessoryToUsers($accessoryB, [$userA, $userB]); | ||
|
||
$this->actingAs(User::factory()->editUsers()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$userA->id, | ||
$userC->id, | ||
], | ||
'status_id' => Statuslabel::factory()->create()->id, | ||
]) | ||
->assertRedirect(route('users.index')); | ||
|
||
$this->assertTrue($userA->fresh()->accessories->isEmpty()); | ||
$this->assertTrue($userB->fresh()->accessories->isNotEmpty()); | ||
$this->assertTrue($userC->fresh()->accessories->isEmpty()); | ||
|
||
// These assertions check against a bug where the wrong value from | ||
// accessories_users was being populated in action_logs.item_id. | ||
$this->assertActionLogCheckInEntryFor($userA, $accessoryA); | ||
$this->assertActionLogCheckInEntryFor($userA, $accessoryB); | ||
$this->assertActionLogCheckInEntryFor($userC, $accessoryA); | ||
} | ||
|
||
public function test_assets_can_be_bulk_checked_in() | ||
{ | ||
[$userA, $userB, $userC] = User::factory()->count(3)->create(); | ||
|
||
$assetForUserA = $this->assignAssetToUser($userA); | ||
$lonelyAsset = $this->assignAssetToUser($userB); | ||
$assetForUserC = $this->assignAssetToUser($userC); | ||
|
||
$this->actingAs(User::factory()->editUsers()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$userA->id, | ||
$userC->id, | ||
], | ||
'status_id' => Statuslabel::factory()->create()->id, | ||
]) | ||
->assertRedirect(route('users.index')); | ||
|
||
$this->assertTrue($userA->fresh()->assets->isEmpty()); | ||
$this->assertTrue($userB->fresh()->assets->isNotEmpty()); | ||
$this->assertTrue($userC->fresh()->assets->isEmpty()); | ||
|
||
$this->assertActionLogCheckInEntryFor($userA, $assetForUserA); | ||
$this->assertActionLogCheckInEntryFor($userC, $assetForUserC); | ||
} | ||
|
||
public function test_consumables_can_be_bulk_checked_in() | ||
{ | ||
[$consumableA, $consumableB] = Consumable::factory()->count(2)->create(); | ||
[$userA, $userB, $userC] = User::factory()->count(3)->create(); | ||
|
||
// Add checkouts for multiple consumables to multiple users to get different ids in the mix | ||
$this->attachConsumableToUsers($consumableA, [$userA, $userB, $userC]); | ||
$this->attachConsumableToUsers($consumableB, [$userA, $userB]); | ||
|
||
$this->actingAs(User::factory()->editUsers()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$userA->id, | ||
$userC->id, | ||
], | ||
'status_id' => Statuslabel::factory()->create()->id, | ||
]) | ||
->assertRedirect(route('users.index')); | ||
|
||
$this->assertTrue($userA->fresh()->consumables->isEmpty()); | ||
$this->assertTrue($userB->fresh()->consumables->isNotEmpty()); | ||
$this->assertTrue($userC->fresh()->consumables->isEmpty()); | ||
|
||
// These assertions check against a bug where the wrong value from | ||
// consumables_users was being populated in action_logs.item_id. | ||
$this->assertActionLogCheckInEntryFor($userA, $consumableA); | ||
$this->assertActionLogCheckInEntryFor($userA, $consumableB); | ||
$this->assertActionLogCheckInEntryFor($userC, $consumableA); | ||
} | ||
|
||
public function test_license_seats_can_be_bulk_checked_in() | ||
{ | ||
[$userA, $userB, $userC] = User::factory()->count(3)->create(); | ||
|
||
$licenseSeatForUserA = LicenseSeat::factory()->assignedToUser($userA)->create(); | ||
$lonelyLicenseSeat = LicenseSeat::factory()->assignedToUser($userB)->create(); | ||
$licenseSeatForUserC = LicenseSeat::factory()->assignedToUser($userC)->create(); | ||
|
||
$this->actingAs(User::factory()->editUsers()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$userA->id, | ||
$userC->id, | ||
], | ||
]) | ||
->assertRedirect(route('users.index')) | ||
->assertSessionHas('success', trans('general.bulk_checkin_success')); | ||
|
||
$this->assertDatabaseMissing('license_seats', [ | ||
'license_id' => $licenseSeatForUserA->license->id, | ||
'assigned_to' => $userA->id, | ||
]); | ||
|
||
$this->assertDatabaseMissing('license_seats', [ | ||
'license_id' => $licenseSeatForUserC->license->id, | ||
'assigned_to' => $userC->id, | ||
]); | ||
|
||
// Slightly different from the other assertions since we use | ||
// the license and not the license seat in this case. | ||
$this->assertDatabaseHas('action_logs', [ | ||
'action_type' => 'checkin from', | ||
'target_id' => $userA->id, | ||
'target_type' => User::class, | ||
'note' => 'Bulk checkin items', | ||
'item_type' => License::class, | ||
'item_id' => $licenseSeatForUserA->license->id, | ||
]); | ||
|
||
$this->assertDatabaseHas('action_logs', [ | ||
'action_type' => 'checkin from', | ||
'target_id' => $userC->id, | ||
'target_type' => User::class, | ||
'note' => 'Bulk checkin items', | ||
'item_type' => License::class, | ||
'item_id' => $licenseSeatForUserC->license->id, | ||
]); | ||
} | ||
|
||
public function test_users_can_be_deleted_in_bulk() | ||
{ | ||
[$userA, $userB, $userC] = User::factory()->count(3)->create(); | ||
|
||
$this->actingAs(User::factory()->editUsers()->create()) | ||
->post(route('users/bulksave'), [ | ||
'ids' => [ | ||
$userA->id, | ||
$userC->id, | ||
], | ||
'delete_user' => '1', | ||
]) | ||
->assertRedirect(route('users.index')) | ||
->assertSessionHas('success', trans('general.bulk_checkin_delete_success')); | ||
|
||
$this->assertSoftDeleted($userA); | ||
$this->assertNotSoftDeleted($userB); | ||
$this->assertSoftDeleted($userC); | ||
} | ||
|
||
private function assignAssetToUser(User $user): Asset | ||
{ | ||
return Asset::factory()->assignedToUser($user)->create(); | ||
} | ||
|
||
private function attachAccessoryToUsers(Accessory $accessory, array $users): void | ||
{ | ||
foreach ($users as $user) { | ||
$accessoryCheckout = $accessory->checkouts()->make(); | ||
$accessoryCheckout->assignedTo()->associate($user); | ||
$accessoryCheckout->save(); | ||
} | ||
} | ||
|
||
private function attachConsumableToUsers(Consumable $consumable, array $users): void | ||
{ | ||
foreach ($users as $user) { | ||
$consumable->users()->attach($consumable->id, [ | ||
'consumable_id' => $consumable->id, | ||
'assigned_to' => $user->id, | ||
]); | ||
} | ||
} | ||
|
||
private function assertActionLogCheckInEntryFor(User $user, Model $model): void | ||
{ | ||
$this->assertDatabaseHas('action_logs', [ | ||
'action_type' => 'checkin from', | ||
'target_id' => $user->id, | ||
'target_type' => User::class, | ||
'note' => 'Bulk checkin items', | ||
'item_type' => get_class($model), | ||
'item_id' => $model->id, | ||
]); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not loving the duplication but there is enough difference that using explicit method names instead of passing a handful of parameters is cleaner I think