Comment on attachment 40035 [details] [review]
[SIGNED-OFF] Bug 13918 - Add waiting expiration date to opac list of holds for user

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

::: Koha/Biblio.pm
@@ +43,5 @@
> +
> +sub subtitle {
> +    my ( $self ) = @_;
> +
> +    return GetRecordValue( 'subtitle', GetMarcBiblio( $self->id() ), GetFrameworkCode( $self->id() ) );

Shouldn't we provide a more efficient way to retrieve subtitle here?
Moreover the frameworkcode could be retrieved with self->frameworkcode.

::: Koha/Hold.pm
@@ +96,5 @@
> +Returns true if hold is a cancelable hold
> +
> +=cut
> +
> +sub is_cancelable {

tests are missing for this subroutine.

@@ +122,5 @@
> +Returns true if hold is a in_transit hold
> +
> +=cut
> +
> +sub is_at_destination {

tests are missing for this subroutine.

::: opac/opac-user.pl
@@ +281,4 @@
>  
> +$template->param(
> +    RESERVES       => $reserves,
> +    reserves_count => $reserves->count(),

Is it still needed?
We can access to the number of reserves from the template.

A bit late to be sure there is no regression, but everything looks good.

Just some minor notes:

> t/db_dependent/BiblioObject.t

I would prefer a better filename, but nothing in mind to suggest.

> +my $field_mappings = Koha::Database->new()->schema()->resultset('Fieldmapping');

Could be better to have a Koha::Fieldmapping[s]

> -    waiting_count      => $wcount,

This variable is still used in the template.

> +    WAITING        => $reserves->waiting()

Why not remove this one and access to the waiting holds using RESERVES.waiting in the template?

Kyle, what does the last patch set?

(In reply to Jonathan Druart from comment #15)
> Kyle, what does the last patch set?

I'm not sure what you mean. Can you explain?

Thanks!

(In reply to Kyle M Hall from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > Kyle, what does the last patch set?
> 
> I'm not sure what you mean. Can you explain?

(Maybe a do is missing)

What does the last patch set do?

You have answered my comment 10 by a new patch set, but I don't want to compare the 2 patch sets (before and after my comment) to know if you have taken into account my comments.

Thanks, I understand what you were asking now.
> > t/db_dependent/BiblioObject.t
> 
> I would prefer a better filename, but nothing in mind to suggest.

I feel the same way, but have nothing better to suggest either.

> > +my $field_mappings = Koha::Database->new()->schema()->resultset('Fieldmapping');
> 
> Could be better to have a Koha::Fieldmapping[s]

I can do this if you'd like. It was just a quick way to create a field mapping, so I went with dbic directly. Just let me know if you want me to go ahead an add a followup to add and use Koha::Fieldmapping[s]



> 
> > -    waiting_count      => $wcount,
> 
> This variable is still used in the template.
> 
> > +    WAITING        => $reserves->waiting()
> 
> Why not remove this one and access to the waiting holds using
> RESERVES.waiting in the template?

I'm working on these ones right now.

Question, I see in the code that there is a "Waiting" tab for only waiting holds, yet I don't see it in the opac ( while on master ). Is this dead code?

(In reply to Kyle M Hall from comment #18)
> Question, I see in the code that there is a "Waiting" tab for only waiting
> holds, yet I don't see it in the opac ( while on master ). Is this dead code?

It looks like more than a bug :)

(In reply to Jonathan Druart from comment #19)
> (In reply to Kyle M Hall from comment #18)
> > Question, I see in the code that there is a "Waiting" tab for only waiting
> > holds, yet I don't see it in the opac ( while on master ). Is this dead code?
> 
> It looks like more than a bug :)

After a deep dive into the code, it seems broken for a while!

(In reply to Jonathan Druart from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > (In reply to Kyle M Hall from comment #18)
> > > Question, I see in the code that there is a "Waiting" tab for only waiting
> > > holds, yet I don't see it in the opac ( while on master ). Is this dead code?
> > 
> > It looks like more than a bug :)
> 
> After a deep dive into the code, it seems broken for a while!

Thanks for looking into that! Any idea how long a while has been? Any idea what the last major version of Koha it worked in was? It seems that maybe it should be considered dead code since nobody has ever noticed, complained, or filed a bug related to it! I did look for a related bug submission, but could find none.

This is also rather superfluous, as the waiting holds are already in the holds tab. There's really no need for yet another tab that lists only the waiting holds.

I would like to remove the dead code, what do you think?

"For a while" meant " I didn't find any versions where it works.

If you check out the commit before "Bug 5917 : Swapping templates over" (d4cd05126da3accef83144d8322fcd455d7eba9b), you will see the same structure in the TT:

128             <!-- TMPL_IF NAME="waiting_count" --><!-- TMPL_IF NAME="atdestination" --><li><a href="/cgi-bin/koha/opac-user.pl#opac-user-waiting">Waiting</a></li><!-- /TMPL_IF --><!-- /TM    PL_IF -->


But no track of a atdestination variable passed to the template.

So if this is correct, I am sure nobody will care if we remove this tab :)

(In reply to Kyle M Hall from comment #18)
> Thanks, I understand what you were asking now.
> > > t/db_dependent/BiblioObject.t
> > 
> > I would prefer a better filename, but nothing in mind to suggest.
> 
> I feel the same way, but have nothing better to suggest either.
> 
> > > +my $field_mappings = Koha::Database->new()->schema()->resultset('Fieldmapping');
> > 
> > Could be better to have a Koha::Fieldmapping[s]
> 
> I can do this if you'd like. It was just a quick way to create a field
> mapping, so I went with dbic directly. Just let me know if you want me to go
> ahead an add a followup to add and use Koha::Fieldmapping[s]

As you like.

> > > -    waiting_count      => $wcount,
> > 
> > This variable is still used in the template.
> > 
> > > +    WAITING        => $reserves->waiting()
> > 
> > Why not remove this one and access to the waiting holds using
> > RESERVES.waiting in the template?
> 
> I'm working on these ones right now.

So setting the status accordingly.

Excellent! Thanks for looking into that Jonathan. I'll pull that code out and get everything fixed up!

Patch failed at 0001 Bug 13918 - Add waiting expiration date to opac list of holds for user

Kyle,
They now apply but I think you forgot to apply 13517 before rebasing the patches.
For instance, the first patch redefines the is_in_transit method.

(In reply to Jonathan Druart from comment #36)
> Kyle,
> They now apply but I think you forgot to apply 13517 before rebasing the
> patches.
> For instance, the first patch redefines the is_in_transit method.

I did apply 13517 first, but it looks like a screwed up the merge conflict fix. Patch updated!

CONFLICT (content): Merge conflict in opac/opac-user.pl

Software error:

Global symbol "$wcount" requires explicit package name at /home/koha/src/opac/opac-user.pl line 323.
Execution of /home/koha/src/opac/opac-user.pl aborted due to compilation errors.

Pushed to master!

Created attachment 46130 [details] [review]
Bug 13918 - Add waiting expiration date to opac list of holds for user

Waiting holds for patrons in the opac should display the hold expiration
date based on the max pickup delay if it is set.

Test Plan:
1) Ensure ReservesMaxPickUpDelay is set
2) Place a hold on a record
3) Check in the item, use it to fill the hold
4) Log into the opac as that user
5) View the holds list, note the 'until <date>' addition to the
   waiting hold line.

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@bugs.koha-community.org>

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

Created attachment 46131 [details] [review]
Bug 13918 [QA Followup] - Unit Tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 46132 [details] [review]
Bug 13918 [QA Followup] - Remove reserves_count

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 46133 [details] [review]
Bug 13918 [QA Followup] - Improve $biblio->subtitles()

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 46134 [details] [review]
Bug 13918 [QA Followup] - Remove dead template code

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 46135 [details] [review]
Bug 13918 [QA Followup] - Remove unnecessary reference to $wcount

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Kyle, since 
  commit 3ff76f3357fe8cdd6bf28384171dfdeb32a82d95
  Bug 13918 - Add waiting expiration date to opac list of holds for user

We now get:
  No method wbrname! at /home/koha/src/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line 603.
  No method wbrcd! at /home/koha/src/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line 603.

(In reply to Jonathan Druart from comment #64)
> Kyle, since 
>   commit 3ff76f3357fe8cdd6bf28384171dfdeb32a82d95
>   Bug 13918 - Add waiting expiration date to opac list of holds for user
> 
> We now get:
>   No method wbrname! at
> /home/koha/src/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line
> 603.
>   No method wbrcd! at
> /home/koha/src/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line
> 603.

See bug 16680 for a patch.