Bug 27330

Summary: Wrong return status when no enrollments in club holds
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: agustinmoyano, andrew, caroline.cyr-la-rose, fridolin.somers, jonathan.druart+koha, martin.renvoize, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27587
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10
Bug Depends on: 19618, 27333    
Bug Blocks: 23857, 27593    
Attachments: Bug 27330: Regression tests
Bug 27330: Return 403 if club has no enrollments
Bug 27330: (follow-up) Cleanup missed on bug 23843
Bug 27330: Use Basic auth in tests
Bug 27330: Regression tests
Bug 27330: Return 403 if club has no enrollments
Bug 27330: (follow-up) Cleanup missed on bug 23843
Bug 27330: (QA follow-up) Return 409 instead
Bug 27330: Use Basic auth in tests
Bug 27330: Regression tests
Bug 27330: Return 403 if club has no enrollments
Bug 27330: (follow-up) Cleanup missed on bug 23843
Bug 27330: (QA follow-up) Return 409 instead
Bug 27330: Use Basic auth in tests
Bug 27330: Regression tests
Bug 27330: Return 403 if club has no enrollments
Bug 27330: (follow-up) Cleanup missed on bug 23843
Bug 27330: (QA follow-up) Return 409 instead

Description Tomás Cohen Arazi 2021-01-04 13:42:55 UTC
It seems we are expecting a 500 if the club doesn't have any enrollments. A 403 should be returned instead. And it highlights some bad logic in the controller, as it is not catching the narrower exception, but the broader (Exceptions Koha::Club::Hold::add can throw) so some error conditions are being treated wrongly (i.e. bad parameters vs. no enrollments on the club).
Comment 1 Tomás Cohen Arazi 2021-01-04 14:37:03 UTC
Created attachment 114798 [details] [review]
Bug 27330: Regression tests
Comment 2 Tomás Cohen Arazi 2021-01-04 14:37:14 UTC
Created attachment 114799 [details] [review]
Bug 27330: Return 403 if club has no enrollments

This patch makes the controller script return a 403 as it should [1]
instead of a plain 500.

To test:
1. Apply the regressions tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] https://www.restapitutorial.com/httpstatuscodes.html
Comment 3 Tomás Cohen Arazi 2021-01-04 14:37:19 UTC
Created attachment 114800 [details] [review]
Bug 27330: (follow-up) Cleanup missed on bug 23843

This patch makes some cleanup missed on bug 23843. The catch condition
on the controller was copied and pasted from other controller and should
be avoided unless there's a known case that needs special handling.
Otherwise it will be catched by the $c->unhandled_exception call.

All the old mappings were inadvertedly kept in the controller as well as
the (unused) _to_api method.

The base class is also cleaned from unnecessary mappings. Only mapped
things need to be added.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/Koha/Club/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 4 Martin Renvoize 2021-01-26 17:08:49 UTC
This feels like it could be a `409 Conflict` response.. but I'm not really sure if it wins us much doing that rather than a `403 Forbidden`... Either way, I think the 4XX is correct, this is a client pre-condition issue.. as in patrons should have been added to a club prior to a hold being placed.
Comment 5 Martin Renvoize 2021-01-26 17:10:50 UTC
Hmm, tests conflict with dependencies so this doesn't apply nicely.. I'll wait for those to get pushed before returning to this one..
Comment 6 Tomás Cohen Arazi 2021-01-26 20:24:16 UTC
After re-reading the original tutorial I rely on for HTTP status codes [1] I think I now read it the same way you do: the user can add the patrons and THEN re-submit. So 409 makes perfect sense. My original interpretation was that some change could be made to the request for resubmitting it.

I agree with 409!

[1] https://www.restapitutorial.com/httpstatuscodes.html
Comment 7 Tomás Cohen Arazi 2021-01-27 10:39:34 UTC
Created attachment 115863 [details] [review]
Bug 27330: Use Basic auth in tests

This patch puts the tests in line with the current codebase. Some bits
are passed through perltidy to aid readability...

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
=> SUCCESS: Tests look more like the rest of the API tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2021-01-27 10:39:38 UTC
Created attachment 115864 [details] [review]
Bug 27330: Regression tests
Comment 9 Tomás Cohen Arazi 2021-01-27 10:39:41 UTC
Created attachment 115865 [details] [review]
Bug 27330: Return 403 if club has no enrollments

This patch makes the controller script return a 403 as it should [1]
instead of a plain 500.

To test:
1. Apply the regressions tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] https://www.restapitutorial.com/httpstatuscodes.html
Comment 10 Tomás Cohen Arazi 2021-01-27 10:39:45 UTC
Created attachment 115866 [details] [review]
Bug 27330: (follow-up) Cleanup missed on bug 23843

This patch makes some cleanup missed on bug 23843. The catch condition
on the controller was copied and pasted from other controller and should
be avoided unless there's a known case that needs special handling.
Otherwise it will be catched by the $c->unhandled_exception call.

All the old mappings were inadvertedly kept in the controller as well as
the (unused) _to_api method.

The base class is also cleaned from unnecessary mappings. Only mapped
things need to be added.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/Koha/Club/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 11 Tomás Cohen Arazi 2021-01-27 10:39:49 UTC
Created attachment 115867 [details] [review]
Bug 27330: (QA follow-up) Return 409 instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2021-01-27 10:41:32 UTC
(In reply to Martin Renvoize from comment #5)
> Hmm, tests conflict with dependencies so this doesn't apply nicely.. I'll
> wait for those to get pushed before returning to this one..

This was missing a dependency (bug 23857) that changed how the tests for this feature are written. As that bug will take a while, I moved the (needed) refactoring here, and added a follow-up for returning 409 instead.

Thanks for the valuable feedback.
Comment 13 Martin Renvoize 2021-01-28 15:51:18 UTC
Created attachment 115968 [details] [review]
Bug 27330: Use Basic auth in tests

This patch puts the tests in line with the current codebase. Some bits
are passed through perltidy to aid readability...

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
=> SUCCESS: Tests look more like the rest of the API tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-01-28 15:51:21 UTC
Created attachment 115969 [details] [review]
Bug 27330: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-01-28 15:51:24 UTC
Created attachment 115970 [details] [review]
Bug 27330: Return 403 if club has no enrollments

This patch makes the controller script return a 403 as it should [1]
instead of a plain 500.

To test:
1. Apply the regressions tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] https://www.restapitutorial.com/httpstatuscodes.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-01-28 15:51:27 UTC
Created attachment 115971 [details] [review]
Bug 27330: (follow-up) Cleanup missed on bug 23843

This patch makes some cleanup missed on bug 23843. The catch condition
on the controller was copied and pasted from other controller and should
be avoided unless there's a known case that needs special handling.
Otherwise it will be catched by the $c->unhandled_exception call.

All the old mappings were inadvertedly kept in the controller as well as
the (unused) _to_api method.

The base class is also cleaned from unnecessary mappings. Only mapped
things need to be added.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/Koha/Club/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-01-28 15:51:30 UTC
Created attachment 115972 [details] [review]
Bug 27330: (QA follow-up) Return 409 instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-01-28 15:51:57 UTC
Works as expected, thanks Tomas :)

Signing off
Comment 19 Katrin Fischer 2021-01-31 14:06:39 UTC
Created attachment 116109 [details] [review]
Bug 27330: Use Basic auth in tests

This patch puts the tests in line with the current codebase. Some bits
are passed through perltidy to aid readability...

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
=> SUCCESS: Tests look more like the rest of the API tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2021-01-31 14:06:44 UTC
Created attachment 116110 [details] [review]
Bug 27330: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2021-01-31 14:06:48 UTC
Created attachment 116111 [details] [review]
Bug 27330: Return 403 if club has no enrollments

This patch makes the controller script return a 403 as it should [1]
instead of a plain 500.

To test:
1. Apply the regressions tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

[1] https://www.restapitutorial.com/httpstatuscodes.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2021-01-31 14:06:53 UTC
Created attachment 116112 [details] [review]
Bug 27330: (follow-up) Cleanup missed on bug 23843

This patch makes some cleanup missed on bug 23843. The catch condition
on the controller was copied and pasted from other controller and should
be avoided unless there's a known case that needs special handling.
Otherwise it will be catched by the $c->unhandled_exception call.

All the old mappings were inadvertedly kept in the controller as well as
the (unused) _to_api method.

The base class is also cleaned from unnecessary mappings. Only mapped
things need to be added.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/clubs_holds.t \
           t/db_dependent/Koha/Club/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2021-01-31 14:06:58 UTC
Created attachment 116113 [details] [review]
Bug 27330: (QA follow-up) Return 409 instead

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2021-01-31 14:07:25 UTC
I've verified the tests pass and taken a look at the code, nothing obvious (to me) stood out. Trusting Martin with the functional testing here.
Comment 25 Katrin Fischer 2021-01-31 14:08:03 UTC
I've verified the tests pass and taken a look at the code, nothing obvious (to me) stood out. Trusting Martin with the functional testing here.
Comment 26 Jonathan Druart 2021-02-01 15:46:46 UTC
(In reply to Katrin Fischer from comment #19)
> Created attachment 116109 [details] [review] [review]
> Bug 27330: Use Basic auth in tests
> 
> This patch puts the tests in line with the current codebase.

Can we fix all the other tests as well please?
Comment 27 Jonathan Druart 2021-02-01 15:47:02 UTC
(On its own bug report of course)
Comment 28 Tomás Cohen Arazi 2021-02-01 15:50:54 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Katrin Fischer from comment #19)
> > Created attachment 116109 [details] [review] [review] [review]
> > Bug 27330: Use Basic auth in tests
> > 
> > This patch puts the tests in line with the current codebase.
> 
> Can we fix all the other tests as well please?

I think it is only:

- acquisitions_vendors.t
- patrons_accounts.t
- patrons_password.t
- stockrotationstage.t
Comment 29 Jonathan Druart 2021-02-01 15:56:34 UTC
+            unless ($item) {
+                return $c->render(
+                    status  => 404,
+                    openapi => { error => "item_id not found." }
+                );
+            }

[...]

+        elsif ($biblio_id) {
+            $biblio = Koha::Biblios->find($biblio_id);
+        }
+        else {
+            return $c->render(
+                status  => 400,
+                openapi => { error => "At least one of biblio_id, item_id should be given" }
+            );


Is that correct? (404 vs 400)
Comment 30 Jonathan Druart 2021-02-01 15:57:30 UTC
(In reply to Tomás Cohen Arazi from comment #28)
> (In reply to Jonathan Druart from comment #26)
> > (In reply to Katrin Fischer from comment #19)
> > > Created attachment 116109 [details] [review] [review] [review] [review]
> > > Bug 27330: Use Basic auth in tests
> > > 
> > > This patch puts the tests in line with the current codebase.
> > 
> > Can we fix all the other tests as well please?
> 
> I think it is only:
> 
> - acquisitions_vendors.t
> - patrons_accounts.t
> - patrons_password.t
> - stockrotationstage.t

"only", so you prefer to fix them here?
Comment 31 Jonathan Druart 2021-02-01 16:26:44 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 32 Tomás Cohen Arazi 2021-02-02 14:28:20 UTC
(In reply to Jonathan Druart from comment #29)
> +            unless ($item) {
> +                return $c->render(
> +                    status  => 404,
> +                    openapi => { error => "item_id not found." }
> +                );
> +            }
> 
> [...]
> 
> +        elsif ($biblio_id) {
> +            $biblio = Koha::Biblios->find($biblio_id);
> +        }
> +        else {
> +            return $c->render(
> +                status  => 400,
> +                openapi => { error => "At least one of biblio_id, item_id
> should be given" }
> +            );
> 
> 
> Is that correct? (404 vs 400)

Wrong line, but good catch :-D
Fixed on bug 27593.
Comment 33 Tomás Cohen Arazi 2021-02-02 14:29:12 UTC
(In reply to Jonathan Druart from comment #30)
> (In reply to Tomás Cohen Arazi from comment #28)
> > (In reply to Jonathan Druart from comment #26)
> > > (In reply to Katrin Fischer from comment #19)
> > > > Created attachment 116109 [details] [review] [review] [review] [review] [review]
> > > > Bug 27330: Use Basic auth in tests
> > > > 
> > > > This patch puts the tests in line with the current codebase.
> > > 
> > > Can we fix all the other tests as well please?
> > 
> > I think it is only:
> > 
> > - acquisitions_vendors.t
> > - patrons_accounts.t
> > - patrons_password.t
> > - stockrotationstage.t
> 
> "only", so you prefer to fix them here?

Patches for this are a bit lengthy. Handled on bug 27330.
Comment 34 Fridolin Somers 2021-02-05 15:47:13 UTC
Can this be backported to 20.11.x ?
Comment 35 Tomás Cohen Arazi 2021-02-05 19:26:20 UTC
(In reply to Fridolin Somers from comment #34)
> Can this be backported to 20.11.x ?

Yes.
Comment 36 Fridolin Somers 2021-02-25 08:57:51 UTC
Pushed to 20.11.x for 20.11.04
Comment 37 Andrew Fuerste-Henry 2021-03-04 17:30:38 UTC
Pushed to 20.05.x for 20.05.10
Comment 38 Victor Grousset/tuxayo 2021-03-18 19:24:21 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.