Bug 17607 - Fix patron definition in Swagger
Summary: Fix patron definition in Swagger
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 16276
Blocks: 17621 17620
  Show dependency treegraph
 
Reported: 2016-11-10 13:21 UTC by Tomás Cohen Arazi
Modified: 2017-06-14 22:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17607: Fix patron definition in Swagger (1.21 KB, patch)
2016-11-10 13:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger (1.31 KB, patch)
2016-11-10 17:00 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger (1.35 KB, patch)
2016-11-10 17:08 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17607: Fix patron definition in Swagger (1.37 KB, patch)
2016-11-10 17:11 UTC, Lari Taskula
Details | Diff | Splinter Review
[PASSED QA] Bug 17607: Fix patron definition in Swagger (1.45 KB, patch)
2016-11-14 06:54 UTC, Martin Renvoize
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 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