Bug 21861 - The MARC modification template actions editor does not always validate user input
Summary: The MARC modification template actions editor does not always validate user i...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Andreas Roussos
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 21867
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-19 11:36 UTC by Andreas Roussos
Modified: 2020-01-06 20:13 UTC (History)
6 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 21861: The MARC modification template actions editor does not always validate user input (5.79 KB, patch)
2018-11-19 12:31 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 21861: The MARC modification template actions editor does not always validate user input (5.79 KB, patch)
2018-11-20 06:34 UTC, paxed
Details | Diff | Splinter Review
Bug 21861: (follow-up) replace onchange() with jQuery equivalent (1.10 KB, patch)
2018-11-22 14:04 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 21861: The MARC modification template actions editor does not always validate user input (5.87 KB, patch)
2018-11-26 13:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21861: (follow-up) replace onchange() with jQuery equivalent (1.19 KB, patch)
2018-11-26 13:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Roussos 2018-11-19 11:36:07 UTC
When editing a MARC modification template action, you have the option
of adding a conditional clause ("if/unless") to the action you want to
perform. The conditional clause can be further fine-tuned by selecting
one of four comparison operators ("exists/doesn't exist/matches/doesn't
match") from a drop-down list.

There are currently four issues when adding such a clause, namely:

1) When you pick a condition (e.g. "unless") and click on 'Add action'
   or 'Update action', the jQuery function that validates your input
   does not check whether you have actually filled in a value for the
   MARC field (mandatory) or the comparison operator (mandatory).

2) If you pick "matches" or "doesn't match" as a comparison operator,
   the textbox next to the operator can be left blank when saving the
   modification action (the textbox _should_ have a value because the
   operator is meaningless without it).

3) If you check the "RegEx" checkbox and then enter a value in the
   relevant textbox, when you Add/Update the action the "Action"
   column in the actions table will display extra spaces between the
   regex pattern and its delimiters.

4) If you check the "RegEx" checkbox and then enter a value in the
   relevant textbox, when you later edit that modification action
   the "m/ /" characters surrounding the regex textbox are not shown
   anymore.
Comment 1 Andreas Roussos 2018-11-19 12:31:11 UTC
Created attachment 82482 [details] [review]
Bug 21861: The MARC modification template actions editor does not always validate user input

The MARC modification template actions editor does not validate
user input when adding a conditional clause, making it possible
to save a modification action without filling in all necessary
fields.

This patch fixes that.

Test plan:
1) Create a MARC modification template and add a new action,
   e.g. "Copy All field(s) 200$a to field 600$a".
   Add a condition ("if" or "unless") but leave the relevant
   field and subfield textboxes empty.
   Add "matches" or "doesn't match" as a comparison operator.
   Check the "RegEx" checkbox, leave the regex textbox empty.
2) Click on "Add action" and notice how it accepts your input.
   Also notice the extra space in the Action column RegEx.
3) Edit your modification action and notice the missing "m/"
   before the conditional RegEx.

4) Apply the patch and hit CTRL-F5 to reload the relevant JS.

5) Edit the modification action you added previously (or add
   a new one). Try leaving the textboxes mentioned above empty
   and then submit your changes. You should get pop-up windows
   informing you about missing fields.
6) Now try typing a value in the conditional regex textbox and
   Update your action. In the Action column, there should be
   no spaces between the delimiters of the matching operator
   and the value you just entered.
7) Edit your action: notice how the "m/" before the regular
   expression and the "/" after it are now showing correctly.
Comment 2 paxed 2018-11-20 06:34:46 UTC
Created attachment 82528 [details] [review]
Bug 21861: The MARC modification template actions editor does not always validate user input

The MARC modification template actions editor does not validate
user input when adding a conditional clause, making it possible
to save a modification action without filling in all necessary
fields.

This patch fixes that.

Test plan:
1) Create a MARC modification template and add a new action,
   e.g. "Copy All field(s) 200$a to field 600$a".
   Add a condition ("if" or "unless") but leave the relevant
   field and subfield textboxes empty.
   Add "matches" or "doesn't match" as a comparison operator.
   Check the "RegEx" checkbox, leave the regex textbox empty.
2) Click on "Add action" and notice how it accepts your input.
   Also notice the extra space in the Action column RegEx.
3) Edit your modification action and notice the missing "m/"
   before the conditional RegEx.

4) Apply the patch and hit CTRL-F5 to reload the relevant JS.

5) Edit the modification action you added previously (or add
   a new one). Try leaving the textboxes mentioned above empty
   and then submit your changes. You should get pop-up windows
   informing you about missing fields.
6) Now try typing a value in the conditional regex textbox and
   Update your action. In the Action column, there should be
   no spaces between the delimiters of the matching operator
   and the value you just entered.
7) Edit your action: notice how the "m/" before the regular
   expression and the "/" after it are now showing correctly.
Comment 3 Marcel de Rooy 2018-11-21 15:30:07 UTC
+    document.getElementById('conditional_regex').onchange();

Please correct.
Comment 4 Marcel de Rooy 2018-11-21 15:39:24 UTC
(In reply to Marcel de Rooy from comment #3)
> +    document.getElementById('conditional_regex').onchange();
> 
> Please correct.

Probably needs a dep tree when rebasing..
Comment 5 Andreas Roussos 2018-11-22 14:04:19 UTC
Created attachment 82581 [details] [review]
Bug 21861: (follow-up) replace onchange() with jQuery equivalent
Comment 6 Marcel de Rooy 2018-11-26 13:21:56 UTC
Created attachment 82641 [details] [review]
Bug 21861: The MARC modification template actions editor does not always validate user input

The MARC modification template actions editor does not validate
user input when adding a conditional clause, making it possible
to save a modification action without filling in all necessary
fields.

This patch fixes that.

Test plan:
1) Create a MARC modification template and add a new action,
   e.g. "Copy All field(s) 200$a to field 600$a".
   Add a condition ("if" or "unless") but leave the relevant
   field and subfield textboxes empty.
   Add "matches" or "doesn't match" as a comparison operator.
   Check the "RegEx" checkbox, leave the regex textbox empty.
2) Click on "Add action" and notice how it accepts your input.
   Also notice the extra space in the Action column RegEx.
3) Edit your modification action and notice the missing "m/"
   before the conditional RegEx.

4) Apply the patch and hit CTRL-F5 to reload the relevant JS.

5) Edit the modification action you added previously (or add
   a new one). Try leaving the textboxes mentioned above empty
   and then submit your changes. You should get pop-up windows
   informing you about missing fields.
6) Now try typing a value in the conditional regex textbox and
   Update your action. In the Action column, there should be
   no spaces between the delimiters of the matching operator
   and the value you just entered.
7) Edit your action: notice how the "m/" before the regular
   expression and the "/" after it are now showing correctly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2018-11-26 13:22:35 UTC
Created attachment 82642 [details] [review]
Bug 21861: (follow-up) replace onchange() with jQuery equivalent

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-11-26 13:22:57 UTC
String changes, heading for 19.05 ;)
Comment 9 Marcel de Rooy 2018-11-26 13:23:40 UTC
(In reply to paxed from comment #2)
You forgot a signoff line..
Comment 10 Marcel de Rooy 2018-11-29 11:27:23 UTC
Moving to PQA now since paxed also tested (see above)
Comment 11 Nick Clemens 2018-11-29 19:16:01 UTC
Awesome work everyone!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2018-12-10 09:48:02 UTC
Pushed to 18.11.x for 18.11.01
Comment 13 Lucas Gass 2018-12-14 17:53:13 UTC
backported to 18.05 for 18.05.07
Comment 14 Fridolin Somers 2018-12-19 09:59:23 UTC
Pushed to 17.11.x for 17.11.13