Bug 19784 - Adapt /v1/patrons to new naming guidelines
Summary: Adapt /v1/patrons to new naming guidelines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 16330
Blocks: 19774
  Show dependency treegraph
 
Reported: 2017-12-08 11:56 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 19:58 UTC (History)
7 users (show)

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


Attachments
Bug 19784: Unit tests for /api/v1/patrons (20.09 KB, patch)
2017-12-08 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.68 KB, patch)
2017-12-08 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (13.41 KB, patch)
2017-12-08 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (21.00 KB, patch)
2017-12-12 22:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.68 KB, patch)
2017-12-12 22:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (13.40 KB, patch)
2017-12-12 22:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.33 KB, patch)
2018-01-17 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.68 KB, patch)
2018-01-17 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (45.66 KB, patch)
2018-01-17 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.51 KB, patch)
2018-01-17 13:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.31 KB, patch)
2018-02-16 13:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.68 KB, patch)
2018-02-16 13:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.65 KB, patch)
2018-02-16 13:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.56 KB, patch)
2018-02-19 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.68 KB, patch)
2018-02-19 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.69 KB, patch)
2018-02-19 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.61 KB, patch)
2018-02-19 18:53 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.72 KB, patch)
2018-02-19 18:53 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.70 KB, patch)
2018-02-19 18:53 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.67 KB, patch)
2018-02-21 15:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.77 KB, patch)
2018-02-21 15:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.75 KB, patch)
2018-02-21 15:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19784: Unit tests for /api/v1/patrons (22.72 KB, patch)
2018-02-24 00:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints (2.83 KB, patch)
2018-02-24 00:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19784: Adapt /v1/patrons to new naming guidelines (44.81 KB, patch)
2018-02-24 00:44 UTC, Kyle M Hall
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-12-08 11:56:21 UTC

    
Comment 1 Tomás Cohen Arazi 2017-12-08 14:16:59 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-12-08 14:17:07 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-12-08 14:17:12 UTC Comment hidden (obsolete)
Comment 4 Dominic Pichette 2017-12-08 20:05:50 UTC
Applying: Bug 19784: Unit tests for /api/v1/patrons
fatal: sha1 information is lacking or useless (t/db_dependent/api/v1/patrons.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 19784: Unit tests for /api/v1/patrons
Comment 5 Tomás Cohen Arazi 2017-12-08 21:37:05 UTC
(In reply to Dominic Pichette from comment #4)
> Applying: Bug 19784: Unit tests for /api/v1/patrons
> fatal: sha1 information is lacking or useless
> (t/db_dependent/api/v1/patrons.t).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 19784: Unit tests for /api/v1/patrons

Please try on top of my 'pagination' branch on github, plus 16330. Dependencies are not easy to apply. It applies cleanly.
Comment 6 Tomás Cohen Arazi 2017-12-11 14:09:42 UTC
Dominic, I'm sorry. I forgot to add one dependency.
Comment 7 Tomás Cohen Arazi 2017-12-12 22:15:30 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2017-12-12 22:15:38 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2017-12-12 22:15:46 UTC Comment hidden (obsolete)
Comment 10 Dilan Johnpullé 2017-12-14 05:24:56 UTC
Attempted to sign off in devbox but test failed in kshell with the following:

t/db_dependent/api/v1/patrons.t .. 1/5 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=5841405127227230000000.000000, 1='1', 2='Manual Debit'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.

        #   Failed test 'Patron created successfully'
        #   at t/db_dependent/api/v1/patrons.t line 210.
        #          got: '500'
        #     expected: '201'

        #   Failed test 'got a patron_id'
        #   at t/db_dependent/api/v1/patrons.t line 210.

        #   Failed test 'exact match for JSON Pointer "/cardnumber"'
        #   at t/db_dependent/api/v1/patrons.t line 210.
        #          got: undef
        #     expected: 'av0WRHT1xJdZ1JDtAZ2'

        #   Failed test 'exact match for JSON Pointer "/surname"'
        #   at t/db_dependent/api/v1/patrons.t line 210.
        #          got: undef
        #     expected: 'iC24_uBTgv'

        #   Failed test 'exact match for JSON Pointer "/firstname"'
        #   at t/db_dependent/api/v1/patrons.t line 210.
        #          got: undef
        #     expected: 'sIxTOBcMXz'
        # Looks like you failed 5 tests of 18.

    #   Failed test 'librarian access tests'
    #   at t/db_dependent/api/v1/patrons.t line 224.
    # Looks like you failed 1 test of 2.
t/db_dependent/api/v1/patrons.t .. 3/5 
#   Failed test 'add() tests'
#   at t/db_dependent/api/v1/patrons.t line 225.
t/db_dependent/api/v1/patrons.t .. 5/5 # Looks like you failed 1 test of 5.
t/db_dependent/api/v1/patrons.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/patrons.t (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=1, Tests=5,  5 wallclock secs ( 0.03 usr  0.02 sys +  3.20 cusr  0.86 csys =  4.11 CPU)
Result: FAIL
Comment 11 David Bourgault 2017-12-15 20:47:11 UTC
Patch does not apply for me on clean master.
Comment 12 Katrin Fischer 2017-12-28 07:20:45 UTC
Same for me, I have applied the dependencies 16330 and 19686 (both apply cleanly) - but this one gives me a lot of trouble.

I feel like we should wait for this patch to QA the dependencies - probably needs an update now too with the discussion at the last meeting / RFC on the wiki?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16330: Add routes to add, update and delete patrons
Using index info to reconstruct a base tree...
M	Koha/Exceptions.pm
M	Koha/Patron.pm
A	Koha/REST/V1/Patron.pm
M	api/v1/swagger/definitions/patron.json
M	api/v1/swagger/paths/patrons.json
M	t/db_dependent/Koha/Patrons.t
M	t/db_dependent/api/v1/patrons.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/api/v1/patrons.t
CONFLICT (content): Merge conflict in t/db_dependent/api/v1/patrons.t
Auto-merging t/db_dependent/Koha/Patrons.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patrons.t
Auto-merging api/v1/swagger/paths/patrons.json
CONFLICT (content): Merge conflict in api/v1/swagger/paths/patrons.json
Auto-merging Koha/SMS/Provider.pm
CONFLICT (content): Merge conflict in Koha/SMS/Provider.pm
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 16330: Add routes to add, update and delete patrons
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16330-Add-routes-to-add-update-and-delete-patr-hbLRSU.patch
Comment 13 Tomás Cohen Arazi 2017-12-28 10:12:08 UTC
16330 got big changes. This is not supposed to apply nor up to date with the voted RFC. I'll fix it today.
Comment 14 Tomás Cohen Arazi 2018-01-17 13:20:06 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2018-01-17 13:20:10 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2018-01-17 13:20:14 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2018-01-17 13:24:00 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2018-01-17 13:27:22 UTC
My last submission implements the changes voted with the RFC as a basis [1]

Tests should pass, and the changes to the patrons.json file should make sense in terms of the RFC.

I removed the dependency on objects.search as the query parameters used in this endpoint require a more fine grained approach as you can see on the patch.

Enjoy!

[1] https://wiki.koha-community.org/wiki/Patrons_endpoint_RFC
Comment 19 Tomás Cohen Arazi 2018-02-15 17:15:53 UTC
Patches need refactoring after recent changes in the codebase.
Comment 20 Tomás Cohen Arazi 2018-02-16 13:52:17 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2018-02-16 13:52:25 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2018-02-16 13:52:31 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2018-02-16 13:53:47 UTC
Rebased on top of (rebased) bug 16330.
Comment 24 Tomás Cohen Arazi 2018-02-19 16:20:16 UTC
Created attachment 71959 [details] [review]
Bug 19784: Unit tests for /api/v1/patrons

This patch adapts the existing endpoint's tests so they expect:
- 'patron_id' for 'borrowernumber'
- 'library_id' for 'branchcode'
- 'category_id' for 'categorycode'

In the process, I tried to make the tests more robust, by creating random
data that gets deleted to make sure our tests cases can't have false
positives.

Independent subtests are wrapped inside transactions to avoid
eventual interference.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: The api doesn't implement the expected behaviour and attributes
for endpoint responses
Comment 25 Tomás Cohen Arazi 2018-02-19 16:20:21 UTC
Created attachment 71960 [details] [review]
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints

This patch removes the $ref occurences for borrowernumber in not
patron-specific endpoints. 'borrowernumber' is still used on them, but as a
hardcoded parameter. The param rename will happen on a separate bug for
each endpoint.
Comment 26 Tomás Cohen Arazi 2018-02-19 16:20:32 UTC
Created attachment 71961 [details] [review]
Bug 19784: Adapt /v1/patrons to new naming guidelines

This patch introduces two functions to the patrons endpoint:
- _to_api
- _to_model

This are in charge of field mappings in order to comply with the
guidelines.

Koha::REST::V1:Auth is adjusted to handle 'patron_id' as well. 'borrowernumber'
handling is kept until the existing endpoints get updated.

To test:
- Apply the patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/*.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 27 Josef Moravec 2018-02-19 18:53:16 UTC
Created attachment 71967 [details] [review]
Bug 19784: Unit tests for /api/v1/patrons

This patch adapts the existing endpoint's tests so they expect:
- 'patron_id' for 'borrowernumber'
- 'library_id' for 'branchcode'
- 'category_id' for 'categorycode'

In the process, I tried to make the tests more robust, by creating random
data that gets deleted to make sure our tests cases can't have false
positives.

Independent subtests are wrapped inside transactions to avoid
eventual interference.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: The api doesn't implement the expected behaviour and attributes
for endpoint responses

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2018-02-19 18:53:21 UTC
Created attachment 71968 [details] [review]
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints

This patch removes the $ref occurences for borrowernumber in not
patron-specific endpoints. 'borrowernumber' is still used on them, but as a
hardcoded parameter. The param rename will happen on a separate bug for
each endpoint.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2018-02-19 18:53:27 UTC
Created attachment 71969 [details] [review]
Bug 19784: Adapt /v1/patrons to new naming guidelines

This patch introduces two functions to the patrons endpoint:
- _to_api
- _to_model

This are in charge of field mappings in order to comply with the
guidelines.

Koha::REST::V1:Auth is adjusted to handle 'patron_id' as well. 'borrowernumber'
handling is kept until the existing endpoints get updated.

To test:
- Apply the patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/*.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Nick Clemens 2018-02-21 15:48:48 UTC
Created attachment 72026 [details] [review]
Bug 19784: Unit tests for /api/v1/patrons

This patch adapts the existing endpoint's tests so they expect:
- 'patron_id' for 'borrowernumber'
- 'library_id' for 'branchcode'
- 'category_id' for 'categorycode'

In the process, I tried to make the tests more robust, by creating random
data that gets deleted to make sure our tests cases can't have false
positives.

Independent subtests are wrapped inside transactions to avoid
eventual interference.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: The api doesn't implement the expected behaviour and attributes
for endpoint responses

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens 2018-02-21 15:48:52 UTC
Created attachment 72027 [details] [review]
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints

This patch removes the $ref occurences for borrowernumber in not
patron-specific endpoints. 'borrowernumber' is still used on them, but as a
hardcoded parameter. The param rename will happen on a separate bug for
each endpoint.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Nick Clemens 2018-02-21 15:48:55 UTC
Created attachment 72028 [details] [review]
Bug 19784: Adapt /v1/patrons to new naming guidelines

This patch introduces two functions to the patrons endpoint:
- _to_api
- _to_model

This are in charge of field mappings in order to comply with the
guidelines.

Koha::REST::V1:Auth is adjusted to handle 'patron_id' as well. 'borrowernumber'
handling is kept until the existing endpoints get updated.

To test:
- Apply the patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/*.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Kyle M Hall 2018-02-24 00:44:46 UTC
Created attachment 72171 [details] [review]
Bug 19784: Unit tests for /api/v1/patrons

This patch adapts the existing endpoint's tests so they expect:
- 'patron_id' for 'borrowernumber'
- 'library_id' for 'branchcode'
- 'category_id' for 'categorycode'

In the process, I tried to make the tests more robust, by creating random
data that gets deleted to make sure our tests cases can't have false
positives.

Independent subtests are wrapped inside transactions to avoid
eventual interference.

To test:
- Apply the patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: The api doesn't implement the expected behaviour and attributes
for endpoint responses

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2018-02-24 00:44:54 UTC
Created attachment 72172 [details] [review]
Bug 19784: Remove reference to 'borrowernumber' param from other endpoints

This patch removes the $ref occurences for borrowernumber in not
patron-specific endpoints. 'borrowernumber' is still used on them, but as a
hardcoded parameter. The param rename will happen on a separate bug for
each endpoint.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Kyle M Hall 2018-02-24 00:44:57 UTC
Created attachment 72173 [details] [review]
Bug 19784: Adapt /v1/patrons to new naming guidelines

This patch introduces two functions to the patrons endpoint:
- _to_api
- _to_model

This are in charge of field mappings in order to comply with the
guidelines.

Koha::REST::V1:Auth is adjusted to handle 'patron_id' as well. 'borrowernumber'
handling is kept until the existing endpoints get updated.

To test:
- Apply the patches
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/*.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Jonathan Druart 2018-03-29 15:03:06 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 37 Nick Clemens 2018-05-14 13:55:51 UTC
Enhancement, not backported for 17.11.x