Bug 17927 - REST API: /holds and /patrons attributes have wrong types
Summary: REST API: /holds and /patrons attributes have wrong types
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 17932
  Show dependency treegraph
 
Reported: 2017-01-17 18:07 UTC by Tomás Cohen Arazi
Modified: 2018-06-04 20:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17927: Fix /holds and /patrons data types (6.43 KB, patch)
2017-01-20 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17927: (followup) Fix /cities cityid type (799 bytes, patch)
2017-01-20 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17927: (QA followup) Fix boolean types (1.63 KB, patch)
2017-01-20 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17927: (QA followup) Fix timestamp nullable in hold.json (915 bytes, patch)
2017-02-03 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-01-17 18:07:42 UTC

    
Comment 1 Tomás Cohen Arazi 2017-01-17 18:08:24 UTC
This bug tries to resurrect the fixes to data types from bug 17008.
Comment 2 Tomás Cohen Arazi 2017-01-20 18:52:25 UTC
Created attachment 59386 [details] [review]
Bug 17927: Fix /holds and /patrons data types

This patch changes current Swagger definitions for patrons and holds to have
data types corresponding to column data types in their database tables.

To test:
1. GET http://yourlibrary/api/v1/patrons/YYY where YYY is existing borrowernumber
2. Observe that numbers / integers are in string data type.
3. Apply this patch
4. Repeat step 1.
5. Observe that numbers / integers are now actually numbers / integers.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2017-01-20 18:52:30 UTC
Created attachment 59387 [details] [review]
Bug 17927: (followup) Fix /cities cityid type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2017-01-20 18:52:35 UTC
Created attachment 59388 [details] [review]
Bug 17927: (QA followup) Fix boolean types

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2017-01-20 18:58:19 UTC
This patches are taken from bug 17008, which got splitted into other bugs that take care of smaller problems. They where signed by Josef, and I'm passing them along with a small followup for bookeans.

Note for the QA team: the current API tests are failing. And this patch is not touching them. I'm working on a solution for the tests and the API as a whole on another bugs. Notably on bug 17932 which will allow us to actually have the API return the data types this patches explicitly define on the swagger files.
Comment 6 Jonathan Druart 2017-02-01 11:15:30 UTC
One test is failing after this patch:

    #   Failed test 'Columns is nullable in DB, not in swagger file for hold: timestamp'
    #   at t/db_dependent/api/v1/swagger/definitions.t line 71.

I also think that we should add tests to check data types, don't you think?
Comment 7 Tomás Cohen Arazi 2017-02-01 12:15:12 UTC
(In reply to Jonathan Druart from comment #6)
> One test is failing after this patch:
> 
>     #   Failed test 'Columns is nullable in DB, not in swagger file for
> hold: timestamp'
>     #   at t/db_dependent/api/v1/swagger/definitions.t line 71.
> 
> I also think that we should add tests to check data types, don't you think?

I think we might better get rid of definitions.t as there's no obvious reason why the swagger definition should match the DB. I can take care of adding 'null' as a valid type for 'timestamp' but I think we should better move that to another bug as the tests are failing without this patches too. See comment #5
Comment 8 Tomás Cohen Arazi 2017-02-01 12:16:05 UTC
(In reply to Jonathan Druart from comment #6)
> One test is failing after this patch:
> 
>     #   Failed test 'Columns is nullable in DB, not in swagger file for
> hold: timestamp'
>     #   at t/db_dependent/api/v1/swagger/definitions.t line 71.
> 
> I also think that we should add tests to check data types, don't you think?

Also, are you sure you tested this along with bug 17932?
Comment 9 Jonathan Druart 2017-02-01 12:25:05 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> I think we might better get rid of definitions.t as there's no obvious
> reason why the swagger definition should match the DB. I can take care of
> adding 'null' as a valid type for 'timestamp' but I think we should better
> move that to another bug as the tests are failing without this patches too.

No, they pass on master.

(In reply to Tomás Cohen Arazi from comment #8)

> Also, are you sure you tested this along with bug 17932?

Yes I did.
Comment 10 Tomás Cohen Arazi 2017-02-03 18:58:32 UTC
Created attachment 59899 [details] [review]
Bug 17927: (QA followup) Fix timestamp nullable in hold.json

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Kyle M Hall 2017-02-07 17:55:53 UTC
Pushed to master for 17.05, thanks Lari, Josef, Tomas!
Comment 12 Katrin Fischer 2017-02-07 22:20:00 UTC
This appears to break /holds on 16.11.x - so not backporting it.