Bug 9497 - Make it easier to add to the the list of possible encodings for Z39.50 servers and add ISO 8859-1
Summary: Make it easier to add to the the list of possible encodings for Z39.50 server...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Magnus Enger
QA Contact:
URL:
Keywords:
: 8081 (view as bug list)
Depends on:
Blocks: 9498
  Show dependency treegraph
 
Reported: 2013-01-27 12:23 UTC by Magnus Enger
Modified: 2013-12-05 20:05 UTC (History)
5 users (show)

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


Attachments
Bug 9497 - Make it easier to add new encodings for Z39.50 servers and add ISO 8859-1 (3.14 KB, patch)
2013-01-27 13:04 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 9497 - Make it easier to add new encodings for Z39.50 servers and add ISO 8859-1 (3.24 KB, patch)
2013-01-27 13:59 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9497 - Make it easier to add new encodings for Z39.50 servers and add ISO 8859-1 (3.35 KB, patch)
2013-01-30 06:40 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 Magnus Enger 2013-01-27 12:23:19 UTC
At least the Norwegian BIBSYS Z39.50 server delivers records in ISO 8859-1, but that is not an option when you edit Z39.50 servers. Patch coming in a moment!
Comment 1 Magnus Enger 2013-01-27 12:49:00 UTC
*** Bug 8081 has been marked as a duplicate of this bug. ***
Comment 2 Magnus Enger 2013-01-27 12:50:13 UTC
Broadening the scope of this bug a bit.
Comment 3 Magnus Enger 2013-01-27 12:58:21 UTC
Problems:

This code in admin/z3950servers.pl was creating separate variables for each encoding:

$template->param( $_ . $data->{$_} => 1)
        for ( qw( syntax encoding ) );

And the corresponding code in the template had to check each of these variables to decide which one should be selected:

[% IF ( encodingutf8 ) %]
<option value="utf8" selected="selected">UTF-8</option>
[% ELSE %]
<option value="utf8">UTF-8</option>
[% END %]
[% IF ( encodingEUC_KR ) %]
<option value="EUC-KR" selected="selected">EUC-KR</option>
[% ELSE %]
<option value="EUC-KR">EUC-KR</option>
[% END %]
etc. 

This has a couple of problems:

- Encodings with a hyphen ("-") in them create invalid variable names in TT, as seen in Bug 8081
- Adding a new encoding means adding 5 lines to the template

My proposed solution creates just one variable for the encoding and sets it to the name of the encoding, thus avoiding the hyphen problem. It also generates the options from a list of encodings (defined in the template), which means that future encodings can just be added to the list.
Comment 4 Magnus Enger 2013-01-27 13:04:56 UTC Comment hidden (obsolete)
Comment 5 Bernardo Gonzalez Kriegel 2013-01-27 13:59:05 UTC Comment hidden (obsolete)
Comment 6 Mason James 2013-01-30 06:40:12 UTC
Created attachment 14933 [details] [review]
Bug 9497 - Make it easier to add new encodings for Z39.50 servers and add ISO 8859-1

This patch does three things:
- Makes admin/z3950servers.pl create one variable for encoding,
  not one variable for each encoding
- Makes the template create encoding options from a list
- Adds ISO 8859-1 to the list of available encodings
See the bug for a lengthier description

To test:
- Apply the patch
- Edit one Z39.50 server several times, choosing each available encoding in turn
- Check that for each encoding, the correct encoding is shown both in the list of
  Z39.50 servers and in the dropdown on the edit screen

Comment: Works as described, no errors.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 7 Mason James 2013-01-30 06:42:10 UTC
(In reply to comment #6)
> Created attachment 14933 [details] [review]
> Bug 9497 - Make it easier to add new encodings for Z39.50 servers and add
> ISO 8859-1
> 
> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
> Signed-off-by: Mason James <mtj@kohaaloha.com>

testing 1 commit(s) (applied to f8686e9 'Bug 9116: Remove some useless tmp var')

passing QA, works well :)
Comment 8 Jared Camins-Esakov 2013-01-31 15:57:21 UTC
This patch has been pushed to master.
Comment 9 Chris Cormack 2013-02-01 08:53:35 UTC
Pushed to 3.10.x, will be in 3.10.3
Comment 10 Magnus Enger 2013-04-03 19:07:11 UTC
Works as expected (tested with packages built off current master).