Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form
Summary: Remove Problematic Logic from Patron Messaging Preferences Form
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 11125
  Show dependency treegraph
 
Reported: 2012-10-31 21:16 UTC by David Cook
Modified: 2014-05-26 21:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form (1.93 KB, patch)
2012-10-31 21:23 UTC, David Cook
Details | Diff | Splinter Review
Staff - patron messaging preferences (21.73 KB, image/tiff)
2012-11-08 01:25 UTC, Melia Meggs
Details
OPAC - patron messaging preferences (35.04 KB, image/tiff)
2012-11-08 01:26 UTC, Melia Meggs
Details
Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form (1.71 KB, patch)
2013-03-18 11:00 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
[PASSED QA] Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form (1.81 KB, patch)
2013-03-30 22:19 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2012-10-31 21:16:56 UTC
If you do not have SMSSendDriver set and you do not have a message_transport_type of "sms", you will get an extra table column filled with a hyphen "-" and no corresponding table heading in the Patron Messaging Preferences table.  

This pushes the table out of alignment so the email checkbox goes under "Digests Only?", the digests checkbox goes under "Do not notify", and the Do not notify checkbox is pushed to the side with no heading.

---

While this doesn't affect new installs, it might affect upgrades, unless the upgrade scripts insert message_transport_type preferences.

---

In any case, it's a couple lines of useless code. It makes more sense to use the same logic as TalkingTechItivaPhone.
Comment 1 David Cook 2012-10-31 21:23:29 UTC Comment hidden (obsolete)
Comment 2 Melia Meggs 2012-11-08 01:25:15 UTC
Hi David,

I wanted to test your patch, but I am not sure I understand the bug.

I am testing on master.  I have SMSSendDriver blank in my system preferences.  I have attached screenshots of what my patron messaging preferences look like in the staff client and in the OPAC.  I think my columns look ok.

Is there some other system preference that I need to change that will knock things out of alignment?  Or is it maybe a browser specific issue?  (I'm using Chrome.)

Thanks,

Melia
Comment 3 Melia Meggs 2012-11-08 01:25:41 UTC
Created attachment 13301 [details]
Staff - patron messaging preferences
Comment 4 Melia Meggs 2012-11-08 01:26:02 UTC
Created attachment 13302 [details]
OPAC - patron messaging preferences
Comment 5 Owen Leonard 2012-11-12 17:49:20 UTC
(In reply to comment #2)
> Is there some other system preference that I need to change that will knock
> things out of alignment?

I don't see the problem in my installation either. From the description this sounds like a problem we had some time ago and fixed.
Comment 6 David Cook 2012-11-14 19:48:43 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > Is there some other system preference that I need to change that will knock
> > things out of alignment?
> 
> I don't see the problem in my installation either. From the description this
> sounds like a problem we had some time ago and fixed.

Thanks for your comments Melia and Owen.

I should've mentioned that message_transport_type is set in the database, so you would have to delete the message_transport_type of "sms" from your database in order to see the layout problem.

I've done some additional research, and I think the message_transport_type of "sms" was introduced back around version 3.1, so it "shouldn't" be missing from the database. I'm not sure why it was missing in the case I encountered. I think 99% of the time, the logic I outlined above wouldn't be a problem.

This patch is for the other 1% and just to remove senseless code. 

To test it...I would suggest applying the patch and then try it with SMSSendDrive turns on and with it turned off. If it doesn't break anything, I would say that it's working.
Comment 7 Jonathan Druart 2012-12-03 11:46:37 UTC
I get the ergonomic issue without the patch (with SMSSendDriver blank) but I have an sms entry in my message_transport_types table.
Comment 8 Sophie MEYNIEUX 2013-03-18 11:00:03 UTC Comment hidden (obsolete)
Comment 9 Sophie MEYNIEUX 2013-03-18 11:10:48 UTC
Without the patch, if SMSSendDriver is blank, there is an extra column without heading in Messaging preferences table and column content is not under the correct heading.
If SMSDriver is not blank, there is a SMS column and the table is correctly displaid.

With the patch, if SMSSendDriver is not blanck, the table is correctly displaid with a SMS column, and if SMSSendDriver is blank, the table is correctly displaid without SMS column.

For me, having or not 'sms' in message_transport_types has no consequences.

Signed-off
Comment 10 Katrin Fischer 2013-03-30 22:19:21 UTC
Created attachment 17062 [details] [review]
[PASSED QA] Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form

If you do not have SMSSendDriver set and you do not have a message_transport_type of "sms", you will get an extra table column filled with a hyphen "-" and no corresponding table heading in the Patron Messaging Preferences table.

This pushes the table out of alignment so the email checkbox goes under "Digests Only?", the digests checkbox goes under "Do not notify", and the Do not notify checkbox is pushed to the side with no heading.

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested for regressions.
All tests and QA script pass.
Comment 11 Jared Camins-Esakov 2013-03-31 01:14:48 UTC
This patch has been pushed to master.
Comment 12 Chris Cormack 2013-03-31 05:58:02 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.5 and 3.8.12
Comment 13 Sophie MEYNIEUX 2013-10-23 15:21:42 UTC
As opac template does not use messaging-preference-form.inc, this problem still exists on OPAC