Skip to content
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

Allows check-ins of unreassignable licenses #16063

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
12 changes: 12 additions & 0 deletions app/Helpers/Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

namespace App\Helpers;
use App\Models\Accessory;
use App\Models\Actionlog;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unused and can be removed.

use App\Models\Asset;
use App\Models\AssetModel;
use App\Models\Component;
use App\Models\Consumable;
use App\Models\CustomField;
use App\Models\CustomFieldset;
use App\Models\Depreciation;
use App\Models\LicenseSeat;
use App\Models\Setting;
use App\Models\Statuslabel;
use App\Models\License;
Expand Down Expand Up @@ -1529,4 +1531,14 @@ static public function getRedirectOption($request, $id, $table, $item_id = null)
}
return redirect()->back()->with('error', trans('admin/hardware/message.checkout.error'));
}
public static function unReassignableCount($license)
{

if (!$license->reassignable) {
$count = LicenseSeat::where('unreassignable_seat', '=', true)
->where('license_id', '=', $license->id)
->count();
return $count;
}
}
Comment on lines +1534 to +1543
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method feels like it would be more at home on the actual License model. Is there any reason it needs to be a helper?

}
8 changes: 7 additions & 1 deletion app/Http/Controllers/Api/LicenseSeatsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@ public function update(Request $request, $licenseId, $seatId) : JsonResponse | a
// nothing to update
return response()->json(Helper::formatStandardApiResponse('success', $licenseSeat, trans('admin/licenses/message.update.success')));
}

if( $touched && $licenseSeat->unreassignable_seat) {
return response()->json(Helper::formatStandardApiResponse('error', $licenseSeat, trans('admin/licenses/message.checkout.unavailable')));
}
// the logging functions expect only one "target". if both asset and user are present in the request,
// we simply let assets take precedence over users...
if ($licenseSeat->isDirty('assigned_to')) {
Expand All @@ -137,6 +139,10 @@ public function update(Request $request, $licenseId, $seatId) : JsonResponse | a

if ($is_checkin) {
$licenseSeat->logCheckin($target, $request->input('note'));
if(!$licenseSeat->license->reassignable){
$licenseSeat->unreassignable_seat = true;
$licenseSeat->save();
}

return response()->json(Helper::formatStandardApiResponse('success', $licenseSeat, trans('admin/licenses/message.update.success')));
}
Expand Down
12 changes: 6 additions & 6 deletions app/Http/Controllers/Licenses/LicenseCheckinController.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,7 @@ public function store(Request $request, $seatId = null, $backTo = null)

$this->authorize('checkout', $license);

if (! $license->reassignable) {
// Not allowed to checkin
Session::flash('error', trans('admin/licenses/message.checkin.not_reassignable') . '.');

return redirect()->back()->withInput();
}

// Declare the rules for the form validation
$rules = [
Expand All @@ -100,13 +95,18 @@ public function store(Request $request, $seatId = null, $backTo = null)
$licenseSeat->assigned_to = null;
$licenseSeat->asset_id = null;
$licenseSeat->notes = $request->input('notes');
if (! $licenseSeat->license->reassignable) {
$licenseSeat->unreassignable_seat = true;
$licenseSeat->notes .= "\n" . trans('admin/licenses/message.checkin.not_reassignable') . '.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we need to append this message. I think we can remove it? If we're going to keep it then it should be in the API controller too.


}

session()->put(['redirect_option' => $request->get('redirect_option')]);


// Was the asset updated?
if ($licenseSeat->save()) {
event(new CheckoutableCheckedIn($licenseSeat, $return_to, auth()->user(), $request->input('notes')));
event(new CheckoutableCheckedIn($licenseSeat, $return_to, auth()->user(), $licenseSeat->notes));


return redirect()->to(Helper::getRedirectOption($request, $license->id, 'Licenses'))->with('success', trans('admin/licenses/message.checkin.success'));
Expand Down
13 changes: 10 additions & 3 deletions app/Http/Controllers/Licenses/LicensesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -248,16 +248,23 @@ public function show($licenseId = null)
}

$users_count = User::where('autoassign_licenses', '1')->count();
$total_seats_count = $license->totalSeatsByLicenseID();
$total_seats_count = (int) $license->totalSeatsByLicenseID();
$available_seats_count = $license->availCount()->count();
$checkedout_seats_count = ($total_seats_count - $available_seats_count);
$unreassignable_seats_count = Helper::unReassignableCount($license);
if(!$license->reassignable){
$checkedout_seats_count = ($total_seats_count - $available_seats_count - $unreassignable_seats_count );
}
else {
$checkedout_seats_count = ($total_seats_count - $available_seats_count);
}
Comment on lines +253 to +259
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic feels like it should live on the model since there's a good chance we'll want to use it elsewhere in the future. We can avoid copy/pasting by moving it there.


$this->authorize('view', $license);
return view('licenses.view', compact('license'))
->with('users_count', $users_count)
->with('total_seats_count', $total_seats_count)
->with('available_seats_count', $available_seats_count)
->with('checkedout_seats_count', $checkedout_seats_count);
->with('checkedout_seats_count', $checkedout_seats_count)
->with('unreassignable_seats_count', $unreassignable_seats_count);

}

Expand Down
15 changes: 15 additions & 0 deletions app/Http/Transformers/LicenseSeatsTransformer.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Http\Transformers;

use App\Models\Actionlog;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unused import can be removed.

use App\Models\License;
use App\Models\LicenseSeat;
use Illuminate\Support\Facades\Gate;
Expand Down Expand Up @@ -48,6 +49,7 @@ public function transformLicenseSeat(LicenseSeat $seat, $seat_count = 0)
'reassignable' => (bool) $seat->license->reassignable,
'notes' => e($seat->notes),
'user_can_checkout' => (($seat->assigned_to == '') && ($seat->asset_id == '')),
'disabled' => $seat->unreassignable_seat,
];

if ($seat_count != 0) {
Expand All @@ -66,4 +68,17 @@ public function transformLicenseSeat(LicenseSeat $seat, $seat_count = 0)

return $array;
}
// private function unReassignable($seat)
// {
// if (!$seat->license->reassignable) {
// $exists = Actionlog::where('action_type', '=', 'checkin from')
// ->where('item_id', '=', $seat->license->id)
// ->where('updated_at', '=', $seat->updated_at)
// ->exists();
// if($exists) {
// return true;
// }
// return false;
// }
// }
Comment on lines +71 to +83
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed.

}
4 changes: 2 additions & 2 deletions app/Http/Transformers/LicensesTransformer.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Illuminate\Support\Facades\Gate;
use Illuminate\Database\Eloquent\Collection;

class LicensesTransformer
class LicensesTransformer
Copy link
Collaborator

Choose a reason for hiding this comment

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

This extra space can be removed.

{
public function transformLicenses(Collection $licenses, $total)
{
Expand Down Expand Up @@ -37,7 +37,7 @@ public function transformLicense(License $license)
'notes' => Helper::parseEscapedMarkedownInline($license->notes),
'expiration_date' => Helper::getFormattedDateObject($license->expiration_date, 'date'),
'seats' => (int) $license->seats,
'free_seats_count' => (int) $license->free_seats_count,
'free_seats_count' => (int) $license->free_seats_count - Helper::unReassignableCount($license),
'min_amt' => ($license->min_amt) ? (int) ($license->min_amt) : null,
'license_name' => ($license->license_name) ? e($license->license_name) : null,
'license_email' => ($license->license_email) ? e($license->license_email) : null,
Expand Down
4 changes: 3 additions & 1 deletion app/Models/License.php
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to use proper casts more. Can we add $casts and set unreassignable_seat to be a boolean?

Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,8 @@ public function remaincount()
{
$total = $this->licenseSeatsCount;
$taken = $this->assigned_seats_count;
$diff = ($total - $taken);
$unreassignable = Helper::unReassignableCount($this);
$diff = ($total - $taken - $unreassignable);

return (int) $diff;
}
Expand Down Expand Up @@ -652,6 +653,7 @@ public function freeSeat()
{
return $this->licenseseats()
->whereNull('deleted_at')
->where('unreassignable_seat', '=', false)
->where(function ($query) {
$query->whereNull('assigned_to')
->whereNull('asset_id');
Expand Down
1 change: 1 addition & 0 deletions database/factories/LicenseSeatFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public function definition()
{
return [
'license_id' => License::factory(),
'unreassignable_seat' => false,
];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php
namespace App\Models\LicenseSeat;
use App\Models\LicenseSeat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unused import can be removed.

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unused import can be removed.

use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
/**
* Run the migrations.
*/
public function up(): void
{
Schema::table('license_seats', function (Blueprint $table) {
$table->addColumn('boolean', 'unreassignable_seat')->default(false)->after('assigned_to');
});
}
/**
* Reverse the migrations.
*/
public function down(): void
{
Schema::table('license_seats', function (Blueprint $table) {
$table->dropColumn('unreassignable_seat');
});
}
};
2 changes: 1 addition & 1 deletion resources/lang/en-US/admin/licenses/message.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

'checkin' => array(
'error' => 'There was an issue checking in the license. Please try again.',
'not_reassignable' => 'License not reassignable',
'not_reassignable' => 'Seat has been used',
'success' => 'The license was checked in successfully'
),

Expand Down
2 changes: 1 addition & 1 deletion resources/views/licenses/view.blade.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<x-icon type="seats" class="fa-2x" />
</span>
<span class="hidden-xs hidden-sm">{{ trans('admin/licenses/form.seats') }}</span>
<span class="badge badge-secondary">{{ number_format($license->availCount()->count()) }} / {{ number_format($license->seats) }}</span>
<span class="badge badge-secondary">{{ number_format($license->availCount()->count() - number_format($unreassignable_seats_count) ) }} / {{ number_format($license->seats)}}</span>

</a>
</li>
Expand Down
9 changes: 6 additions & 3 deletions resources/views/partials/bootstrap-table.blade.php
Original file line number Diff line number Diff line change
Expand Up @@ -431,13 +431,16 @@ function checkboxEnabledFormatter (value, row) {
// Checkouts need the license ID, checkins need the specific seat ID

function licenseSeatInOutFormatter(value, row) {
if(row.disabled) {
return '<a href="{{ config('app.url') }}/licenses/' + row.id + '/checkin" class="btn btn-sm bg-maroon disabled" data-tooltip="true" title="{{ trans('general.checkin_tooltip') }}">{{ trans('general.checkout') }}</a>';
} else
// The user is allowed to check the license seat out and it's available
if ((row.available_actions.checkout === true) && (row.user_can_checkout === true) && ((!row.asset_id) && (!row.assigned_to))) {
return '<a href="{{ config('app.url') }}/licenses/' + row.license_id + '/checkout/'+row.id+'" class="btn btn-sm bg-maroon" data-tooltip="true" title="{{ trans('general.checkout_tooltip') }}">{{ trans('general.checkout') }}</a>';
} else {
return '<a href="{{ config('app.url') }}/licenses/' + row.id + '/checkin" class="btn btn-sm bg-purple" data-tooltip="true" title="{{ trans('general.checkin_tooltip') }}">{{ trans('general.checkin') }}</a>';
}

else {
return '<a href="{{ config('app.url') }}/licenses/' + row.id + '/checkin" class="btn btn-sm bg-purple" data-tooltip="true" title="{{ trans('general.checkin_tooltip') }}">{{ trans('general.checkin') }}</a>';
}
}

function genericCheckinCheckoutFormatter(destination) {
Expand Down
12 changes: 5 additions & 7 deletions tests/Feature/Checkins/Ui/LicenseCheckinTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,19 @@ public function testCheckingInLicenseRequiresCorrectPermission()
->assertForbidden();
}

public function testCannotCheckinNonReassignableLicense()
public function testNonReassignableLicenseSeatIsUnavailable()
{
$licenseSeat = LicenseSeat::factory()
->notReassignable()
->assignedToUser()
->create();

$this->actingAs(User::factory()->checkoutLicenses()->create())
->post(route('licenses.checkin.save', $licenseSeat), [
'notes' => 'my note',
'redirect_option' => 'index',
])
->assertSessionHas('error', trans('admin/licenses/message.checkin.not_reassignable') . '.');
->post(route('licenses.checkin.save', $licenseSeat));

$licenseSeat->refresh();

$this->assertNotNull($licenseSeat->fresh()->assigned_to);
$this->assertEquals(1, $licenseSeat->unreassignable_seat);
}

public function testCannotCheckinLicenseThatIsNotAssigned()
Expand Down
Loading