Merge pull request #15016 from snipe/improved_user_merge

Fixed #15005 - Improvements  on user merge
This commit is contained in:
snipe 2024-07-03 23:19:50 +01:00 committed by GitHub
commit a6d04509a5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 481 additions and 14 deletions

View file

@ -2,6 +2,7 @@
namespace App\Console\Commands; namespace App\Console\Commands;
use App\Events\UserMerged;
use App\Models\User; use App\Models\User;
use Carbon\Carbon; use Carbon\Carbon;
use Illuminate\Console\Command; use Illuminate\Console\Command;
@ -51,7 +52,7 @@ class MergeUsersByUsername extends Command
$bad_users = User::where('username', '=', trim($parts[0])) $bad_users = User::where('username', '=', trim($parts[0]))
->whereNull('deleted_at') ->whereNull('deleted_at')
->with('assets', 'manager', 'userlog', 'licenses', 'consumables', 'accessories', 'managedLocations') ->with('assets', 'manager', 'userlog', 'licenses', 'consumables', 'accessories', 'managedLocations','uploads', 'acceptances')
->get(); ->get();
@ -105,10 +106,26 @@ class MergeUsersByUsername extends Command
$managedLocation->save(); $managedLocation->save();
} }
foreach ($bad_user->uploads as $upload) {
$this->info('Updating upload log record '.$upload->id.' to user '.$user->id);
$upload->item_id = $user->id;
$upload->save();
}
foreach ($bad_user->acceptances as $acceptance) {
$this->info('Updating acceptance log record '.$acceptance->id.' to user '.$user->id);
$acceptance->item_id = $user->id;
$acceptance->save();
}
// Mark the user as deleted // Mark the user as deleted
$this->info('Marking the user as deleted'); $this->info('Marking the user as deleted');
$bad_user->deleted_at = Carbon::now()->timestamp; $bad_user->deleted_at = Carbon::now()->timestamp;
$bad_user->save(); $bad_user->save();
event(new UserMerged($bad_user, $user, null));
} }
} }
} }

View file

@ -15,7 +15,7 @@ class UserMerged
* *
* @return void * @return void
*/ */
public function __construct(User $from_user, User $to_user, User $admin) public function __construct(User $from_user, User $to_user, ?User $admin)
{ {
$this->merged_from = $from_user; $this->merged_from = $from_user;
$this->merged_to = $to_user; $this->merged_to = $to_user;

View file

@ -42,7 +42,7 @@ class BulkUsersController extends Controller
// Get the list of affected users // Get the list of affected users
$user_raw_array = request('ids'); $user_raw_array = request('ids');
$users = User::whereIn('id', $user_raw_array) $users = User::whereIn('id', $user_raw_array)
->with('groups', 'assets', 'licenses', 'accessories')->get(); ->with('assets', 'manager', 'userlog', 'licenses', 'consumables', 'accessories', 'managedLocations','uploads', 'acceptances')->get();
// bulk edit, display the bulk edit form // bulk edit, display the bulk edit form
if ($request->input('bulk_actions') == 'edit') { if ($request->input('bulk_actions') == 'edit') {
@ -317,7 +317,7 @@ class BulkUsersController extends Controller
// Get the users // Get the users
$merge_into_user = User::find($request->input('merge_into_id')); $merge_into_user = User::find($request->input('merge_into_id'));
$users_to_merge = User::whereIn('id', $user_ids_to_merge)->with('assets', 'licenses', 'consumables','accessories')->get(); $users_to_merge = User::whereIn('id', $user_ids_to_merge)->with('assets', 'manager', 'userlog', 'licenses', 'consumables', 'accessories', 'managedLocations','uploads', 'acceptances')->get();
$admin = User::find(Auth::user()->id); $admin = User::find(Auth::user()->id);
// Walk users // Walk users
@ -344,10 +344,20 @@ class BulkUsersController extends Controller
} }
foreach ($user_to_merge->userlog as $log) { foreach ($user_to_merge->userlog as $log) {
$log->target_id = $user_to_merge->id; $log->target_id = $merge_into_user->id;
$log->save(); $log->save();
} }
foreach ($user_to_merge->uploads as $upload) {
$upload->item_id = $merge_into_user->id;
$upload->save();
}
foreach ($user_to_merge->acceptances as $acceptance) {
$acceptance->item_id = $merge_into_user->id;
$acceptance->save();
}
User::where('manager_id', '=', $user_to_merge->id)->update(['manager_id' => $merge_into_user->id]); User::where('manager_id', '=', $user_to_merge->id)->update(['manager_id' => $merge_into_user->id]);
foreach ($user_to_merge->managedLocations as $managedLocation) { foreach ($user_to_merge->managedLocations as $managedLocation) {
@ -356,7 +366,6 @@ class BulkUsersController extends Controller
} }
$user_to_merge->delete(); $user_to_merge->delete();
//$user_to_merge->save();
event(new UserMerged($user_to_merge, $merge_into_user, $admin)); event(new UserMerged($user_to_merge, $merge_into_user, $admin));

View file

@ -111,7 +111,7 @@ class LogListener
$logaction->target_type = User::class; $logaction->target_type = User::class;
$logaction->action_type = 'merged'; $logaction->action_type = 'merged';
$logaction->note = trans('general.merged_log_this_user_from', $to_from_array); $logaction->note = trans('general.merged_log_this_user_from', $to_from_array);
$logaction->user_id = $event->admin->id; $logaction->user_id = $event->admin->id ?? null;
$logaction->save(); $logaction->save();
// Add a record to the users being merged TO // Add a record to the users being merged TO
@ -122,7 +122,7 @@ class LogListener
$logaction->item_type = User::class; $logaction->item_type = User::class;
$logaction->action_type = 'merged'; $logaction->action_type = 'merged';
$logaction->note = trans('general.merged_log_this_user_into', $to_from_array); $logaction->note = trans('general.merged_log_this_user_into', $to_from_array);
$logaction->user_id = $event->admin->id; $logaction->user_id = $event->admin->id ?? null;
$logaction->save(); $logaction->save();

View file

@ -481,8 +481,6 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
/** /**
* Establishes the user -> uploads relationship * Establishes the user -> uploads relationship
* *
* @todo I don't think we use this?
*
* @author A. Gianotto <snipe@snipe.net> * @author A. Gianotto <snipe@snipe.net>
* @since [v3.0] * @since [v3.0]
* @return \Illuminate\Database\Eloquent\Relations\Relation * @return \Illuminate\Database\Eloquent\Relations\Relation
@ -496,6 +494,21 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
->orderBy('created_at', 'desc'); ->orderBy('created_at', 'desc');
} }
/**
* Establishes the user -> acceptances relationship
*
* @author A. Gianotto <snipe@snipe.net>
* @since [v7.0.7]
* @return \Illuminate\Database\Eloquent\Relations\Relation
*/
public function acceptances()
{
return $this->hasMany(\App\Models\Actionlog::class, 'target_id')
->where('target_type', self::class)
->where('action_type', '=', 'accepted')
->orderBy('created_at', 'desc');
}
/** /**
* Establishes the user -> requested assets relationship * Establishes the user -> requested assets relationship
* *

View file

@ -105,4 +105,64 @@ class ActionlogFactory extends Factory
]; ];
}); });
} }
public function filesUploaded()
{
return $this->state(function () {
return [
'created_at' => $this->faker->dateTimeBetween('-1 years', 'now', date_default_timezone_get()),
'action_type' => 'uploaded',
'item_type' => User::class,
'filename' => $this->faker->unixTime('now'),
];
});
}
public function acceptedSignature()
{
return $this->state(function () {
$asset = Asset::factory()->create();
return [
'created_at' => $this->faker->dateTimeBetween('-1 years', 'now', date_default_timezone_get()),
'action_type' => 'accepted',
'item_id' => $asset->id,
'item_type' => Asset::class,
'target_type' => User::class,
'accept_signature' => $this->faker->unixTime('now'),
];
});
}
public function acceptedEula()
{
return $this->state(function () {
$asset = Asset::factory()->create();
return [
'created_at' => $this->faker->dateTimeBetween('-1 years', 'now', date_default_timezone_get()),
'action_type' => 'accepted',
'item_id' => $asset->id,
'item_type' => Asset::class,
'target_type' => User::class,
'filename' => $this->faker->unixTime('now'),
];
});
}
public function userUpdated()
{
return $this->state(function () {
return [
'created_at' => $this->faker->dateTimeBetween('-1 years', 'now', date_default_timezone_get()),
'action_type' => 'update',
'target_type' => User::class,
'item_type' => User::class,
];
});
}
} }

View file

@ -7,6 +7,7 @@ use App\Models\Company;
use App\Models\Consumable; use App\Models\Consumable;
use App\Models\Manufacturer; use App\Models\Manufacturer;
use App\Models\User; use App\Models\User;
use Carbon\Carbon;
use Illuminate\Database\Eloquent\Factories\Factory; use Illuminate\Database\Eloquent\Factories\Factory;
use App\Models\Supplier; use App\Models\Supplier;
@ -116,4 +117,16 @@ class ConsumableFactory extends Factory
$consumable->category->update(['require_acceptance' => 1]); $consumable->category->update(['require_acceptance' => 1]);
}); });
} }
public function checkedOutToUser(User $user = null)
{
return $this->afterCreating(function (Consumable $consumable) use ($user) {
$consumable->users()->attach($consumable->id, [
'consumable_id' => $consumable->id,
'created_at' => Carbon::now(),
'user_id' => User::factory()->create()->id,
'assigned_to' => $user->id ?? User::factory()->create()->id,
]);
});
}
} }

View file

@ -66,6 +66,10 @@
<i class="fas fa-tint fa-fw" aria-hidden="true" style="font-size: 17px;"></i> <i class="fas fa-tint fa-fw" aria-hidden="true" style="font-size: 17px;"></i>
<span class="sr-only">{{ trans('general.consumables') }}</span> <span class="sr-only">{{ trans('general.consumables') }}</span>
</th> </th>
<th class="col-md-1 text-right">
<i class="fas fa-paperclip fa-fw" aria-hidden="true" style="font-size: 17px;"></i>
<span class="sr-only">{{ trans('general.files') }}</span>
</th>
</tr> </tr>
</thead> </thead>
<tbody> <tbody>
@ -93,16 +97,19 @@
@endforeach @endforeach
</td> </td>
<td class="text-right"> <td class="text-right">
{{ number_format($user->assets()->count()) }} {{ number_format($user->assets->count()) }}
</td> </td>
<td class="text-right"> <td class="text-right">
{{ number_format($user->accessories()->count()) }} {{ number_format($user->accessories->count()) }}
</td> </td>
<td class="text-right"> <td class="text-right">
{{ number_format($user->licenses()->count()) }} {{ number_format($user->licenses->count()) }}
</td> </td>
<td class="text-right"> <td class="text-right">
{{ number_format($user->consumables()->count()) }} {{ number_format($user->consumables->count()) }}
</td>
<td class="text-right">
{{ number_format($user->uploads->count()) }}
</td> </td>
</tr> </tr>
@endforeach @endforeach

View file

@ -0,0 +1,135 @@
<?php
namespace Tests\Feature\Console;
use App\Models\Accessory;
use App\Models\Asset;
use App\Models\Consumable;
use App\Models\LicenseSeat;
use App\Models\User;
use App\Models\Actionlog;
use Tests\TestCase;
class MergeUsersTest extends TestCase
{
public function testAssetsAreTransferredOnUserMerge()
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Asset::factory()->count(3)->assignedToUser($user1)->create();
Asset::factory()->count(3)->assignedToUser($user_to_merge_into)->create();
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->assets->count());
$this->assertEquals(0, $user1->refresh()->assets->count());
}
public function testLicensesAreTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
LicenseSeat::factory()->count(3)->create(['assigned_to' => $user1->id]);
LicenseSeat::factory()->count(3)->create(['assigned_to' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->licenses->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->licenses->count());
$this->assertEquals(0, $user1->refresh()->licenses->count());
}
public function testAccessoriesTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Accessory::factory()->count(3)->checkedOutToUser($user1)->create();
Accessory::factory()->count(3)->checkedOutToUser($user_to_merge_into)->create();
$this->assertEquals(3, $user_to_merge_into->refresh()->accessories->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->accessories->count());
$this->assertEquals(0, $user1->refresh()->accessories->count());
}
public function testConsumablesTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Consumable::factory()->count(3)->checkedOutToUser($user1)->create();
Consumable::factory()->count(3)->checkedOutToUser($user_to_merge_into)->create();
$this->assertEquals(3, $user_to_merge_into->refresh()->consumables->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->consumables->count());
$this->assertEquals(0, $user1->refresh()->consumables->count());
}
public function testFilesAreTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Actionlog::factory()->count(3)->filesUploaded()->create(['item_id' => $user1->id]);
Actionlog::factory()->count(3)->filesUploaded()->create(['item_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->uploads->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->uploads->count());
$this->assertEquals(0, $user1->refresh()->uploads->count());
}
public function testAcceptancesAreTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Actionlog::factory()->count(3)->acceptedSignature()->create(['target_id' => $user1->id]);
Actionlog::factory()->count(3)->acceptedSignature()->create(['target_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->acceptances->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
$this->assertEquals(6, $user_to_merge_into->refresh()->acceptances->count());
$this->assertEquals(0, $user1->refresh()->acceptances->count());
}
public function testUserUpdateHistoryIsTransferredOnUserMerge(): void
{
$user1 = User::factory()->create(['username' => 'user1']);
$user_to_merge_into = User::factory()->create(['username' => 'user1@example.com']);
Actionlog::factory()->count(3)->userUpdated()->create(['target_id' => $user1->id, 'item_id' => $user1->id]);
Actionlog::factory()->count(3)->userUpdated()->create(['target_id' => $user_to_merge_into->id, 'item_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->userlog->count());
$this->artisan('snipeit:merge-users')->assertExitCode(0);
// This needs to be more than the otherwise expected because the merge action itself is logged for the two merging users
$this->assertEquals(7, $user_to_merge_into->refresh()->userlog->count());
$this->assertEquals(1, $user1->refresh()->userlog->count());
}
}

View file

@ -0,0 +1,213 @@
<?php
namespace Tests\Feature\Users\Ui;
use App\Models\Accessory;
use App\Models\Asset;
use App\Models\Consumable;
use App\Models\LicenseSeat;
use App\Models\User;
use App\Models\Actionlog;
use Tests\TestCase;
class MergeUsersTest extends TestCase
{
public function testAssetsAreTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Asset::factory()->count(3)->assignedToUser($user1)->create();
Asset::factory()->count(3)->assignedToUser($user2)->create();
Asset::factory()->count(3)->assignedToUser($user_to_merge_into)->create();
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->assets->count());
$this->assertEquals(0, $user1->refresh()->assets->count());
$this->assertEquals(0, $user2->refresh()->assets->count());
}
public function testLicensesAreTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
LicenseSeat::factory()->count(3)->create(['assigned_to' => $user1->id]);
LicenseSeat::factory()->count(3)->create(['assigned_to' => $user2->id]);
LicenseSeat::factory()->count(3)->create(['assigned_to' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->licenses->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->licenses->count());
$this->assertEquals(0, $user1->refresh()->licenses->count());
$this->assertEquals(0, $user2->refresh()->licenses->count());
}
public function testAccessoriesTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Accessory::factory()->count(3)->checkedOutToUser($user1)->create();
Accessory::factory()->count(3)->checkedOutToUser($user2)->create();
Accessory::factory()->count(3)->checkedOutToUser($user_to_merge_into)->create();
$this->assertEquals(3, $user_to_merge_into->refresh()->accessories->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->accessories->count());
$this->assertEquals(0, $user1->refresh()->accessories->count());
$this->assertEquals(0, $user2->refresh()->accessories->count());
}
public function testConsumablesTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Consumable::factory()->count(3)->checkedOutToUser($user1)->create();
Consumable::factory()->count(3)->checkedOutToUser($user2)->create();
Consumable::factory()->count(3)->checkedOutToUser($user_to_merge_into)->create();
$this->assertEquals(3, $user_to_merge_into->refresh()->consumables->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->consumables->count());
$this->assertEquals(0, $user1->refresh()->consumables->count());
$this->assertEquals(0, $user2->refresh()->consumables->count());
}
public function testFilesAreTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Actionlog::factory()->count(3)->filesUploaded()->create(['item_id' => $user1->id]);
Actionlog::factory()->count(3)->filesUploaded()->create(['item_id' => $user2->id]);
Actionlog::factory()->count(3)->filesUploaded()->create(['item_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->uploads->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->uploads->count());
$this->assertEquals(0, $user1->refresh()->uploads->count());
$this->assertEquals(0, $user2->refresh()->uploads->count());
}
public function testAcceptancesAreTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Actionlog::factory()->count(3)->acceptedSignature()->create(['target_id' => $user1->id]);
Actionlog::factory()->count(3)->acceptedSignature()->create(['target_id' => $user2->id]);
Actionlog::factory()->count(3)->acceptedSignature()->create(['target_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->acceptances->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
$this->assertEquals(9, $user_to_merge_into->refresh()->acceptances->count());
$this->assertEquals(0, $user1->refresh()->acceptances->count());
$this->assertEquals(0, $user2->refresh()->acceptances->count());
}
public function testUserUpdateHistoryIsTransferredOnUserMerge()
{
$user1 = User::factory()->create();
$user2 = User::factory()->create();
$user_to_merge_into = User::factory()->create();
Actionlog::factory()->count(3)->userUpdated()->create(['target_id' => $user1->id, 'item_id' => $user1->id]);
Actionlog::factory()->count(3)->userUpdated()->create(['target_id' => $user2->id, 'item_id' => $user2->id]);
Actionlog::factory()->count(3)->userUpdated()->create(['target_id' => $user_to_merge_into->id, 'item_id' => $user_to_merge_into->id]);
$this->assertEquals(3, $user_to_merge_into->refresh()->userlog->count());
$response = $this->actingAs(User::factory()->editUsers()->viewUsers()->create())
->post(route('users.merge.save', $user1->id),
[
'ids_to_merge' => [$user1->id, $user2->id],
'merge_into_id' => $user_to_merge_into->id
])
->assertStatus(302)
->assertRedirect(route('users.index'));
$this->followRedirects($response)->assertSee('Success');
// This needs to be 2 more than the otherwise expected because the merge action itself is logged for the two merging users
$this->assertEquals(11, $user_to_merge_into->refresh()->userlog->count());
$this->assertEquals(2, $user1->refresh()->userlog->count());
$this->assertEquals(2, $user2->refresh()->userlog->count());
}
}