Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned
Summary: When SMS is enabled the OPAC messaging table is misaligned
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Fridolin Somers
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 6458 6726
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-29 14:26 UTC by Fridolin Somers
Modified: 2019-10-14 19:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned (3.70 KB, patch)
2017-05-29 14:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Email notifications can no longer be edited with patch (41.97 KB, image/png)
2017-05-29 16:11 UTC, Marc Véron
Details
Bug 18692: (follow-up) Fix misplaced columns and add this feature for phone as well (5.64 KB, patch)
2017-05-31 14:33 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned (2.77 KB, patch)
2017-06-01 15:31 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18692 - same with syspref TalkingTechItivaPhone (1.73 KB, patch)
2017-06-01 15:31 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18692 - same with syspref TalkingTechItivaPhone (2.46 KB, patch)
2017-06-01 15:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned (2.83 KB, patch)
2017-06-16 18:24 UTC, Michael Cabus
Details | Diff | Splinter Review
Bug 18692 - same with syspref TalkingTechItivaPhone (2.53 KB, patch)
2017-06-16 18:24 UTC, Michael Cabus
Details | Diff | Splinter Review
Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned (2.89 KB, patch)
2017-07-21 10:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 18692 - same with syspref TalkingTechItivaPhone (2.58 KB, patch)
2017-07-21 10:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 18692 - intranet part (2.83 KB, patch)
2017-08-03 08:04 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18692 - intranet part (2.89 KB, patch)
2017-08-11 15:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18692 - intranet part (2.93 KB, patch)
2017-09-01 12:59 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2017-05-29 14:26:55 UTC
Bug 6726 had corrected the fact that when SMS is enabled the messaging table is missing a column.
Bug 6458 has broken this.
I think the SMS column is missing an else case with cell containing only "-" like other columns.
Comment 1 Fridolin Somers 2017-05-29 14:38:57 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-05-29 16:11:21 UTC
Created attachment 63830 [details]
Email notifications can no longer be edited with patch

Patch is applied, syspref SMSSendDriver is empty.
Result: In column email we have a minus sign (-) instead of check boxes as without patch.
See screenshot.
Comment 3 Axelle Aix-Marseille Université 2017-05-30 09:21:15 UTC
Patch tested on sandbox 1 by Biblibre.

I agree with Marc Véron.

When syspref SMSDriver is full filled (for example with email), columns are ok.

When syspref SMSDriver is empty, it doesn't work because in column email we should see check boxes instead of "-". Check boxes should be in colunm email and in column not notify to allow our user to choose the messages they want to receive.
Comment 4 Lari Taskula 2017-05-31 14:31:31 UTC
I'm not sure I understood the original problem, perhaps screenshots would help. Is message_transports / message_transport_types table properly configured?

Anyway I actually wanted to comment that I also noticed "sms" is missing "-" in the messaging preferences table both in OPAC and staff when it has no sms transport configured in message_transports database table. The same for "phone". To my understanding, message_transports table defines availability of transport options for the messaging preferences table. sms and phone should not have checkboxes when they are removed from those options. This feature used to exist but for some reason the "-" got completely removed to fix misalignment issues.
Comment 5 Lari Taskula 2017-05-31 14:33:40 UTC Comment hidden (obsolete)
Comment 6 Fridolin Somers 2017-06-01 15:10:32 UTC
Ahhhh, I did not see that the TT vars where different :

messaging_preference.transport_sms and messaging_preference.transport[s]_sms

I will correct my path.
Comment 7 Fridolin Somers 2017-06-01 15:31:03 UTC Comment hidden (obsolete)
Comment 8 Fridolin Somers 2017-06-01 15:31:18 UTC Comment hidden (obsolete)
Comment 9 Fridolin Somers 2017-06-01 15:36:43 UTC Comment hidden (obsolete)
Comment 10 Fridolin Somers 2017-06-01 15:37:05 UTC
@Lari Taskula, I have adapted you patch.
Tell me if its ok.
Comment 11 Michael Cabus 2017-06-16 18:24:40 UTC Comment hidden (obsolete)
Comment 12 Michael Cabus 2017-06-16 18:24:51 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens (kidclamp) 2017-07-21 10:59:14 UTC
Created attachment 65170 [details] [review]
Bug 18692 - When SMS is enabled the OPAC messaging table is misaligned

Bug 6726 had corrected the fact that when SMS is enabled the messaging table is missing a column.
Bug 6458 has broken this.
The SMS column is missing an else case with cell containing only "-" like other columns.

Test plan :
- set SMSSendDriver preference empty
- go to OPAC patron messaging
- column SMS should not be visible
- set SMSSendDriver preference not empty
- go to OPAC patron messaging
- column SMS appears with checkboxes

Signed-off-by: Michael Andrew Cabus <michael@bywatersolutons.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens (kidclamp) 2017-07-21 10:59:32 UTC
Created attachment 65171 [details] [review]
Bug 18692 - same with syspref TalkingTechItivaPhone

Fixes misplaced columns introduced by previous patch and adds the "-" for phone
transport type.

To test:
1. Set SMSSendDriver system preference on
2. Go to intra and OPAC messaging preferences
3. By default you should see checkboxes for all messages for SMS
4. Ensure columns are not misplaced (pushing one column too much to the right)
5. Delete sms method from one of the messages in message_transports table
6. Observe that "-" is displayed instead of checkbox for that message for SMS
7. Repeat same for TalkingTechItivaPhoneNotification system preference.
   By default it may not have transports in message_transports, so make sure
   to assign some in order to have the checkboxes visible.

https://bugs.koha-community.org/show_bug.cgi?id=8692

Signed-off-by: Michael Andrew Cabus <michael@bywatersolutons.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens (kidclamp) 2017-07-21 11:00:12 UTC
Patches work nicely, Lari's patches included fixes for the intranet as well and those seem to have been dropped. Can we add those back in?
Comment 16 Fridolin Somers 2017-08-03 08:02:17 UTC
(In reply to Nick Clemens from comment #15)
> Patches work nicely, Lari's patches included fixes for the intranet as well
> and those seem to have been dropped. Can we add those back in?

Oh indeed.
I create a follow up from patch https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63872
Comment 17 Fridolin Somers 2017-08-03 08:04:20 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2017-08-11 15:23:52 UTC
Created attachment 65906 [details] [review]
[SIGNED-OFF] Bug 18692 - intranet part

Fixes misplaced columns introduced by previous patch and adds the "-" for phone
transport type.

To test:
1. Set SMSSendDriver system preference on
2. Go to intranet messaging preferences
3. By default you should see checkboxes for all messages for SMS
4. Ensure columns are not misplaced (pushing one column too much to the right)
5. Delete sms method from one of the messages in message_transports table
6. Observe that "-" is displayed instead of checkbox for that message for SMS
7. Repeat same for TalkingTechItivaPhoneNotification system preference.
   By default it may not have transports in message_transports, so make sure
   to assign some in order to have the checkboxes visible.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Nick Clemens (kidclamp) 2017-09-01 12:59:50 UTC
Created attachment 66731 [details] [review]
Bug 18692 - intranet part

Fixes misplaced columns introduced by previous patch and adds the "-" for phone
transport type.

To test:
1. Set SMSSendDriver system preference on
2. Go to intranet messaging preferences
3. By default you should see checkboxes for all messages for SMS
4. Ensure columns are not misplaced (pushing one column too much to the right)
5. Delete sms method from one of the messages in message_transports table
6. Observe that "-" is displayed instead of checkbox for that message for SMS
7. Repeat same for TalkingTechItivaPhoneNotification system preference.
   By default it may not have transports in message_transports, so make sure
   to assign some in order to have the checkboxes visible.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Jonathan Druart 2017-09-01 16:05:58 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 21 Fridolin Somers 2017-10-03 09:30:45 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 22 Katrin Fischer 2017-10-06 06:10:57 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.
Comment 23 Mason James 2017-11-23 23:09:03 UTC
Pushed to 16.05.x, for 16.05.18 release