|
Description
Emmi Takkinen
2024-02-20 06:37:52 UTC
Corrected (In reply to Emmi Takkinen from comment #1) > Corrected ... the title, we should extend expiration dates, not pick up dates. Created attachment 163021 [details] [review] Bug 36135: Add new permission batch_modify_holds This patch adds new permission "batch_modify_holds". To test: 1. Apply this patch. 2. Run updatedatabase.pl. 3. Check that new permission was added to Patrons->Set permissions. Sponsored-by: Koha-Suomi Oy Created attachment 163022 [details] [review] Bug 36135: Add links to the batch modify holds tool This patch adds links to new tool batch modify holds tool. To test: 1. Apply this patch. 2. Add patron permission "batch_modify_holds". 3. Login as chosen patron. 4. Navigate to Tools page. => Note there's a link "Batch modify holds". 5. Remove permission from user. => Link to new tool is not displayed. Sponsored-by: Koha-Suomi Oy Created attachment 163023 [details] [review] Bug 36135: Add new tool to batch modify holds We sometimes need to modify holds due library closing down etc. For this we need tool to batch modify holds which allows us to filter holds and modify them. Tool allows users to select holds with following parameters: - expiration date -> if patron_expiration_date is set, it is used as main search filter -> otherwise expirationdate is used - library - found status - suspended status - suspend until date - hold note Users are able to modify following with tool: - expiration date (not expiration date set by patron) - pick up library - suspend status (only if hold has no found status) - suspend until date (only if suspend status is set) - hold note To test: 1. Apply this patch. 2. Navigate to new tool Tools->Batch modify holds. 3. Fill in some criteria to search holds with. => Confirm that holds found match your search criteria. 4. Set e.g. new expiration date for all or just selected holds. => Confirm that holds were modified as they should have. => Confirm this also from database. To test suspending holds with found status: 1. Find holds with found status. 2. Attempt to modify their suspend status. => Alert box with text "One or more holds have found status and can't be suspended." should be displayed. Play around, test different search compinations and modifications. Sponsored-by: Koha-Suomi Oy Created attachment 163024 [details] [review] Bug 36135: Allow using SQL results in batch modify holds tool This patch adds ability to use SQL results for batch holds tool if report contains column reserve_id. To test: 1. Create SQL report which contains column for reserve_id. => e.g "SELECT reserve_id FROM reserves limit 10". 2. Run report. 3. Over report select "Batch operations" should be displayed and under that there should be option "Batch modify holds". 4. Click "Batch modify holds" option. => Confirm that holds displayed in "Batch mofidy holds" tool are same as found with your report. Sponsored-by: Koha-Suomi Oy We did some briefing on this and ended up adding ability to modify other fields as well, not just expiration date. I'll adjust bugs title accordingly. Hi Emmi, I am running into the following error when attempting to use the Batch Modify Holds tool: Programming error - op 'list' must start with 'cud-' for POST http://localhost:8081/intranet/tools/batch_modify_holds.pl (referer: http://localhost:8081/cgi-bin/koha/tools/batch_modify_holds.pl) Created attachment 163529 [details] [review] Bug 36135: (follow-up) Adjust for CSRF Created attachment 163530 [details] [review] Bug 36135: (follow-up) Adjust for CSRF Created attachment 163540 [details] [review] Bug 36135: Add new permission batch_modify_holds This patch adds new permission "batch_modify_holds". To test: 1. Apply this patch. 2. Run updatedatabase.pl. 3. Check that new permission was added to Patrons->Set permissions. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 163541 [details] [review] Bug 36135: Add links to the batch modify holds tool This patch adds links to new tool batch modify holds tool. To test: 1. Apply this patch. 2. Add patron permission "batch_modify_holds". 3. Login as chosen patron. 4. Navigate to Tools page. => Note there's a link "Batch modify holds". 5. Remove permission from user. => Link to new tool is not displayed. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 163542 [details] [review] Bug 36135: Add new tool to batch modify holds We sometimes need to modify holds due library closing down etc. For this we need tool to batch modify holds which allows us to filter holds and modify them. Tool allows users to select holds with following parameters: - expiration date -> if patron_expiration_date is set, it is used as main search filter -> otherwise expirationdate is used - library - found status - suspended status - suspend until date - hold note Users are able to modify following with tool: - expiration date (not expiration date set by patron) - pick up library - suspend status (only if hold has no found status) - suspend until date (only if suspend status is set) - hold note To test: 1. Apply this patch. 2. Navigate to new tool Tools->Batch modify holds. 3. Fill in some criteria to search holds with. => Confirm that holds found match your search criteria. 4. Set e.g. new expiration date for all or just selected holds. => Confirm that holds were modified as they should have. => Confirm this also from database. To test suspending holds with found status: 1. Find holds with found status. 2. Attempt to modify their suspend status. => Alert box with text "One or more holds have found status and can't be suspended." should be displayed. Play around, test different search compinations and modifications. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 163543 [details] [review] Bug 36135: Allow using SQL results in batch modify holds tool This patch adds ability to use SQL results for batch holds tool if report contains column reserve_id. To test: 1. Create SQL report which contains column for reserve_id. => e.g "SELECT reserve_id FROM reserves limit 10". 2. Run report. 3. Over report select "Batch operations" should be displayed and under that there should be option "Batch modify holds". 4. Click "Batch modify holds" option. => Confirm that holds displayed in "Batch mofidy holds" tool are same as found with your report. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 163544 [details] [review] Bug 36135: (follow-up) Adjust for CSRF Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Hi! Thank you again for working on this patch! It works as described. :) These could be follow-up bugs, but I think it would be helpful to have a column in the batch modify holds tool that either has the item barcode, or item number / bib record hyperlinked for easy access. It would also be helpful to display which items have a holds status that can not be suspended. It is difficult to determine at a glance which item cannot be suspended and it might be frustrating to libraries who are trying to suspend hundreds of holds at a time. (In reply to Laura Escamilla from comment #16) > Hi! > > Thank you again for working on this patch! It works as described. :) > > These could be follow-up bugs, but I think it would be helpful to have a > column in the batch modify holds tool that either has the item barcode, or > item number / bib record hyperlinked for easy access. > > It would also be helpful to display which items have a holds status that can > not be suspended. It is difficult to determine at a glance which item cannot > be suspended and it might be frustrating to libraries who are trying to > suspend hundreds of holds at a time. Hi! Thank you for feedback :) I'll take a look what I can do to these and other feedback I received from our superlibrarians. I'll probably provide separate feedback patch. Created attachment 163796 [details] [review] Bug 36135: Add several new features This patch adds following new features to batch modify holds tool. - Do not automatically select login library as search param - Add biblios title and items barcode to found holds table - Add count of found holds at top of the table - Add ability to do not change pick up library - Display those hold rows with some found status in different color Sponsored-by: Koha-Suomi Oy This looks good but there are some small fixes to be made:
1. WARN installer/data/mysql/atomicupdate/bug_36135.pl
WARN tidiness
The file is less tidy than before (bad/messy lines before: 0, now: 4)
Perltidy your atomicupdate
2. forbidden pattern: simple-quote string (line 312)
Line 312: tools/batch_modify_holds.tt, needs double quotes, not single.
3. FAIL tt_valid
lines 134
This probably needs to be something like:
[% IF hold.found %]
<tr class="highlighted-row">
[% ELSE %]
<tr>
[% END %]
4. Line 123: <th>Title</title>
5. Line 111: Missing a closing </div>
6. Overall there needs to be some improvement to the HTML:
<label for="suspend_until">Expiration date:</label>
<label for="suspend_until">Suspended until:</label>
These should probably be <legend> not <label>.
<li> elements inside of <li>? Not a blocker but should these be separate fieldsets?
7. These both use the ID "to" and they should use #expirationdate_to and #suspend_until_to, respectively.
56 <li>
57 <label for="expirationdate_to">To</label>
58 <input type="text" size="10" id="to" name="expirationdate_to" class="flatpickr"/>
59 </li>
91 <label for="suspend_until_to">To</label>
92 <input type="text" size="10" id="to" name="suspend_until_to" class="flatpickr"/>
Created attachment 164037 [details] [review] Bug 36135: Fix QA issues and add improvemnents This patch: - Fixes issues reported by qa-tool - Adds info about how many holds were modified - Add title and barcode columns to table of modified holds - Adds div with class "page-section" to "Modify holds" table Sponsored-by: Koha-Suomi Oy Fixed QA issues which Lucas pointed out. For this
><label for="suspend_until">Expiration date:</label>
><label for="suspend_until">Suspended until:</label>
>These should probably be <legend> not <label>.
I removed them and added labels "Expiration date from:" and "Expiration date to:" labels for corresponding li elements. Same for suspend until. Legend is used for as caption of fieldset so it wasn't suitable to be used in this case.
Created attachment 164051 [details] [review] Bug 36135: Fix QA issues and add improvements This patch: - Fixes issues reported by qa-tool - Adds info about how many holds were modified - Add title and barcode columns to table of modified holds - Adds div with class "page-section" to "Modify holds" table Sponsored-by: Koha-Suomi Oy Thanks Emmi! This looks great. Created attachment 164057 [details] [review] Bug 36135: Add new permission batch_modify_holds This patch adds new permission "batch_modify_holds". To test: 1. Apply this patch. 2. Run updatedatabase.pl. 3. Check that new permission was added to Patrons->Set permissions. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164058 [details] [review] Bug 36135: Add links to the batch modify holds tool This patch adds links to new tool batch modify holds tool. To test: 1. Apply this patch. 2. Add patron permission "batch_modify_holds". 3. Login as chosen patron. 4. Navigate to Tools page. => Note there's a link "Batch modify holds". 5. Remove permission from user. => Link to new tool is not displayed. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164059 [details] [review] Bug 36135: Add new tool to batch modify holds We sometimes need to modify holds due library closing down etc. For this we need tool to batch modify holds which allows us to filter holds and modify them. Tool allows users to select holds with following parameters: - expiration date -> if patron_expiration_date is set, it is used as main search filter -> otherwise expirationdate is used - library - found status - suspended status - suspend until date - hold note Users are able to modify following with tool: - expiration date (not expiration date set by patron) - pick up library - suspend status (only if hold has no found status) - suspend until date (only if suspend status is set) - hold note To test: 1. Apply this patch. 2. Navigate to new tool Tools->Batch modify holds. 3. Fill in some criteria to search holds with. => Confirm that holds found match your search criteria. 4. Set e.g. new expiration date for all or just selected holds. => Confirm that holds were modified as they should have. => Confirm this also from database. To test suspending holds with found status: 1. Find holds with found status. 2. Attempt to modify their suspend status. => Alert box with text "One or more holds have found status and can't be suspended." should be displayed. Play around, test different search compinations and modifications. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164060 [details] [review] Bug 36135: Allow using SQL results in batch modify holds tool This patch adds ability to use SQL results for batch holds tool if report contains column reserve_id. To test: 1. Create SQL report which contains column for reserve_id. => e.g "SELECT reserve_id FROM reserves limit 10". 2. Run report. 3. Over report select "Batch operations" should be displayed and under that there should be option "Batch modify holds". 4. Click "Batch modify holds" option. => Confirm that holds displayed in "Batch mofidy holds" tool are same as found with your report. Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164061 [details] [review] Bug 36135: (follow-up) Adjust for CSRF Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164062 [details] [review] Bug 36135: Add several new features This patch adds following new features to batch modify holds tool. - Do not automatically select login library as search param - Add biblios title and items barcode to found holds table - Add count of found holds at top of the table - Add ability to do not change pick up library - Display those hold rows with some found status in different color Sponsored-by: Koha-Suomi Oy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 164063 [details] [review] Bug 36135: Fix QA issues and add improvements This patch: - Fixes issues reported by qa-tool - Adds info about how many holds were modified - Add title and barcode columns to table of modified holds - Adds div with class "page-section" to "Modify holds" table Sponsored-by: Koha-Suomi Oy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> I like this a lot - self contained and makes minimal changes, the patches are clean and easy to read, but I think we need to consider some things here before this moves ahead Must fix: The JS alerts to prevent submission fire an alert per warning, so you need to click through several if you make a mistake - these should be combined into a single alert There is no way to see the limits you have placed on your search - I think these need to be shown to the user, ideally they would be editable, but showing them would be a start Should fix: While I like the idea of minimalizing data for privacy, excluding the borrower info on the holds feels a bit too far. Ideally this would be under columns configuration, and we have patron-title.inc for showing borrower info only to staff who should see it. The current column selection feels The display should be paginated, ideally it would use the holds API to load the holds to ensure performance, as scrolling can get really long really fast. It would be nice too to be able to select all in a filtered search - right now 'Select all' selects even those not visible in the table Could fix: If implemented with the datatables API this could then work like the patron search, with filters present on the page. The batch edit implies more batch actions - cancellation, reverting, etc. but these can all be follow-ups I think (In reply to Nick Clemens (kidclamp) from comment #31) > I like this a lot - self contained and makes minimal changes, the patches > are clean and easy to read, but I think we need to consider some things here > before this moves ahead > > Must fix: > The JS alerts to prevent submission fire an alert per warning, so you need > to click through several if you make a mistake - these should be combined > into a single alert > > There is no way to see the limits you have placed on your search - I think > these need to be shown to the user, ideally they would be editable, but > showing them would be a start > > Should fix: > While I like the idea of minimalizing data for privacy, excluding the > borrower info on the holds feels a bit too far. Ideally this would be under > columns configuration, and we have patron-title.inc for showing borrower > info only to staff who should see it. The current column selection feels > > The display should be paginated, ideally it would use the holds API to load > the holds to ensure performance, as scrolling can get really long really > fast. It would be nice too to be able to select all in a filtered search - > right now 'Select all' selects even those not visible in the table > > Could fix: > If implemented with the datatables API this could then work like the patron > search, with filters present on the page. > > The batch edit implies more batch actions - cancellation, reverting, etc. > but these can all be follow-ups I think Thank you Nick for feedback. First two points are indeed something that should be fixed. I'm just bit unsure how to display those search terms in search result page. I also agree that using API does have benefits and table should be paginated (even my test data might produce result of over 2000 holds and that is cumbersome for users go through). I'll take a look what I can do to these. Setting this as assigned for now. Moving on with this really depends on whether API is used or not in this page. Otherwise adding it later would probably mean making same work again with some of the features Nick suggested. As for the filters displayed on page, I think it would be better if they are displayed like in item search page behind "Edit search" button. Putting them on margin just doesn't seem (visually) good idea since there are other links there already. Couple problems, holds endpoint doesn't show patron_expiration_date column (see bug 29975). It also doesn't allow fetching items barcode, adding items to datatables embed property leads to error. So we need to make changes to holds endpoint to make this function properly. |