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

FEATURE: Option for Notes to be Required on Asset Checkin/Checkout #15208

Merged
merged 32 commits into from
Nov 26, 2024

Conversation

akemidx
Copy link
Collaborator

@akemidx akemidx commented Aug 1, 2024

Description

This adds an option in the admin settings to turn on whether the Notes field is required on the asset checkin/checkout screens.

Screenshot 2024-08-01 at 5 08 46 PM Screenshot 2024-08-01 at 5 09 11 PM

26415

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

Copy link

what-the-diff bot commented Aug 1, 2024

PR Summary

  • Addition of New Settings Option
    A new setting option, named require_checkinout_notes, has been included. This will allow for more detailed check-in or check-out notes, which can be useful for precise tracking and record-keeping.

  • Configuration Changes
    The settings controller has been adjusted to incorporate the new require_checkinout_notes setting. These changes facilitate the control of this newly-introduced option.

  • Language File Update
    Updates have been made to a language file (general.php), including incorporating the new require_checkinout_notes setting. This allows for the new setting to be properly labeled and described.

  • Interface Changes for Check-in/Check-out
    Depending on whether the require_checkinout_notes setting is enabled, the check-in and check-out interfaces have been altered to include a text area for notes. This allows the user to provide detailed notes during check-in or check-out if the option is activated.

  • Settings Interface Update
    The settings interface has been updated to allow users to easily enable or disable the require_checkinout_notes setting. The update includes the addition of a checkbox for effortless toggling.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

Solid work overall 👍🏾

I didn't actually pull this down but have a couple ideas. Only the default value for require_checkinout_notes is one something I'm feeling strongly about.

resources/views/hardware/checkout.blade.php Outdated Show resolved Hide resolved
Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

Just the few small things in the comments

@snipe
Copy link
Owner

snipe commented Aug 20, 2024

@akemidx - any progress on this one?

@akemidx
Copy link
Collaborator Author

akemidx commented Aug 20, 2024

doing this one today

@akemidx
Copy link
Collaborator Author

akemidx commented Sep 4, 2024

validation added, but now some tests are failing. looks like something to do with delete user.

there is front end validation, in which the note field can't be empty, so that's squared away. just need to get the rest working

@akemidx akemidx requested a review from marcusmoore October 1, 2024 20:45
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

Looking good so far but a few minor changes por favor.

app/Http/Controllers/Assets/AssetCheckinController.php Outdated Show resolved Hide resolved
app/Http/Requests/AssetCheckinRequest.php Outdated Show resolved Hide resolved
Comment on lines 105 to 108
if($settings->require_checkinout_notes=="1" && (is_null($request->note))) {
return redirect()->to("hardware/$assetId/checkout")->with('error', trans('admin/hardware/message.update.no_note'));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above but it'll go in AssetCheckoutRequest.

…c-26415

# Conflicts:
#	app/Http/Controllers/Assets/AssetCheckinController.php
#	app/Http/Controllers/Assets/AssetCheckoutController.php
#	app/Http/Requests/AssetCheckinRequest.php
@akemidx akemidx requested a review from uberbrady as a code owner October 2, 2024 22:52
@snipe
Copy link
Owner

snipe commented Oct 7, 2024

Screenshot 2024-10-07 at 12 36 36 PM

😬

Looks like you've got some failing tests too.

@akemidx akemidx requested a review from marcusmoore November 13, 2024 23:47
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

One changed needed for validation

app/Http/Requests/AssetCheckinRequest.php Outdated Show resolved Hide resolved
resources/lang/en-US/admin/hardware/message.php Outdated Show resolved Hide resolved
@akemidx akemidx requested a review from marcusmoore November 26, 2024 20:10
@snipe
Copy link
Owner

snipe commented Nov 26, 2024

This looks great - nice work!

@snipe snipe merged commit 82108f8 into snipe:develop Nov 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants