Created attachment 41664 [details] [review] 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 Still testing this, still see an issue here. Finish it tomorrow. Created attachment 42050 [details] [review] 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. Created attachment 42051 [details] [review] 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> The same line should be affected at the intranet too (members/memberentry.pl) Created attachment 42162 [details] [review] 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> 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> 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> 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> I feel a bit like Marcel kidnapped the bug ;)... but the changes make sense to me. Patches pushed to master. Thanks Joonas! Pushed to 3.20.x will be in 3.20.4 Pushed to 3.18.x will be in 3.18.11 |
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