Bug 8218 - Add a script to sanitize entity where "&" character was wrongly replaced with "&"
Summary: Add a script to sanitize entity where "&" character was wrongly replaced with...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-09 13:36 UTC by Sophie MEYNIEUX
Modified: 2015-06-04 23:33 UTC (History)
9 users (show)

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


Attachments
Bug 8218 : Adding SanitizeEntity and entity_clean sub which replace & by & in a record (6.22 KB, patch)
2012-06-09 15:05 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 8218 : Adding SanitizeEntity and entity_clean sub which replace & by & in a record (6.28 KB, patch)
2012-07-01 03:28 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8218 : Fixing copyright headers and license and description (1.52 KB, patch)
2012-07-01 03:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8218: Followup: Perltidy and QA FIX (5.09 KB, patch)
2012-08-22 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
this patch is finalised (7.87 KB, patch)
2012-09-28 14:18 UTC, Christophe Croullebois
Details | Diff | Splinter Review
patch finalized (7.86 KB, patch)
2012-11-26 13:05 UTC, Christophe Croullebois
Details | Diff | Splinter Review
proposed patch (8.09 KB, patch)
2013-03-02 13:33 UTC, Christophe Croullebois
Details | Diff | Splinter Review
proposed patch (8.78 KB, patch)
2013-03-14 11:18 UTC, Christophe Croullebois
Details | Diff | Splinter Review
proposed patch (8.79 KB, patch)
2013-03-22 08:47 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 8218 : Add a maintenance script to sanitize biblio records (12.28 KB, patch)
2014-04-02 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8218 : Add a maintenance script to sanitize biblio records (12.57 KB, patch)
2014-04-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8218: Follow-up for some small typos (2.09 KB, patch)
2014-07-17 12:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8218 : Add a maintenance script to sanitize biblio records (12.51 KB, patch)
2014-07-17 12:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8218: Follow-up for some small typos (2.05 KB, patch)
2014-07-17 12:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8218 : Add a maintenance script to sanitize biblio records (12.62 KB, patch)
2014-07-17 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8218: Follow-up for some small typos (2.15 KB, patch)
2014-07-17 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8218: Follow-up for some small typos (2.17 KB, patch)
2014-07-17 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 8218 : Add a maintenance script to sanitize biblio records (12.64 KB, patch)
2014-07-25 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8218: Follow-up for some small typos (2.24 KB, patch)
2014-07-25 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8218: qa followup (4.42 KB, patch)
2014-11-05 15:10 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 Sophie MEYNIEUX 2012-06-09 13:36:21 UTC
In some import dealing with authorities like "vendangeur", some bug introduced some misleading "&" character in title or comment.

This script remove them.
Comment 1 Sophie MEYNIEUX 2012-06-09 15:05:13 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-07-01 03:28:47 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-07-01 03:31:37 UTC
Follow up is textual changes on, so setting status to signed off
Comment 4 Chris Cormack 2012-07-01 03:31:43 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-07-18 10:48:16 UTC
QA Comments:

1/ Please use 
   use strict;
   use warnings;
OR
   use Modern::Perl;

2/ Perltidied for misc/maintenance/batchSanitizeEntity.pl

3/ Bad encoding (N°)

Technically:
4/ $count += 1; => $count++;
5/
  +# We remove spaces
  +my @biblio;
  +foreach my $bib(@biblios) {
  +    $bib =~ s/(^\s*|\s*$)//g;
  +    next if $bib eq "";
  +    push @biblio, $bib;
  +}
  +@biblios = @biblio;

The second tmp array is useless:

   # We remove spaces
   s/(^\s*|\s*$)//g for @biblios;
   # Remove empty lines
   @biblios = grep {!/^$/} @biblios;

6/ if ($biblios =~ /\//)
is more readable as:
  if ( $biblio =~ m|/| )
for example

7/ +    while (my $bib = $sth-> fetchrow_array()){
There is a useless space ;-)

Marking as Failed QA for 1 and 2
Comment 6 Jonathan Druart 2012-08-22 13:05:55 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-08-22 13:06:53 UTC
Please check there is no regression with the last patch.
Comment 8 Christophe Croullebois 2012-09-28 14:18:02 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-10-05 14:01:08 UTC
Applying: Bug 8218 : A script for maintenance to clean any &
/home/koha/kohaclone/.git/rebase-apply/patch:145: trailing whitespace.
    
fatal: sha1 information is lacking or useless (C4/Charset.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 10 Christophe Croullebois 2012-11-26 13:05:02 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-12-07 13:55:48 UTC
I'm not sure if I am testing this right. Can you please provide a test plan?

What I did:
1) Picked a record, added "Includes index. amp;amp;" in a 505a note.
2) Ran script with --run --where search
3) Script says it has fixed 1 biblio
4) The field still looks the same to me in the marc editor.

Also:
* In the help '-where' should be '--where'

* Shouldn't the line
    '--run'   => \$run,
be
    'run' => \$run,
?
Comment 12 Mirko Tietgen 2013-02-16 11:59:47 UTC
Is there a reason why this is labelled »enhancement«? Isn't it a bug fix?
Comment 13 Bernardo Gonzalez Kriegel 2013-02-19 00:28:42 UTC
The script works as described, at last changes & to &

But search as explained by help do not works, or description is incorrect
--batch do not prevent script to ask confirmation
--cron reindexing fails, rebuild_zebra complains about
wrong arguments, I think for newline in system() argument.

Also, koha-qa complains:
pod errors in C4/Charset.pm
forbidden pattern (====) in misc/maintenance/batchSanitizeEntity.pl
Comment 14 Christophe Croullebois 2013-03-02 13:33:42 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2013-03-02 15:56:22 UTC
Hi Christophe,
now it works well :)

A few things more (sorry):
1) in C4/Charset.pm, the line with SanitizeEntity has a tab, replace it with
spaces.

2) in C4/Charset.pm are pod errors, you could write this as 
=head2 SanitizeEntity

  SanitizeEntity($marcrecord);

Sanitize Entity

=cut

BTW, description is a little short :)

3) in C4/Charset.pm, you put this function at the bottom. As an exported function I think it must be moved up, below nsb_clean.

4) Important: make your script executable, only run if you do "perl misc/maintenance/batchSanitizeEntity.pl"

5) You can change '===' with '---' so koha-qa do not complain. 
Not important really (rebuild_zebra has lots)

6) The confirmation question could be 'Proceed? [yes/no]'

I think that 1-4 could make this fail QA, 5-6 are suggestions.
Comment 16 Christophe Croullebois 2013-03-14 11:18:04 UTC Comment hidden (obsolete)
Comment 17 Koha Team AMU 2013-03-22 07:59:54 UTC
Tested on a local instance.
Frido began to write it again (no change when amp;amp).
To be continued...
Comment 18 Christophe Croullebois 2013-03-22 08:47:55 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2013-04-30 22:57:35 UTC
Hi,
it works with search, but...

1) --batch option continues to ask confirmation

2) koha-qa complains. You are using Modern::Perl,
so you must change the way to open a file
change this
 open( FILE, "$biblios" ) || die("Can't open $biblios");
to this
 open my $FILE,"<","$biblios" or die("Can't open $biblios");
and the closing/reading accordingly.

3) You are checking for a filename assuming a '/' in filename (absolute)
if ( $biblios =~ m|/| )
change to 
if ( -e $filename )
to allow any file name,
then, if not exists, check if the resulting string is composed by numbers.

4) You are reading the file with
@bibliofile = <FILE>;
that reads 1 line only

5) --where filename do not work
I put 3 biblionumber on a file, run command and the result 
was weird, prints that was about to check 100 bibs, and enter
endless loop

Not ready yet.
Comment 20 Jonathan Druart 2014-04-01 14:02:25 UTC
I am going to rewrite this script according coding guidelines.
Comment 21 Jonathan Druart 2014-04-02 10:25:21 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-04-02 10:29:46 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-07-17 12:06:19 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2014-07-17 12:09:35 UTC
I would like to suggest an adjustment for this report:
The confirm flag is not doing what I would expect it to do. I would expect that after running the sanitize check on the record, confirm would determine if the record gets saved. But looking at the code, I see that you just skip the whole record when confirm is off. Note that the print messages are not that clear too. You say "will be processed" but skip the record. After that you report 0 records to clean. But I knew that I had at least one record..
Comment 25 Jonathan Druart 2014-07-17 12:35:26 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-07-17 12:35:31 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2014-07-17 13:05:15 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2014-07-17 13:05:21 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2014-07-17 13:17:27 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2014-07-17 13:18:01 UTC
Thank you Marcel for your follow-up and signoff!
Comment 31 Kyle M Hall 2014-07-25 14:36:13 UTC
Created attachment 30120 [details] [review]
[PASSED QA] Bug 8218 : Add a maintenance script to sanitize biblio records

This patch adds:
- a new maintenance script batch_sanitize_records
- a new subroutine C4::Charset::SanitizeRecord
- new unit tests for the new subroutine

Test plan:
1/ prove t/db_dependent/Charset.t
2/ Create a record containing "&amp;amp;" (could be follow with as many
'amp;' as you want) in one of its fields and the same for the field
linked to biblioitems.url.
The url should not be sanitized, it may contain "&amp;".
3/ Launch the maintenance script with the -h parameter to see how to use
it.
4/ Launch the script using the different parameters:
 --filename=FILENAME
 --biblionumbers='XXX'
 --auto-search

The auto-search permits to sanitize all records containing "&amp;amp;" in
the marcxml field.

Use the verbose flag for testing.
Without the --confirm flag, nothing is done.

5/ Use the --confirm flag and verify in the biblioitems.marcxml field
that the record has been sanitized.

6/ Try the --reindex flag to reindex records which have been modified.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2014-07-25 14:36:28 UTC
Created attachment 30121 [details] [review]
[PASSED QA] Bug 8218: Follow-up for some small typos

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Tomás Cohen Arazi 2014-10-31 18:37:23 UTC
I'm willing to push the feature, because I like it, and it doesn't break people's funcionality, and hence marked as rel_3_18_candidate. This means you are not catch by feature freeze.

Can you please work a bit on the fact that the feature is about ampersands only?
I propose

- keeping the SanitizeRecord name general.
- renaming _entity_clean as _clean_ampersand
- renaming the script to sanitize_records.pl
- add a --fix-ampersand switch (the only one FOR NOW, enabled by default) so it is obvious what the script does.
- make POD and usage reflect this changes.

I mark it FAILED-QA until the authors answer this proposal.
Comment 34 Jonathan Druart 2014-11-05 15:10:32 UTC
Created attachment 33248 [details] [review]
Bug 8218: qa followup

This patch
- rename _entity_clean as _clean_ampersand
- rename the script to sanitize_records.pl
- add a --fix-ampersand switch (the only one FOR NOW, enabled by
  default) so it is obvious what the script does.
- make POD and usage reflect this changes.
Comment 35 Jonathan Druart 2014-11-05 15:10:54 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> I'm willing to push the feature, because I like it, and it doesn't break
> people's funcionality, and hence marked as rel_3_18_candidate. This means
> you are not catch by feature freeze.
> 
> Can you please work a bit on the fact that the feature is about ampersands
> only?
> I propose
> 
> - keeping the SanitizeRecord name general.
> - renaming _entity_clean as _clean_ampersand
> - renaming the script to sanitize_records.pl
> - add a --fix-ampersand switch (the only one FOR NOW, enabled by default) so
> it is obvious what the script does.
> - make POD and usage reflect this changes.
> 
> I mark it FAILED-QA until the authors answer this proposal.

I agree, all done in the last patch, back to passed QA.
Comment 36 Tomás Cohen Arazi 2014-11-11 18:45:07 UTC
Feature pushed to master.

Thanks Jonathan!