Bug 34920 - ERM breaks if an ERM authorized value is missing a description
Summary: ERM breaks if an ERM authorized value is missing a description
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Pedro Amorim
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 17390 36203
  Show dependency treegraph
 
Reported: 2023-09-26 15:40 UTC by Laura Escamilla
Modified: 2024-07-25 11:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 34920: Make av description nullable in API spec (930 bytes, patch)
2024-02-12 14:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Default to using av.value is av.description is falsy (1.85 KB, patch)
2024-02-12 14:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Make av description nullable in API spec (995 bytes, patch)
2024-02-13 16:41 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 34920: Default to using av.value is av.description is falsy (1.91 KB, patch)
2024-02-13 16:41 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 34920: (QA follow-up): Return sorted av_cats to be able to test (1.20 KB, patch)
2024-02-29 13:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: (QA follow-up): Add API endpoint tests (3.33 KB, patch)
2024-02-29 13:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Return sorted av_cats to be able to test (1.18 KB, patch)
2024-02-29 15:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Add API endpoint tests (3.31 KB, patch)
2024-02-29 15:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null (1.62 KB, patch)
2024-02-29 15:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Add cypress tests (36.75 KB, patch)
2024-02-29 16:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34920: Return sorted av_cats to be able to test (1.25 KB, patch)
2024-03-21 15:24 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 34920: Add API endpoint tests (3.38 KB, patch)
2024-03-21 15:24 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null (1.69 KB, patch)
2024-03-21 15:24 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 34920: Add cypress tests (36.81 KB, patch)
2024-03-21 15:25 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 34920: Return sorted av_cats to be able to test (1.31 KB, patch)
2024-06-28 15:10 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 34920: Add API endpoint tests (3.45 KB, patch)
2024-06-28 15:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null (1.82 KB, patch)
2024-06-28 15:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 34920: Add cypress tests (36.88 KB, patch)
2024-06-28 15:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laura Escamilla 2023-09-26 15:40:24 UTC
To recreate: 
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface. 
3. Go to the authorized values, select "ERM_AGREEMENT_CLOSURE_REASON" and edit the "Cancelled" authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error. 
4. Go back to the ERM module and notice that there is an error. "Something went wrong: Error: Expected string - got null."
Comment 1 Matt Blenkinsop 2023-09-26 15:56:05 UTC
Description is a required property on the API so either we need to set it as unrequired and allow it to be NULL, or we need to put a mechanism in place to prevent a description being deleted in the admin area so that no AV can have a NULL description

Not sure what the best approach is here but it feels like we shouldn't be allowing NULL descriptions
Comment 2 Pedro Amorim 2024-02-12 14:46:26 UTC
I don't think this is specific to ERM. This will affect any module using the authorised_values REST endpoint (Preservation module now also uses this same REST endpoint).

This is to_api_mapping from Koha/AuthorisedValue.pm:

        id               => 'authorised_value_id',
        category         => 'category_name',
        authorised_value => 'value',
        lib              => 'description',
        lib_opac         => 'opac_description',
        imageurl         => 'image_url',

I think in other places in Koha we default to using authorised_value ("value" attribute in API) if lib ("description" in API) is falsy (null or empty).

Although I agree with Matt that we shouldn't allow for NULL descriptions, the least we can do for now is prevent a staff user from possibly breaking usage of a module through allowed configuration options (in this case, editing an authorised value as described by Laura). On top of that, allowing the description to be null in the API will at least make it consistent with the database schema.
Comment 3 Pedro Amorim 2024-02-12 14:47:44 UTC
Created attachment 162036 [details] [review]
Bug 34920: Make av description nullable in API spec
Comment 4 Pedro Amorim 2024-02-12 14:47:46 UTC
Created attachment 162037 [details] [review]
Bug 34920: Default to using av.value is av.description is falsy

Test plan:
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface.
3. Go to the authorized values, select 'ERM_AGREEMENT_CLOSURE_REASON' and edit the 'Cancelled' authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error.
4. Go back to the ERM module and notice that there is an error. 'Something went wrong: Error: Expected string - got null.'
5. Apply patch. Run yarn js:build. Repeat test plan
Comment 5 Martin Renvoize (ashimema) 2024-02-13 16:41:17 UTC
Created attachment 162120 [details] [review]
Bug 34920: Make av description nullable in API spec

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize (ashimema) 2024-02-13 16:41:19 UTC
Created attachment 162121 [details] [review]
Bug 34920: Default to using av.value is av.description is falsy

Test plan:
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface.
3. Go to the authorized values, select 'ERM_AGREEMENT_CLOSURE_REASON' and edit the 'Cancelled' authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error.
4. Go back to the ERM module and notice that there is an error. 'Something went wrong: Error: Expected string - got null.'
5. Apply patch. Run yarn js:build. Repeat test plan

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2024-02-13 16:42:01 UTC
I agree with this approach and am as such signing off.. all works nicely in testing.
Comment 8 Jonathan Druart 2024-02-29 10:43:44 UTC
I think there are several wrong things here.

1. Missing a cypress test
2. Missing a test to cover the REST API spec change
3. Do you really want this logic in the low level api-client fetch? What if you want to display a form to edit the AV? You will display the value in the description, which does not reflect the correct values
4. The dropdown list has an empty entry: https://snipboard.io/cAhiq0.jpg

I am not sure what's best here. Maybe we need a fallback at higher level (in the  template)? But that will need to be done in several places.

For discussion.
Comment 9 Pedro Amorim 2024-02-29 13:47:15 UTC
Created attachment 162600 [details] [review]
Bug 34920: (QA follow-up): Return sorted av_cats to be able to test
Comment 10 Pedro Amorim 2024-02-29 13:47:18 UTC
Created attachment 162601 [details] [review]
Bug 34920: (QA follow-up): Add API endpoint tests

prove koha/t/db_dependent/api/v1/authorised_values.t
Comment 11 Jonathan Druart 2024-02-29 14:57:05 UTC
After further discussions it seems that we do not fallback to the code in other part of Koha. We just display an empty string if the description is not present.

This seems wrong and not needed, we should require a description but that's for another bug report (add NOT NULL constraint at DB level).

For now we could simply return an empty string if the value is NULL, and not change the REST API specs.
Comment 12 Pedro Amorim 2024-02-29 15:47:53 UTC
Created attachment 162604 [details] [review]
Bug 34920: Return sorted av_cats to be able to test
Comment 13 Pedro Amorim 2024-02-29 15:47:56 UTC
Created attachment 162605 [details] [review]
Bug 34920: Add API endpoint tests

prove koha/t/db_dependent/api/v1/authorised_values.t
Comment 14 Pedro Amorim 2024-02-29 15:49:46 UTC
Created attachment 162608 [details] [review]
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null

Test plan:
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface.
3. Go to the authorized values, select 'ERM_AGREEMENT_CLOSURE_REASON' and edit the 'Cancelled' authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error.
4. Go back to the ERM module and notice that there is an error. 'Something went wrong: Error: Expected string - got null.'
5. Apply patch. Run yarn js:build. Repeat test plan
6. Create a new agreement. Pick status 'Closed'.
7. Open 'Closure reason'. Confirm there is an entry for 'Expired' and an empty entry below that.
Comment 15 Pedro Amorim 2024-02-29 15:53:10 UTC Comment hidden (obsolete)
Comment 16 Pedro Amorim 2024-02-29 16:07:23 UTC
Created attachment 162609 [details] [review]
Bug 34920: Add cypress tests

Run the agreements cypress test spec:

cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts
Comment 17 Laura Escamilla 2024-03-21 15:24:44 UTC
Created attachment 163628 [details] [review]
Bug 34920: Return sorted av_cats to be able to test

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 18 Laura Escamilla 2024-03-21 15:24:55 UTC
Created attachment 163629 [details] [review]
Bug 34920: Add API endpoint tests

prove koha/t/db_dependent/api/v1/authorised_values.t

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 19 Laura Escamilla 2024-03-21 15:24:58 UTC
Created attachment 163630 [details] [review]
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null

Test plan:
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface.
3. Go to the authorized values, select 'ERM_AGREEMENT_CLOSURE_REASON' and edit the 'Cancelled' authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error.
4. Go back to the ERM module and notice that there is an error. 'Something went wrong: Error: Expected string - got null.'
5. Apply patch. Run yarn js:build. Repeat test plan
6. Create a new agreement. Pick status 'Closed'.
7. Open 'Closure reason'. Confirm there is an entry for 'Expired' and an empty entry below that.

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 20 Laura Escamilla 2024-03-21 15:25:00 UTC
Created attachment 163631 [details] [review]
Bug 34920: Add cypress tests

Run the agreements cypress test spec:

cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 21 Marcel de Rooy 2024-05-07 10:13:00 UTC
Shouldnt this be a bugfix ?
Comment 22 Pedro Amorim 2024-05-07 11:34:28 UTC
(In reply to Marcel de Rooy from comment #21)
> Shouldnt this be a bugfix ?

Yes.
Comment 23 Martin Renvoize (ashimema) 2024-06-28 15:10:59 UTC
Created attachment 168257 [details] [review]
Bug 34920: Return sorted av_cats to be able to test

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize (ashimema) 2024-06-28 15:11:02 UTC
Created attachment 168258 [details] [review]
Bug 34920: Add API endpoint tests

prove koha/t/db_dependent/api/v1/authorised_values.t

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize (ashimema) 2024-06-28 15:11:05 UTC
Created attachment 168259 [details] [review]
Bug 34920: Ensure 'description' property is an empty string if 'lib' is null

Test plan:
1. Enable ERM module
2. Confirm that you can open the ERM module in the staff interface.
3. Go to the authorized values, select 'ERM_AGREEMENT_CLOSURE_REASON' and edit the 'Cancelled' authorized value by removing the description. Making this change with any of the ERM authorized values will result in an error.
4. Go back to the ERM module and notice that there is an error. 'Something went wrong: Error: Expected string - got null.'
5. Apply patch. Run yarn js:build. Repeat test plan
6. Create a new agreement. Pick status 'Closed'.
7. Open 'Closure reason'. Confirm there is an entry for 'Expired' and an empty entry below that.

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize (ashimema) 2024-06-28 15:11:08 UTC
Created attachment 168260 [details] [review]
Bug 34920: Add cypress tests

Run the agreements cypress test spec:

cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize (ashimema) 2024-06-28 15:12:22 UTC
All very sensible, covered by tests and working as described.

Passing QA
Comment 28 Katrin Fischer 2024-07-01 16:58:11 UTC
Pushed for 24.11!

Well done everyone, thank you!