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!!
Created attachment 21806 [details] [review] Proposed Patch To test export a record containing a numeric subfield (e.g. 100 $9 ) with exclusion specified thus 1009. Without patch the $9 is in the export. With patch it should be stripped
Patch applied cleanly, go forth and signoff
Created attachment 21934 [details] [review] Cannot exclude numeric subfields on export
I sign off the patch
Created attachment 21974 [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 Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Passes koha-qa.pl, works as advertised.
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.
Created attachment 22009 [details] [review] Amended patch Changed test to be definedness only so it meets the case Galen spotted
(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.
Reverting status
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.
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
Created attachment 22586 [details] [review] Amended patch Expanded the patch to ensure that all occurences of the tag are processed not just the first as previously was the case
Reset status to needs signoff. Would be grateful if folks could retest and confirm that deletes are being applied to all occurences
Created attachment 22596 [details] [review] Cannot exclude numeric subfields on export The deletes are being applied to all occurences. I sign off the patch.
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.
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>
Pushed to master. Thanks, Colin!
This patch has been pushed to 3.14.x, will be in 3.14.1
This patch has been pushed to 3.12.x, will be in 3.12.8. Thanks Colin!
This patch introduced a bug, see bug 12860 for a fix.