Bug 17607

Summary: Fix patron definition in Swagger
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: hector.hecaxmmx, jonathan.druart, kyle, lari.taskula, martin.renvoize, mtj
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: 16276    
Bug Blocks: 17621, 17620    
Attachments: Bug 17607: Fix patron definition in Swagger
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger
[PASSED QA] Bug 17607: Fix patron definition in Swagger

Description Tomás Cohen Arazi 2016-11-10 13:21:29 UTC
Bug 16276 changed the Patron attributes (adding lastseen) but failed to update the swagger/definitions/patron.json file accordingly.

To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: Definitions are not complete compared to DB schema
Comment 1 Tomás Cohen Arazi 2016-11-10 13:23:53 UTC
Created attachment 57423 [details] [review]
Bug 17607: Fix patron definition in Swagger

This patch makes the patron swagger definition match the DB schema.
To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: 'lastseen' field is not declared in the swagger definition
- Apply the patch
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> SUCCESS: Tests pass!
- Sigh off :-D

Sponsored-by: ByWater Solutions
Comment 2 Héctor Eduardo Castro Avalos 2016-11-10 17:00:43 UTC
Created attachment 57432 [details] [review]
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger

This patch makes the patron swagger definition match the DB schema.
To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: 'lastseen' field is not declared in the swagger definition
- Apply the patch
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> SUCCESS: Tests pass!
- Sigh off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly
Comment 3 Lari Taskula 2016-11-10 17:08:47 UTC
Created attachment 57433 [details] [review]
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger

This patch makes the patron swagger definition match the DB schema.
To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: 'lastseen' field is not declared in the swagger definition
- Apply the patch
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> SUCCESS: Tests pass!
- Sigh off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 4 Lari Taskula 2016-11-10 17:11:03 UTC
Created attachment 57435 [details] [review]
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger

This patch makes the patron swagger definition match the DB schema.
To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: 'lastseen' field is not declared in the swagger definition
- Apply the patch
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> SUCCESS: Tests pass!
- Sigh off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Fixed a typo: seed -> seen
Comment 5 Héctor Eduardo Castro Avalos 2016-11-10 17:12:56 UTC
(In reply to Lari Taskula from comment #4)
> Fixed a typo: seed -> seen
:) +1
Comment 6 Martin Renvoize 2016-11-11 07:27:16 UTC
QA Remark: We should probably be moving towards stricter validation application wide, and this may be a good one to start with.

datelastseen looks like a date for me ;) So we should probably return it in an internationally recognised format. Swagger allows for this with data type format modifiers: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types

{ type: string, format: date-time } or { type: string, format: date } should suffice depending upon whether this is a date-time or just a short date.

What do we think?  API consumers like explicit specification and consistency for this sort of thing ;)
Comment 7 Jonathan Druart 2016-11-11 08:10:33 UTC
Yes, I agree with Martin. We need to standardise our output to make sure we always return the same date type.
On the other hand this is a follow-up for bug 16276, and I guess we need it before the release.
Comment 8 Tomás Cohen Arazi 2016-11-12 03:32:41 UTC
(In reply to Jonathan Druart from comment #7)
> Yes, I agree with Martin. We need to standardise our output to make sure we
> always return the same date type.
> On the other hand this is a follow-up for bug 16276, and I guess we need it
> before the release.

Exactly, what Martin proposes is an enhancement, which I agree with, but is out of the scope of this one, that is needed for the next release in less than two weeks!
Comment 9 Martin Renvoize 2016-11-12 07:52:52 UTC
If it were as simple as adding "format: date" to each of the date fields then I'd have said lets do it here.. but the update-on field is a date-time (and one that's not in standard format yet at that)..

So lets just go with this for now and add a followup to specify formats in another bug.
Comment 10 Martin Renvoize 2016-11-14 06:54:10 UTC
Created attachment 57467 [details] [review]
[PASSED QA] Bug 17607: Fix patron definition in Swagger

This patch makes the patron swagger definition match the DB schema.
To test:
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> FAIL: 'lastseen' field is not declared in the swagger definition
- Apply the patch
- Run:
  $ prove t/db_dependent/api/v1/swagger/definitions.t
=> SUCCESS: Tests pass!
- Sigh off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Fixed a typo: seed -> seen

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Kyle M Hall 2016-11-14 11:25:25 UTC
Pushed to master for 16.11, thanks Tomas!
Comment 12 Mason James 2016-11-30 11:22:40 UTC
(In reply to Kyle M Hall from comment #11)
> Pushed to master for 16.11, thanks Tomas!

blocked by enhancement, passing for 16.05