Web Client: SMS notification dropped from Place Holds recently

Bug #1716475 reported by tji@sitka.bclibraries.ca
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Medium
Unassigned
2.12
Fix Released
Medium
Unassigned

Bug Description

The server is the master about three weeks. 'Suspend this hold?' is added to this server. But the SMS Notification Carrier and Number are gone.

The option is on an earlier version server without 'Suspend this hold?'.

Tags: pullrequest
Revision history for this message
Kathy Lussier (klussier) wrote :

Tina,

Does your test server have SMS enabled in the library settings editor?

Kathy

Revision history for this message
tji@sitka.bclibraries.ca (tji) wrote :

Hi Kathy,

The setting was set to True for the branch when the option was not on Place Hold.

I just tested setting it to True for CONS. Now the option shows up, but with a weird top line in the Carrier list. Attached screenshot.

I also tried to set up the setting for SYS. But the option did not show up.

Tina

Revision history for this message
tji@sitka.bclibraries.ca (tji) wrote :

I tested the old server of the setting. The behaviour is the same. The setting has effect only when it's on CONS level. Settings on branch or system level (True or False) have no effect.

The Carrier list looks good on the older server.

Revision history for this message
Ben Shum (bshum) wrote :

Tina, I think you've stumbled on a potential odd interaction with the PO template file tpac.

I was mentioning to Galen earlier today that I saw that we're adding an empty string translation as part of this file:

Open-ILS/src/templates/opac/parts/sms_carrier_selector.tt2, around line 22 in master

[% l('') %]

That's coming into the PO template for master as msgid "" msgstr "" needing translation. But at the top of the tpac.pot file, there's also a msgid "" with mgstr "" containing the metadata for translation info and generating the PO template.

Looks like you're getting that translation metadata pulled in as the first entry, instead of the empty "" string as expected.

I believe Cesar and Galen intended that the first entry in the sms carrier list show up as blank, so maybe we should alter the line in sms_carrier_selector.tt2 to be translating for ' ' (one space) so that it's different than the default metadata tag and does not cause duplication errors. Or we should put a proper value in that first field somehow? Or remove the translation from it entirely, and leave it empty space, but not translatable.

Revision history for this message
tji@sitka.bclibraries.ca (tji) wrote :

It is probably not necessary to have a blank top line in the list. Since there is a checkbox for the option, I assume if it is not selected, no carrier will be written into the hold record.

Revision history for this message
Kathy Lussier (klussier) wrote :

I don't think we want to revert to listing a valid SMS carrier as the first option, because then we'll see the problem reported in bug 1669534 again.

Maybe we can have a label for that first option, like "Please Select SMS Carrier"?

Revision history for this message
Kathy Lussier (klussier) wrote :

I have a working branch at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1716475-add-label-for-sms-selector that adds a label inside the menu.

In addition to addressing the translation issue, I think this the label is recommended for usability. See Labeling Select Menus here: http://uxmovement.com/forms/stop-misusing-select-menus/

tags: added: pullrequest
Changed in evergreen:
importance: Undecided → Medium
milestone: none → 3.0-beta2
Revision history for this message
Ben Shum (bshum) wrote :

Having a clearly defined label fixes the issue I saw with the bad LP metadata being pulled in for translation in other locales, and also makes it clear that the user should make a choice. Tested and it still required me to pick an sms carrier too.

Pushed to master and backported to rel_2_12. Thanks Kathy!

Changed in evergreen:
status: New → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.