Bug 27209 - Add Koha::Hold->set_pickup_location
Summary: Add Koha::Hold->set_pickup_location
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18729 27205
  Show dependency treegraph
 
Reported: 2020-12-11 19:32 UTC by Tomás Cohen Arazi
Modified: 2021-12-13 21:09 UTC (History)
4 users (show)

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


Attachments
Bug 27209: Unit tests (4.18 KB, patch)
2020-12-11 19:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27209: Add Koha::Hold->set_pickup_location (2.62 KB, patch)
2020-12-11 19:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27209: (follow-up) Add ->is_pickup_location_valid (5.44 KB, patch)
2020-12-14 10:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27209: Unit tests (4.24 KB, patch)
2020-12-14 12:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27209: Add Koha::Hold->set_pickup_location (2.67 KB, patch)
2020-12-14 12:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27209: (follow-up) Add ->is_pickup_location_valid (5.50 KB, patch)
2020-12-14 12:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27209: Unit tests (4.31 KB, patch)
2020-12-16 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27209: Add Koha::Hold->set_pickup_location (2.74 KB, patch)
2020-12-16 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27209: (follow-up) Add ->is_pickup_location_valid (5.57 KB, patch)
2020-12-16 15:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27209: Unit tests (4.30 KB, patch)
2020-12-16 15:15 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 27209: Add Koha::Hold->set_pickup_location (2.73 KB, patch)
2020-12-16 15:15 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 27209: (follow-up) Add ->is_pickup_location_valid (5.56 KB, patch)
2020-12-16 15:15 UTC, Josef Moravec
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 2020-12-11 19:32:38 UTC
We need a clean and fully tested method to set a hold's pickup location. It should throw exceptions if:
- No pickup location passed
- The passed pickup location is not a valid one
Comment 1 Tomás Cohen Arazi 2020-12-11 19:55:48 UTC
Created attachment 114356 [details] [review]
Bug 27209: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-12-11 19:55:52 UTC
Created attachment 114357 [details] [review]
Bug 27209: Add Koha::Hold->set_pickup_location

This patch introduces a method to safely update a hold's pickup
location. It will raise exceptionis if the passed parameters are invalid
or absent.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS; Tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-12-14 10:42:34 UTC
Created attachment 114374 [details] [review]
Bug 27209: (follow-up) Add ->is_pickup_location_valid

This patch simply refactors the pickup location check into a method that
can be called on its own. Tests are added, and the tests for
->set_pickup_location should pass unmodified.

To test:
1. Apply the first two patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: tests pass
3. Apply this refactoring patch
4. Notice the tests are similar, but check for boolean output
5. Repeat 2
=> SUCCESS: New tests pass, set_pickup_location() behavior unchanged.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Victor Grousset/tuxayo 2020-12-14 12:47:24 UTC
Created attachment 114378 [details] [review]
Bug 27209: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2020-12-14 12:47:27 UTC
Created attachment 114379 [details] [review]
Bug 27209: Add Koha::Hold->set_pickup_location

This patch introduces a method to safely update a hold's pickup
location. It will raise exceptionis if the passed parameters are invalid
or absent.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS; Tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2020-12-14 12:47:30 UTC
Created attachment 114380 [details] [review]
Bug 27209: (follow-up) Add ->is_pickup_location_valid

This patch simply refactors the pickup location check into a method that
can be called on its own. Tests are added, and the tests for
->set_pickup_location should pass unmodified.

To test:
1. Apply the first two patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: tests pass
3. Apply this refactoring patch
4. Notice the tests are similar, but check for boolean output
5. Repeat 2
=> SUCCESS: New tests pass, set_pickup_location() behavior unchanged.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Kyle M Hall 2020-12-16 15:12:07 UTC
Created attachment 114443 [details] [review]
Bug 27209: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2020-12-16 15:13:05 UTC
Created attachment 114444 [details] [review]
Bug 27209: Add Koha::Hold->set_pickup_location

This patch introduces a method to safely update a hold's pickup
location. It will raise exceptionis if the passed parameters are invalid
or absent.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS; Tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2020-12-16 15:13:08 UTC
Created attachment 114445 [details] [review]
Bug 27209: (follow-up) Add ->is_pickup_location_valid

This patch simply refactors the pickup location check into a method that
can be called on its own. Tests are added, and the tests for
->set_pickup_location should pass unmodified.

To test:
1. Apply the first two patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: tests pass
3. Apply this refactoring patch
4. Notice the tests are similar, but check for boolean output
5. Repeat 2
=> SUCCESS: New tests pass, set_pickup_location() behavior unchanged.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Josef Moravec 2020-12-16 15:15:03 UTC
Created attachment 114448 [details] [review]
Bug 27209: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2020-12-16 15:15:08 UTC
Created attachment 114449 [details] [review]
Bug 27209: Add Koha::Hold->set_pickup_location

This patch introduces a method to safely update a hold's pickup
location. It will raise exceptionis if the passed parameters are invalid
or absent.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS; Tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2020-12-16 15:15:13 UTC
Created attachment 114450 [details] [review]
Bug 27209: (follow-up) Add ->is_pickup_location_valid

This patch simply refactors the pickup location check into a method that
can be called on its own. Tests are added, and the tests for
->set_pickup_location should pass unmodified.

To test:
1. Apply the first two patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: tests pass
3. Apply this refactoring patch
4. Notice the tests are similar, but check for boolean output
5. Repeat 2
=> SUCCESS: New tests pass, set_pickup_location() behavior unchanged.
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Jonathan Druart 2020-12-21 09:20:34 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2020-12-28 13:40:18 UTC
Pushed to 20.11.x for 20.11.01
Comment 15 Andrew Fuerste-Henry 2020-12-28 14:58:16 UTC
Pushed to 20.05.x for 20.05.07
Comment 16 Victor Grousset/tuxayo 2020-12-28 18:27:55 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.
Comment 17 Victor Grousset/tuxayo 2020-12-28 18:42:11 UTC
Tried to backport to 19.11.x it because it's the dependency of a major bug (bug 27205) but the tests don't pass.
Help needed here in order to backport bug 27205.

kohadev-koha@6a2e5bab0d87:/kohadevbox/koha$ prove t/db_dependent/Koha/Hold.t
t/db_dependent/Koha/Hold.t ..     # No tests run!
t/db_dependent/Koha/Hold.t .. 1/3 
#   Failed test 'No tests run for subtest "patron() tests"'
#   at t/db_dependent/Koha/Hold.t line 55.
The method Koha::Hold->patron is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 661
Koha::Object::AUTOLOAD('Koha::Hold=HASH(0x556f6652aa00)') called at t/db_dependent/Koha/Hold.t line 50
main::__ANON__ at /usr/share/perl5/Test/Builder.pm line 310
eval {...} at /usr/share/perl5/Test/Builder.pm line 310
Test::Builder::subtest('Test::Builder=HASH(0x556f5d6558b0)', 'patron() tests', 'CODE(0x556f5d5c2918)') called at /usr/share/perl5/Test/More.pm line 807
Test::More::subtest('patron() tests', 'CODE(0x556f5d5c2918)') called at t/db_dependent/Koha/Hold.t line 55
# Looks like your test exited with 255 just after 1.
t/db_dependent/Koha/Hold.t .. Dubious, test returned 255 (wstat 65280, 0xff00)