Bug 14683 - Unable to clear SMS number
Summary: Unable to clear SMS number
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: j.kylmala
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17419
  Show dependency treegraph
 
Reported: 2015-08-14 14:26 UTC by Lari Taskula
Modified: 2016-10-10 14:08 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14683: Unable to clear SMS number (2.26 KB, patch)
2015-08-19 13:40 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 14683: Unable to clear SMS number (2.17 KB, patch)
2015-08-28 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14683: [QA Follow-up] Mixup between mobile and smsalertnumber (3.44 KB, patch)
2015-08-28 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14683: [QA Follow-up] Similar change for staff (1.84 KB, patch)
2015-09-01 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 14683: Unable to clear SMS number (2.19 KB, patch)
2015-09-01 23:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14683: [QA Follow-up] Mixup between mobile and smsalertnumber (3.46 KB, patch)
2015-09-01 23:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14683: [QA Follow-up] Similar change for staff (1.87 KB, patch)
2015-09-01 23:02 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2015-08-14 14:26:45 UTC
In opac-messaging.pl (your messaging) you cannot unset your SMS number once it has been set if your mobile (other phone) field is empty.

When checking for modified SMS number, we also check if SMS number does not equal mobile phone. This is causing the bug:

$query->param('SMSnumber') ne $borrower->{'mobile'}

Since mobile is empty and the given SMS number is also empty, the given SMS number will not be saved. Does anybody have an idea why we have this kind of condition?

to reproduce:
-2. Make sure borrower does not have mobile (other phone) set
-1. Make sure you have SMS messaging enabled
1. Navigate to your messaging
2. Insert SMS number and submit
3. Clear SMS number and submit
4. Observe that the sms number did not change
Comment 1 j.kylmala 2015-08-19 13:40:15 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-08-27 13:34:00 UTC
Still testing this, still see an issue here. Finish it tomorrow.
Comment 3 Marcel de Rooy 2015-08-28 08:17:45 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-08-28 08:17:50 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-09-01 10:21:12 UTC
The same line should be affected at the intranet too (members/memberentry.pl)
Comment 6 Marcel de Rooy 2015-09-01 14:21:40 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2015-09-01 23:02:07 UTC
Created attachment 42187 [details] [review]
[PASSED QA] Bug 14683: Unable to clear SMS number

Enables to clear SMS number.

To test:
1. Go to opac-messaging.pl
2. Insert SMS number and submit
3. Clear SMS number and submit
4. Observe that the sms number did not change
5. Apply patch
6. Clear SMS number and submit
7. Observe that the sms number changes

Sponsored-by: Vaara-kirjastot

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Adding a follow-up.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2015-09-01 23:02:10 UTC
Created attachment 42188 [details] [review]
[PASSED QA] Bug 14683: [QA Follow-up] Mixup between mobile and smsalertnumber

This is an issue discussed on older reports already in the past.
Column mobile in borrowers is actually 'Other phone', not necessary a
mobile number. The name of the field is confusing. (Renaming it is
outside the scope of this report.)
The field that we are editing here is smsalertnumber. It should not be
compared with mobile at all.

What could be the side-effect of this correction?
===
First, the change is only relevant for libraries with pref SMSSendDriver
enabled.
In the past patrons editing their message preferences saw mobile (read:
other phone) in their smsalertnumber field (if the latter was still empty).
If they saved it, it was copied to smsalertnumber.
This change does not affect these patrons. They just have the same number
in two columns. No big deal.
What if a patron does not yet have a smsalertnumber? In that case no sms
is sent in Letters.pm. So no change in behavior. If he submits
opac-messaging now, he will no longer copy his other phone to smsalert [we
cannot assume that it was mobile anyway!]. If he enters a mobile number,
it will be saved correctly in the right field.

Conclusion: this change will not break things or hurt anyone. It only
prevents unwanted copying other phone to smsalertnumber.

Also modified the compare to prevent uninitialized warnings.
And removed a commented warn.

Test plan:
[1] Add, edit or delete the SMS number on opac-messaging regardless of
    the value of Other Phone (in the badly named mobile field).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2015-09-01 23:02:13 UTC
Created attachment 42189 [details] [review]
[PASSED QA] Bug 14683: [QA Follow-up] Similar change for staff

Script memberentry.pl contained a similar line.
Solution is simpler here.

Test plan:
[1] Add, change or clear the sms number at staff side.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2015-09-01 23:04:04 UTC
I feel a bit like Marcel kidnapped the bug ;)... but the changes make sense to me.
Comment 11 Tomás Cohen Arazi 2015-09-07 15:27:26 UTC
Patches pushed to master.

Thanks Joonas!
Comment 12 Chris Cormack 2015-09-22 17:00:15 UTC
Pushed to 3.20.x will be in 3.20.4
Comment 13 Liz Rea 2015-09-22 23:18:45 UTC
Pushed to 3.18.x will be in 3.18.11