Also status "AVAILABLE" breaks things.

Can you provide regression tests?

I confirm the issue. You can have additional values for suggestions.STATUS field coming from SUGGEST_STATUS authorised_values list.

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

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

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>

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>

Created attachment 146594 [details] [review]
Bug 32942: (QA follow-up) Document possible values

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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.

(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?

Created attachment 154642 [details] [review]
Bug 32942: (QA follow-up) Add tests for STATUS checking in Suggestion::store

Created attachment 154643 [details] [review]
Bug 32942: (QA follow-up) Moving Suggestion->STATUS check to Suggestion::store

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>

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>

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>

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>

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>

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>

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

Pushed to master for 23.11.

Nice work everyone, thanks!

Hi,

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

cateogry => category

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.

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

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.

Thanks for the follow-ups. Pushed to master!

Pushed to 23.05.x for 23.05.04

Nice work everyone!

Pushed to oldstable for 22.11.x