Feature: Ad-Hoc Notes on Assets #15525
Conversation
PR Summary
|
|
note: assets need to be rebuilt for note modal to close on save |
|
Taking a look at this now 👍🏾 |
| if(!id || !name) { | ||
| console.error("Could not find resulting name or ID from modal-create. Name: "+name+", id: "+id); | ||
| return false; | ||
| if(!hasnopayload) { |
There was a problem hiding this comment.
I'm not sure why we'd need this? The modal is just a form post
There was a problem hiding this comment.
The note that is added doesn't have an id (it's not an actual modal) and the existing code expects the response to have an id (var id = result.payload.id). The if check allows getting around that chunk of expectations without re-writing too much of the existing functionality.
There was a problem hiding this comment.
It should not require any payload. It should post to the controller, same as files and every other modal (except for the dynamic ones, via the "new" buttons).
There was a problem hiding this comment.
Without looking into it, I think this follows the same pattern, but I think we might be talking about different things?
The existing modal code expects an id to come back in the response after the thing is created:
In this implementation, notes don't actually have an id because they are stored in the action log so the introduction of this parameter is to skip the existing logic that expects it in the response.
In other words, without the added if check we would hit the return false on line 129 and the modal wouldn't close even though the note was saved. I suggested this approach as a way to avoid re-writing too much of the existing modal code.
There was a problem hiding this comment.
You’re looking at the bespoke modal code - look at the regular file upload modal. There is no callback needed here. It’s just a regular form in a modal
There was a problem hiding this comment.
Ah I see what you're saying now. Thanks for diving deeper.
The benefit of the current approach is that we're exposing and hitting a dedicated API endpoint for creating notes but we can switch that over if we don't want to do that?
@akemidx if we do switch it over then instead of doing onsubmit="return false" in the add-note template we can do what the upload-file template does and fire off a regular form submission which results in the page refreshing. Then the new stuff in snipeit_modals.js can be removed.
There was a problem hiding this comment.
Where did we net out on decisions here?
| @@ -184,7 +184,9 @@ public static function icon($type) { | |||
| return 'fa-regular fa-id-card'; | |||
| case 'department' : | |||
| return 'fa-solid fa-building-user'; | |||
|
|
|||
| case 'note': | |||
There was a problem hiding this comment.
this is called at line 260 on the hardware.view blade
<x-icon type="note" />
it gives us the icon on the Add Note button
There was a problem hiding this comment.
this mimics the rest of our icon typing in the IconHelper such as
case 'assets':
case 'asset':
return 'fas fa-barcode';
case 'accessories':
case 'accessory':
return 'far fa-keyboard';
|
See my comments. I also think probably "Add Note" as a title for the modal and "Add Note" as the form prompt seems redundant. I'd either change the text or remove the label. |
added pull right to save
|
@uberbrady and I have been discussing this PR (and feature overall) and I think we're sort of split on 1) whether the user should be able to edit a note and 2) if yes to the first question, are we actually going to edit the I'm okay accepting it (when the outstanding questions have been addressed) without an edit function, but it will be something we'll need to add soon after IMHO. We can discuss this on the dev meeting later today tho. I personally do not like to make exceptions except when absolutely necessary, and |
|
From a high level, to me, it seems like a user should be able to edit a note and have that logged as an action. At a lower level, I heavily lean towards not modifying action logs. As for the implementation, the original version of this had Note as an eloquent model. Maybe going back to that method would make this easier now and in the future? (I forget why we moved away from the eloquent model approach so it might be totally valid that it won't work.) |
|
It does seem like it could make sense to have Notes be an eloquent model, since as soon as we release this for assets, folks are going to want it everywhere. Weird tho since it doesn't have its own table, as most Eloquent models do. I agree on not changing the behavior of action_logs. It just feels kinda gross and weird. I think we mark the old one as deleted (also a first for us, but less destructive) and then add a new one. |
|
We should definitely NOT edit the action logs. if anything it would be another entry to the action logs when note edit is made. And while this can get messy, at some point you just have to let people constantly edit their notes and make logs. (i've actually used logs to see users/sytem/etc fighting back and forth over an edit.) Soft delete could work. There is another idea that I have that is a separate feature, and I dare not unleash that can of worms just yet. I think the Eloquent model approach was just overly complicated (at the time) for what we were doing and we kinda scoped down after a dev meeting or something. could also have been something to do with the modal, but my notes are a bit iffy about why we switched. |
|
We probably also need a UI to delete notes. For a longer-lived device (not laptops, but, say, oil rigs or vehicles) those notes could get very out of hand. I think we need to log that as an event as well tho. |
|
If we do the way we've just discussed, we'd need to introduce a few new actions. add_note, edit_note, and delete_note, since we want all of those actions reflected in the history, but also want to be able to filter on them for display and reporting purposes. |
|
(Or, if Notes is its own model, we can use the regular create/update/delete action_types with |
|
Yes, I was thinking of something akin to reporting on the action log. and I agree on those actions. |
| @@ -31,4 +31,13 @@ public function store(Request $request) | |||
|
|
|||
| return response()->json(Helper::formatStandardApiResponse('success')); | |||
| } | |||
|
|
|||
| public function update(Request $request) | |||
| { | |||
There was a problem hiding this comment.
yes, those are not complete. that was all framing for my edification. Sorry for any confusion.
The ability to add a note to an asset uncoupled from an action (such as Checkin)
A pop-up window will open, and the note will save to the Asset History (as well as the activity log)
This replaces #14987
Type of change
How Has This Been Tested?
Ran snipe-it test suite through command line
Test Configuration:
Checklist: