Bug 17736 - Move GetReservesFromBiblionumber to Koha::Biblio->holds
Summary: Move GetReservesFromBiblionumber to Koha::Biblio->holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on: 17630
Blocks: 17728 17737
  Show dependency treegraph
 
Reported: 2016-12-07 14:46 UTC by Jonathan Druart
Modified: 2017-04-02 16:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method (3.06 KB, patch)
2016-12-07 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds (29.93 KB, patch)
2016-12-07 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber (3.07 KB, patch)
2016-12-07 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Holds (45.30 KB, image/png)
2016-12-09 09:44 UTC, Josef Moravec
Details
Hold Detail (27.84 KB, image/png)
2016-12-09 09:45 UTC, Josef Moravec
Details
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method (3.06 KB, patch)
2016-12-12 10:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds (29.91 KB, patch)
2016-12-12 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber (3.07 KB, patch)
2016-12-12 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Rename to current_holds (4.84 KB, patch)
2017-01-06 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors (1.51 KB, patch)
2017-01-06 10:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t (1018 bytes, patch)
2017-01-06 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel (987 bytes, patch)
2017-01-06 11:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl (2.46 KB, patch)
2017-01-06 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui (3.38 KB, patch)
2017-01-06 12:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant (2.73 KB, patch)
2017-01-06 12:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method (3.16 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds (30.02 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber (3.17 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Rename to current_holds (4.84 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors (1.51 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t (1018 bytes, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel (987 bytes, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl (2.46 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui (3.44 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant (2.73 KB, patch)
2017-01-06 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant (2.13 KB, patch)
2017-01-06 12:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method (3.16 KB, patch)
2017-02-03 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds (30.05 KB, patch)
2017-02-03 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber (3.17 KB, patch)
2017-02-03 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Rename to current_holds (4.84 KB, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors (1.51 KB, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t (1018 bytes, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel (987 bytes, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl (2.46 KB, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui (3.44 KB, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant (2.13 KB, patch)
2017-02-03 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Fix typo ->new vs ->next (965 bytes, patch)
2017-02-03 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17736: Fix typo ->new vs ->next (1.06 KB, patch)
2017-02-03 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method (3.18 KB, patch)
2017-03-21 19:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds (30.07 KB, patch)
2017-03-21 19:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber (3.19 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Rename to current_holds (4.87 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors (1.53 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t (1.02 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel (1012 bytes, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl (2.48 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui (3.46 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant (2.16 KB, patch)
2017-03-21 19:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17736: Fix typo ->new vs ->next (1.09 KB, patch)
2017-03-21 19:32 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 Jonathan Druart 2016-12-07 14:46:49 UTC

    
Comment 1 Jonathan Druart 2016-12-07 16:24:10 UTC
Created attachment 58015 [details] [review]
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method
Comment 2 Jonathan Druart 2016-12-07 16:24:13 UTC
Created attachment 58016 [details] [review]
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.

But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR

- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.

- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today

Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.
Comment 3 Jonathan Druart 2016-12-07 16:24:16 UTC
Created attachment 58017 [details] [review]
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase
Comment 4 Josef Moravec 2016-12-09 09:44:02 UTC
Created attachment 58056 [details]
Holds

Trying to delete record with hold (title, or item level) through batch delete records tool:

Bibliographic record 1 was not deleted. An error occurred. (The error was: Can't use string ("Koha::Biblio") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/Koha/Object.pm line 219. , see the Koha log file for more information). 

Also, when returned reserved document has to be transfered, after the checking in on target library, everything looks ok. But the hold is not set to waiting status, see attached screenshot. It was title level hold.
Comment 5 Josef Moravec 2016-12-09 09:45:01 UTC
Created attachment 58057 [details]
Hold Detail

Maybe the transit was not finished correctly?
Comment 6 Jonathan Druart 2016-12-12 10:18:56 UTC
Created attachment 58106 [details] [review]
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method
Comment 7 Jonathan Druart 2016-12-12 10:19:00 UTC
Created attachment 58107 [details] [review]
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.

But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR

- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.

- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today

Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.
Comment 8 Jonathan Druart 2016-12-12 10:19:05 UTC
Created attachment 58108 [details] [review]
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase
Comment 9 Jonathan Druart 2016-12-12 10:23:03 UTC
(In reply to Josef Moravec from comment #4)
> Also, when returned reserved document has to be transfered, after the
> checking in on target library, everything looks ok. But the hold is not set
> to waiting status, see attached screenshot. It was title level hold.

Could you give me more information on this please, I don't think I recreate it.
Comment 10 Marcel de Rooy 2017-01-06 09:28:55 UTC
Testing this now
Comment 11 Marcel de Rooy 2017-01-06 09:51:12 UTC
I would rather rename the sub. It is not about when the hold was 'placed' but if the hold pertains to the future or not.
So my suggestion would be: Koha::Biblio->current_holds.
Comment 12 Marcel de Rooy 2017-01-06 09:57:21 UTC
Created attachment 58628 [details] [review]
Bug 17736: [Follow-up] Rename to current_holds

It is not about when the hold was 'placed' but if the hold pertains to
the future or not.

Test plan:
[1] Git grep on holds_placed_before_today.
[2] Run t/db_dependent/Koha/Biblios.t
[3] Run t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-01-06 09:59:58 UTC
(In reply to Marcel de Rooy from comment #11)
> I would rather rename the sub. It is not about when the hold was 'placed'
> but if the hold pertains to the future or not.
> So my suggestion would be: Koha::Biblio->current_holds.

And also note that before today is not correct: should include today too.
Added a proposal patch.
Still testing.
Comment 14 Marcel de Rooy 2017-01-06 10:24:45 UTC
Resolving a few errors like this too:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.

This happens because perl is looking first in Koha::Object (with AUTOLOAD) before it checks C4/Biblio.

Rearranging the module order did not help either (moving C4/Biblio).
Comment 15 Marcel de Rooy 2017-01-06 10:27:57 UTC
Created attachment 58629 [details] [review]
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors

Like:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.

Resolved by not importing them but fully qualifying them.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2017-01-06 10:46:52 UTC
Created attachment 58630 [details] [review]
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t

FAIL   t/db_dependent/Holds.t
"my" variable $hold masks earlier declaration in same scope

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-01-06 11:00:16 UTC
Ha: my $biblio = Koha::Biblios->find( $order->{ordernumber} );
Comment 18 Marcel de Rooy 2017-01-06 11:04:31 UTC
Created attachment 58631 [details] [review]
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-01-06 11:50:12 UTC
Bug party!
Do not assume that find gives you a Koha Object. Just see the following examples :)
Comment 20 Marcel de Rooy 2017-01-06 11:50:30 UTC
Created attachment 58632 [details] [review]
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl

Crash in basket.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/basket.pl line 466.
Comes from a biblionumber == NULL in aqorders where I cancelled the order and deleted the biblio.

Crash in parcel.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/parcel.pl line 246.
Similar fix.

Trivial fixes indeed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2017-01-06 12:26:30 UTC
Created attachment 58633 [details] [review]
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui

The solution of Jonathan can be applied in two other cases, effectively
making GetItemHolds obsolete.

Test plan:
[1] Git grep on GetItemHolds
[2] Add an order, place a hold, delete order.
[3] Add an order, receive, place hold, delete order.
Comment 22 Marcel de Rooy 2017-01-06 12:50:29 UTC
Created attachment 58634 [details] [review]
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.

Primarily caused by sub edi_close_and_order.

Easy fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2017-01-06 12:55:07 UTC
Created attachment 58635 [details] [review]
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2017-01-06 12:55:12 UTC
Created attachment 58636 [details] [review]
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.

But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR

- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.

- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today

Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2017-01-06 12:55:16 UTC
Created attachment 58637 [details] [review]
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2017-01-06 12:55:19 UTC
Created attachment 58638 [details] [review]
Bug 17736: [Follow-up] Rename to current_holds

It is not about when the hold was 'placed' but if the hold pertains to
the future or not.

Test plan:
[1] Git grep on holds_placed_before_today.
[2] Run t/db_dependent/Koha/Biblios.t
[3] Run t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2017-01-06 12:55:23 UTC
Created attachment 58639 [details] [review]
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors

Like:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.

Resolved by not importing them but fully qualifying them.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2017-01-06 12:55:27 UTC
Created attachment 58640 [details] [review]
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t

FAIL   t/db_dependent/Holds.t
"my" variable $hold masks earlier declaration in same scope

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2017-01-06 12:55:31 UTC
Created attachment 58641 [details] [review]
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2017-01-06 12:55:34 UTC
Created attachment 58642 [details] [review]
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl

Crash in basket.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/basket.pl line 466.
Comes from a biblionumber == NULL in aqorders where I cancelled the order and deleted the biblio.

Crash in parcel.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/parcel.pl line 246.
Similar fix.

Trivial fixes indeed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2017-01-06 12:55:38 UTC
Created attachment 58643 [details] [review]
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui

The solution of Jonathan can be applied in two other cases, effectively
making GetItemHolds obsolete.

Test plan:
[1] Git grep on GetItemHolds
[2] Add an order, place a hold, delete order.
[3] Add an order, receive, place hold, delete order.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2017-01-06 12:55:42 UTC
Created attachment 58644 [details] [review]
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.

Primarily caused by sub edi_close_and_order.

Easy fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2017-01-06 12:56:37 UTC
This got out of hand somewhere in the process ;)
But it works now (imo). The logic in basket.tt about showing hold numbers is still not working as it should. But should be further dealt with on another report. In parcel I used holds now instead of current_holds.
Comment 34 Marcel de Rooy 2017-01-06 12:59:54 UTC
Created attachment 58645 [details] [review]
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.

Primarily caused by sub edi_close_and_order.

Easy fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2017-01-06 13:00:24 UTC
Removed a debug warn.
Comment 36 Marcel de Rooy 2017-02-03 08:16:49 UTC
Created attachment 59846 [details] [review]
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2017-02-03 08:16:54 UTC
Created attachment 59847 [details] [review]
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.

But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR

- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.

- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today

Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2017-02-03 08:16:58 UTC
Created attachment 59848 [details] [review]
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2017-02-03 08:17:02 UTC
Created attachment 59849 [details] [review]
Bug 17736: [Follow-up] Rename to current_holds

It is not about when the hold was 'placed' but if the hold pertains to
the future or not.

Test plan:
[1] Git grep on holds_placed_before_today.
[2] Run t/db_dependent/Koha/Biblios.t
[3] Run t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2017-02-03 08:17:06 UTC
Created attachment 59850 [details] [review]
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors

Like:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.

Resolved by not importing them but fully qualifying them.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2017-02-03 08:17:10 UTC
Created attachment 59851 [details] [review]
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t

FAIL   t/db_dependent/Holds.t
"my" variable $hold masks earlier declaration in same scope

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2017-02-03 08:17:15 UTC
Created attachment 59852 [details] [review]
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2017-02-03 08:17:19 UTC
Created attachment 59853 [details] [review]
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl

Crash in basket.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/basket.pl line 466.
Comes from a biblionumber == NULL in aqorders where I cancelled the order and deleted the biblio.

Crash in parcel.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/parcel.pl line 246.
Similar fix.

Trivial fixes indeed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2017-02-03 08:17:23 UTC
Created attachment 59854 [details] [review]
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui

The solution of Jonathan can be applied in two other cases, effectively
making GetItemHolds obsolete.

Test plan:
[1] Git grep on GetItemHolds
[2] Add an order, place a hold, delete order.
[3] Add an order, receive, place hold, delete order.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2017-02-03 08:17:27 UTC
Created attachment 59855 [details] [review]
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.

Primarily caused by sub edi_close_and_order.

Easy fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2017-02-03 08:18:10 UTC
trivial rebase on c4/biblio; giving 17737 a try
Comment 47 Jonathan Druart 2017-02-03 09:45:59 UTC
Created attachment 59859 [details] [review]
Bug 17736: Fix typo ->new vs ->next

while ->new is terrible, let's call that a typo...
Comment 48 Marcel de Rooy 2017-02-03 09:48:02 UTC
Created attachment 59860 [details] [review]
Bug 17736: Fix typo ->new vs ->next

while ->new is terrible, let's call that a typo...

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
opac-detail works again !
Comment 49 Martin Renvoize 2017-03-21 19:31:50 UTC
Created attachment 61406 [details] [review]
Bug 17736: Add the Koha::Biblio->holds_placed_before_today method

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2017-03-21 19:31:56 UTC
Created attachment 61407 [details] [review]
Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.

But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR

- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.

- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today

Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2017-03-21 19:32:01 UTC
Created attachment 61408 [details] [review]
Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2017-03-21 19:32:07 UTC
Created attachment 61409 [details] [review]
Bug 17736: [Follow-up] Rename to current_holds

It is not about when the hold was 'placed' but if the hold pertains to
the future or not.

Test plan:
[1] Git grep on holds_placed_before_today.
[2] Run t/db_dependent/Koha/Biblios.t
[3] Run t/db_dependent/Reserves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2017-03-21 19:32:17 UTC
Created attachment 61410 [details] [review]
Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors

Like:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.

Resolved by not importing them but fully qualifying them.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2017-03-21 19:32:22 UTC
Created attachment 61411 [details] [review]
Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t

FAIL   t/db_dependent/Holds.t
"my" variable $hold masks earlier declaration in same scope

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2017-03-21 19:32:28 UTC
Created attachment 61412 [details] [review]
Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2017-03-21 19:32:33 UTC
Created attachment 61413 [details] [review]
Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl

Crash in basket.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/basket.pl line 466.
Comes from a biblionumber == NULL in aqorders where I cancelled the order and deleted the biblio.

Crash in parcel.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/parcel.pl line 246.
Similar fix.

Trivial fixes indeed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2017-03-21 19:32:38 UTC
Created attachment 61414 [details] [review]
Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui

The solution of Jonathan can be applied in two other cases, effectively
making GetItemHolds obsolete.

Test plan:
[1] Git grep on GetItemHolds
[2] Add an order, place a hold, delete order.
[3] Add an order, receive, place hold, delete order.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2017-03-21 19:32:44 UTC
Created attachment 61415 [details] [review]
Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.

Primarily caused by sub edi_close_and_order.

Easy fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Martin Renvoize 2017-03-21 19:32:51 UTC
Created attachment 61416 [details] [review]
Bug 17736: Fix typo ->new vs ->next

while ->new is terrible, let's call that a typo...

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
opac-detail works again !

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Martin Renvoize 2017-03-21 19:33:57 UTC
Tested pretty thoroughly and is looking solid to me. Thanks for the efforys Jonathan and for the thorough testing Marcel.

Passing QA!
Comment 61 Kyle M Hall 2017-03-31 12:45:21 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 62 Katrin Fischer 2017-04-02 16:58:00 UTC
This won't get ported back to 16.11.x as it is an enhancement.