In some import dealing with authorities like "vendangeur", some bug introduced some misleading "&" character in title or comment. This script remove them.
Created attachment 10039 [details] [review] Bug 8218 : Adding SanitizeEntity and entity_clean sub which replace & by & in a record Bug 8218 : adding script for maintenance to clean any & in strings
Created attachment 10588 [details] [review] Bug 8218 : Adding SanitizeEntity and entity_clean sub which replace & by & in a record Bug 8218 : adding script for maintenance to clean any & in strings Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Follow up is textual changes on, so setting status to signed off
Created attachment 10589 [details] [review] Bug 8218 : Fixing copyright headers and license and description
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
Created attachment 11744 [details] [review] Bug 8218: Followup: Perltidy and QA FIX
Please check there is no regression with the last patch.
Created attachment 12591 [details] [review] this patch is finalised Two subs written by Alex Arnaud in C4/Charset.pm, SanitizeEntity and entity_clean I had created two scripts for Biblibre, one to be used in cron, it reindexes all transformed records. Sophie Meynieux brought together the two scripts, she has created a batch mode, usefull but without the reindexing So I have rewrited it and I have integrated "Fixing copyright headers and license and description" written by Chris Cormack and "Followup: Perltidy and QA FIX" written by Jonathan Druart Thx to all
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.
Created attachment 13678 [details] [review] patch finalized ok rebased. thx
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, ?
Is there a reason why this is labelled »enhancement«? Isn't it a bug fix?
The script works as described, at last changes &amp; 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
Created attachment 15826 [details] [review] proposed patch Hi Bernardo, thx for the test. I corrected the 'search' option, --where "search" is now ok, I added also a message. I tried to be more explicit in help. --batch is corrected now --cron is ok now, you were right about newline ;o) About the qa message... hum what do you suggest ? I use (====) to make the outpout more visual ... not very important for me I hope I haven't forget something. thx
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.
Created attachment 16122 [details] [review] proposed patch Hi Bernardo, thx for your help. Now it must be ok.
Tested on a local instance. Frido began to write it again (no change when amp;amp). To be continued...
Created attachment 16720 [details] [review] proposed patch
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.
I am going to rewrite this script according coding guidelines.
Created attachment 26754 [details] [review] 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;" (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 cans contain "&". 3/ Launch the maintenance script with the -h paramters 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;" 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.
Created attachment 26755 [details] [review] 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;" (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 cans contain "&". 3/ Launch the maintenance script with the -h paramters 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;" 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.
Created attachment 29792 [details] [review] Bug 8218: Follow-up for some small typos
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..
Created attachment 29793 [details] [review] 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;" (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 cans contain "&". 3/ Launch the maintenance script with the -h paramters 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;" 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.
Created attachment 29794 [details] [review] Bug 8218: Follow-up for some small typos
Created attachment 29795 [details] [review] 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;" (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 "&". 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;" 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>
Created attachment 29796 [details] [review] Bug 8218: Follow-up for some small typos Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 29798 [details] [review] 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>
Thank you Marcel for your follow-up and signoff!
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;" (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 "&". 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;" 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>
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>
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.
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.
(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.
Feature pushed to master. Thanks Jonathan!