Description
Alex Arnaud
2018-12-06 08:54:14 UTC
Created attachment 82902 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. When I use regex on 2 items that have the same value on a field and their field changes, it's said that 2 fields have been modified. Ex : 1) It's a note. 2) It's a note. s/note/note2/g However, if it's not the same value, it's said that 3 fields have been modified. Ex : 1) It's a note. 2) This is a note. s/note/note2/g Can you please check why it happens? Created attachment 83392 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. (In reply to Pierre-Marc Thibault from comment #2) > When I use regex on 2 items that have the same value on a field and their > field changes, it's said that 2 fields have been modified. > > Ex : 1) It's a note. 2) It's a note. > s/note/note2/g > > However, if it's not the same value, it's said that 3 fields have been > modified. > > Ex : 1) It's a note. 2) This is a note. > s/note/note2/g > > Can you please check why it happens? Please try the replacement patch. The code may be quite tricky but it should work. Created attachment 83470 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com> Some things: 1) Tiny typo in "subflied": $(this).parent().find('.hint').html(_("Enter a regular expression for this subflied")); 2) I am not happy witht he GUI. The delete checkbox always takes some time explaining and I think having now 2 unlabelled checkboxes makes it worse. Maybe a link or small button "RegEx" would work? Adding Owen to see if he has an idea. 3) During our discussion on bug 15836 we came to the conclusion that a separate permission should be added for that page as there are some potential issues with the regex. In our libraries a lot of people have item batch edit permissions, as this is a super useful tool. In wonder if we need to take extra precautions here. Adding some people in hope for a second opinion. I should also say: I've tested it a bit and it all worked ok so far. I tried to test this but I keep getting an error: "Can't use string ("0") as an ARRAY ref while "strict refs" in use at /home/vagrant/kohaclone/tools/batchMod.pl line 279, <$fh> line 100." (In reply to Owen Leonard from comment #8) > I tried to test this but I keep getting an error Never mind, that error is unrelated. (In reply to Katrin Fischer from comment #6) > Some things: > > 1) Tiny typo in "subflied": > > $(this).parent().find('.hint').html(_("Enter a regular expression for this > subflied")); > > 2) I am not happy witht he GUI. The delete checkbox always takes some time > explaining and I think having now 2 unlabelled checkboxes makes it worse. > Maybe a link or small button "RegEx" would work? Adding Owen to see if he > has an idea. Definitely, we should not have a second checkbox > 3) During our discussion on bug 15836 we came to the conclusion that a > separate permission should be added for that page as there are some > potential issues with the regex. In our libraries a lot of people have item > batch edit permissions, as this is a super useful tool. In wonder if we need > to take extra precautions here. Adding some people in hope for a second > opinion. Nope, not in this case, see how MMT works. However we should list the available modifiers, see Koha/SimpleMARC.pm:627 my @available_modifiers = qw( i g ); 4) ident must be 4 spaces 5) why do you use eval for the regex? Same as before, I feel like you should mimick Koha::SimpleMARC::_modify_values Created attachment 87323 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com> Created attachment 87324 [details] [review] Bug 21959: use a link instead of a checkbox Created attachment 87325 [details] [review] Bug 21959: allow only i and g regex modifiers (In reply to Jonathan Druart from comment #10) > > 5) why do you use eval for the regex? Same as before, I feel like you should > mimick Koha::SimpleMARC::_modify_values This is the only way i've found to make the regex work with modifiers in a variable. But i don't need it anymore since i've limited modifiers to i and g like in Koha::SimpleMARC::_modify_values I re-used a part of the code in Koha::SimpleMARC::_modify_values. And i don't like code duplicating so much. So i'm thinking about a code refactoring: - directly use _modify_values (and take into account specificities), - create a common apply_regex() sub with unit tests. But where ? In Koha/Util/* ? and use it in _modify_values - other ? Any thougths ? I can definitely not answer Alex's question, but only had an opinion : that's a great enhancement ! Unfortunately, I’m really bad at regular expressions (but this feature is to good motivation to learn !), do you have some examples or reference pages to test further ? (In reply to Séverine Queune from comment #15) > Unfortunately, I’m really bad at regular expressions (but this feature is to > good motivation to learn !), do you have some examples or reference pages to > test further ? Thanks for your interest in this :) You can try the following regex «s/foo/bar/g» In the content of the subfield, it will replace every occurrences of «foo» by «bar» Hi guys, is it this patch ready to test or we should change statu to "In duscussion"? Thank you Patch tested with a sandbox, by Mathilde Formery <mathilde.formery@ville-nimes.fr> Created attachment 92870 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com> Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Created attachment 92871 [details] [review] Bug 21959: use a link instead of a checkbox Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Created attachment 92872 [details] [review] Bug 21959: allow only i and g regex modifiers Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Can you please rebase? Should be easy, but don't want to risk it: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 21959: Batch item modification - ability to apply regular expression on text fields Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-edit.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 21959: Batch item modification - ability to apply regular expression on text fields The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-21959-Batch-item-modification---ability-to-app-HQtELZ.patch Created attachment 95432 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com> Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Created attachment 95433 [details] [review] Bug 21959: use a link instead of a checkbox Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Created attachment 95434 [details] [review] Bug 21959: allow only i and g regex modifiers Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Patch rebased Created attachment 96636 [details] [review] Bug 21959: Batch item modification - ability to apply regular expression on text fields Test plan: - go to Tools > Batch item modification, - select items to modify, - for one or some text fields, check the second checkbox (Use regular expression) and type your regex, - check the regex has been applied. Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com> Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 96637 [details] [review] Bug 21959: use a link instead of a checkbox Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 96638 [details] [review] Bug 21959: allow only i and g regex modifiers Signed-off-by: Mathilde Formery <mathilde.formery@ville-nimes.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Additional info: It's sponsored by the City of Nîmes. Nice work everyone! Pushed to master for 20.05 Nice work everyone! Pushed to master for 20.05 Enhancement not backported to 19.11.x branch. |