Summary: | Cannot exclude numeric subfields on export | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | Tools | Assignee: | Colin Campbell <colin.campbell> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | fridolin.somers, gitbot, gmcharlt, jonathan.druart, paola.rossi, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 12860 | ||
Attachments: |
Proposed Patch
Cannot exclude numeric subfields on export Bug 10996 Allow numeric subfields to be stripped on export Amended patch Amended patch Cannot exclude numeric subfields on export Bug 10996 Allow numeric subfields to be stripped on export |
Description
Colin Campbell
2013-10-04 09:58:54 UTC
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. |