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."
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
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.
Created attachment 162036 [details] [review] Bug 34920: Make av description nullable in API spec
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
Created attachment 162120 [details] [review] Bug 34920: Make av description nullable in API spec Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
I agree with this approach and am as such signing off.. all works nicely in testing.
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.
Created attachment 162600 [details] [review] Bug 34920: (QA follow-up): Return sorted av_cats to be able to test
Created attachment 162601 [details] [review] Bug 34920: (QA follow-up): Add API endpoint tests prove koha/t/db_dependent/api/v1/authorised_values.t
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.
Created attachment 162604 [details] [review] Bug 34920: Return sorted av_cats to be able to test
Created attachment 162605 [details] [review] Bug 34920: Add API endpoint tests prove koha/t/db_dependent/api/v1/authorised_values.t
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.
I'm unable to attach the following patch: https://github.com/PTFS-Europe/koha/commit/9d1047e1eb3e604868602a60ffd2cd04e0801a84 Error: 413 Request Entity Too Large
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
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>
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>
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>
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>
Shouldnt this be a bugfix ?
(In reply to Marcel de Rooy from comment #21) > Shouldnt this be a bugfix ? Yes.
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>
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>
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>
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>
All very sensible, covered by tests and working as described. Passing QA
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.06