Bug 18409 - Error when updating pickup library on patron pages
Summary: Error when updating pickup library on patron pages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-10 13:54 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
4 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:


Attachments
Bug 18409: Make the controller for holds use Koha::Holds (3.61 KB, patch)
2017-04-21 23:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18409: Make the controller for holds use Koha::Holds (3.70 KB, patch)
2017-04-22 11:11 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18409: Make the controller for holds use Koha::Holds (3.76 KB, patch)
2017-04-24 13:44 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-04-10 13:54:57 UTC
On the "Check out" and "Details" pages, attempting to update a pickup library from the holds tab results in this alert: "There was an error:error Internal Server Error". This appears to be an issue with the holds rest api not functioning correctly.
Comment 1 Tomás Cohen Arazi 2017-04-13 16:46:08 UTC
This is bad. Tests for holds are raising 500 actually, so things might have been broken a while back. I'll take a look and provide a fix on monday, Kyle.
Comment 2 Tomás Cohen Arazi 2017-04-21 23:51:56 UTC
Created attachment 62564 [details] [review]
Bug 18409: Make the controller for holds use Koha::Holds

Recently, there's been a major fix on the REST api swagger spec,
which involved fixing boolean values so they are actually booleans
and Koha::Object was extended to handle that.
While the swagger spec for this endpoint got fixed, such is not the case
with the implementation (the controller class).

This patch fixes this situation by:
- Specifying boolean properties as boolean in the schema file
- Fixes the controller so it returns Koha::Hold objects instead of the
  hashref returned by GetReserve, which is wrong.
- Better (than empty) descriptions are added to 'suspend',
  'suspend_until' and 'lowestPriority' on the spec.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, mostly due to error 500 results.
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

This can also be tested using any interface for REST apis.

Note: This endpoint lacks several of the new guidelines and is not
complete (there's no GET for single holds, etc). It is also missing
exception handling. There are probably
other bug reports for that, just thought it was worth mentioning.
Comment 3 Alex Buckley 2017-04-22 11:11:27 UTC
Created attachment 62568 [details] [review]
Bug 18409: Make the controller for holds use Koha::Holds

Recently, there's been a major fix on the REST api swagger spec,
which involved fixing boolean values so they are actually booleans
and Koha::Object was extended to handle that.
While the swagger spec for this endpoint got fixed, such is not the case
with the implementation (the controller class).

This patch fixes this situation by:
- Specifying boolean properties as boolean in the schema file
- Fixes the controller so it returns Koha::Hold objects instead of the
  hashref returned by GetReserve, which is wrong.
- Better (than empty) descriptions are added to 'suspend',
  'suspend_until' and 'lowestPriority' on the spec.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, mostly due to error 500 results.
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

This can also be tested using any interface for REST apis.

Note: This endpoint lacks several of the new guidelines and is not
complete (there's no GET for single holds, etc). It is also missing
exception handling. There are probably
other bug reports for that, just thought it was worth mentioning.

Followed test plan and this patch worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 4 Kyle M Hall 2017-04-24 13:44:35 UTC
Created attachment 62601 [details] [review]
Bug 18409: Make the controller for holds use Koha::Holds

Recently, there's been a major fix on the REST api swagger spec,
which involved fixing boolean values so they are actually booleans
and Koha::Object was extended to handle that.
While the swagger spec for this endpoint got fixed, such is not the case
with the implementation (the controller class).

This patch fixes this situation by:
- Specifying boolean properties as boolean in the schema file
- Fixes the controller so it returns Koha::Hold objects instead of the
  hashref returned by GetReserve, which is wrong.
- Better (than empty) descriptions are added to 'suspend',
  'suspend_until' and 'lowestPriority' on the spec.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, mostly due to error 500 results.
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
- Sign off :-D

This can also be tested using any interface for REST apis.

Note: This endpoint lacks several of the new guidelines and is not
complete (there's no GET for single holds, etc). It is also missing
exception handling. There are probably
other bug reports for that, just thought it was worth mentioning.

Followed test plan and this patch worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2017-04-24 13:44:50 UTC
Pushed to master for 17.05, thanks Tomas!
Comment 6 Katrin Fischer 2017-05-13 11:42:28 UTC
Patch doesn't apply cleanly to 16.11.x - please rebase/comment if this is needed there!
Comment 7 Katrin Fischer 2017-09-16 06:57:56 UTC
No response so far - closing.