Description
Alex Buckley
2019-07-10 01:54:32 UTC
Created attachment 91436 [details] [review] Bug 23291: Find patrons by smsalertnumber in the batch patron modification Add ability to paste in sms numbers (one per line) or upload a file of sms numbers and find patrons with matching smsalertnumber values. If an sms number is not found then this is displayed in a yellow box to the librarian i.e. the batch patron modification tool will behave as it does currently when a cardnumber is not found. Test plan: 1. Go to Tools > Batch patron modification 2. Notice there is a file upload (for cardnumbers) and a text input area (for cardnumbers) 3. Apply patch and restart memached and plack 4. Refresh the page and notice a file upload (for sms numbers) and file text input area (for sms numbers) have been added 5. Create a file with valid sms numbers matching smsalertnumber values of patrons (one per line) 6. Select/upload the file into the sms number file upload and submit the form 7. Notice the patrons are found and you can batch modify them. Select a date in the 'Restriction expiration' datepicker and submit and notice this is added to the selected borrowers in the borrowers.debarred, borrowers.debarredcomment fields and borrower_debarment table. 8. Create a file with some valid and some invalid sms numbers (i.e. some smsnumbers belonging to patrons in their smsalertnumber field and some not matching) 9. Upload your file from 8 in the SMS number file upload and notice that error handling means the invalid/non-matching sms numbers are shown below the yellow box at the top of the page whilst matching sms numbers have returned patrons which you can edit 10. Confirm card number file upload still works by making a file of valid cardnumbers and uploading it in the Batch patron modification tool and noticing the patrons are successfully returned 11. In the batch patron modification tool paste valid sms numbers into the sms number text area input (0ne per line) and submit the batch patron modification. Notice that the patrons are successfully returned and displayed. 12. Repeat step 11 but this time paste in valid and invalid sms numbers and notice error handling means sms numbers which did not match borrowers, smsalertnumber are shown under the yellow box at the top of the page and patrons who matched sms numbers are shown below in the table 13. Repeat step 11 this time input only invalid sms numbers and submit. Notice that a page showing the yellow notice box and a list of all smsnumbers which could not be found are returned. 14. Repeat step 11 submitting valid smsnumbers in the cardnumber text area field and you will notice that a page showing the yellow notice box and a list of all non matching cardnumbers. 15. In your Koha home directory run: sudo koha-shell <instancename> Then run: prove xt 16. Confirm all test pass 17. Sign off Sponsored-By: Brimbank Library, Australia It passes most of the test plan for me, but there's a problem if you have two patrons using the same SMS alert number. On a second test of SMS numbers I entered 5 patrons with the same SMS number in order to see how the patch would handle a situation like this (we have lots of patrons where a parent will use their SMS number form their own account + for the accounts of their children) and if the SMS numbers are not unique, only the first patron that is using that SMS number appears in the batch patron modification tool - and their data is repeated 1 line per SMS number. The other thing I'd add is that I don't like the separate interfaces for "Use a file of cardnumbers" and "Use a file of cardnumbers" or the seperate "Or list cardnumbers one by one" and "Or list SMS numbers one by one." I think it would be better to have the interface look like the Batch item modification tool where the first option is "Use a file" and then there are radio buttons to indicate whether its a "Barcode file" or "Item number file." If you had a similar option for "Enter patrons one by one" and then had a radio button for "cardnumbers" and "SMS numbers" and had the radio buttons default to cardnumber it would make the UI better. Created attachment 92429 [details] [review] Bug 23291: Retrieve all patrons with given SMS number Created attachment 92430 [details] [review] Bug 23291: Retrieve all patrons with given SMS number, and change inputs This patch now handles situations where more than one patron has a given SMS number. The inputs are now more concise as radio buttons are now present for users to indicate whether the file they are uploading or the list of numbers they are entering are cardnumbers or SMS numbers. Test plan same as first patch, although take into account slight changes to inputs. Also test an SMS number that more than one patron has and check that all patrons with that number are displayed. Sponsored-by: Brimbank Library, Australia Authored-by: Alex Buckley and Hayley Mapley (Catalyst IT) (In reply to George Williams (NEKLS) from comment #2) > It passes most of the test plan for me, but there's a problem if you have > two patrons using the same SMS alert number. On a second test of SMS > numbers I entered 5 patrons with the same SMS number in order to see how the > patch would handle a situation like this (we have lots of patrons where a > parent will use their SMS number form their own account + for the accounts > of their children) and if the SMS numbers are not unique, only the first > patron that is using that SMS number appears in the batch patron > modification tool - and their data is repeated 1 line per SMS number. > > The other thing I'd add is that I don't like the separate interfaces for > "Use a file of cardnumbers" and "Use a file of cardnumbers" or the seperate > "Or list cardnumbers one by one" and "Or list SMS numbers one by one." I > think it would be better to have the interface look like the Batch item > modification tool where the first option is "Use a file" and then there are > radio buttons to indicate whether its a "Barcode file" or "Item number > file." If you had a similar option for "Enter patrons one by one" and then > had a radio button for "cardnumbers" and "SMS numbers" and had the radio > buttons default to cardnumber it would make the UI better. Thank you for testing George. We have made the changes you suggested, and the patch is ready to be tested again! Since Bug 23206 you can run batch patron modification from reports. Could you use it to select patrons by SMS alert number ? (In reply to Fridolin SOMERS from comment #6) > Since Bug 23206 you can run batch patron modification from reports. > Could you use it to select patrons by SMS alert number ? I think not easily, if you have a list of numbers, there is no easy (or possible?) way to enter them into a runtim parameter. So it's a lot of more steps to get the link. I wonder how difficult it would be to allow the user to choose any phone number for matching? sms, primary, secondary, or other? Created attachment 92666 [details] [review] Bug 23291: Find patrons by smsalertnumber in the batch patron modification Add ability to paste in sms numbers (one per line) or upload a file of sms numbers and find patrons with matching smsalertnumber values. If an sms number is not found then this is displayed in a yellow box to the librarian i.e. the batch patron modification tool will behave as it does currently when a cardnumber is not found. Test plan: 1. Go to Tools > Batch patron modification 2. Notice there is a file upload (for cardnumbers) and a text input area (for cardnumbers) 3. Apply patch and restart memached and plack 4. Refresh the page and notice a file upload (for sms numbers) and file text input area (for sms numbers) have been added 5. Create a file with valid sms numbers matching smsalertnumber values of patrons (one per line) 6. Select/upload the file into the sms number file upload and submit the form 7. Notice the patrons are found and you can batch modify them. Select a date in the 'Restriction expiration' datepicker and submit and notice this is added to the selected borrowers in the borrowers.debarred, borrowers.debarredcomment fields and borrower_debarment table. 8. Create a file with some valid and some invalid sms numbers (i.e. some smsnumbers belonging to patrons in their smsalertnumber field and some not matching) 9. Upload your file from 8 in the SMS number file upload and notice that error handling means the invalid/non-matching sms numbers are shown below the yellow box at the top of the page whilst matching sms numbers have returned patrons which you can edit 10. Confirm card number file upload still works by making a file of valid cardnumbers and uploading it in the Batch patron modification tool and noticing the patrons are successfully returned 11. In the batch patron modification tool paste valid sms numbers into the sms number text area input (0ne per line) and submit the batch patron modification. Notice that the patrons are successfully returned and displayed. 12. Repeat step 11 but this time paste in valid and invalid sms numbers and notice error handling means sms numbers which did not match borrowers, smsalertnumber are shown under the yellow box at the top of the page and patrons who matched sms numbers are shown below in the table 13. Repeat step 11 this time input only invalid sms numbers and submit. Notice that a page showing the yellow notice box and a list of all smsnumbers which could not be found are returned. 14. Repeat step 11 submitting valid smsnumbers in the cardnumber text area field and you will notice that a page showing the yellow notice box and a list of all non matching cardnumbers. 15. In your Koha home directory run: sudo koha-shell <instancename> Then run: prove xt 16. Confirm all test pass 17. Sign off Sponsored-By: Brimbank Library, Australia Signed-off-by: George Williams <george@nekls.org> Created attachment 92667 [details] [review] Bug 23291: Retrieve all patrons with given SMS number, and change inputs This patch now handles situations where more than one patron has a given SMS number. The inputs are now more concise as radio buttons are now present for users to indicate whether the file they are uploading or the list of numbers they are entering are cardnumbers or SMS numbers. Test plan same as first patch, although take into account slight changes to inputs. Also test an SMS number that more than one patron has and check that all patrons with that number are displayed. Sponsored-by: Brimbank Library, Australia Authored-by: Alex Buckley and Hayley Mapley (Catalyst IT) Signed-off-by: George Williams <george@nekls.org> Created attachment 93859 [details] [review] Bug 23291: (follow-up) Fix capitalization on SMS There was one sms hidden between all the SMS, fixed it. There is a QA script fail: FAIL tools/modborrowers.pl FAIL critic # Variables::RequireLexicalLoopIterators: Got 1 violation(s). Can you please check? Created attachment 101931 [details] [review] Bug 23291: Find patrons by smsalertnumber in the batch patron modification Add ability to paste in sms numbers (one per line) or upload a file of sms numbers and find patrons with matching smsalertnumber values. If an sms number is not found then this is displayed in a yellow box to the librarian i.e. the batch patron modification tool will behave as it does currently when a cardnumber is not found. Test plan: 1. Go to Tools > Batch patron modification 2. Notice there is a file upload (for cardnumbers) and a text input area (for cardnumbers) 3. Apply patch and restart memached and plack 4. Refresh the page and notice a file upload (for sms numbers) and file text input area (for sms numbers) have been added 5. Create a file with valid sms numbers matching smsalertnumber values of patrons (one per line) 6. Select/upload the file into the sms number file upload and submit the form 7. Notice the patrons are found and you can batch modify them. Select a date in the 'Restriction expiration' datepicker and submit and notice this is added to the selected borrowers in the borrowers.debarred, borrowers.debarredcomment fields and borrower_debarment table. 8. Create a file with some valid and some invalid sms numbers (i.e. some smsnumbers belonging to patrons in their smsalertnumber field and some not matching) 9. Upload your file from 8 in the SMS number file upload and notice that error handling means the invalid/non-matching sms numbers are shown below the yellow box at the top of the page whilst matching sms numbers have returned patrons which you can edit 10. Confirm card number file upload still works by making a file of valid cardnumbers and uploading it in the Batch patron modification tool and noticing the patrons are successfully returned 11. In the batch patron modification tool paste valid sms numbers into the sms number text area input (0ne per line) and submit the batch patron modification. Notice that the patrons are successfully returned and displayed. 12. Repeat step 11 but this time paste in valid and invalid sms numbers and notice error handling means sms numbers which did not match borrowers, smsalertnumber are shown under the yellow box at the top of the page and patrons who matched sms numbers are shown below in the table 13. Repeat step 11 this time input only invalid sms numbers and submit. Notice that a page showing the yellow notice box and a list of all smsnumbers which could not be found are returned. 14. Repeat step 11 submitting valid smsnumbers in the cardnumber text area field and you will notice that a page showing the yellow notice box and a list of all non matching cardnumbers. 15. In your Koha home directory run: sudo koha-shell <instancename> Then run: prove xt 16. Confirm all test pass 17. Sign off Sponsored-By: Brimbank Library, Australia Signed-off-by: George Williams <george@nekls.org> Created attachment 101932 [details] [review] Bug 23291: Retrieve all patrons with given SMS number, and change inputs This patch now handles situations where more than one patron has a given SMS number. The inputs are now more concise as radio buttons are now present for users to indicate whether the file they are uploading or the list of numbers they are entering are cardnumbers or SMS numbers. Test plan same as first patch, although take into account slight changes to inputs. Also test an SMS number that more than one patron has and check that all patrons with that number are displayed. Sponsored-by: Brimbank Library, Australia Authored-by: Alex Buckley and Hayley Mapley (Catalyst IT) Signed-off-by: George Williams <george@nekls.org> Created attachment 101933 [details] [review] Bug 23291: (follow-up) Fix capitalization on SMS There was one sms hidden between all the SMS, fixed it. (In reply to Katrin Fischer from comment #12) > There is a QA script fail: > > FAIL tools/modborrowers.pl > FAIL critic > # Variables::RequireLexicalLoopIterators: Got 1 violation(s). > > Can you please check? All qa tests pass for me. Rebased and ready for testing Bug 23291 - Allow selecting patrons by smsalertnumber in batch patron modification 101931 - Bug 23291: Find patrons by smsalertnumber in the batch patron modification 101932 - Bug 23291: Retrieve all patrons with given SMS number, and change inputs 101933 - Bug 23291: (follow-up) Fix capitalization on SMS Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 23291: Find patrons by smsalertnumber in the batch patron modification Using index info to reconstruct a base tree... M tools/modborrowers.pl Falling back to patching base and 3-way merge... Auto-merging tools/modborrowers.pl CONFLICT (content): Merge conflict in tools/modborrowers.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 23291: Find patrons by smsalertnumber in the batch patron modification The copy of the patch that failed is found in: .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-23291-Find-patrons-by-smsalertnumber-in-the-ba-HjqO44.patch Created attachment 112469 [details] [review] Bug 23291: Find patrons by smsalertnumber in the batch patron modification Add ability to paste in sms numbers (one per line) or upload a file of sms numbers and find patrons with matching smsalertnumber values. If an sms number is not found then this is displayed in a yellow box to the librarian i.e. the batch patron modification tool will behave as it does currently when a cardnumber is not found. Test plan: 1. Go to Tools > Batch patron modification 2. Notice there is a file upload (for cardnumbers) and a text input area (for cardnumbers) 3. Apply patch and restart memached and plack 4. Refresh the page and notice a file upload (for sms numbers) and file text input area (for sms numbers) have been added 5. Create a file with valid sms numbers matching smsalertnumber values of patrons (one per line) 6. Select/upload the file into the sms number file upload and submit the form 7. Notice the patrons are found and you can batch modify them. Select a date in the 'Restriction expiration' datepicker and submit and notice this is added to the selected borrowers in the borrowers.debarred, borrowers.debarredcomment fields and borrower_debarment table. 8. Create a file with some valid and some invalid sms numbers (i.e. some smsnumbers belonging to patrons in their smsalertnumber field and some not matching) 9. Upload your file from 8 in the SMS number file upload and notice that error handling means the invalid/non-matching sms numbers are shown below the yellow box at the top of the page whilst matching sms numbers have returned patrons which you can edit 10. Confirm card number file upload still works by making a file of valid cardnumbers and uploading it in the Batch patron modification tool and noticing the patrons are successfully returned 11. In the batch patron modification tool paste valid sms numbers into the sms number text area input (0ne per line) and submit the batch patron modification. Notice that the patrons are successfully returned and displayed. 12. Repeat step 11 but this time paste in valid and invalid sms numbers and notice error handling means sms numbers which did not match borrowers, smsalertnumber are shown under the yellow box at the top of the page and patrons who matched sms numbers are shown below in the table 13. Repeat step 11 this time input only invalid sms numbers and submit. Notice that a page showing the yellow notice box and a list of all smsnumbers which could not be found are returned. 14. Repeat step 11 submitting valid smsnumbers in the cardnumber text area field and you will notice that a page showing the yellow notice box and a list of all non matching cardnumbers. 15. In your Koha home directory run: sudo koha-shell <instancename> Then run: prove xt 16. Confirm all test pass 17. Sign off Sponsored-By: Brimbank Library, Australia Signed-off-by: George Williams <george@nekls.org> Created attachment 112470 [details] [review] Bug 23291: Retrieve all patrons with given SMS number, and change inputs This patch now handles situations where more than one patron has a given SMS number. The inputs are now more concise as radio buttons are now present for users to indicate whether the file they are uploading or the list of numbers they are entering are cardnumbers or SMS numbers. Test plan same as first patch, although take into account slight changes to inputs. Also test an SMS number that more than one patron has and check that all patrons with that number are displayed. Sponsored-by: Brimbank Library, Australia Authored-by: Alex Buckley and Hayley Mapley (Catalyst IT) Signed-off-by: George Williams <george@nekls.org> Created attachment 112471 [details] [review] Bug 23291: (follow-up) Fix capitalization on SMS There was one sms hidden between all the SMS, fixed it. Hi Aleisha. In step 4 of the test plan I get an error when refreshing the page Could not compile /kohadevbox/koha/tools/modborrowers.pl: Bareword "I" not allowed while "strict subs" in use at /kohadevbox/koha/tools/modborrowers.pl line 118. BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/tools/modborrowers.pl line 489. at /usr/share/perl5/CGI/Compile.pm line 144 in CGI::Compile::compile at /usr/share/perl5/CGI/Compile.pm line 144 141: my $code = $self->_eval($eval); 142: my $exception = $@; 143: 144: die "Could not compile $script: $exception" if $exception; 145: 146: subname "${package}::$subname", sub { 147: my @args = @_; Show function arguments David Working on rebasing this against master. Created attachment 128641 [details] [review] Bug 23291: Patron batch modification based on SMS number This patch is based on the patchset on bug 24019. It enabled librarians to list SMS numbers (one per line) or upload a file of SMS numbers, just like they can currently do with borrowernumbers or card numbers. Test plan: 1. Apply patch 2. Restart services 3. Create a patron list in: Tools > Patrons lists 4. Navigate to Tools > Batch patron modification 5. Observe there are 4 tabs: 'By card number', 'By SMS number', 'By borrowernumber', 'By patron list' 6. Test each option for batch patron modifications: - By cardnumber file - By cardnumber list - By SMS number file - By SMS number list - By borrowernumber file - By borrowernumber list - By patron list 7. In all of the above cases the correct patrons should be retrieved by Koha, and modified. 8. Test uploading a SMS number shared by multiple patrons and confirm all patrons with that SMS number are returned 9. Confirm the 'order of operations' for card numbers, SMS numbers and borrowernumbers is followed: - If a file is uploaded AND a list of number submitted then the list of numbers will be used 10. Confirm batches are only submitted from the active tab - If you upload a file OR input a list SMS numbers, and then switch to a different tab and input a list of borrowernumbers then the SMS numbers originally inputted should be ignored. Sponsored-By: Brimbank Library, Australia Hi all, I've merged the 3 previous commits and rebased this patchset against master. I tried making this patchset, and it's test plan, as close as possible to bug 23291 Ready to test again! Thanks, Alex Created attachment 128876 [details] Bug 23291 - Duplicated patrons Testing notes (koha-testing-docker): 1. I created three text files that listed card numbers, borrow numbers, and SMS numbers for four patrons. One was duplicated (see the lists at the end). 2. Enabled adding and sending messages by SMS: . Set SMSSendDriver system preference to Email . Not sure whether this is necessary, but for SMS providers (Administration > Additional parameters > SMS cellular providers) I added: - Name: TEST - Domain: example.com 3. Added SMS numbers and SMS provider to patrons. 4. Went through the test plan. 5. When modifying data I just changed the ZIP/Postal code each time. Other notes/things I noted - this is existing behavour, so should be new bugs for fixing them if it is required or warranted: 1. Each batch patrons modification listing page has a 'Mobile' column - this matches to 'Other phone' in the contact information section when editing the patron. It is not the same as the SMS number. 2. If an SMS number applies to one or more patrons and it is included in the input twice or more (either in the file or the input form), then each patron with that number is shown twice on the batch patrons modification page - see the screenshot. This behaviour is exhibited for the by card number and by borrower number options, so is not something introduced by this bug. 3. Should 'By borrowernumber' be 'By borower number'? Realising that is the field name used in the query/URL, but is not "human friendly". Or perhaps 'System patron number' or just 'Patron number'? It is also shown on the patron details page in the summary box on the top left. Details for the three separate files (mentioned in item 1): Card numbers: 23529000035676 23529000651225 23529000080862 23529000695412 Borrower numbers: 19 49 7 38 SMS numbers: 2125551212 2125551212 2125551213 2125551214 I will add my sign-off once I sort out an issue with my local git repository! (In reply to David Nind from comment #26) > I will add my sign-off once I sort out an issue with my local git repository! Hi David, Thank you very much for your testing! I believe it's not necessary to add SMS providers. Or at least my patchset does not check if those values are set, however, I don't think it hurts to do so. Good spotting regarding those 3 points, are you happy to create new bug reports for them? I can if you don't. Thanks again! Alex (In reply to Alex Buckley from comment #27) > Good spotting regarding those 3 points, are you happy to create new bug > reports for them? I can if you don't. I'll add bugs for these shortly. (In reply to David Nind from comment #28) > (In reply to Alex Buckley from comment #27) > > > Good spotting regarding those 3 points, are you happy to create new bug > > reports for them? I can if you don't. > > I'll add bugs for these shortly. Thank you! Created attachment 128877 [details] [review] Bug 23291: Patron batch modification based on SMS number This patch is based on the patchset on bug 24019. It enabled librarians to list SMS numbers (one per line) or upload a file of SMS numbers, just like they can currently do with borrowernumbers or card numbers. Test plan: 1. Apply patch 2. Restart services 3. Create a patron list in: Tools > Patrons lists 4. Navigate to Tools > Batch patron modification 5. Observe there are 4 tabs: 'By card number', 'By SMS number', 'By borrowernumber', 'By patron list' 6. Test each option for batch patron modifications: - By cardnumber file - By cardnumber list - By SMS number file - By SMS number list - By borrowernumber file - By borrowernumber list - By patron list 7. In all of the above cases the correct patrons should be retrieved by Koha, and modified. 8. Test uploading a SMS number shared by multiple patrons and confirm all patrons with that SMS number are returned 9. Confirm the 'order of operations' for card numbers, SMS numbers and borrowernumbers is followed: - If a file is uploaded AND a list of number submitted then the list of numbers will be used 10. Confirm batches are only submitted from the active tab - If you upload a file OR input a list SMS numbers, and then switch to a different tab and input a list of borrowernumbers then the SMS numbers originally inputted should be ignored. Sponsored-By: Brimbank Library, Australia Signed-off-by: David Nind <david@davidnind.com> (In reply to David Nind from comment #25) I have added bugs for the things I noted when testing: > 1. Each batch patrons modification listing page has a 'Mobile' column - this > matches to 'Other phone' in the contact information section when editing the > patron. It is not the same as the SMS number. See bug 29762 - Patron batch modification tool - mobile phone number column naming. > 2. If an SMS number applies to one or more patrons and it is included in the > input twice or more (either in the file or the input form), then each patron > with that number is shown twice on the batch patrons modification page - see > the screenshot. This behaviour is exhibited for the by card number and by > borrower number options, so is not something introduced by this bug. See bug 29761 - Patron batch modification tool - duplicated information on the listing page > 3. Should 'By borrowernumber' be 'By borower number'? Realising that is the > field name used in the query/URL, but is not "human friendly". Or perhaps > 'System patron number' or just 'Patron number'? It is also shown on the > patron details page in the summary box on the top left. See bug 29763 - Patron information - terminology: borrowernumber (In reply to Fridolin Somers from comment #6) > Since Bug 23206 you can run batch patron modification from reports. > Could you use it to select patrons by SMS alert number ? This is possible and way more flexible than what suggests this patch. Create a new report: select cardnumber from borrowers where smsalertnumber IN <<numbers|list>> Run it and paste the list of your smsalertnumber. Then you can batch edit your patrons. And it works with whichever attributes you wish! In my opinion this patch is not necessary. This is possible *since* bug 27380. Alex, what do you think? Hi Jonathan, My apologies for my delay in replying. I agree that bug 27380 now looks to have addressed the problem that Katrin noted in comment #7 and it means librarians can now easily batch modify by sms, primary, or secondary phone numbers easily. My feeling is it's fine for this bug report to be closed. I do think bug 29761 is necessary, however, because a librarian could still upload a list of cardnumbers or borrowernumbers containing duplicates. Thanks Alex Agreed! Marked as fixed, see comment 32. |