|
Description
Liz Rea
2023-06-20 16:10:38 UTC
Just a note that we keep some, but not all data. For example we delete the patron attributes, alerts etc. But it could still be helpful of course and some links remain. Created attachment 188299 [details] [review] Bug 34069: Add restore_deleted_borrowers permission Created attachment 188302 [details] [review] Bug 34069: Implement tool for restoring deleted patrons I think we want both a permission and a system preference to control this feature. Created attachment 188988 [details] [review] Bug 34069: Log patron restoration Created attachment 188989 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Created attachment 188990 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Created attachment 189027 [details] [review] Bug 34069: Add restore_deleted_borrowers permission and new system preference Created attachment 189028 [details] [review] Bug 34069: Implement tool for restoring deleted patrons Created attachment 189029 [details] [review] Bug 34069: Log patron restoration Created attachment 189030 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Created attachment 189037 [details] [review] Bug 34069: Add ability to search on updated_on dates Created attachment 189066 [details] [review] Bug 34069: Deal with view_borrower_infos_from_any_libraries Created attachment 189067 [details] [review] Bug 34069: Add information about what can and cannot be restored Created attachment 189370 [details] [review] Bug 34069: Add restore_deleted_borrowers permission and new system preference Created attachment 189371 [details] [review] Bug 34069: Implement tool for restoring deleted patrons Created attachment 189372 [details] [review] Bug 34069: Log patron restoration Created attachment 189373 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Created attachment 189374 [details] [review] Bug 34069: Add ability to search on updated_on dates Created attachment 189375 [details] [review] Bug 34069: Deal with view_borrower_infos_from_any_libraries Created attachment 189376 [details] [review] Bug 34069: Add information about what can and cannot be restored Created attachment 189377 [details] [review] Bug 34069: Move permission from borrower to tools I know this isn't done but I took a look and wanted to mention that you probably want <div class="alert alert-info"> instead of "dialog alert bg-info-subtle". I also think the heading inside the alert should be <h2> instead of <h3> Created attachment 189642 [details] [review] Bug 34069: Fix HTML, Bootstrap classes, and QA complaints Created attachment 189645 [details] [review] Bug 34069: Add unit tests A handful of small-ish things that should be addressed: - Deletin from patron record gives a message "this cannot be undone," but that won't be true anymore with these patches - Submitting a blank search in the Restore Borrowers tool returns no patrons. We should either disallow blank search OR give clear message that blank search doesn't work - Categorycode and branchcode are blank in deleted patron list (even though they have values in deletedborrowers) - After restoration, "View patron" should open in new tab - Restore Borrowers tool is not in the Tools sidebar - Search by date should only look at date portion of updated_on -- currently a search for date range 11/17/2025 - 11/18/2025 does not return patrons deleted on 11/18/2025, but a search 11/18 - 11/19 does - When user is limited to only see patrons in their library group they should be able to select "All libraries" and get only those they have access to -- currently such a user can restore borrowers for any library in their group, but only 1 library at a time - RESTORE action should not be all caps in action logs Created attachment 189718 [details] [review] Bug 34069: Fix categorycode/branch info, open in new tab, and other fixes Created attachment 189773 [details] [review] Bug 34069: Adjust before and after date search Created attachment 189774 [details] [review] Bug 34069: Prevent search if no search critrea Created attachment 189779 [details] [review] Bug 34069: Allow for multiselection of branch and category Created attachment 189780 [details] [review] Bug 34069: Logaction should not be in all caps Created attachment 189781 [details] [review] Bug 34069: Add restore_deleted_borrowers permission and new system preference 1. APPLY PATCH, updatedatabase, restart_all 2. Now setup two users for testing, a superlibrarian and a limited permission user. The limited permission should have staff access and the restore_deleted_borrowers (tools) permission. /* SUPERLIBRARIAN */ 3. As superlibrarian, find the 'AllowDeletedPatronRestoration' system preference and enable it. Also make sure BorrowersLog is turned on. 4. Go to Tools -> Restore deleted patrons 5. Read the warning/information about what can and cannot be restored. Ensure it all makes sense. 6. Now we must delete some borrowers, either 1 by 1 or with the batch deletion tool. 7. Back to the deletion tool ( Tools -> Restore deleted patrons ) 8. Search for some of your deleted patrons by any of the critrea available. ( Cardnumber, borrowernumber, first name, surname, category, branch, or date ) 9. Ensure that searching works well. 10. Now bring up a deleted patron, check the checbox to the left and click "Restore deleted patrons" 11. Now you should be able to see the that the patron was restored. 12. Click "View patron" to look at the restored patron record. 13. Go to the action logs and make sure that the restoration was logged. /* LIMITED PERMISSION USER */ 14. Now log in as your limited permission user. 15. Go to Tools -> Restore deleted patrons 16. As a limited permssion user you should only be able to search for deleted borrowers from your branch, or branches in your library group. 17. Repeat all the steps to restore a patron, ensuring everything works. Created attachment 189782 [details] [review] Bug 34069: Implement tool for restoring deleted patrons Created attachment 189783 [details] [review] Bug 34069: Log patron restoration Created attachment 189784 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Created attachment 189785 [details] [review] Bug 34069: Add ability to search on updated_on dates Created attachment 189786 [details] [review] Bug 34069: Deal with view_borrower_infos_from_any_libraries Created attachment 189787 [details] [review] Bug 34069: Add information about what can and cannot be restored Created attachment 189788 [details] [review] Bug 34069: Move permission from borrower to tools Created attachment 189789 [details] [review] Bug 34069: Fix HTML, Bootstrap classes, and QA complaints Created attachment 189790 [details] [review] Bug 34069: Add unit tests Created attachment 189791 [details] [review] Bug 34069: Fix categorycode/branch info, open in new tab, and other fixes Created attachment 189792 [details] [review] Bug 34069: Adjust before and after date search Created attachment 189793 [details] [review] Bug 34069: Prevent search if no search critrea Created attachment 189794 [details] [review] Bug 34069: Allow for multiselection of branch and category Created attachment 189795 [details] [review] Bug 34069: Logaction should not be in all caps Hi! Here are a few remarks: I'm wondering if we should add "from the deletedborrowers table" in the syspref and tool description. It sounds more like a developer thing than a user one. It's okay to have it in the tool itself with the explanation of why not everything is restored though. It could be useful to say in the syspref description that the option will be available on the tools page once it is enabled. In members-home.pl, searching without any criteria brings all the patrons. I would expect the same behavior here (return all the deleted patrons instead of the "Please fill out at least one search field." message). It should not be an issue since the search results are paginated. Otherwise, it works as expected and I'd be happy to sign-off. Created attachment 189940 [details] [review] Bug 34069: Add restore_deleted_borrowers permission and new system preference 1. APPLY PATCH, updatedatabase, restart_all 2. Now setup two users for testing, a superlibrarian and a limited permission user. The limited permission should have staff access and the restore_deleted_borrowers (tools) permission. /* SUPERLIBRARIAN */ 3. As superlibrarian, find the 'AllowDeletedPatronRestoration' system preference and enable it. Also make sure BorrowersLog is turned on. 4. Go to Tools -> Restore deleted patrons 5. Read the warning/information about what can and cannot be restored. Ensure it all makes sense. 6. Now we must delete some borrowers, either 1 by 1 or with the batch deletion tool. 7. Back to the deletion tool ( Tools -> Restore deleted patrons ) 8. Search for some of your deleted patrons by any of the critrea available. ( Cardnumber, borrowernumber, first name, surname, category, branch, or date ) 9. Ensure that searching works well. 10. Now bring up a deleted patron, check the checbox to the left and click "Restore deleted patrons" 11. Now you should be able to see the that the patron was restored. 12. Click "View patron" to look at the restored patron record. 13. Go to the action logs and make sure that the restoration was logged. /* LIMITED PERMISSION USER */ 14. Now log in as your limited permission user. 15. Go to Tools -> Restore deleted patrons 16. As a limited permssion user you should only be able to search for deleted borrowers from your branch, or branches in your library group. 17. Repeat all the steps to restore a patron, ensuring everything works. Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189941 [details] [review] Bug 34069: Implement tool for restoring deleted patrons Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189942 [details] [review] Bug 34069: Log patron restoration Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189943 [details] [review] Bug 34069: Add a CannotRestore exception and throw when the userid, borrowernumber, or cardnumber is already in use Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189944 [details] [review] Bug 34069: Add ability to search on updated_on dates Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189945 [details] [review] Bug 34069: Deal with view_borrower_infos_from_any_libraries Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189946 [details] [review] Bug 34069: Add information about what can and cannot be restored Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189947 [details] [review] Bug 34069: Move permission from borrower to tools Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189948 [details] [review] Bug 34069: Fix HTML, Bootstrap classes, and QA complaints Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189949 [details] [review] Bug 34069: Add unit tests Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189950 [details] [review] Bug 34069: Fix categorycode/branch info, open in new tab, and other fixes Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189951 [details] [review] Bug 34069: Adjust before and after date search Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189952 [details] [review] Bug 34069: Prevent search if no search critrea Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189953 [details] [review] Bug 34069: Allow for multiselection of branch and category Signed-off-by: Katie Bliss <kebliss@dmpl.org> Created attachment 189954 [details] [review] Bug 34069: Logaction should not be in all caps Signed-off-by: Katie Bliss <kebliss@dmpl.org> |