Bug 13918

Summary: Add waiting expiration date to opac list of holds for user
Product: Koha Reporter: Kyle M Hall <kyle>
Component: OPACAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: amy, jonathan.druart, kyle.m.hall, nicole, techservspec
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13517    
Bug Blocks: 14310, 16680, 16686    
Attachments: Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 - Add waiting expiration date to opac list of holds for user
[SIGNED-OFF] Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup]
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 [QA Followup] - Remove unnecessary reference to $wcount
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 [QA Followup] - Remove unnecessary reference to $wcount
Bug 13918 - Add waiting expiration date to opac list of holds for user
Bug 13918 [QA Followup] - Unit Tests
Bug 13918 [QA Followup] - Remove reserves_count
Bug 13918 [QA Followup] - Improve $biblio->subtitles()
Bug 13918 [QA Followup] - Remove dead template code
Bug 13918 [QA Followup] - Remove unnecessary reference to $wcount

Description Kyle M Hall 2015-03-26 13:49:55 UTC
Waiting holds for patrons in the opac should display the hold expiration date based on the max pickup delay if it is set.
Comment 1 Kyle M Hall 2015-03-26 13:52:57 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-03-26 13:56:46 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-06-09 17:26:39 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-06-09 17:34:27 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-06-16 12:16:35 UTC
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.
Comment 6 Kyle M Hall 2015-08-24 14:48:50 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-08-24 14:56:01 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-08-24 15:12:03 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-08-24 15:36:58 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-08-25 16:11:34 UTC
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?
Comment 11 Kyle M Hall 2015-08-25 16:48:22 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-08-25 16:48:29 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-08-25 16:48:31 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-08-25 16:48:34 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-08-26 13:39:42 UTC
Kyle, what does the last patch set?
Comment 16 Kyle M Hall 2015-08-27 17:55:44 UTC
(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!
Comment 17 Jonathan Druart 2015-08-28 08:18:46 UTC
(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.
Comment 18 Kyle M Hall 2015-08-28 14:16:46 UTC
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?
Comment 19 Jonathan Druart 2015-08-28 15:42:52 UTC
(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 :)
Comment 20 Jonathan Druart 2015-08-28 15:47:53 UTC
(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!
Comment 21 Kyle M Hall 2015-08-31 12:05:54 UTC
(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?
Comment 22 Jonathan Druart 2015-09-01 07:37:15 UTC
"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 :)
Comment 23 Jonathan Druart 2015-09-03 10:35:38 UTC
(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.
Comment 24 Kyle M Hall 2015-09-03 13:12:56 UTC
Excellent! Thanks for looking into that Jonathan. I'll pull that code out and get everything fixed up!
Comment 25 Kyle M Hall 2015-09-17 12:39:35 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-09-17 12:39:41 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-09-17 12:39:44 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-09-17 12:39:46 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-09-17 12:39:49 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-09-21 15:37:10 UTC
Patch failed at 0001 Bug 13918 - Add waiting expiration date to opac list of holds for user
Comment 31 Kyle M Hall 2015-09-21 17:18:55 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2015-09-21 17:19:05 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2015-09-21 17:19:07 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2015-09-21 17:19:11 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-09-21 17:19:13 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2015-09-22 09:23:28 UTC
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.
Comment 37 Kyle M Hall 2015-09-22 13:03:22 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-09-22 13:03:29 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2015-09-22 13:03:32 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-09-22 13:03:34 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-09-22 13:03:37 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2015-09-22 13:05:28 UTC
(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!
Comment 43 Jonathan Druart 2015-10-15 09:51:12 UTC
CONFLICT (content): Merge conflict in opac/opac-user.pl
Comment 44 Kyle M Hall 2015-10-15 12:53:24 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2015-10-15 12:53:51 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2015-10-15 12:53:54 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2015-10-15 12:53:57 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2015-10-15 12:54:00 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2015-10-15 12:59:43 UTC
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.
Comment 50 Kyle M Hall 2015-10-15 13:45:17 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2015-10-16 13:17:36 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2015-10-16 13:17:39 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2015-10-16 13:17:43 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2015-10-16 13:17:47 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2015-10-16 13:17:50 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2015-10-16 13:17:54 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2015-12-31 12:44:51 UTC
Pushed to master!
Comment 58 Kyle M Hall 2015-12-31 12:45:16 UTC
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>
Comment 59 Kyle M Hall 2015-12-31 12:45:23 UTC
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>
Comment 60 Kyle M Hall 2015-12-31 12:45:25 UTC
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>
Comment 61 Kyle M Hall 2015-12-31 12:45:28 UTC
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>
Comment 62 Kyle M Hall 2015-12-31 12:45:31 UTC
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>
Comment 63 Kyle M Hall 2015-12-31 12:45:34 UTC
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>
Comment 64 Jonathan Druart 2016-06-07 09:42:53 UTC
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.
Comment 65 Jonathan Druart 2016-06-07 09:47:34 UTC
(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.