Bug 21819 - Marc modification templates action always checks Regexp checkbox
Summary: Marc modification templates action always checks Regexp checkbox
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Andreas Roussos
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 21867
  Show dependency treegraph
 
Reported: 2018-11-13 12:23 UTC by paxed
Modified: 2020-01-06 20:14 UTC (History)
5 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:
Circulation function:


Attachments
Bug 21819: Marc modification templates action always checks Regexp checkbox (2.82 KB, patch)
2018-11-17 12:44 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 21819: Marc modification templates action always checks Regexp checkbox (2.88 KB, patch)
2018-11-19 06:14 UTC, paxed
Details | Diff | Splinter Review
Bug 21819: Marc modification templates action always checks Regexp checkbox (2.97 KB, patch)
2018-11-20 15:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21819: (QA follow-up) Replace onchange() in touched code (2.10 KB, patch)
2018-11-20 15:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21819: (QA follow-up) Replace onchange() in touched code (2.13 KB, patch)
2018-11-20 16:35 UTC, Andreas Roussos
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2018-11-13 12:23:49 UTC
When you add an action to a Marc modification template, there's a RegEx checkbox for replacing data in the field. This checkbox is automatically checked if you edit the action, even when you did not check it when saving the action.

To reproduce:
1) Go to Tools > Marc modification templates, and create a new template
2) Add a new action to the template
3) Edit the action so it's "Move" field to another field.
   Do not check the RegEx checkbox after the "To" field.
4) Save the action.
5) Edit the same action again. The checkbox is now checked.
Comment 1 Andreas Roussos 2018-11-17 12:44:02 UTC
Created attachment 82424 [details] [review]
Bug 21819: Marc modification templates action always checks Regexp checkbox

When editing a MARC modification template action, the 'RegEx'
checkbox to the right of the 'to field' textboxes will always
appear checked, even if no regular expression has been added.

This patch fixes that.

Test plan (steps 1-5 are copied from the original report -- thanks Pasi!):
1) Go to Tools > Marc modification templates, and create a new template
2) Add a new action to the template
3) Edit the action so it's "Move" field to another field.
   Do not check the RegEx checkbox after the "To" field.
4) Save the action.
5) Edit the same action again. The checkbox is now checked.
6) Apply the patch and hit Ctrl-F5 to do a hard refresh in your browser.
7) Edit the action you saved in 4): this time the 'RegEx' checkbox
   should be unchecked.
8) For completeness, add a regular expression to the "Move" action
   created in 3) and click on 'Update action'. When you re-edit this
   action the 'RegEx' checkbox should be checked.
Comment 2 paxed 2018-11-19 06:14:07 UTC
Created attachment 82427 [details] [review]
Bug 21819: Marc modification templates action always checks Regexp checkbox

When editing a MARC modification template action, the 'RegEx'
checkbox to the right of the 'to field' textboxes will always
appear checked, even if no regular expression has been added.

This patch fixes that.

Test plan (steps 1-5 are copied from the original report -- thanks Pasi!):
1) Go to Tools > Marc modification templates, and create a new template
2) Add a new action to the template
3) Edit the action so it's "Move" field to another field.
   Do not check the RegEx checkbox after the "To" field.
4) Save the action.
5) Edit the same action again. The checkbox is now checked.
6) Apply the patch and hit Ctrl-F5 to do a hard refresh in your browser.
7) Edit the action you saved in 4): this time the 'RegEx' checkbox
   should be unchecked.
8) For completeness, add a regular expression to the "Move" action
   created in 3) and click on 'Update action'. When you re-edit this
   action the 'RegEx' checkbox should be checked.

Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi>
Comment 3 Marcel de Rooy 2018-11-20 15:41:41 UTC
Created attachment 82551 [details] [review]
Bug 21819: Marc modification templates action always checks Regexp checkbox

When editing a MARC modification template action, the 'RegEx'
checkbox to the right of the 'to field' textboxes will always
appear checked, even if no regular expression has been added.

This patch fixes that.

Test plan (steps 1-5 are copied from the original report -- thanks Pasi!):
1) Go to Tools > Marc modification templates, and create a new template
2) Add a new action to the template
3) Edit the action so it's "Move" field to another field.
   Do not check the RegEx checkbox after the "To" field.
4) Save the action.
5) Edit the same action again. The checkbox is now checked.
6) Apply the patch and hit Ctrl-F5 to do a hard refresh in your browser.
7) Edit the action you saved in 4): this time the 'RegEx' checkbox
   should be unchecked.
8) For completeness, add a regular expression to the "Move" action
   created in 3) and click on 'Update action'. When you re-edit this
   action the 'RegEx' checkbox should be checked.

Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2018-11-20 15:41:45 UTC
Created attachment 82552 [details] [review]
Bug 21819: (QA follow-up) Replace onchange() in touched code

AFAIK we should not call document...onchange() and assume that it works.
Apparently, it does at least in some browsers. But we are not sure.
We could easily replace them here by the documented jQuery counterpart.

Note: If you try git grep -l -E "onchange\(" you will find some
occurrences in two libraries and only in one js file (this one).
We should replace all occurrences here; will open a new report for that.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Verified that the change event was triggered: span should expand/collapse.
Comment 5 Andreas Roussos 2018-11-20 16:35:13 UTC
Created attachment 82555 [details] [review]
Bug 21819: (QA follow-up) Replace onchange() in touched code

AFAIK we should not call document...onchange() and assume that it works.
Apparently, it does at least in some browsers. But we are not sure.
We could easily replace them here by the documented jQuery counterpart.

Note: If you try git grep -l -E "onchange\(" you will find some
occurrences in two libraries and only in one js file (this one).
We should replace all occurrences here; will open a new report for that.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Verified that the change event was triggered: span should expand/collapse.

Signed-off-by: Andreas Roussos <arouss1980@gmail.com>
Comment 6 Nick Clemens (kidclamp) 2018-11-21 11:35:54 UTC
Awesome work all!

Pushed to master for 18.11
Comment 7 Jesse Maseto 2018-12-07 22:06:19 UTC
Backported to 18.05 for 18.05.07
Comment 8 Fridolin Somers 2018-12-19 09:50:11 UTC
Pushed to 17.11.x for 17.11.13