Description
Kyle M Hall (khall)
2012-11-14 16:39:47 UTC
Created attachment 13449 [details] [review] Bug 9076 - Add ability to delete borrowers by expiration date and category code to cleanborrowers.pl Created attachment 13450 [details] [review] Bug 9076 - Followup - Perltidy cleanborrowers.pl Test plan: 1) Apply patch 2) Attempt to delete borrowers using each combination of criteria: a) Hasn't borrowed since b) Expiration date c) Category code d) a & b e) a & c f) b & c g) a & b & c With the sample data, there are 5 staff patrons defined (or so - I had 6 but I believe I added one more). This tool, when asked to delete staff patrons, indicated there were no patrons that met the criteria of "category: staff." The other categories worked as expected. Can this tool not delete staff patrons? Is that by design? If it is by design, then that categorycode needs to be removed from the list. (In reply to comment #4) > With the sample data, there are 5 staff patrons defined (or so - I had 6 but > I believe I added one more). This tool, when asked to delete staff patrons, > indicated there were no patrons that met the criteria of "category: staff." > The other categories worked as expected. > > Can this tool not delete staff patrons? Is that by design? If it is by > design, then that categorycode needs to be removed from the list. That is correct. It has never been able to delete staff patrons. I would assume that was a 'safety' feature. I'll add a followup to remove staff from the list. Created attachment 13491 [details] [review] Bug 9076 - Add ability to delete borrowers by expiration date and category code to cleanborrowers.pl Created attachment 13492 [details] [review] Bug 9076 - Followup - Perltidy cleanborrowers.pl Fixed. Created attachment 13541 [details] [review] Bug 9076 - Add ability to delete borrowers by expiration date and category code to cleanborrowers.pl Signed-off-by: Liz Rea <liz@catalyst.net.nz> Tested per plan, works. Staff category is not shown, and patch passes automated testing. Created attachment 13542 [details] [review] Bug 9076 - Followup - Perltidy cleanborrowers.pl Signed-off-by: Liz Rea <liz@catalyst.net.nz> Created attachment 13551 [details] [review] Bug 9076: Followup: FIX ergonomic issues If no criteria is given, the form was submitted. Labels were wrong Created attachment 13552 [details] [review] Bug 9076: Followup: FIX ergonomic issues If no criteria is given, the form was submitted. Labels were wrong Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> QA Comment: Great improvements for the clean borrowers tool. Patch clean. Marked as Passed QA. I have a question about your changes to the GetBorrowersWhoHaveNotBorrowedSince routine. It looks to me like you have changed the routine so that it will use a number of different criteria to decide whether a borrower should be deleted. Is that correct? If so, shouldn't we change the name to reflect what the routine actually does? Perhaps something like "GetBorrowersToExpunge" would more accurately describe what it does now? (In reply to comment #14) > I have a question about your changes to the > GetBorrowersWhoHaveNotBorrowedSince routine. It looks to me like you have > changed the routine so that it will use a number of different criteria to > decide whether a borrower should be deleted. Is that correct? If so, > shouldn't we change the name to reflect what the routine actually does? > Perhaps something like "GetBorrowersToExpunge" would more accurately > describe what it does now? I totally agree. I'll submit a followup to fix this unless someone else beats me to it. Created attachment 14008 [details] [review] Bug 9076 - QA Followup - Rename GetBorrowersWhoHaveNotBorrowedSince to GetBorrowersToExpunge This patch is broken by Bug 7919 (categorycode is null so we cannot filter by patron category) I'm seeing the category pulldown populated correctly. Can you give me a bit more detail about the problem? (In reply to comment #17) > This patch is broken by Bug 7919 (categorycode is null so we cannot filter > by patron category) (In reply to comment #18) > I'm seeing the category pulldown populated correctly. Can you give me a bit > more detail about the problem? The fix has been pushed so the problem does not exist anymore. so this still needs a QA look please. :) Last patch passes QA. The feature still works. Created attachment 14406 [details] [review] Bug 9076 - Add ability to delete borrowers by expiration date and category code to cleanborrowers.pl Signed-off-by: Liz Rea <liz@catalyst.net.nz> Tested per plan, works. Staff category is not shown, and patch passes automated testing. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 14407 [details] [review] Bug 9076 - Followup - Perltidy cleanborrowers.pl Signed-off-by: Liz Rea <liz@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 14408 [details] [review] Bug 9076: Followup: FIX ergonomic issues If no criteria is given, the form was submitted. Labels were wrong Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 14409 [details] [review] Bug 9076 - QA Followup - Rename GetBorrowersWhoHaveNotBorrowedSince to GetBorrowersToExpunge Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This patch has been pushed to master, though not without some concern about the layout. Liz has promised a speedy follow-up, so I will open another bug about it. *** Bug 6404 has been marked as a duplicate of this bug. *** |