Bug 23291 - Patron batch modification based on SMS number
Summary: Patron batch modification based on SMS number
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact:
URL:
Keywords: Manual
Depends on: 13552
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-10 01:54 UTC by Alex Buckley
Modified: 2022-12-12 21:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds modifying patron information using SMS numbers to the patron batch modification tool (Tools > Patrons > Patron batch modification). It adds a new tab 'By SMS number', and can use a list of SMS numbers by either uploading a file or entering the numbers into the input form - the same as for modifying patron information using borrower numbers or card numbers. If an SMS number is shared by multiple patrons, all patrons with that SMS number are returned.
Version(s) released in:


Attachments
Bug 23291: Find patrons by smsalertnumber in the batch patron modification (12.65 KB, patch)
2019-07-10 03:59 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23291: Retrieve all patrons with given SMS number (1.58 KB, patch)
2019-08-22 01:29 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23291: Retrieve all patrons with given SMS number, and change inputs (9.21 KB, patch)
2019-08-22 03:02 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 23291: Find patrons by smsalertnumber in the batch patron modification (12.71 KB, patch)
2019-09-09 16:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23291: Retrieve all patrons with given SMS number, and change inputs (9.25 KB, patch)
2019-09-09 16:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23291: (follow-up) Fix capitalization on SMS (1.19 KB, patch)
2019-10-07 22:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23291: Find patrons by smsalertnumber in the batch patron modification (12.91 KB, patch)
2020-03-27 01:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: Retrieve all patrons with given SMS number, and change inputs (9.35 KB, patch)
2020-03-27 01:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: (follow-up) Fix capitalization on SMS (1.19 KB, patch)
2020-03-27 01:24 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: Find patrons by smsalertnumber in the batch patron modification (12.83 KB, patch)
2020-10-25 03:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: Retrieve all patrons with given SMS number, and change inputs (9.30 KB, patch)
2020-10-25 03:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: (follow-up) Fix capitalization on SMS (1.20 KB, patch)
2020-10-25 03:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23291: Patron batch modification based on SMS number (11.20 KB, patch)
2021-12-17 02:13 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23291 - Duplicated patrons (64.43 KB, image/png)
2021-12-23 01:35 UTC, David Nind
Details
Bug 23291: Patron batch modification based on SMS number (11.21 KB, patch)
2021-12-23 04:44 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2019-07-10 01:54:32 UTC
Use case: When SMS messages from Koha fail at the SMS gateway layer and a library receives the list of failed SMS messages in an SMS gateway report they may wish to restrict patrons by their smsalertnumbers so that they are prompted to correct their smsalertnumber. 

This patch would add a new file upload and text input to the Batch patron modification tool. 

The user will be able to upload a file of sms numbers (one per line) as they currently can with cardnumbers. 

Similarly, they will be able to paste in a list of smsnumbers into the text input area (one per line) as they can currently with cardnumbers.

If a matching smsalertnumber cannot be found then the sms will be skipped.

Once the list of patrons is returned building on the functionality added in Bug 13552 the librarian will be able to place restrictions on a batch of patrons.
Comment 1 Alex Buckley 2019-07-10 03:59:29 UTC Comment hidden (obsolete)
Comment 2 George Williams (NEKLS) 2019-07-30 16:05:56 UTC
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.
Comment 3 Alex Buckley 2019-08-22 01:29:27 UTC Comment hidden (obsolete)
Comment 4 Hayley Pelham 2019-08-22 03:02:28 UTC Comment hidden (obsolete)
Comment 5 Hayley Pelham 2019-08-22 03:03:20 UTC
(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!
Comment 6 Fridolin Somers 2019-09-02 13:20:36 UTC
Since Bug 23206 you can run batch patron modification from reports.
Could you use it to select patrons by SMS alert number ?
Comment 7 Katrin Fischer 2019-09-02 15:11:36 UTC
(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.
Comment 8 Owen Leonard 2019-09-05 13:12:12 UTC
I wonder how difficult it would be to allow the user to choose any phone number for matching? sms, primary, secondary, or other?
Comment 9 ByWater Sandboxes 2019-09-09 16:16:07 UTC Comment hidden (obsolete)
Comment 10 ByWater Sandboxes 2019-09-09 16:16:10 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2019-10-07 22:29:01 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2019-10-07 22:29:32 UTC
There is a QA script fail:

 FAIL	tools/modborrowers.pl
   FAIL	  critic
		# Variables::RequireLexicalLoopIterators: Got 1 violation(s). 

Can you please check?
Comment 13 Aleisha Amohia 2020-03-27 01:24:01 UTC Comment hidden (obsolete)
Comment 14 Aleisha Amohia 2020-03-27 01:24:06 UTC Comment hidden (obsolete)
Comment 15 Aleisha Amohia 2020-03-27 01:24:10 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2020-03-27 01:25:35 UTC
(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
Comment 17 Lisette Scheer 2020-10-24 21:02:38 UTC
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
Comment 18 Aleisha Amohia 2020-10-25 03:44:07 UTC
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>
Comment 19 Aleisha Amohia 2020-10-25 03:44:12 UTC
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>
Comment 20 Aleisha Amohia 2020-10-25 03:44:17 UTC
Created attachment 112471 [details] [review]
Bug 23291: (follow-up) Fix capitalization on SMS

There was one sms hidden between all the SMS, fixed it.
Comment 21 David Nind 2021-08-09 13:07:23 UTC
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
Comment 22 Alex Buckley 2021-12-16 21:43:36 UTC
Working on rebasing this against master.
Comment 23 Alex Buckley 2021-12-17 02:13:55 UTC
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
Comment 24 Alex Buckley 2021-12-17 02:25:34 UTC
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
Comment 25 David Nind 2021-12-23 01:35:46 UTC
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
Comment 26 David Nind 2021-12-23 01:36:33 UTC
I will add my sign-off once I sort out an issue with my local git repository!
Comment 27 Alex Buckley 2021-12-23 01:46:42 UTC
(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
Comment 28 David Nind 2021-12-23 01:57:21 UTC
(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.
Comment 29 Alex Buckley 2021-12-23 02:11:16 UTC
(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!
Comment 30 David Nind 2021-12-23 04:44:34 UTC
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>
Comment 31 David Nind 2021-12-23 05:57:02 UTC
(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
Comment 32 Jonathan Druart 2022-01-27 16:41:48 UTC
(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.
Comment 33 Jonathan Druart 2022-01-27 16:42:33 UTC
This is possible *since* bug 27380.
Comment 34 Jonathan Druart 2022-02-08 17:50:24 UTC
Alex, what do you think?
Comment 35 Alex Buckley 2022-02-10 05:46:16 UTC
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
Comment 36 Jonathan Druart 2022-02-17 16:11:29 UTC
Agreed!

Marked as fixed, see comment 32.