Bug 36880 - Record overlay rules are not validated on add or edit
Summary: Record overlay rules are not validated on add or edit
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Jan Kissig
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 37209
  Show dependency treegraph
 
Reported: 2024-05-16 08:07 UTC by Jan Kissig
Modified: 2024-07-22 11:22 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the record overlay rules page so that a tag is now required when adding and editing a rule.
Version(s) released in:
24.11.00,24.05.02
Circulation function:


Attachments
Bug 36880: Record overlay rules are not validated on add or edit (1.56 KB, patch)
2024-05-16 08:17 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 36880: Record overlay rules are not validated (9.26 KB, patch)
2024-05-26 18:36 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 36880: (follow-up) Translation of confirm was missing (1003 bytes, patch)
2024-05-27 08:02 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 36880: Record overlay rules are not validated (9.74 KB, patch)
2024-05-28 08:28 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 36880: Record overlay rules are not validated (9.79 KB, patch)
2024-05-29 06:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 36880: Record overlay rules are not validated (9.84 KB, patch)
2024-06-25 17:23 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kissig 2024-05-16 08:07:38 UTC
The validation of record overlay rules does not work when adding a new rule or editing an existing rule.

To reproduce:
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

Expected behavior: the input 'tag' should be marked as required
Comment 1 Jan Kissig 2024-05-16 08:17:24 UTC
Created attachment 166808 [details] [review]
Bug 36880: Record overlay rules are not validated on add or edit

This patch fixes the validation of record overlay rules on operation add or edit.

Testplan:
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

apply patch:
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) the input for Tag gets marked as required
Comment 2 David Nind 2024-05-19 13:23:36 UTC
Failing QA, because if you edit an existing rule, you can remove the value in the tag field and still save (when you click on save, the tag input field is briefly red, but it still saves without a value).

I also added Jan as the assignee.

Testing notes (using KTD):

1. Go to: Administration > Catalog > Record overlay rules
2. Before the patch, if you click '+Add rule' a single rule is added, even if no value is entered in the tag field. Clicking on '+Add rule' after this results in table refreshing, but no new rule added.
3. After the patch, the tag field is highlighted in a dark red and a rule isn't added until a value is entered.

I haven't noticed this pattern elsewhere in Koha, maybe there needs to be a separate bug for colour contrast as the black text on the dark red background for the input field could be hard to read for some.
Comment 3 Jan Kissig 2024-05-26 18:36:28 UTC
Created attachment 167182 [details] [review]
Bug 36880: Record overlay rules are not validated

This patch fixes validation of rules, confirmation on deletion of rules and removes some unused source code.

1) Validation of record overlay rules on edit and add action
2) Validation when editing an existing rule
3) Adds confirm when deleting multiple rules

Test plan:
1)
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

2)
 a) edit an existing rule
 b) empty input value for tag
 c) click Save and check that the rule has now an empty value for tag

3)
 a) delete a rule by checking the checkbox and clicking Delete selected
 b) delete a rule by clicking the Delete button under Actions
 c) notice that b) asks for confirmation

apply patch
1) redo steps and check that form does not get submitted and the input is marked as required

2) redo steps and check that clicking on Save will not submit the form and mark input as required

3) redo steps and check that a standard confirm popup appears
Comment 4 Jan Kissig 2024-05-27 08:02:43 UTC
Created attachment 167190 [details] [review]
Bug 36880: (follow-up) Translation of confirm was missing
Comment 5 David Nind 2024-05-27 21:17:27 UTC
Have changed to Failed QA:

1. After the patch, with no rules:
   . Without changing anything or adding a tag, I can click + Add rule, and a rule is added
   . After the first rule is added, clicking + Add rule again (without changing anything) just refreshes the page
   . If I then change one of the settings, such as changing skip to append in the appended column, a new rule is added without a tag being required
  . Can edit any existing rule, without needing to have anything in the tag field
Comment 6 Jan Kissig 2024-05-28 08:28:13 UTC
Created attachment 167207 [details] [review]
Bug 36880: Record overlay rules are not validated

This patch fixes validation of rules, confirmation on deletion of rules and removes some unused source code.

1) Validation of record overlay rules on edit and add action
2) Validation when editing an existing rule
3) Adds confirm when deleting multiple rules

Test plan:
1)
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

2)
 a) edit an existing rule
 b) empty input value for tag
 c) click Save and check that the rule has now an empty value for tag

3)
 a) delete a rule by checking the checkbox and clicking Delete selected
 b) delete a rule by clicking the Delete button under Actions
 c) notice that b) asks for confirmation

apply patch
1) redo steps and check that form does not get submitted and the input is marked as required

2) redo steps and check that clicking on Save will not submit the form and mark input as required

3) redo steps and check that a standard confirm popup appears
Comment 7 Jan Kissig 2024-05-28 08:35:46 UTC
(In reply to David Nind from comment #5)
> Have changed to Failed QA:
> 
> 1. After the patch, with no rules:
>    . Without changing anything or adding a tag, I can click + Add rule, and
> a rule is added
>    . After the first rule is added, clicking + Add rule again (without
> changing anything) just refreshes the page
>    . If I then change one of the settings, such as changing skip to append
> in the appended column, a new rule is added without a tag being required
>   . Can edit any existing rule, without needing to have anything in the tag
> field

Hi David,
sorry for the troubles. The reason it did not work was that some important changes were part of the obsoleted patch. On my local machine everything worked as expected.

I now merged (squashed) the 3 parts into one new patch and submitted it. It should work as expected now.
Comment 8 David Nind 2024-05-29 06:18:56 UTC
Thanks Jan!

Sign off on the way.

I'll leave it up to the QA to comment on the background colour of the required field and how to display it. In other areas, yellow is used to highlight required fields with no entry once the submit/action button is selected.

David
Comment 9 David Nind 2024-05-29 06:19:23 UTC
Created attachment 167234 [details] [review]
Bug 36880: Record overlay rules are not validated

This patch fixes validation of rules, confirmation on deletion of rules and removes some unused source code.

1) Validation of record overlay rules on edit and add action
2) Validation when editing an existing rule
3) Adds confirm when deleting multiple rules

Test plan:
1)
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

2)
 a) edit an existing rule
 b) empty input value for tag
 c) click Save and check that the rule has now an empty value for tag

3)
 a) delete a rule by checking the checkbox and clicking Delete selected
 b) delete a rule by clicking the Delete button under Actions
 c) notice that b) asks for confirmation

apply patch
1) redo steps and check that form does not get submitted and the input is marked as required

2) redo steps and check that clicking on Save will not submit the form and mark input as required

3) redo steps and check that a standard confirm popup appears

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Nick Clemens (kidclamp) 2024-06-25 17:23:03 UTC
Created attachment 168088 [details] [review]
Bug 36880: Record overlay rules are not validated

This patch fixes validation of rules, confirmation on deletion of rules and removes some unused source code.

1) Validation of record overlay rules on edit and add action
2) Validation when editing an existing rule
3) Adds confirm when deleting multiple rules

Test plan:
1)
 a) open http://localhost:8081/cgi-bin/koha/admin/marc-overlay-rules.pl
 b) just click + Add rule
 c) a new rule with an empty tag is saved

2)
 a) edit an existing rule
 b) empty input value for tag
 c) click Save and check that the rule has now an empty value for tag

3)
 a) delete a rule by checking the checkbox and clicking Delete selected
 b) delete a rule by clicking the Delete button under Actions
 c) notice that b) asks for confirmation

apply patch
1) redo steps and check that form does not get submitted and the input is marked as required

2) redo steps and check that clicking on Save will not submit the form and mark input as required

3) redo steps and check that a standard confirm popup appears

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens (kidclamp) 2024-06-25 17:24:27 UTC
This page should be updated to use validation as per coding guidelines, and the styling improved, however, this fixes the current validation and improves the deletion process so I think we should move this forward.
Comment 12 Katrin Fischer 2024-06-27 09:22:11 UTC
(In reply to Nick Clemens (kidclamp) from comment #11)
> This page should be updated to use validation as per coding guidelines, and
> the styling improved, however, this fixes the current validation and
> improves the deletion process so I think we should move this forward.

Could we please have a new bug for this?
Comment 13 Katrin Fischer 2024-06-27 09:52:35 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 14 Jan Kissig 2024-06-27 10:27:01 UTC
I opened Bug 37209(In reply to Katrin Fischer from comment #12)
> (In reply to Nick Clemens (kidclamp) from comment #11)
> > This page should be updated to use validation as per coding guidelines, and
> > the styling improved, however, this fixes the current validation and
> > improves the deletion process so I think we should move this forward.
> 
> Could we please have a new bug for this?

 BUG 37209 is ready to be fixed
Comment 15 Katrin Fischer 2024-06-27 11:16:33 UTC
Thanks!
Comment 16 Lucas Gass 2024-07-19 14:36:02 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 17 Fridolin Somers 2024-07-22 09:43:21 UTC
Does not apply easy on 23.11.x
Please provide patches if needed
Comment 18 Jan Kissig 2024-07-22 11:22:45 UTC
(In reply to Fridolin Somers from comment #17)
> Does not apply easy on 23.11.x
> Please provide patches if needed

This bug is related to CSRF changes in 24.05 and the original code works in 23.11.x