Bug 32495 - Required fields in API and UI form don't match
Summary: Required fields in API and UI form don't match
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Matt Blenkinsop
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 33235
  Show dependency treegraph
 
Reported: 2022-12-19 17:42 UTC by Matt Blenkinsop
Modified: 2023-12-28 20:42 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement changes the new agreement form so that the description field is no longer required (to match with the API).
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 32495: Mis-matched required fields in UI and api (1.60 KB, patch)
2023-01-09 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32495: Mis-matched required fields in UI and api (1.63 KB, patch)
2023-01-10 22:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 32495: Mis-matched required fields in UI and api (1.73 KB, patch)
2023-02-24 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32495: (follow-up) Remove required property (1.39 KB, patch)
2023-03-01 10:48 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32495: (follow-up) Remove required property (1.43 KB, patch)
2023-03-01 11:50 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2022-12-19 17:42:46 UTC
The required fields in the API for adding a new agreement and the form on the UI to add it don't match.

The API endpoint for POST "/api/v1/erm/agreements" requires "name, status, is_perpetual"
The form on the UI requires the fields above (is_perpetual is a checkbox with a default value selected) but also requires "Description".

Description should either be added to the API or the flag on the UI should be removed.
Comment 1 Jonathan Druart 2023-01-04 15:31:33 UTC
Jonathan, what's correct here?
Comment 2 Jonathan Field 2023-01-04 15:44:17 UTC
I would say that the Description could definitely be an optional field (in UI and therefore API).
In fact, is_perpetual could also be deemed not required. It is in the UI right now as it's a Yes/No option.
Comment 3 Matt Blenkinsop 2023-01-09 15:15:54 UTC
Created attachment 145149 [details] [review]
Bug 32495: Mis-matched required fields in UI and api

The "description" field is marked as required in the UI when creating a new Agreement. This flag has been removed to match the API where it is not a required field. Is Perpetual has not been marked as required as it is a checkbox with a default selection so will always provide a value.

Test plan:
1) In the ERM module, add a new agreement and observe that the Description field is required
2) Apply patch
3) Repeat step 1 but this time the Required flag will not be visible.
Comment 4 David Nind 2023-01-10 22:17:29 UTC
Created attachment 145205 [details] [review]
Bug 32495: Mis-matched required fields in UI and api

The "description" field is marked as required in the UI when creating a new Agreement. This flag has been removed to match the API where it is not a required field. Is Perpetual has not been marked as required as it is a checkbox with a default selection so will always provide a value.

Test plan:
1) In the ERM module, add a new agreement and observe that the Description field is required
2) Apply patch
3) Repeat step 1 but this time the Required flag will not be visible.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-01-10 22:49:30 UTC
Testing notes (using KTD):

1. After step 2 (apply the patch), you have to yarn build_js

2. Then I flushed the cached (flush_memcached) and restarted everything (restart_all).
Comment 6 Marcel de Rooy 2023-02-24 10:46:59 UTC
Created attachment 147290 [details] [review]
Bug 32495: Mis-matched required fields in UI and api

The "description" field is marked as required in the UI when creating a new Agreement. This flag has been removed to match the API where it is not a required field. Is Perpetual has not been marked as required as it is a checkbox with a default selection so will always provide a value.

Test plan:
1) In the ERM module, add a new agreement and observe that the Description field is required
2) Apply patch
3) Repeat step 1 but this time the Required flag will not be visible.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Tomás Cohen Arazi 2023-02-24 19:56:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 8 Matt Blenkinsop 2023-03-01 10:48:35 UTC
Created attachment 147555 [details] [review]
Bug 32495: (follow-up) Remove required property

This follow-up removes the required property from the text area as this
was missed previously.

Test plan:
1) In the ERM module, add a new agreement and observe that the Description field is required
2) Apply patch
3) Repeat step 1 but this time the Required flag will not be visible.
4) Submit the form with the Description field blank - the submission
   should work
Comment 9 Pedro Amorim 2023-03-01 11:50:42 UTC
Created attachment 147558 [details] [review]
Bug 32495: (follow-up) Remove required property

This follow-up removes the required property from the text area as this
was missed previously.

Test plan:
1) In the ERM module, add a new agreement and observe that the Description field is required
2) Apply patch
3) Repeat step 1 but this time the Required flag will not be visible.
4) Submit the form with the Description field blank - the submission
   should work

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 10 Tomás Cohen Arazi 2023-03-02 11:42:34 UTC
Follow-up pushed to master.
Comment 11 Jonathan Druart 2023-03-02 14:45:50 UTC
You broke the tests.
Comment 12 Jonathan Druart 2023-03-02 14:46:46 UTC
(In reply to Jonathan Druart from comment #11)
> You broke the tests.

Timed out retrying after 4000ms: Not enough elements found. Found '2', expected '3'.
t/cypress/integration/Agreements_spec.ts:188:65
  186 | 
  187 |         cy.get("#agreements_add").contains("Submit").click();
> 188 |         cy.get("input:invalid,textarea:invalid,select:invalid").should(
      |                                                                 ^
  189 |             "have.length",
  190 |             3
  191 |         );
Comment 13 Matt Blenkinsop 2023-03-17 17:20:50 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 14 Lucas Gass 2023-04-21 16:50:54 UTC
Not needed in 22.05.x, no backport