Bug 36880

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

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.

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

Created attachment 167190 [details] [review]
Bug 36880: (follow-up) Translation of confirm was missing

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

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

(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.

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

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>

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>

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.

(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?

Pushed for 24.11!

Well done everyone, thank you!

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

Thanks!

Backported to 24.05.x for upcoming 24.05.02

Does not apply easy on 23.11.x
Please provide patches if needed

(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