Bug 13613 - Don't allow digest to be selected without a digest-able transport selected
Summary: Don't allow digest to be selected without a digest-able transport selected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29227 30175
  Show dependency treegraph
 
Reported: 2015-01-22 17:25 UTC by Kyle M Hall
Modified: 2022-06-06 20:31 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06


Attachments
Bug 13613 - Don't allow digest to be selected without a digest-able transport selected (16.10 KB, patch)
2015-01-22 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13613 - Don't allow digest to be selected without a digest-able transport selected (16.92 KB, patch)
2021-03-17 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none' (8.19 KB, patch)
2021-03-17 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: (follow-up) Use more unique classes (12.78 KB, patch)
2021-03-17 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: (follow-up) Use more unique classes (12.78 KB, patch)
2021-03-23 13:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: (follow-up) Use more unique classes (12.75 KB, patch)
2021-03-24 13:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: Don't allow digest to be selected without a digest-able transport selected (16.96 KB, patch)
2021-04-08 15:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none' (8.24 KB, patch)
2021-04-08 15:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13613: (follow-up) Use more unique classes (12.80 KB, patch)
2021-04-08 15:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13613: (follow-up) Add tooltip when disabled (8.92 KB, patch)
2021-04-23 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13613: Don't allow digest to be selected without a digest-able transport selected (17.02 KB, patch)
2021-05-09 12:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none' (8.30 KB, patch)
2021-05-09 12:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13613: (follow-up) Use more unique classes (12.86 KB, patch)
2021-05-09 12:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13613: (follow-up) Add tooltip when disabled (8.98 KB, patch)
2021-05-09 12:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13613: Move similar JS code to a function (2.93 KB, patch)
2021-05-10 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13613: Move similar JS code to a function (5.21 KB, patch)
2021-05-11 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-01-22 17:25:41 UTC
By default, only the email transport is enabled. This gives the messaging preferences the look of having email and digest and two options. I.E. to some users it appears that you check email for single emails, *or* you check digest for a digest email.

To help remove this possible confusion, the digest mode checkbox should be disabled if no digest-able transports are checked for a given notice.
Comment 1 Kyle M Hall 2015-01-22 17:40:38 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2015-02-07 15:14:48 UTC
I tried testing, in terms of not letting me check digest box until I checked email it worked.  But I ran into some issues:

1 - When initially trying to click on the digest box (with nothing else checked) my browser focus jumps to the top of the page, same when I click on email box.  Third try stayed and let me click email box.

2 - If I had only email enabled I was unable to save any preferences, I got a "Please enter a valid email address" note in red next to the checkbox.  Entering a valid email did not clear it, entering secondary etc. emails did not.  Editing the account to add a valid email, saving, then trying to check a messaging box also failed.  The only way I could past this error was to enable additional transports, phone or SMS in any combination removed the error amd actually let me check email whether or not I had a valid email address on file.  When I switch to master I cannot get this error

3 - While I was prevented from checking digest until another transport was selected, digest wasn't cleared when I unchecked the emial box.  It didn't save, which is good, but the box should be cleared too.
Comment 3 Nick Clemens 2021-03-17 13:51:22 UTC
Created attachment 118379 [details] [review]
Bug 13613 - Don't allow digest to be selected without a digest-able transport selected

By default, only the email transport is enabled. This gives the
messaging preferences the look of having email and digest and two
options. I.E. to some users it appears that you check email for single
emails, *or* you check digest for a digest email.

To help remove this possible confusion, the digest mode checkbox should
be disabled if no digest-able transports are checked for a given notice.

Test Plan:
1) Apply this patch
2) For both the staff interface and the opac, note that the digest checkbox
   is disabled any time that no digest enabled transport ( email, sms if enabled )
   is checked for the patron editor.
Comment 4 Nick Clemens 2021-03-17 13:51:24 UTC
Created attachment 118380 [details] [review]
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none'

There was a function and a set of onclick events to handle the 'Do not notify'
checkboxes.

Those have been removed, so should this code

To test:
1 - Sign in to opac
2 - Click on 'your messaging' tab from 'your account'
3 - Check/uncheck some boxes
4 - Note error in the console:
    Uncaught TypeError: document.opacmessaging.none2 is undefined
5 - Apply patch
6 - repeat
7 - no more error
Comment 5 Nick Clemens 2021-03-17 13:51:27 UTC Comment hidden (obsolete)
Comment 6 Andrew Fuerste-Henry 2021-03-17 13:58:39 UTC
I get the following error when trying to save a patron category with default messaging prefs of SMS, Email, Digest only for Item Due:

C4::Members::Messaging::SetMessagingPreference(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`borrower_message_transport_preferences`, CONSTRAINT `borrower_message_transport_preferences_ibfk_2` FOREIGN KEY (`message_transport_type`) REFERENCES `message_transport_types` ) [for Statement "INSERT INTO borrower_message_transport_preferences
(borrower_message_preference_id, message_transport_type)
VALUES
(?, ?)
" with ParamValues: 0='1', 1="pmp_sms"] at /kohadevbox/koha/C4/Form/MessagingPreferences.pm line 99
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 7 Nick Clemens 2021-03-23 13:27:24 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2021-03-24 13:10:02 UTC
Created attachment 118742 [details] [review]
Bug 13613: (follow-up) Use more unique classes

This resolves the issue of being unable to save email as it was being hit by the validator
Comment 9 Owen Leonard 2021-04-08 15:22:55 UTC
Created attachment 119331 [details] [review]
Bug 13613: Don't allow digest to be selected without a digest-able transport selected

By default, only the email transport is enabled. This gives the
messaging preferences the look of having email and digest and two
options. I.E. to some users it appears that you check email for single
emails, *or* you check digest for a digest email.

To help remove this possible confusion, the digest mode checkbox should
be disabled if no digest-able transports are checked for a given notice.

Test Plan:
1) Apply this patch
2) For both the staff interface and the opac, note that the digest
   checkbox is disabled any time that no digest enabled transport (
   email, sms if enabled ) is checked for the patron editor.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Owen Leonard 2021-04-08 15:23:00 UTC
Created attachment 119332 [details] [review]
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none'

There was a function and a set of onclick events to handle the 'Do not notify'
checkboxes.

Those have been removed, so should this code

To test:
1 - Sign in to opac
2 - Click on 'your messaging' tab from 'your account'
3 - Check/uncheck some boxes
4 - Note error in the console:
    Uncaught TypeError: document.opacmessaging.none2 is undefined
5 - Apply patch
6 - repeat
7 - no more error

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2021-04-08 15:23:04 UTC
Created attachment 119333 [details] [review]
Bug 13613: (follow-up) Use more unique classes

This resolves the issue of being unable to save email as it was being hit by the validator

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Katrin Fischer 2021-04-22 22:26:27 UTC
I like the idea of this, but I feel it would work better with a tool tip and if the checkboxes were greyed out more visible while being unusable. What do you think?

We have an example for exactly the tooltip at least on the patron attributes administration page. To test: Add the same attribute with same value to 2 patrons and try to make it unique.
Comment 13 Nick Clemens 2021-04-23 12:21:13 UTC
Created attachment 120081 [details] [review]
Bug 13613: (follow-up) Add tooltip when disabled

This patch adds a tooltip to the digest checkboxes when disabled.

When enabled the tooltip is hidden
Comment 14 Katrin Fischer 2021-05-09 12:44:38 UTC
Created attachment 120737 [details] [review]
Bug 13613: Don't allow digest to be selected without a digest-able transport selected

By default, only the email transport is enabled. This gives the
messaging preferences the look of having email and digest and two
options. I.E. to some users it appears that you check email for single
emails, *or* you check digest for a digest email.

To help remove this possible confusion, the digest mode checkbox should
be disabled if no digest-able transports are checked for a given notice.

Test Plan:
1) Apply this patch
2) For both the staff interface and the opac, note that the digest
   checkbox is disabled any time that no digest enabled transport (
   email, sms if enabled ) is checked for the patron editor.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2021-05-09 12:44:43 UTC
Created attachment 120738 [details] [review]
Bug 13613: (Bug 22744 follow-up) Remove js handling for 'none'

There was a function and a set of onclick events to handle the 'Do not notify'
checkboxes.

Those have been removed, so should this code

To test:
1 - Sign in to opac
2 - Click on 'your messaging' tab from 'your account'
3 - Check/uncheck some boxes
4 - Note error in the console:
    Uncaught TypeError: document.opacmessaging.none2 is undefined
5 - Apply patch
6 - repeat
7 - no more error

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2021-05-09 12:44:48 UTC
Created attachment 120739 [details] [review]
Bug 13613: (follow-up) Use more unique classes

This resolves the issue of being unable to save email as it was being hit by the validator

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2021-05-09 12:44:54 UTC
Created attachment 120740 [details] [review]
Bug 13613: (follow-up) Add tooltip when disabled

This patch adds a tooltip to the digest checkboxes when disabled.

When enabled the tooltip is hidden

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2021-05-09 12:45:59 UTC
Thx for putting in the tooltip!
Comment 19 Jonathan Druart 2021-05-10 09:55:24 UTC
Created attachment 120760 [details] [review]
Bug 13613: Move similar JS code to a function
Comment 20 Jonathan Druart 2021-05-10 09:58:53 UTC
It's not clear to me how strong we want to prevent that.

1. existing values:
  a. should we update the DB?
  b. what if the form is loaded with all checked?
2. Shouldn't it be forbidden at lower level?
Comment 21 Nick Clemens 2021-05-10 11:06:32 UTC
(In reply to Jonathan Druart from comment #20)
> It's not clear to me how strong we want to prevent that.
> 
> 1. existing values:
>   a. should we update the DB?
>   b. what if the form is loaded with all checked?
> 2. Shouldn't it be forbidden at lower level?

TLDR - I think these are all correct, but this bug is very old and offers improvement - we should file new bugs for remaining issues

1 -
  a - The current behaviour if only digest selected is to send nothing - we check if they want a digest, then loop the transports - we could clean up, but loading a patron after this bug will fix things and behaviour won't change
  b - All checked is fine - digestible notices will be digested, and others will not be
2 - Yes, but things are handled in C4/Form/MessagingsPreferences.pm - this needs tests/moved to Koha
Comment 22 Katrin Fischer 2021-05-10 11:22:47 UTC
I see this more as a "helper" kind of thing. Nothing bad is going to happen if you check digest and no transport afaik - it just doesn't make sense?
Comment 23 Jonathan Druart 2021-05-11 13:13:54 UTC
Created attachment 120838 [details] [review]
Bug 13613: Move similar JS code to a function
Comment 24 Jonathan Druart 2021-05-11 13:39:04 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 25 Fridolin Somers 2021-05-12 15:13:30 UTC
Pushed to 20.11.x for 20.11.06
Comment 26 Andrew Fuerste-Henry 2021-05-25 13:21:47 UTC
Doesn't apply cleanly to 20.05.x, please rebase if needed.