Bug 30482 - Potential for bad string concatenation in cataloging validation error message
Summary: Potential for bad string concatenation in cataloging validation error message
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords: rel_21_11_candidate
Depends on: 30159
Blocks:
  Show dependency treegraph
 
Reported: 2022-04-07 09:31 UTC by Owen Leonard
Modified: 2023-06-08 22:26 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:
22.05.00,21.11.06


Attachments
Screenshot of an extra "0" output with a validation error (155.26 KB, image/png)
2022-04-07 09:51 UTC, Owen Leonard
Details
Bug 30482: Potential for bad string concatenation in cataloging validation error message (5.51 KB, patch)
2022-04-07 09:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30482: Potential for bad string concatenation in cataloging validation error message (5.56 KB, patch)
2022-04-20 23:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 30482: Potential for bad string concatenation in cataloging validation error message (5.63 KB, patch)
2022-04-22 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2022-04-07 09:31:25 UTC Comment hidden (obsolete)
Comment 1 Owen Leonard 2022-04-07 09:33:57 UTC
We run AreFieldsNotOk() twice and concatenate the result when showing errors on the screen. If the second run of AreFieldsNotOk() returns false, the string concatenation adds "0"
Comment 2 Owen Leonard 2022-04-07 09:51:59 UTC
Created attachment 133061 [details]
Screenshot of an extra "0" output with a validation error
Comment 3 Owen Leonard 2022-04-07 09:53:19 UTC
Created attachment 133062 [details] [review]
Bug 30482: Potential for bad string concatenation in cataloging validation error message

In validating the basic MARC editor form before submission, we run
AreFieldsNotOk() twice (once with the parameter "false") and concatenate
the result for output. This creates the potential for the error string
to be appended with "0" if AreFieldsNotOk() returns false.

This patch improves the logic around building the error string.

To test, apply the patch and make sure one of your MARC frameworks
contains at least one mandatory field and at least one important field.

- Edit or create a MARC record in the basic MARC editor.
- Submit the form in various states of completion:
  - If a mandatory and an important field are empty, you should see two
    error messages at the top. "The following mandatory subfields aren't
    filled" and "The following important subfields aren’t filled."
  - If a mandatory OR an important field is empty, you should see a
    single message.
    - If it's the important field which is empty, a confirmation will
      ask if you want to save the record anyway. Test that both answers
      to this confirmation work correctly.
  - If no mandatory or important fields are empty the form should
    submit.
Comment 4 David Nind 2022-04-20 23:37:39 UTC
Created attachment 133506 [details] [review]
Bug 30482: Potential for bad string concatenation in cataloging validation error message

In validating the basic MARC editor form before submission, we run
AreFieldsNotOk() twice (once with the parameter "false") and concatenate
the result for output. This creates the potential for the error string
to be appended with "0" if AreFieldsNotOk() returns false.

This patch improves the logic around building the error string.

To test, apply the patch and make sure one of your MARC frameworks
contains at least one mandatory field and at least one important field.

- Edit or create a MARC record in the basic MARC editor.
- Submit the form in various states of completion:
  - If a mandatory and an important field are empty, you should see two
    error messages at the top. "The following mandatory subfields aren't
    filled" and "The following important subfields aren’t filled."
  - If a mandatory OR an important field is empty, you should see a
    single message.
    - If it's the important field which is empty, a confirmation will
      ask if you want to save the record anyway. Test that both answers
      to this confirmation work correctly.
  - If no mandatory or important fields are empty the form should
    submit.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Jonathan Druart 2022-04-22 12:38:10 UTC
Created attachment 133675 [details] [review]
Bug 30482: Potential for bad string concatenation in cataloging validation error message

In validating the basic MARC editor form before submission, we run
AreFieldsNotOk() twice (once with the parameter "false") and concatenate
the result for output. This creates the potential for the error string
to be appended with "0" if AreFieldsNotOk() returns false.

This patch improves the logic around building the error string.

To test, apply the patch and make sure one of your MARC frameworks
contains at least one mandatory field and at least one important field.

- Edit or create a MARC record in the basic MARC editor.
- Submit the form in various states of completion:
  - If a mandatory and an important field are empty, you should see two
    error messages at the top. "The following mandatory subfields aren't
    filled" and "The following important subfields aren’t filled."
  - If a mandatory OR an important field is empty, you should see a
    single message.
    - If it's the important field which is empty, a confirmation will
      ask if you want to save the record anyway. Test that both answers
      to this confirmation work correctly.
  - If no mandatory or important fields are empty the form should
    submit.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Fridolin Somers 2022-04-27 01:04:21 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 7 Kyle M Hall 2022-05-06 16:27:06 UTC
Pushed to 21.11.x for 21.11.06
Comment 8 Victor Grousset/tuxayo 2022-06-17 03:17:47 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.