|
Description
Arthur Suzuki
2023-02-10 15:11:33 UTC
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
Created attachment 155900 [details] [review] Bug 32942: (follow-up) Fix test I missed a test case when doing follow-ups before.. this patch catches that case and create the required authorized value to allow ModSuggestion to succeed. Created attachment 155901 [details] [review] Bug 32942L (follow-up) Fix typo Spelling typo fix Created attachment 155902 [details] [review] Bug 32942: (follow-up) Restore missing statuses At some stage we lost some of the core statuses. This patch simply adds them back into the list. 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. Created attachment 155903 [details] [review] Bug 32942: (follow-up) Fix test I missed a test case when doing follow-ups before.. this patch catches that case and create the required authorized value to allow ModSuggestion to succeed. Created attachment 155904 [details] [review] Bug 32942: (follow-up) Fix test I missed a test case when doing follow-ups before.. this patch catches that case and create the required authorized value to allow ModSuggestion to succeed. Created attachment 155905 [details] [review] Bug 32942: (follow-up) Fix typo Spelling typo fix Created attachment 155906 [details] [review] Bug 32942: (follow-up) Restore missing statuses At some stage we lost some of the core statuses. This patch simply adds them back into the list. 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 |