Bug 17927

This bug tries to resurrect the fixes to data types from bug 17008.

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>

Created attachment 59387 [details] [review]
Bug 17927: (followup) Fix /cities cityid type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 59388 [details] [review]
Bug 17927: (QA followup) Fix boolean types

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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.

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?

(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

(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?

(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.

Created attachment 59899 [details] [review]
Bug 17927: (QA followup) Fix timestamp nullable in hold.json

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master for 17.05, thanks Lari, Josef, Tomas!

This appears to break /holds on 16.11.x - so not backporting it.