Bug 10996 - Cannot exclude numeric subfields on export
Summary: Cannot exclude numeric subfields on export
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Colin Campbell
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 12860
  Show dependency treegraph
 
Reported: 2013-10-04 09:58 UTC by Colin Campbell
Modified: 2015-12-03 22:00 UTC (History)
6 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
Proposed Patch (2.64 KB, patch)
2013-10-04 10:48 UTC, Colin Campbell
Details | Diff | Splinter Review
Cannot exclude numeric subfields on export (2.70 KB, patch)
2013-10-10 07:47 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 10996 Allow numeric subfields to be stripped on export (2.78 KB, patch)
2013-10-11 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Amended patch (2.79 KB, patch)
2013-10-16 08:45 UTC, Colin Campbell
Details | Diff | Splinter Review
Amended patch (3.10 KB, patch)
2013-10-30 12:43 UTC, Colin Campbell
Details | Diff | Splinter Review
Cannot exclude numeric subfields on export (3.16 KB, patch)
2013-10-30 14:31 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 10996 Allow numeric subfields to be stripped on export (3.21 KB, patch)
2013-11-21 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2013-10-04 09:58:54 UTC
Tools export allows you to specify tags and subfields to strip from exported records. However if the subfields you specify are numeric they are not stripped

Cause the regexp used to parse the exclusion list considers all numerics to be part of the tag number so an attempt to strip $9 from tag 110 ends up in stripping tag 1109!!
Comment 1 Colin Campbell 2013-10-04 10:48:05 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-10-05 01:20:43 UTC
Patch applied cleanly, go forth and signoff
Comment 3 Paola Rossi 2013-10-10 07:47:33 UTC Comment hidden (obsolete)
Comment 4 Paola Rossi 2013-10-10 07:47:54 UTC
I sign off the patch
Comment 5 Kyle M Hall 2013-10-11 17:41:33 UTC Comment hidden (obsolete)
Comment 6 Galen Charlton 2013-10-15 16:17:42 UTC
Comment on attachment 21974 [details] [review]
Bug 10996 Allow numeric subfields to be stripped on export

Review of attachment 21974 [details] [review]:
-----------------------------------------------------------------

::: tools/export.pl
@@ +365,5 @@
> +                            my $subfield = $2;
> +
> +                            # skip if this record doesn't have this field
> +                            next if not defined $record->field($field);
> +                            if ($subfield) {

This is not the right test -- consider somebody who wants to exclude a subfield $0 (zero) from export.
Comment 7 Colin Campbell 2013-10-16 08:45:49 UTC Comment hidden (obsolete)
Comment 8 Colin Campbell 2013-10-16 08:46:47 UTC
(In reply to Galen Charlton from comment #6)
> Comment on attachment 21974 [details] [review] [review]
> Bug 10996 Allow numeric subfields to be stripped on export
> 
> Review of attachment 21974 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/export.pl
> @@ +365,5 @@
> > +                            my $subfield = $2;
> > +
> > +                            # skip if this record doesn't have this field
> > +                            next if not defined $record->field($field);
> > +                            if ($subfield) {
> 
> This is not the right test -- consider somebody who wants to exclude a
> subfield $0 (zero) from export.

Amended patch to cover this case.
Comment 9 Colin Campbell 2013-10-23 09:45:32 UTC
Reverting status
Comment 10 Paola Rossi 2013-10-28 16:32:23 UTC
Now the patch let a user exclude a subfield $0 (zero) from export, as required.
However, when the tag is repeatable and a biblio has two tags of these, koha let the user exclude the numeric subfield of the first tag, but not the second one (case of the subfield $4 in two different 700 of a biblio).
So I pass the patch to "Failed QA" status.
Comment 11 Colin Campbell 2013-10-29 13:22:40 UTC
Actually irrespective of this patch testing reveals if you specify a subfield then it only gets stripped from the first occurence. Looks like the existing functionality was in need of fixing
Comment 12 Colin Campbell 2013-10-30 12:43:20 UTC Comment hidden (obsolete)
Comment 13 Colin Campbell 2013-10-30 12:44:57 UTC
Reset status to needs signoff. Would be grateful if folks could retest and confirm that deletes are being applied to all occurences
Comment 14 Paola Rossi 2013-10-30 14:31:39 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-11-21 15:51:30 UTC
Good catch Paola!

QA comment:
The patch works great, the code was completely buggy!
Maybe a good thing should be to replace the syntax XXXY with XXX$Y (but XXXY is used in several places into Koha it seems to me).
Marked as Pased QA.

switch the severity to normal.
Comment 16 Jonathan Druart 2013-11-21 15:52:15 UTC
Created attachment 23067 [details] [review]
Bug 10996 Allow numeric subfields to be stripped on export

Regex assumed all numeric characters were part of the tag number
This is obviously false as $9 would be a likely candidate
to be removed on export. Constrain the tag by length

The code can be any printing ascii character other than space
according to LoC's website

Also changed regexp to no longer allow a zero length tag number
which is nonsensical.
The old regex would accept shorter than 3 digit tags but these were
not stripped so I've removed that option considering it a bug
not a feature

NB assumption that the code makes that a tag is always numeric is
incorrect but works in practice. Handling non-numeric tags is
a 'To be done'

Made code dependent on the regex succeeding. Picking up results
from a previous regex on failure can lead to weird hard to
identify bugs

In the course of testing Paola Rossi <paola.rossi@cineca.it>
spotted that the delete operation was flawed, only removing
the first occurence of the specified tag/tag subfield. Reworked
the delete loop to operate on all occurences

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 17 Galen Charlton 2013-11-21 17:57:00 UTC
Pushed to master.  Thanks, Colin!
Comment 18 Fridolin SOMERS 2013-12-13 08:02:38 UTC
This patch has been pushed to 3.14.x, will be in 3.14.1
Comment 19 Tomás Cohen Arazi 2013-12-16 13:38:57 UTC
This patch has been pushed to 3.12.x, will be in 3.12.8.

Thanks Colin!
Comment 20 Jonathan Druart 2015-01-29 15:19:15 UTC
This patch introduced a bug, see bug 12860 for a fix.