Description
Pedro Amorim
2025-07-15 14:59:31 UTC
Created attachment 184109 [details] [review] Bug 40395: Preparation: suspension_bulk REST API endpoint Created attachment 184110 [details] [review] Bug 40395: Preparation: DRY cancel_hold_modal Created attachment 184111 [details] [review] Bug 40395: Preparation: Move holds table JS from request.tt to holds.js This is so that tables from patron-detail-tabs.inc can also make use of this JS Created attachment 184112 [details] [review] Bug 40395: Use 'deny' CSS class for red color Created attachment 184113 [details] [review] Bug 40395: Reimplement suspend Created attachment 184114 [details] [review] Bug 40395: Reimplement delete Created attachment 184115 [details] [review] Bug 40395: Refactor: Move suspend modal to its own tt BLOCK Created attachment 184116 [details] [review] Bug 40395: Add checkboxes column to holds table Test plan, k-t-d, patches applied: 1) Search for 'music': http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=music 2) Click the 'Select all' link on the left of the search toolbar 3) Click the 'Place hold' button on the same toolbar 4) On the patron input, add 'koha' 5) You need to pick the 'Pickup location' specifically for each hold. Click 'Place holds' 6) On this screen (reserve/request.pl) ensure the table(s) work. Selecting all works. Canceling selected works (it doesn't refresh immediatelly, it's a background job). Suspend works as before, sorting, etc. 7) Now visit the patron details page: http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8) Click the 'Holds' tab. 9) Verify we now have a checkbox column at the start. 10) Test that selecting multiple holds and canceling selected works. Same for suspending. 11) Test the canceling or suspending a specific hold works. 12) Test that this table is compatible with AutoResumeSuspendedHolds sys pref i.e. you're unable to set a resume date when suspending in bulk or single if this sys pref is turned off. 13) Repeat steps 8-12 but for the circulation page from patron details at: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 14) Ensure tests added for API REST endpoint added here pass: prove t/db_dependent/api/v1/holds.t Created attachment 184134 [details] [review] Bug 40395: Preparation: suspension_bulk REST API endpoint Created attachment 184135 [details] [review] Bug 40395: Preparation: DRY cancel_hold_modal Created attachment 184136 [details] [review] Bug 40395: Preparation: Move holds table JS from request.tt to holds.js This is so that tables from patron-detail-tabs.inc can also make use of this JS Created attachment 184137 [details] [review] Bug 40395: Use 'deny' CSS class for red color Created attachment 184138 [details] [review] Bug 40395: Reimplement suspend Created attachment 184139 [details] [review] Bug 40395: Reimplement delete Created attachment 184140 [details] [review] Bug 40395: Refactor: Move suspend modal to its own tt BLOCK Created attachment 184141 [details] [review] Bug 40395: Add checkboxes column to holds table Test plan, k-t-d, patches applied: 1) Search for 'music': http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=music 2) Click the 'Select all' link on the left of the search toolbar 3) Click the 'Place hold' button on the same toolbar 4) On the patron input, add 'koha' 5) You need to pick the 'Pickup location' specifically for each hold. Click 'Place holds' 6) On this screen (reserve/request.pl) ensure the table(s) work. Selecting all works. Canceling selected works (it doesn't refresh immediatelly, it's a background job). Suspend works as before, sorting, etc. 7) Now visit the patron details page: http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8) Click the 'Holds' tab. 9) Verify we now have a checkbox column at the start. 10) Test that selecting multiple holds and canceling selected works. Same for suspending. 11) Test the canceling or suspending a specific hold works. 12) Test that this table is compatible with AutoResumeSuspendedHolds sys pref i.e. you're unable to set a resume date when suspending in bulk or single if this sys pref is turned off. 13) Repeat steps 8-12 but for the circulation page from patron details at: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 14) Ensure tests added for API REST endpoint added here pass: prove t/db_dependent/api/v1/holds.t These patches remove some functionality from this page. Before the patches, one can select a hold cancellation reason when cancelling several holds at once at can set a suspend-until date when suspending multiple holds at once. (In reply to Andrew Fuerste-Henry from comment #17) > These patches remove some functionality from this page. Before the patches, > one can select a hold cancellation reason when cancelling several holds at > once at can set a suspend-until date when suspending multiple holds at once. Don't mind me, both functions are now in a modal that pops up on clicking their respective buttons. Created attachment 184398 [details] [review] Bug 40395: Preparation: suspension_bulk REST API endpoint Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184399 [details] [review] Bug 40395: Preparation: DRY cancel_hold_modal Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184400 [details] [review] Bug 40395: Preparation: Move holds table JS from request.tt to holds.js This is so that tables from patron-detail-tabs.inc can also make use of this JS Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184401 [details] [review] Bug 40395: Use 'deny' CSS class for red color Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184402 [details] [review] Bug 40395: Reimplement suspend Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184403 [details] [review] Bug 40395: Reimplement delete Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184404 [details] [review] Bug 40395: Refactor: Move suspend modal to its own tt BLOCK Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184405 [details] [review] Bug 40395: Add checkboxes column to holds table Test plan, k-t-d, patches applied: 1) Search for 'music': http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=music 2) Click the 'Select all' link on the left of the search toolbar 3) Click the 'Place hold' button on the same toolbar 4) On the patron input, add 'koha' 5) You need to pick the 'Pickup location' specifically for each hold. Click 'Place holds' 6) On this screen (reserve/request.pl) ensure the table(s) work. Selecting all works. Canceling selected works (it doesn't refresh immediatelly, it's a background job). Suspend works as before, sorting, etc. 7) Now visit the patron details page: http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8) Click the 'Holds' tab. 9) Verify we now have a checkbox column at the start. 10) Test that selecting multiple holds and canceling selected works. Same for suspending. 11) Test the canceling or suspending a specific hold works. 12) Test that this table is compatible with AutoResumeSuspendedHolds sys pref i.e. you're unable to set a resume date when suspending in bulk or single if this sys pref is turned off. 13) Repeat steps 8-12 but for the circulation page from patron details at: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 14) Ensure tests added for API REST endpoint added here pass: prove t/db_dependent/api/v1/holds.t Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 184434 [details] [review] Bug 40395: (QA follow-up): Add checkbox column to table settings This fixes the empty entry when clicking 'Columns' on the top right of the table Created attachment 184435 [details] [review] Bug 40395: (QA follow-up) Ensure checkboxes are unchecked after suspending To test, select a few holds to suspend (but not all). Click 'Suspend selected'. Notice the selection doesn't reset. After this patch, it does reset. Created attachment 184437 [details] [review] Bug 40395: (QA follow-up): Add checkbox column to table settings This fixes the empty entry when clicking 'Columns' on the top right of the table. Also ensures sorting does not exist on the checkbox column and exists on the one it was before (reservedate) This is for holds table on both patron pages: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=19 http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=19 No longer applies after bug 40372 Created attachment 184470 [details] [review] Bug 40395: Preparation: suspension_bulk REST API endpoint Created attachment 184471 [details] [review] Bug 40395: Preparation: DRY cancel_hold_modal Created attachment 184472 [details] [review] Bug 40395: Preparation: Move holds table JS from request.tt to holds.js This is so that tables from patron-detail-tabs.inc can also make use of this JS Created attachment 184473 [details] [review] Bug 40395: Use 'deny' CSS class for red color Created attachment 184474 [details] [review] Bug 40395: Reimplement suspend Created attachment 184475 [details] [review] Bug 40395: Reimplement delete Created attachment 184476 [details] [review] Bug 40395: Refactor: Move suspend modal to its own tt BLOCK Created attachment 184477 [details] [review] Bug 40395: Add checkboxes column to holds table Test plan, k-t-d, patches applied: 1) Search for 'music': http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=music 2) Click the 'Select all' link on the left of the search toolbar 3) Click the 'Place hold' button on the same toolbar 4) On the patron input, add 'koha' 5) You need to pick the 'Pickup location' specifically for each hold. Click 'Place holds' 6) On this screen (reserve/request.pl) ensure the table(s) work. Selecting all works. Canceling selected works (it doesn't refresh immediatelly, it's a background job). Suspend works as before, sorting, etc. 7) Now visit the patron details page: http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8) Click the 'Holds' tab. 9) Verify we now have a checkbox column at the start. 10) Test that selecting multiple holds and canceling selected works. Same for suspending. 11) Test the canceling or suspending a specific hold works. 12) Test that this table is compatible with AutoResumeSuspendedHolds sys pref i.e. you're unable to set a resume date when suspending in bulk or single if this sys pref is turned off. 13) Repeat steps 8-12 but for the circulation page from patron details at: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 14) Ensure tests added for API REST endpoint added here pass: prove t/db_dependent/api/v1/holds.t Created attachment 184478 [details] [review] Bug 40395: (QA follow-up): Add checkbox column to table settings This fixes the empty entry when clicking 'Columns' on the top right of the table Created attachment 184479 [details] [review] Bug 40395: (QA follow-up) Ensure checkboxes are unchecked after suspending To test, select a few holds to suspend (but not all). Click 'Suspend selected'. Notice the selection doesn't reset. After this patch, it does reset. I'm not loving all the code that I'm seeing, but I can see that code in question is already existing code, which is just being moved around, so that's OK. Test plan was missing a couple steps: - redocly bundle --ext json api/v1/swagger/swagger.yaml --output api/v1/swagger/swagger_bundle.json - koha-plack --restart kohadev Otherwise, overall, I think this looks good. Created attachment 184765 [details] [review] Bug 40395: Preparation: suspension_bulk REST API endpoint Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184766 [details] [review] Bug 40395: Preparation: DRY cancel_hold_modal Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184767 [details] [review] Bug 40395: Preparation: Move holds table JS from request.tt to holds.js This is so that tables from patron-detail-tabs.inc can also make use of this JS Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184768 [details] [review] Bug 40395: Use 'deny' CSS class for red color Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184769 [details] [review] Bug 40395: Reimplement suspend Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184770 [details] [review] Bug 40395: Reimplement delete Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184771 [details] [review] Bug 40395: Refactor: Move suspend modal to its own tt BLOCK Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184772 [details] [review] Bug 40395: Add checkboxes column to holds table Test plan, k-t-d, patches applied: 1) Search for 'music': http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=music 2) Click the 'Select all' link on the left of the search toolbar 3) Click the 'Place hold' button on the same toolbar 4) On the patron input, add 'koha' 5) You need to pick the 'Pickup location' specifically for each hold. Click 'Place holds' 6) On this screen (reserve/request.pl) ensure the table(s) work. Selecting all works. Canceling selected works (it doesn't refresh immediatelly, it's a background job). Suspend works as before, sorting, etc. 7) Now visit the patron details page: http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8) Click the 'Holds' tab. 9) Verify we now have a checkbox column at the start. 10) Test that selecting multiple holds and canceling selected works. Same for suspending. 11) Test the canceling or suspending a specific hold works. 12) Test that this table is compatible with AutoResumeSuspendedHolds sys pref i.e. you're unable to set a resume date when suspending in bulk or single if this sys pref is turned off. 13) Repeat steps 8-12 but for the circulation page from patron details at: http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 14) Ensure tests added for API REST endpoint added here pass: prove t/db_dependent/api/v1/holds.t Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184773 [details] [review] Bug 40395: (QA follow-up): Add checkbox column to table settings This fixes the empty entry when clicking 'Columns' on the top right of the table Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 184774 [details] [review] Bug 40395: (QA follow-up) Ensure checkboxes are unchecked after suspending To test, select a few holds to suspend (but not all). Click 'Suspend selected'. Notice the selection doesn't reset. After this patch, it does reset. Signed-off-by: David Cook <dcook@prosentient.com.au> [FAIL] koha-tmpl/intranet-tmpl/prog/js/holds.js FAIL forbidden_patterns forbidden pattern: Use __("") instead of _("") (line 687) forbidden pattern: Use __("") instead of _("") (line 688) forbidden pattern: Use __("") instead of _("") (line 717) forbidden pattern: Use __("") instead of _("") (line 718) Created attachment 185508 [details] [review] Bug 40395: (QA follow-up): Address forbidden pattern QA complaints Nice work everyone! Pushed to main for 25.11 Please add release notes! This causes xt/find-missing-op-in-forms.t to fail. Can you check? It looks like coming from d06ba0860bc87031819ecdeff9bccd5c51d1fa8b Created attachment 185623 [details] [review] Bug 40395: (QA follow-up): Remove unused form This form is no longer used. We're now using a single cancellation hold modal and them form resides there, not here on this table To test on the UI: Apply this patch and ensure cancelling holds works as before, from both pages (tested using borrowernumber 19: <staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19 <staff_url>/cgi-bin/koha/reserve/request.pl?biblionumber=76&biblionumber=437&biblionumber=77 Fixed the failing test: prove xt/find-missing-op-in-forms.t Follow-ups pushed to main. |