Hi, it would be fine to have a column for vendor's note in late order page. This would be useful to add a note if you know a vendor still has time delivery, a vendor supplier for example. Regards, Marjorie
I would add that it would be good to have as a reference to the order number and isbn. Marjorie
Created attachment 67349 [details] [review] Bug 12502 - Add vendor and internal note to late orders page
Created attachment 67354 [details] [review] Bug 12502 - Add ordernumber and ISBN to late order Adds ordernumber as 2nd column and ISBN as last
Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed.
Created attachment 67586 [details] [review] Bug 12502 - Add vendor and internal note to late orders page Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 67587 [details] [review] Bug 12502 - Add ordernumber and ISBN to late order Adds ordernumber as 2nd column and ISBN as last Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
David, don't forget to add you as Assignee when you plan to provide patches for a bug!
Hi, I like what this is trying to do, but at the moment it doesn't work and will need more work in order to make it in. 1) This patch adds a new routine ModNotesOrder to the C4 namespace. You should add it to the Koha namespace instead, for example: Koha::Acq::Order->add_note. It also needs to be covered by unit tests. 2) You change the CSV header, but only add the 2 notes fields. Why not the ISBN? 3) Entering/saving notes does currently not work for me at all. I think the input fields in the table take up a lot of space and are not easy to handle. I suggest taking a look at how adding messages in the patron account works (button in the toolbar). The change should show immediately (Ajax) or the page should reload automatically after entering a note (no need to F5). 4) Tiny: The order search uses 'order line' for 'Order no.' - I suggest to also use order line here to make it consistent. 5) The late orders table can be configured using the column visibility settings. The new columns need to be added there as well so you can control when and where they are visible. 6) Last is just an idea/question: If we are showing ISBN, should we show ISSN too? Maybe a combined column? 1) and 5) are the biggest issues.
*** Bug 16722 has been marked as a duplicate of this bug. ***
Created attachment 72168 [details] [review] Bug 12502 - Add vendor and internal note to late orders page Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 72169 [details] [review] Bug 12502 - Add ordernumber and ISBN to late order Adds ordernumber as 2nd column and ISBN as last Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 72170 [details] [review] Bug 12502: QA Follow-up 1) The method under C4 was deleted. There wasn't any reason for a method under Koha, in the end, because it's a simple update that requires no additional processing. If I had to do any legwork with the data before updating the Order fields, I would've written a wrapper. As it stands right now, the work can be done directly in the controller without polluting the Koha/Acquisition/Orders.pm class. 2) Added the ISBN field to csv_header, and added that value to lateorders-export I've also changed the string internaldate to internalnote, small typo fix. 3) Made adjustements client-side and server-side. In the browser, you can now see a small loading icon to inform the user that the 'Save note' button actually triggered an action. A new alert message also informs the user if something awry happened while updating the orders' notes. I took a look at the user page, to see how notes were saved, but I was not satisfied with the idea of mixing an Ajax call with a window.location.reload; doesn't make sense in my mind to uselessly reload the page if the information had already reached the server, and since it was already coded that way, I felt like this change was the best course of action. Server-side, it is now possible to save an empty field. Before this patch, 'if($note)' would actually fail on an empty string, which meant you could never erase a note, for any reason whatsoever. After testing this patch, if the width of the input is still something that you'd like to see changed, I'll have no problem reducing it a little, but you'll have to tell me how large is _too_ large :) 4) Order no. -> Order line 5) Added the new columns to the admin/visibility yaml 6) It would be possible to also display an ISSN, but I won't make this patch any large than it is for now.
Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 12502 - Add vendor and internal note to late orders page Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt Failed to merge in the changes. Patch failed at 0001 Bug 12502 - Add vendor and internal note to late orders page The copy of the patch that failed is found in: /var/repositories/koha/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-12502---Add-vendor-and-internal-note-to-late-o-KaZRtc.patch
Created attachment 73499 [details] [review] Bug 12502 - Add vendor and internal note to late orders page Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 73500 [details] [review] Bug 12502 - Add ordernumber and ISBN to late order Adds ordernumber as 2nd column and ISBN as last Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 73501 [details] [review] Bug 12502 - Add vendor and internal note to late orders page Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 73502 [details] [review] Bug 12502 - Add ordernumber and ISBN to late order Adds ordernumber as 2nd column and ISBN as last Followed test plan and patch worked as described. Also ran QA test tools and all modified files passed Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 73503 [details] [review] Bug 12502: QA Follow-up 1) The method under C4 was deleted. There wasn't any reason for a method under Koha, in the end, because it's a simple update that requires no additional processing. If I had to do any legwork with the data before updating the Order fields, I would've written a wrapper. As it stands right now, the work can be done directly in the controller without polluting the Koha/Acquisition/Orders.pm class. 2) Added the ISBN field to csv_header, and added that value to lateorders-export I've also changed the string internaldate to internalnote, small typo fix. 3) Made adjustements client-side and server-side. In the browser, you can now see a small loading icon to inform the user that the 'Save note' button actually triggered an action. A new alert message also informs the user if something awry happened while updating the orders' notes. I took a look at the user page, to see how notes were saved, but I was not satisfied with the idea of mixing an Ajax call with a window.location.reload; doesn't make sense in my mind to uselessly reload the page if the information had already reached the server, and since it was already coded that way, I felt like this change was the best course of action. Server-side, it is now possible to save an empty field. Before this patch, 'if($note)' would actually fail on an empty string, which meant you could never erase a note, for any reason whatsoever. After testing this patch, if the width of the input is still something that you'd like to see changed, I'll have no problem reducing it a little, but you'll have to tell me how large is _too_ large :) 4) Order no. -> Order line 5) Added the new columns to the admin/visibility yaml 6) It would be possible to also display an ISSN, but I won't make this patch any large than it is for now.
It would be great if this could be rebased.
Created attachment 79231 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Request and you shall receive. Ran qa-test on it, fixed issues, squashed, but did not test.
Created attachment 80160 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Can you please fix the issues pointed out by the QA script? I could fix the filtes, but not sure about the first: FAIL acqui/lateorders.pl FAIL critic # Variables::ProhibitUnusedVariables: Got 1 violation(s). FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt FAIL filters missing_filter at line 84 ( [% lateorder.ordernumber %]) missing_filter at line 143 ( <td><input name="vendornote" id="vendornote" type="text" value="[% lateorder.vendornote %]"></td>) missing_filter at line 144 ( <td>[% lateorder.isbn %]</td>) missing_filter at line 142 ( <td><input name="internalnote" id="internalnote" type="text" value="[% lateorder.internalnote %]"></td>) missing_filter at line 269 ( $("#savenotes").after("<span id=\"loading\" style=\"margin:4px;\"><img src=\"[% interface %]/[% theme %]/img/loading-small.gif\" alt=\"\" /> "+_("Loading")+"</span>");)
Hi Katrin, are you talking about koha-qa.pl ? I seem to have the latest version, and am not getting those messages. Any parameter I'm missing?
Hi Blou, that's trange. What's the latest commit you have? Did you move to the new repository? https://gitlab.com/koha-community/qa-test-tools
My last commit: Jonathan Druart : Make sure commit messages are correctly formatted (6 months ago)
You are not using the right repository (we moved a lot of them), last change was only 3 weeks ago, see the link above and here: https://gitlab.com/koha-community/qa-test-tools/commits/master
Created attachment 80602 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Next time as a separate follow-up patch please! ;) Looking at this again I have some issues, I am sorry to come up with this a little lately :( 1) Usability There is visual indicator to tell apart an already stored from a changed/unsafed note. It's really easy to forget to press the button. If you happen to end your note with pressing "Enter" you trigger the error message "No order selected" It really doen't work well for notes that are longer than the input field is. They don't display in full and it's hard to move around in the input field. I think I'd like this much better following the existing design pattern with links and a modal (see basket summary). I am adding Owen in hope he has time to take a look. 2) Search Searching on the notes column doesn't work. 3) Export The notes are not exported, neither using the "Export as CSV" nor the new "Excel" export at the bottom of the table.
(In reply to Katrin Fischer from comment #29) > 2) Search > Searching on the notes column doesn't work. > > 3) Export > The notes are not exported, neither using the "Export as CSV" nor the new > "Excel" export at the bottom of the table. I think using the link + modal model from acqui/basket.pl would work well here. I think it would solve both of these problems, each caused by the fact that the table cell contains an input field instead of text.
I quickly tried the solution of Owen. I'm in a state where CSV works, but only the one from the top, I guess I should do something else to make it work. I tried to add the modal and the links, the result looks fine, but there is something I would like to ask about it. Should the description comes along with the icon? When I add an icon to add or edit a note with the description (i.e. Edit internal note), the description is exported as well. If the description is not here, it's well exported. Since I don't find how to hide only the description and am not even sure if it should be there in the first place, I ask if the link's description should be added too.
Created attachment 87236 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Patch rebased. I am still waiting for advises before continuing.
(In reply to Katrin Fischer from comment #29) > Next time as a separate follow-up patch please! ;) > > Looking at this again I have some issues, I am sorry to come up with this a > little lately :( > > 1) Usability > There is visual indicator to tell apart an already stored from a > changed/unsafed note. It's really easy to forget to press the button. > > If you happen to end your note with pressing "Enter" you trigger the error > message "No order selected" > > It really doen't work well for notes that are longer than the input field > is. They don't display in full and it's hard to move around in the input > field. > > I think I'd like this much better following the existing design pattern with > links and a modal (see basket summary). I am adding Owen in hope he has time > to take a look. > > 2) Search > Searching on the notes column doesn't work. > > 3) Export > The notes are not exported, neither using the "Export as CSV" nor the new > "Excel" export at the bottom of the table. Sorry Pierre-Marc, all the issues Katrin noticed on comment 29 are still present.
Created attachment 91564 [details] [review] Bug 12502: (follow-up) Use modal to add/edit order notes Replace inputs to edit notes by a link opening a modal, similar to the basket summary page. This makes it possible for the notes to appear in the datatable exports. Also fix exporting with the export as csv button at the bottom of the page to export notes. Test plan : 1) In the late orders table (acqui/lateorders.pl), a link should appear in both the internal and vendor note column, following the current value if it exist. -> This link should read "Add ..." if there is no existing note or "Edit ..." if there is. 2) Click on the link. A modal should appear. Confirm that saving a note works as expected. 3) Try exporting to any format using either the button at the top of the table or the one at the bottom of the page. -> The three new columns and their values should appear in the export.
Used links + modal for editing the notes and fixed the exports. However, as Pierre-Marc mentioned, when exporting, the labels for the links do appear in the data ("Add internal note", ...). I feel that this is not optimal, but since the same behavior is present in the basket page as well, I left it as is.
I have some questions. If I use the Export option at the top of the table, only the displayed datas are exported (hide columns / perform a search). If I use the Export option at the bottom, all datas for the selected orders are exported even the hidden ones. Is that the expected behaviour ? The "Save notes" button now seems useless as datas are automatically saved using the modal, can you confirm ? I agree with Pierre-Marc : the label of "Add / Edit note" should not be exported. I will open a new ticket to fix it on the basket export because it adds work for the staff working on acquisition to suppress these informations before sending the file to the vendor. Will you agree to suppress it in the present patch ? Except these points, patch works fine !
Created attachment 95418 [details] [review] Bug 12502: (QA follow-up) Remove the Save button
Created attachment 95419 [details] [review] Bug 12502: (follow-up) Adds formatting for datatables export Add a format option to the datatables export that remove .noExport elements present in a data cell, just as .noExport columns are not exported. To test: 1. For late orders (which uses .noExport elements in data cells) : Export data in any format using the export button at the top of the datatable. => data should exclude "Add/Edit note" label, but otherwise look the same as before the patch. 2. Try exporting data in other datatables in Koha and make sure exported data is the same before and after applying the patch.
Thank you for testing and for your comment. I would say the different behaviors for the export buttons is expected since the button as the top is a part of the datatables plugin and uses it's filters to determine which row to export and the button at the bottom is part of Koha and uses the checkboxes to filter rows to export. I agree this behavior might be questionable, but I believe it is not in the scope of this bug which is about order notes. I did forget to remove the "Save notes" button, thank you for the catch. I've also added a patch to remove the "Add/Edit note" label. It adds a new formatting option to the datatables to accept the use of the .noExport class on data cell elements to remove them from exported data. It is now ready to be tested again.
Created attachment 95438 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95439 [details] [review] Bug 12502: (follow-up) Use modal to add/edit order notes Replace inputs to edit notes by a link opening a modal, similar to the basket summary page. This makes it possible for the notes to appear in the datatable exports. Also fix exporting with the export as csv button at the bottom of the page to export notes. Test plan : 1) In the late orders table (acqui/lateorders.pl), a link should appear in both the internal and vendor note column, following the current value if it exist. -> This link should read "Add ..." if there is no existing note or "Edit ..." if there is. 2) Click on the link. A modal should appear. Confirm that saving a note works as expected. 3) Try exporting to any format using either the button at the top of the table or the one at the bottom of the page. -> The three new columns and their values should appear in the export. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95440 [details] [review] Bug 12502: (QA follow-up) Remove the Save button Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95441 [details] [review] Bug 12502: (follow-up) Adds formatting for datatables export Add a format option to the datatables export that remove .noExport elements present in a data cell, just as .noExport columns are not exported. To test: 1. For late orders (which uses .noExport elements in data cells) : Export data in any format using the export button at the top of the datatable. => data should exclude "Add/Edit note" label, but otherwise look the same as before the patch. 2. Try exporting data in other datatables in Koha and make sure exported data is the same before and after applying the patch. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Thank you Maryse ! The new version is ok for me :)
Created attachment 97427 [details] [review] Bug 12502: Add vendor note, internal note, ordernumber and isbn to late orders page Test plan: 0) Make sure you have a late order a) Create a test vendor unless you have one b) Create a test basket unless you have one c) Close the basket to mark the order as late 1) Go to the late orders table Before patch : Orderno, ISBN, vendor notes, internal notes are absent After patch: fields are present 2) Edit the vendor note and/or the internal note 3) Press the save notes button 4) Refresh page and cache (CTRL+F5), if notes have been correctly saved, your modifications should be displayed. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 97428 [details] [review] Bug 12502: (follow-up) Use modal to add/edit order notes Replace inputs to edit notes by a link opening a modal, similar to the basket summary page. This makes it possible for the notes to appear in the datatable exports. Also fix exporting with the export as csv button at the bottom of the page to export notes. Test plan : 1) In the late orders table (acqui/lateorders.pl), a link should appear in both the internal and vendor note column, following the current value if it exist. -> This link should read "Add ..." if there is no existing note or "Edit ..." if there is. 2) Click on the link. A modal should appear. Confirm that saving a note works as expected. 3) Try exporting to any format using either the button at the top of the table or the one at the bottom of the page. -> The three new columns and their values should appear in the export. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 97429 [details] [review] Bug 12502: (QA follow-up) Remove the Save button Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 97430 [details] [review] Bug 12502: (follow-up) Adds formatting for datatables export Add a format option to the datatables export that remove .noExport elements present in a data cell, just as .noExport columns are not exported. To test: 1. For late orders (which uses .noExport elements in data cells) : Export data in any format using the export button at the top of the datatable. => data should exclude "Add/Edit note" label, but otherwise look the same as before the patch. 2. Try exporting data in other datatables in Koha and make sure exported data is the same before and after applying the patch. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Thx a lot for your patience with this one - I feel it works really nicely now! Also passes prove t/db_dependent/Acquisition.t.
Nice work everyone! Pushed to master for 20.05
Enhancement not backported to 19.11.x branch