Created attachment 69643 [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
Created attachment 69644 [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.
Created attachment 69645 [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. To test: - Apply the patches - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off :-D
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
(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.
Dominic, I'm sorry. I forgot to add one dependency.
Created attachment 69753 [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
Created attachment 69754 [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.
Created attachment 69755 [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. To test: - Apply the patches - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off :-D
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
Patch does not apply for me on clean master.
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
16330 got big changes. This is not supposed to apply nor up to date with the voted RFC. I'll fix it today.
Created attachment 70609 [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
Created attachment 70610 [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.
Created attachment 70611 [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. To test: - Apply the patches - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 70612 [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. To test: - Apply the patches - Run: $ kshell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! - Sign off :-D
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
Patches need refactoring after recent changes in the codebase.
Created attachment 71768 [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
Created attachment 71769 [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.
Created attachment 71770 [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
Rebased on top of (rebased) bug 16330.
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
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.
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
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>
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>
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>
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>
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>
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>
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>
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>
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>
Pushed to master for 18.05, thanks to everybody involved!
Enhancement, not backported for 17.11.x