Bug 8202 - Add authorities to export tool
Summary: Add authorities to export tool
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jared Camins-Esakov
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 7986 8203 8212 8268
  Show dependency treegraph
 
Reported: 2012-06-07 12:34 UTC by Jared Camins-Esakov
Modified: 2013-12-05 20:01 UTC (History)
3 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 8202: Add authority export to tools/export.pl (15.05 KB, patch)
2012-06-15 17:13 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl (15.25 KB, patch)
2012-06-15 19:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl (15.08 KB, patch)
2012-07-02 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8202 follow-up UNIMARC authority support (904 bytes, patch)
2012-07-03 16:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8202 follow-up UNIMARC authority support (1012 bytes, patch)
2012-07-03 16:53 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-06-07 12:34:25 UTC
Right now it is not possible to export authorities from the staff client. It should be possible to export authorities using the export tool.
Comment 1 Jared Camins-Esakov 2012-06-15 17:13:01 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-06-15 19:32:03 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2012-06-21 11:44:33 UTC
Arf, A patch for Bug 7986 introduces a lot of modifications for the same file tools/export.pl
Comment 4 Katrin Fischer 2012-06-21 11:48:57 UTC
Hi Jonathan, I know it's annoying, but as this patch is already signed off, I don't think the other should block it. This way 2 patches will have to be reworked.
Comment 5 Jonathan Druart 2012-06-22 10:00:55 UTC
QA comments:

Hi Jared,

Your modifications seem good and they are close to mine (for export.pl).
About your patch, I think you can repropose it modifying the indentation of export.tt (ex. 
+        <ol><li>

+        </li></ol>

+[% ELSE %]

+<div id="exporttype" class="toptabs">
+<ul>
)

I agree with you, I quibble...

However, more important, I found a bug in this script while testing.
I can't exclude from export some fields, as 500$3 for example. The regexp to split field and subfield is /^(\d*)(\w)?$/;
This is not what we want.

And, the subfield's deletion:
if( $subfield ) {
     $record->field($field)->delete_subfields($subfield);
}
only delete in the first field.
Comment 6 Jonathan Druart 2012-06-22 10:05:27 UTC
Sorry, you didn't introduce these bugs.

And I can't failed QA only for indentation, I switch back to signed off.
Comment 7 Jared Camins-Esakov 2012-06-22 11:27:24 UTC
(In reply to comment #5)
> QA comments:
> 
> Hi Jared,
> 
> Your modifications seem good and they are close to mine (for export.pl).
> About your patch, I think you can repropose it modifying the indentation of
> export.tt (ex. 
> +        <ol><li>
> 
> +        </li></ol>
> 
> +[% ELSE %]
> 
> +<div id="exporttype" class="toptabs">
> +<ul>
> )
> 
> I agree with you, I quibble...

I would be happy to submit a follow-up fixing indentation. I was very tempted to reindent export.tt, actually, but section 1.3 in the coding guidelines says don't refactor needlessly, and don't spend time changing someone else's style to yours. :)
Comment 8 Jonathan Druart 2012-07-02 14:43:43 UTC
Created attachment 10607 [details] [review]
[SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl

Rebased patch
Comment 9 Jonathan Druart 2012-07-02 14:43:55 UTC
Marking as Passed QA
Comment 10 Paul Poulain 2012-07-03 16:42:25 UTC
(In reply to comment #7)
> I would be happy to submit a follow-up fixing indentation. I was very
> tempted to reindent export.tt, actually, but section 1.3 in the coding
> guidelines says don't refactor needlessly, and don't spend time changing
> someone else's style to yours. :)

We also have decided to use the default perltidy formatting, so, this rule "wins" against the "don't spend time..."

Anyway, there's something wronger than this : you test the MARCflavour, and, if it's UNIMARC you send do ->as_xml('unimarc')
When it's an authority, you must do ->as_xml('UNIMARCAUTH')

The 100 -coded- field in UNIMARCAUTH has a different size than UNIMARC (biblio)

So you get a nasty:
Unsupported UNIMARC character encoding [] for XML output for UNIMARC; 100$a -> 20091001afrey50      ba0 at /usr/share/perl5/MARC/File/XML.pm line 505.

(trying to decode a 100 "biblio", but it's an auth, so considered wrong size)

yes, that's silly, but UNIMARC is silly ;-)

Please signoff the follow-up and i'll push your patch with my (tiny) follow-up
Comment 11 Paul Poulain 2012-07-03 16:42:37 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2012-07-03 16:53:23 UTC
Created attachment 10630 [details] [review]
Bug 8202 follow-up UNIMARC authority support

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 13 Jared Camins-Esakov 2012-12-03 13:29:48 UTC
This feature is available in Master.