Bug 7557 - Change hardcoded routing list note into a system preference
Summary: Change hardcoded routing list note into a system preference
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-19 08:59 UTC by Katrin Fischer
Modified: 2012-10-25 23:09 UTC (History)
5 users (show)

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


Attachments
hardcoded note on routing list (29.78 KB, image/png)
2012-02-19 08:59 UTC, Katrin Fischer
Details
Bug 7557: Change hardcoded routing list note into a system preference (6.31 KB, patch)
2012-02-19 11:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7557: Change hardcoded routing list note into a system preference (6.37 KB, patch)
2012-02-21 01:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7557: Change hardcoded routing list note into a system preference (7.11 KB, patch)
2012-03-12 00:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7557 - corrects missing semi-colon ';' (2.56 KB, patch)
2012-03-22 11:18 UTC, Mason James
Details | Diff | Splinter Review
Bug 7557 - corrects missing semi-colon ';', removes extra '`' (2.58 KB, patch)
2012-03-22 12:44 UTC, Mason James
Details | Diff | Splinter Review
Bug 7557 - corrects missing semi-colon ';', removes extra '`' (2.58 KB, patch)
2012-03-22 12:45 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2012-02-19 08:59:34 UTC
Created attachment 7756 [details]
hardcoded note on routing list

Remove the hardcoded note from the routing list in favor of a new system preference for displaying a note on all routing lists.

Notes: Please return this item promptly as others are waiting for it.
Comment 1 Katrin Fischer 2012-02-19 11:39:29 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-02-21 01:58:17 UTC
Passes valid template tests
rorohiko:[git/7557-]:~/git/koha% perl xt/tt_valid.t 
1..1
ok 1 - TT syntax: not using TT directive within HTML tag

Passes translation tests
rorohiko:[git/7557-]:~/git/koha% perl xt/author/translatable-templates.t       
1..2
ok 1 - opac templates are translatable
ok 2 - intranet templates are translatable

Functionality works as described and the note is now editable, signed off patch to follow
Comment 3 Chris Cormack 2012-02-21 01:58:51 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2012-02-29 15:01:03 UTC
HELP: Patch fails to apply!
Comment 5 Owen Leonard 2012-02-29 15:05:20 UTC
This is a run of the mill updatedatabase.pl conflict. It should be easy to resolve. I don't think it's necessary to ask the patch submitter to resubmit.
Comment 6 Marcel de Rooy 2012-02-29 15:39:42 UTC
Forget my comment: vim created it.
Comment 7 Marcel de Rooy 2012-03-01 11:06:15 UTC
Asking a dumb question here: The hardcoded note just comes from a template file. So it should be translatable. Why put in a pref? Note that when you switch language, the pref does not change with you. So I do not really understand the advantage of this solution. Please clarify.
Comment 8 Katrin Fischer 2012-03-01 13:22:06 UTC
The advantage is, that it's configurable. At the moment the note is hardcoded and it does assume that this is the way routing lists in libraries work. You could always add a note for each routing slip below the hardcoded note - but you could not get rid of the harcoded note or hide it. I thought about removing it, but then on second thought I was thinking that having a note that will appear on all routing lists + an individual note wouuld be nice.
As you have noted, there is the problem of translatability. like there is for all things being stored in the database. In my opinion this was that the routing slip would only appear in the "main" language. Here practice is that you print the slip and attach it to the issue so people can note when they got the issue or when they gave it to the next person on list. So I don't see a loss of functionality here.
Comment 9 Marcel de Rooy 2012-03-01 13:32:33 UTC
As Katrin clarified, there is a need for individual libraries to change this default note. Editing the template is not an option then.

Patch looks good. Marked as Passed QA.
Comment 10 Paul Poulain 2012-03-02 17:20:22 UTC
yikes, big bug, the patch can't be included:

=> 
Operator or semicolon missing before &searchfield at installer/data/mysql/updatedatabase.pl line 4731.
Ambiguous use of & resolved as operator & at installer/data/mysql/updatedatabase.pl line 4731.
Can't modify bitwise and (&) in scalar assignment at installer/data/mysql/updatedatabase.pl line 4732, near "RoutingListNote#jumped">RoutlingListNote</a> system preference.','Define a note to be shown on all routing lists','70|10','Textarea');");


(thx my pre-apply that detected this)

=> the updatedatabase containt a do("INSERT ... <a href="...">")
" of href must be escaped
Comment 11 Marcel de Rooy 2012-03-02 17:58:18 UTC
Good catch, Paul!
Looks good was not is good :-)
Hard to see when browsing through the code, but this error should have been seen in the output of the webinstaller when running the update?
Comment 12 Paul Poulain 2012-03-06 16:54:34 UTC
marcel: yes, it should. It probably means the tester installed a new database from scratch, and not did an update

(is it me or the "reply" link doesn't copy the original comment anymore ?
Comment 13 Katrin Fischer 2012-03-06 16:56:27 UTC
And before you ask... I didn't notice because the link was something I put in last minute. I will fix the patch :) QA++
Comment 14 Owen Leonard 2012-03-06 17:05:57 UTC
(In reply to comment #12)
> (is it me or the "reply" link doesn't copy the original comment anymore ?

I think you have to make sure you're not using a cached copy of a javascript file. I noticed the same thing and a shift-reload fixed it.
Comment 15 Paul Poulain 2012-03-06 17:13:50 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > (is it me or the "reply" link doesn't copy the original comment anymore ?
> I think you have to make sure you're not using a cached copy of a javascript
> file. I noticed the same thing and a shift-reload fixed it.

good catch, that's it, as you can see
Comment 16 Katrin Fischer 2012-03-12 00:28:11 UTC
Created attachment 8170 [details] [review]
Bug 7557: Change hardcoded routing list note into a system preference

Adds a new system preference RoutingListNote under the Serials tab.

The note will display above the note from the subscription and replace
the current hardcoded note:

"Notes: Please return this item promptly as others are waiting for it."

The patch adds unique ids to all notes and the note in general, so it
can be styled using CSS.

Also corrects the routing slip template to follow the HTML4 rule.

Update 2012-03-12: Fixed problem in updatedatebase.
Comment 17 Paul Poulain 2012-03-14 15:35:07 UTC
(considering the signed-off is still valid, as this bug already made a full loop and failed QA because of a small typo)

QA comment:

one of our rule is that a new feature should be introduced without trouble for existing setups.
The idea of having
To change this note edit RoutlingListNote system preference.

into the syspref by default may seem good, but if the user don't spot this, or don't care, it will be written suddenly on printed routing slips.

So i've removed this default RoutingListNote

Other than this, passed QA, will push
Comment 18 Katrin Fischer 2012-03-14 15:37:12 UTC
I was torn here. Now the old sentence will no longer be printed out - but putting the english sentence in the system prefernece would not have worked either. I think removing it is ok, but will not advertise the new feature. There is a disadvantage to every solution I could think of.
Comment 19 Paul Poulain 2012-03-14 15:39:40 UTC
> I think removing it is ok, but will not advertise the new feature.

=> that's what Release Notes are made for (even if I know many ppl don't read them)
Comment 20 Nicole C. Engard 2012-03-14 20:05:12 UTC
This says pushed to master but I don't see it in my admin area - was there a problem with my upgrade?
Comment 21 Katrin Fischer 2012-03-14 20:28:26 UTC
Hi Nicole, I think it was not pushed - i can't see it on git. I reset the status to "Passed QA" so it shows up in Paul's list :)
Comment 22 Paul Poulain 2012-03-15 05:31:53 UTC
sorry, I pushed the branch, but forgot to push master before closing my computer yesterday !
Comment 23 Mason James 2012-03-22 11:18:53 UTC Comment hidden (obsolete)
Comment 24 Mason James 2012-03-22 12:44:33 UTC Comment hidden (obsolete)
Comment 25 Mason James 2012-03-22 12:45:16 UTC
Created attachment 8505 [details] [review]
Bug 7557 - corrects missing semi-colon ';', removes extra '`'
Comment 26 Katrin Fischer 2012-03-31 17:26:40 UTC
Hi Mason, updatedatabase got fixed - I am closing this bug. Please reopen if I missed something.