Bug 17927

Summary: REST API: /holds and /patrons attributes have wrong types
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Web servicesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, josef.moravec, katrin.fischer, kyle, lari.taskula
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17932    
Attachments: Bug 17927: Fix /holds and /patrons data types
Bug 17927: (followup) Fix /cities cityid type
Bug 17927: (QA followup) Fix boolean types
Bug 17927: (QA followup) Fix timestamp nullable in hold.json

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.