-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
FEATURE: Option for Notes to be Required on Asset Checkin/Checkout #15208
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
Conversation
PR Summary
|
marcusmoore
left a comment
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.
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.
database/migrations/2024_08_01_201721_add_required_notes_setting.php
Outdated
Show resolved
Hide resolved
snipe
left a comment
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.
Just the few small things in the comments
|
@akemidx - any progress on this one? |
|
doing this one today |
|
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 |
marcusmoore
left a comment
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.
Looking good so far but a few minor changes por favor.
| 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')); | ||
| } | ||
|
|
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.
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
marcusmoore
left a comment
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.
One changed needed for validation
|
This looks great - nice work! |

Description
This adds an option in the admin settings to turn on whether the
Notesfield is required on the asset checkin/checkout screens.26415
Type of change
Please delete options that are not relevant.
Checklist: