Bug 32942 - Suggestion API doesn't support custom statuses
Summary: Suggestion API doesn't support custom statuses
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Frédéric Demians
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 27183
  Show dependency treegraph
 
Reported: 2023-02-10 15:11 UTC by Arthur Suzuki
Modified: 2023-10-12 16:00 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.11


Attachments
Bug 32942: Suggestion API route when non 'standard' status are present (1.38 KB, patch)
2023-02-10 17:31 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 32942: Regression tests (1.27 KB, patch)
2023-02-13 18:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32942: Suggestion API route when non 'standard' status are present (1.44 KB, patch)
2023-02-13 18:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Document possible values (1.04 KB, patch)
2023-02-13 18:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Add tests for STATUS checking in Suggestion::store (1.58 KB, patch)
2023-08-18 11:32 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Moving Suggestion->STATUS check to Suggestion::store (3.49 KB, patch)
2023-08-18 11:32 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 32942: Regression tests (1.33 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: Suggestion API route when non 'standard' status are present (1.50 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Document possible values (1.11 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Add tests for STATUS checking in Suggestion::store (1.71 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Moving Suggestion->STATUS check to Suggestion::store (3.63 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (QA follow-up) Fix regression test (1.22 KB, patch)
2023-09-18 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Fix test (1.04 KB, patch)
2023-09-19 15:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942L (follow-up) Fix typo (817 bytes, patch)
2023-09-19 15:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Restore missing statuses (1006 bytes, patch)
2023-09-19 15:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Fix test (1.04 KB, patch)
2023-09-19 15:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Fix test (1.04 KB, patch)
2023-09-19 15:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Fix typo (817 bytes, patch)
2023-09-19 15:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32942: (follow-up) Restore missing statuses (1006 bytes, patch)
2023-09-19 15:41 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2023-02-10 15:11:33 UTC
To reproduce, create a couple of suggestion and set them to status either blank or "ORDERED".
Query the api to fetch all the suggestions : 
https://[your koha]/api/v1/suggestions

If any of the suggestion you would like to get with a query has no status, or not within fixed list of values, you would get this error message :

<pre>
{"errors":[{"message":"Not in enum list: ASKED, CHECKED, ACCEPTED, REJECTED.","path":"\/1\/status"}],"status":200}
</pre>

A suggestion may also have status "ORDERED" which is not in the list, or no status at all.
API should not throw this error on status and give the other details for the suggestion instead.
Comment 1 Arthur Suzuki 2023-02-10 15:22:04 UTC
Also status "AVAILABLE" breaks things.
Comment 2 Tomás Cohen Arazi 2023-02-10 15:42:31 UTC
Can you provide regression tests?
Comment 3 Frédéric Demians 2023-02-10 17:13:13 UTC
I confirm the issue. You can have additional values for suggestions.STATUS field coming from SUGGEST_STATUS authorised_values list.
Comment 4 Frédéric Demians 2023-02-10 17:22:09 UTC
It's here:

https://git.koha-community.org/Koha-community/Koha/src/branch/master/api/v1/swagger/definitions/suggestion.yaml#L65

I suppose this part should be shaved:

enum:
  - ASKED
  - CHECKED
  - ACCEPTED
  - REJECTED
Comment 5 Frédéric Demians 2023-02-10 17:31:32 UTC
Created attachment 146508 [details] [review]
Bug 32942: Suggestion API route when non 'standard' status are present

The API route for listing all suggestion:

/api/v1/suggestions

send back an error message when there is a suugestion with non standard
status (ASKED, CHECKED, ACCEPTED, REJECTED).

This patch fixes this too restrictive restriction.

TO TEST:

1. Add a status in SUGGEST_STATUS AV list.
2. Create a suggestion, and assign it to the previsous status.
3. Call /api/v1/suggestion
3. You get an error message:
   {
    "errors": [ {
      "message":"Not in enum list: ASKED, CHECKED, ACCEPTED, REJECTED.",
      "path":"\/1\/status"
    }],
    "status":200
   }
4. Apply the patch. Call /api/v1/suggestion
Comment 6 Tomás Cohen Arazi 2023-02-13 18:28:21 UTC
Created attachment 146592 [details] [review]
Bug 32942: Regression tests

Having a custom status shouldn't cause issues

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2023-02-13 18:28:24 UTC
Created attachment 146593 [details] [review]
Bug 32942: Suggestion API route when non 'standard' status are present

The API route for listing all suggestion:

/api/v1/suggestions

send back an error message when there is a suugestion with non standard
status (ASKED, CHECKED, ACCEPTED, REJECTED).

This patch fixes this too restrictive restriction.

TO TEST:

1. Add a status in SUGGEST_STATUS AV list.
2. Create a suggestion, and assign it to the previsous status.
3. Call /api/v1/suggestion
3. You get an error message:
   {
    "errors": [ {
      "message":"Not in enum list: ASKED, CHECKED, ACCEPTED, REJECTED.",
      "path":"\/1\/status"
    }],
    "status":200
   }
4. Apply the patch. Call /api/v1/suggestion

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2023-02-13 18:28:27 UTC
Created attachment 146594 [details] [review]
Bug 32942: (QA follow-up) Document possible values

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Jonathan Druart 2023-02-14 11:25:20 UTC
If the spec is no longer doing the check, shouldn't the controller have a test about it? It's not an enum for the rest api specs because we cannot list the values there, but it's still a list of possible values. Other values must be rejected.
Comment 10 Arthur Suzuki 2023-02-17 09:41:20 UTC
(In reply to Jonathan Druart from comment #9)
> If the spec is no longer doing the check, shouldn't the controller have a
> test about it? It's not an enum for the rest api specs because we cannot
> list the values there, but it's still a list of possible values. Other
> values must be rejected.

Couldn't we just set status to "ASKED" or "UNKNOWN" for any value other than the one defined in the authorized_value?
Comment 11 Arthur Suzuki 2023-08-18 11:32:08 UTC
Created attachment 154642 [details] [review]
Bug 32942: (QA follow-up) Add tests for STATUS checking in Suggestion::store
Comment 12 Arthur Suzuki 2023-08-18 11:32:21 UTC
Created attachment 154643 [details] [review]
Bug 32942: (QA follow-up) Moving Suggestion->STATUS check to Suggestion::store
Comment 13 Martin Renvoize 2023-09-18 12:02:14 UTC
Created attachment 155801 [details] [review]
Bug 32942: Regression tests

Having a custom status shouldn't cause issues

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-09-18 12:02:17 UTC
Created attachment 155802 [details] [review]
Bug 32942: Suggestion API route when non 'standard' status are present

The API route for listing all suggestion:

/api/v1/suggestions

send back an error message when there is a suugestion with non standard
status (ASKED, CHECKED, ACCEPTED, REJECTED).

This patch fixes this too restrictive restriction.

TO TEST:

1. Add a status in SUGGEST_STATUS AV list.
2. Create a suggestion, and assign it to the previsous status.
3. Call /api/v1/suggestion
3. You get an error message:
   {
    "errors": [ {
      "message":"Not in enum list: ASKED, CHECKED, ACCEPTED, REJECTED.",
      "path":"\/1\/status"
    }],
    "status":200
   }
4. Apply the patch. Call /api/v1/suggestion

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-09-18 12:02:20 UTC
Created attachment 155803 [details] [review]
Bug 32942: (QA follow-up) Document possible values

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-09-18 12:02:22 UTC
Created attachment 155804 [details] [review]
Bug 32942: (QA follow-up) Add tests for STATUS checking in Suggestion::store

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2023-09-18 12:02:25 UTC
Created attachment 155805 [details] [review]
Bug 32942: (QA follow-up) Moving Suggestion->STATUS check to Suggestion::store

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2023-09-18 12:02:27 UTC
Created attachment 155806 [details] [review]
Bug 32942: (QA follow-up) Fix regression test

With the move to checking status during store, we broke the api
regression test introduced earlier in the patch set.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-09-18 12:03:09 UTC
Fix QA script issues inline and added a final follow-up to fix the failing regression test we broke with the migration to checking in store.

Passing QA
Comment 20 Tomás Cohen Arazi 2023-09-18 18:54:40 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 21 Fridolin Somers 2023-09-18 21:10:48 UTC
Hi,

There is a typo in changes by "(QA follow-up) Document possible values" :
+      * Values from the `SUGGEST_STATUS` av cateogry

cateogry => category
Comment 22 Tomás Cohen Arazi 2023-09-18 21:12:18 UTC
Also this:

kohadev-koha@kohadevbox:koha(master)$ prove t/db_dependent/Suggestions.t
t/db_dependent/Suggestions.t .. 1/91
#   Failed test 'ModSuggestion status warning is correct'
#   at t/db_dependent/Suggestions.t line 213.
# didn't find a warning
# expected to find warning: No suggestions STALLED letter transported by email

#   Failed test 'ModSuggestion Status OK'
#   at t/db_dependent/Suggestions.t line 215.
#          got: '0'
#     expected: '1'
t/db_dependent/Suggestions.t .. 88/91 # Looks like you failed 2 tests of 91.
Comment 23 Fridolin Somers 2023-09-18 21:27:45 UTC
Oh and I see that ORDERED and AVAILABLE are missing from my @status_constants no ?

 > git grep 'ASKED.*REJECTED' origin/master 
origin/master:Koha/Schema/Result/Suggestion.pm:suggestion status (ASKED, CHECKED, ACCEPTED, REJECTED, ORDERED, AVAILABLE or a value from the SUGGEST_STATUS authorised value category)
origin/master:Koha/Suggestion.pm:    my @status_constants = qw(ASKED CHECKED ACCEPTED REJECTED);
origin/master:installer/data/mysql/kohastructure.sql:  `STATUS` varchar(10) NOT NULL DEFAULT '' COMMENT 'suggestion status (ASKED, CHECKED, ACCEPTED, REJECTED, ORDERED, AVAILABLE or a value from the SUGGEST_STATUS authorised value category)',
origin/master:misc/cronjobs/archive_purchase_suggestions.pl:    push @available_statuses, qw( ASKED ACCEPTED CHECKED REJECTED ORDERED AVAILABLE );
origin/master:misc/cronjobs/archive_purchase_suggestions.pl:It must be one of the 6 default statuses (ASKED, ACCEPTED, CHECKED, REJECTED, ORDERED or AVAILABLE), or one define in the SUGGEST_STATUS authorized value's category.
origin/master:suggestion/suggestion.pl:        unless ( grep { /$criteriumvalue/ } qw(ASKED ACCEPTED REJECTED CHECKED ORDERED AVAILABLE) ) {

In fact @status_constants should be a constant in Koha::Suggestion, to be used by other scripts, no ?
Like we currently have Koha::Serials::MISSING_STATUSES
Comment 24 Martin Renvoize 2023-09-19 15:21:15 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2023-09-19 15:24:06 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2023-09-19 15:38:14 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize 2023-09-19 15:39:41 UTC
OK, those three follow-ups should get us back up and running.

I opted not so switch to a global constant.. I don't really see the benefit and even lookin at the example given by C4::Serials.. I don't actually see the constants referred to much.
Comment 28 Martin Renvoize 2023-09-19 15:40:55 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2023-09-19 15:41:23 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2023-09-19 15:41:25 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2023-09-19 15:41:28 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2023-09-19 18:52:13 UTC
Thanks for the follow-ups. Pushed to master!
Comment 33 Fridolin Somers 2023-09-21 06:04:20 UTC
Pushed to 23.05.x for 23.05.04
Comment 34 Matt Blenkinsop 2023-10-12 16:00:42 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x