| Summary: | Remove Problematic Logic from Patron Messaging Preferences Form | ||
|---|---|---|---|
| Product: | Koha | Reporter: | David Cook <dcook> |
| Component: | Patrons | Assignee: | David Cook <dcook> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | chris, gmcharlt, jonathan.druart, kyle.m.hall, melia, sophie.meynieux |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 11125 | ||
| Attachments: |
Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form
Staff - patron messaging preferences OPAC - patron messaging preferences Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form [PASSED QA] Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form |
||
|
Description
David Cook
2012-10-31 21:16:56 UTC
Created attachment 13159 [details] [review] Bug 9002 - Remove Problematic Logic from Patron Messaging Preferences Form It's a super basic code removal. In most cases, its presence would probably not cause a problem. Currently, if SMSSendDriver isn't set, then there is no SMS column and no SMS checkbox. However, the logic says that if there is a message_transport_type, there should be a checkbox, and if there is not, there should be a hyphen. Since there is no heading, this could cause layout issues for people without the right message_transport_types. It makes sense to use SMSSendDriver as the condition rather than the message_transport_type... 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 Created attachment 13301 [details]
Staff - patron messaging preferences
Created attachment 13302 [details]
OPAC - patron messaging preferences
(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. (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. I get the ergonomic issue without the patch (with SMSSendDriver blank) but I have an sms entry in my message_transport_types table. Created attachment 16226 [details] [review] 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> 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 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. This patch has been pushed to master. Pushed to 3.10.x and 3.8.x will be in 3.10.5 and 3.8.12 As opac template does not use messaging-preference-form.inc, this problem still exists on OPAC |