Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects
Summary: Replace waiting holds logic in circulation.pl with Koha Objects
Status: CLOSED 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: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13030 14557 14616 15324
  Show dependency treegraph
 
Reported: 2015-03-17 13:34 UTC by Kyle M Hall
Modified: 2016-12-05 21:23 UTC (History)
5 users (show)

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


Attachments
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.13 KB, patch)
2015-03-17 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.11 KB, patch)
2015-03-27 11:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.42 KB, patch)
2015-04-20 11:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.43 KB, patch)
2015-06-01 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.43 KB, patch)
2015-06-09 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.62 KB, patch)
2015-06-09 17:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 [QA Followup] - Unit Tests (2.19 KB, patch)
2015-06-17 17:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.66 KB, patch)
2015-06-18 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13851 [QA Followup] - Unit Tests (2.25 KB, patch)
2015-06-18 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13851: Fix typo in POD (666 bytes, patch)
2015-06-18 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects (19.60 KB, patch)
2015-06-26 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851 [QA Followup] - Unit Tests (2.25 KB, patch)
2015-06-26 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13851: Fix typo in POD (674 bytes, patch)
2015-06-26 14:14 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 Kyle M Hall 2015-03-17 13:34:55 UTC
This is the original patch for bug 12892 and replaces the older style of fetching the holds data with Koha Objects.
Comment 1 Kyle M Hall 2015-03-17 13:40:45 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-03-17 13:54:09 UTC
Kyle, please fix the POD, it's incorrect in some places.
Comment 3 Jonathan Druart 2015-03-18 11:23:38 UTC
Comment on attachment 36947 [details] [review]
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects

Review of attachment 36947 [details] [review]:
-----------------------------------------------------------------

Please provide tests for new packages.

::: circ/circulation.pl
@@ +389,4 @@
>  # BUILD HTML
>  # show all reserves of this borrower, and the position of the reservation ....
>  if ($borrowernumber) {
> +    my $holds = Koha::Holds->new()->search( { borrowernumber => $borrowernumber } );

Koha::Holds->search
should be enough.
Comment 4 Kyle M Hall 2015-03-27 11:18:35 UTC Comment hidden (obsolete)
Comment 5 Jason Burds 2015-03-30 15:59:20 UTC
Kyle,

I ran through the steps on a sandbox.

1. "Waiting at" is still bold, but not different colored.
2. Selecting Hold in the circulation.pl continues to show the "Processing" box.
Comment 6 Kyle M Hall 2015-04-20 11:02:24 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-04-20 11:03:59 UTC
(In reply to Jason Burds from comment #5)
> Kyle,
> 
> I ran through the steps on a sandbox.
> 
> 1. "Waiting at" is still bold, but not different colored.

That is exactly how it should be! I apologize for not being more specific in my test plan. The only difference is the color change, the text is bold for holds both at your branch and other branches.

> 2. Selecting Hold in the circulation.pl continues to show the "Processing"
> box.

That appears to be fixed in master and was unrelated to this patch.
Comment 8 Kyle M Hall 2015-06-01 13:11:44 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-06-09 16:34:55 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-06-09 17:37:42 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-06-16 12:34:45 UTC
Comment on attachment 40039 [details] [review]
[SIGNED-OFF] Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects

Review of attachment 40039 [details] [review]:
-----------------------------------------------------------------

There is a lack of tests for new module added by this patch.

::: Koha/Branches.pm
@@ +28,5 @@
> +use base qw(Koha::Objects);
> +
> +=head1 NAME
> +
> +Koha::Branches - Koha Reserve object set class

s/Reserve/Branch

::: Koha/Hold.pm
@@ +45,5 @@
> +
> +sub biblio {
> +    my ($self) = @_;
> +
> +    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );

Why not
    return $self->{_result}->biblio;
?

@@ +59,5 @@
> +
> +sub item {
> +    my ($self) = @_;
> +
> +    $self->{_item} ||= Koha::Items->find( $self->itemnumber() );

Same as before.

@@ +73,5 @@
> +
> +sub branch {
> +    my ($self) = @_;
> +
> +    $self->{_branch} ||= Koha::Branches->find( $self->branchcode() );

Same as before.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
@@ +763,5 @@
>  
> +            [% IF ( WaitingHolds ) %]
> +                <div id="holdswaiting" class="circmessage">
> +                    <h4>Holds waiting:</h4>
> +                    [% FOREACH w IN WaitingHolds %]

WaitingHolds is an array??
You have passed
    WaitingHolds => scalar $holds->waiting()
in the pl script.
Comment 12 Kyle M Hall 2015-06-17 17:21:02 UTC
> There is a lack of tests for new module added by this patch.

I'll add some unit tests!

> ::: Koha/Hold.pm
> @@ +45,5 @@
> > +
> > +sub biblio {
> > +    my ($self) = @_;
> > +
> > +    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );
> 
> Why not
>     return $self->{_result}->biblio;
> ?

That would return a DBIC restultset and not a Koha::Biblios object.

> ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
> @@ +763,5 @@
> >  
> > +            [% IF ( WaitingHolds ) %]
> > +                <div id="holdswaiting" class="circmessage">
> > +                    <h4>Holds waiting:</h4>
> > +                    [% FOREACH w IN WaitingHolds %]
> 
> WaitingHolds is an array??
> You have passed
>     WaitingHolds => scalar $holds->waiting()
> in the pl script.

WaitingHolds is a Koha::Holds object, which Template Toolkit can iterate over.
Comment 13 Kyle M Hall 2015-06-17 17:23:10 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-06-18 08:48:22 UTC
(In reply to Kyle M Hall from comment #12)
> > There is a lack of tests for new module added by this patch.
> 
> I'll add some unit tests!

Thanks!

> > ::: Koha/Hold.pm
> > @@ +45,5 @@
> > > +
> > > +sub biblio {
> > > +    my ($self) = @_;
> > > +
> > > +    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );
> > 
> > Why not
> >     return $self->{_result}->biblio;
> > ?
> 
> That would return a DBIC restultset and not a Koha::Biblios object.

Yes, you are right, sorry.
We will have to create a method for each FK.
I don't have a better idea to suggest at the moment.

> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
> > @@ +763,5 @@
> > >  
> > > +            [% IF ( WaitingHolds ) %]
> > > +                <div id="holdswaiting" class="circmessage">
> > > +                    <h4>Holds waiting:</h4>
> > > +                    [% FOREACH w IN WaitingHolds %]
> > 
> > WaitingHolds is an array??
> > You have passed
> >     WaitingHolds => scalar $holds->waiting()
> > in the pl script.
> 
> WaitingHolds is a Koha::Holds object, which Template Toolkit can iterate
> over.

Ok, I thought you passed the number of holds, but actually it's a Koha::Holds ref.
Comment 15 Jonathan Druart 2015-06-18 08:50:47 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-06-18 08:50:51 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-06-18 08:50:55 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-06-26 14:14:41 UTC
Created attachment 40668 [details] [review]
Bug 13851 - Replace waiting holds logic in circulation.pl with Koha Objects

This is the original patch for bug 12892 and replaces the older style of
fetching the holds data with Koha Objects. It will be used as a
foundation for future features.

Test Plan:
1) Apply this patch
2) Create a hold, set to waiting
3) Browse to circulation.pl for that patron
4) Note you see the list of waiting holds
5) Switch your logged in branch to a different branch
6) Note the "Waiting at" line is no longer emphasized.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Cathi Wiggins <CWIGGINS@ci.arcadia.ca.us>

Signed-off-by: Megan Wianecki <mwianecki@mplmain.mtpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 19 Kyle M Hall 2015-06-26 14:14:48 UTC
Created attachment 40669 [details] [review]
Bug 13851 [QA Followup] - Unit Tests

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 20 Kyle M Hall 2015-06-26 14:14:51 UTC
Created attachment 40670 [details] [review]
Bug 13851: Fix typo in POD

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 21 Tomás Cohen Arazi 2015-07-29 19:31:49 UTC
Patches pushed to master.

Thanks Kyle!
Comment 22 Olli-Antti Kivilahti 2015-07-31 16:22:34 UTC
Hi there!
Tomás asked me to comment on this.

I have nothing major to complain, all looks good to me.
I spotted one optimization however:

+++ b/Koha/Hold.pm
+sub biblio {
+    my ($self) = @_;
+
+    $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() );
+
+    return $self->{_biblio};
+}

When you get the linked object like this 'Koha::Biblios->find()', you need to instantiate a new DBIx::Class::ResultSet to make the search. There is already an instantiated resultset in Koha::Hold (the caller class).
Should we instead get the linked data like this?

#The relationship is already defined in the DBIx::Schema
my $b = $self->_resultset()->biblio()
$b = $self->_new_from_dbic($b);

using the DBIx::Schema's preconfigured relationships?
Comment 23 Tomás Cohen Arazi 2015-07-31 18:49:48 UTC
(In reply to Olli-Antti Kivilahti from comment #22)
> Hi there!
> Tomás asked me to comment on this.
> 
> I have nothing major to complain, all looks good to me.
> I spotted one optimization however:

Olli, would you be kind and fill a bug report with your enhancement proposal?