Description
Kyle M Hall (khall)
2015-01-22 17:25:41 UTC
Created attachment 35471 [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. 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. 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. 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 Created attachment 118381 [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 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 Created attachment 118648 [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 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 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> 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> 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> 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. 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 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> 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> 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> 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> Thx for putting in the tooltip! Created attachment 120760 [details] [review] Bug 13613: Move similar JS code to a function 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? (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 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? Created attachment 120838 [details] [review] Bug 13613: Move similar JS code to a function Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.06 Doesn't apply cleanly to 20.05.x, please rebase if needed. |