Bug 16686 - Fix "Item in transit from since" in Holds tab
Summary: Fix "Item in transit from since" in Holds tab
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 13918
Blocks: 24295
  Show dependency treegraph
 
Reported: 2016-06-07 14:55 UTC by Marc Véron
Modified: 2020-09-17 15:06 UTC (History)
5 users (show)

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


Attachments
Bug 16686 - Fix "Item in transit from since" in Holds tab (2.75 KB, patch)
2016-06-07 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16686 - Fix "Item in transit from since" in Holds tab (4.01 KB, patch)
2016-06-07 16:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16686 - Fix "Item in transit from since" in Holds tab (4.01 KB, patch)
2016-06-07 16:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16686 - Fix "Item in transit from since" in Holds tab (3.54 KB, patch)
2016-06-08 11:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16686 - Fix "Item in transit from since" in Holds tab (5.99 KB, patch)
2016-06-27 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16686 - Fix "Item in transit from since" in Holds tab (6.17 KB, patch)
2016-07-03 12:19 UTC, Marc Véron
Details | Diff | Splinter Review
[ALT] Bug 16686: Fix "Item in transit from since" in Holds tab (2.11 KB, patch)
2016-07-03 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16686: Fix "Item in transit from since" in Holds tab (2.28 KB, patch)
2016-07-04 14:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16686: Rename the transfer method to get_transfer (2.47 KB, patch)
2016-07-07 17:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16686: Add Koha::Object based class for Koha::Item::Transfer[s] (2.96 KB, patch)
2016-07-07 17:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer (6.33 KB, patch)
2016-07-07 17:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer (6.50 KB, patch)
2016-07-08 15:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16686: Fix "Item in transit from since" in Holds tab (2.39 KB, patch)
2016-08-07 19:55 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16686: Rename the transfer method to get_transfer (2.59 KB, patch)
2016-08-07 19:56 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16686: Add Koha::Object based class for Koha::Item::Transfer[s] (3.00 KB, patch)
2016-08-07 19:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer (6.54 KB, patch)
2016-08-07 19:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16686: Fix "Item in transit from since" in Holds tab (2.44 KB, patch)
2016-08-12 17:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16686: Rename the transfer method to get_transfer (2.64 KB, patch)
2016-08-12 17:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16686: Add Koha::Object based class for Koha::Item::Transfer[s] (3.05 KB, patch)
2016-08-12 17:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer (6.59 KB, patch)
2016-08-12 17:49 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2016-06-07 14:55:46 UTC
"From branch" and date do not show up in patron's "Holds" tab: "Item in transit from since"

OPAC log:  opac-user.pl: No method frombranch! at /usr/share/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt line 607

This is related to Bug 13918 (see comment #3 in Bug 16680)
Comment 1 Kyle M Hall 2016-06-07 15:50:54 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-06-07 15:51:10 UTC
New methods should probably have unit tests.
Comment 3 Kyle M Hall 2016-06-07 16:10:13 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2016-06-07 16:10:47 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-06-07 19:49:25 UTC
With patch I get a lot of errors (even on OPAC main page or on Staff login page):

syntax error at /usr/share/kohaclone/Koha/Item.pm line 101, near "unless defined"
syntax error at /usr/share/kohaclone/Koha/Item.pm line 106, near "}"
Global symbol "$self" requires explicit package name at /usr/share/kohaclone/Koha/Item.pm line 108.
Can't use global @_ in "my" at /usr/share/kohaclone/Koha/Item.pm line 117, near "= @_"
Global symbol "$self" requires explicit package name at /usr/share/kohaclone/Koha/Item.pm line 119.
Global symbol "$self" requires explicit package name at /usr/share/kohaclone/Koha/Item.pm line 119.
syntax error at /usr/share/kohaclone/Koha/Item.pm line 130, near "}"
Can't use global @_ in "my" at /usr/share/kohaclone/Koha/Item.pm line 139, near "= @_"
syntax error at /usr/share/kohaclone/Koha/Item.pm line 144, near "}"
Can't use global @_ in "my" at /usr/share/kohaclone/Koha/Item.pm line 153, near "= @_"
/usr/share/kohaclone/Koha/Item.pm has too many errors.
Comment 6 Kyle M Hall 2016-06-08 11:07:27 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2016-06-08 11:09:26 UTC
(In reply to Marc Véron from comment #5)
> With patch I get a lot of errors (even on OPAC main page or on Staff login
> page):

It looks like some unwanted additions got merged into my patch. Please try this new version! Thanks!
Comment 8 Marc Véron 2016-06-08 14:23:42 UTC
Now, if I log in to the OPAC with the user who triggers the transit-message, I get:
Template process failed: undef error - Can't call method "_columns" without a package or object reference at /usr/share/kohaclone/Koha/Object.pm line 266.


OPAC log:

[Wed Jun 08 16:13:10 2016] [error] [client 192.168.1.20] [Wed Jun  8 16:13:10 2016] opac-user.pl: The given value does not seem to be interpretable as a Boolean value at /usr/share/kohaclone/C4/Context.pm line 545, referer: http://192.168.1.35/cgi-bin/koha/opac-main.pl?logout.x=1

[Wed Jun 08 16:13:11 2016] [error] [client 192.168.1.20] [Wed Jun  8 16:13:11 2016] opac-user.pl: Use of inherited AUTOLOAD for non-method Koha::Item::GetTransfers() is deprecated at /usr/share/kohaclone/Koha/Item.pm line 123., referer: http://192.168.1.35/cgi-bin/koha/opac-main.pl?logout.x=1

[Wed Jun 08 16:13:11 2016] [error] [client 192.168.1.20] [Wed Jun  8 16:13:11 2016] opac-user.pl: Template process failed: undef error - Can't call method "_columns" without a package or object reference at /usr/share/kohaclone/Koha/Object.pm line 266., referer: http://192.168.1.35/cgi-bin/koha/opac-main.pl?logout.x=1
Comment 9 Marc Véron 2016-06-17 13:33:22 UTC
Re-tested. I can no longer log-in to the OPAC with a patron who has the hold on an item in transit.

[Fri Jun 17 14:16:40 2016] [error] [client 192.168.1.35] [Fri Jun 17 14:16:40 2016] opac-user.pl: Use of inherited AUTOLOAD for non-method Koha::Item::GetTransfers() is deprecated at /usr/share/kohaclone/Koha/Item.pm line 123., referer: http://192.168.1.35/
[Fri Jun 17 14:16:40 2016] [error] [client 192.168.1.35] [Fri Jun 17 14:16:40 2016] opac-user.pl: Template process failed: undef error - Can't call method "_columns" without a package or object reference at /usr/share/kohaclone/Koha/Object.pm line 266., referer: http://192.168.1.35/

Without patch, I can log in with this patron.
Comment 10 Katrin Fischer 2016-06-19 13:51:49 UTC
Ran into this too - please fix!
Comment 11 Kyle M Hall 2016-06-27 15:12:24 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2016-07-03 12:19:09 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-07-03 14:34:07 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-07-03 14:35:59 UTC
Kyle, could you have a look at this counter-patch?
It seems easier to use the DBIC relationship than to create 3 new methods.
Comment 15 Marc Véron 2016-07-04 14:07:51 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2016-07-07 14:22:08 UTC
(In reply to Jonathan Druart from comment #14)
> Kyle, could you have a look at this counter-patch?
> It seems easier to use the DBIC relationship than to create 3 new methods.

While I agree, I think it adds confusion. The method name 'transfer' is a verb, so it sounds like you would call $item->transfer() to initiate a transfer. Can you change the method name to get_latest_transfer or something like that?

I'm also a bit reticent about having a Koha Object return a DBIC object. Would you create Koha::Item::Transfer(s) and return that? It would also avoid a second db query just to get the branch name.
Comment 17 Jonathan Druart 2016-07-07 17:21:26 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-07-07 17:21:32 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-07-07 17:21:36 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-07-07 17:23:26 UTC
(In reply to Kyle M Hall from comment #16)
> (In reply to Jonathan Druart from comment #14)
> > Kyle, could you have a look at this counter-patch?
> > It seems easier to use the DBIC relationship than to create 3 new methods.
> 
> While I agree, I think it adds confusion. The method name 'transfer' is a
> verb, so it sounds like you would call $item->transfer() to initiate a
> transfer. Can you change the method name to get_latest_transfer or something
> like that?

Indeed, good idea!
I have chosen "get_transfer" which returns the current transfer, or undef if the item is not in transit.

> I'm also a bit reticent about having a Koha Object return a DBIC object.
> Would you create Koha::Item::Transfer(s) and return that? It would also
> avoid a second db query just to get the branch name.

Done, but I am not sure to understand how it will avoid the second query. I now use the Branches TT plugin to retrieve the branch name.
Comment 21 Marc Véron 2016-07-08 12:22:44 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 53192 [details] [review] [review]
> Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer

Hi Jonathan, I think there is a typo in the file name: Tranfers.t (schould be Transfers.t)
If I run the test, I get a syntax error on line 37 (similar typo) and other errors.
Comment 22 Jonathan Druart 2016-07-08 15:11:07 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-07-08 15:11:41 UTC
(In reply to Marc Véron from comment #21)
> (In reply to Jonathan Druart from comment #19)
> > Created attachment 53192 [details] [review] [review] [review]
> > Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer
> 
> Hi Jonathan, I think there is a typo in the file name: Tranfers.t (schould
> be Transfers.t)
> If I run the test, I get a syntax error on line 37 (similar typo) and other
> errors.

Oops, sorry about that. I did not finish to write these tests!
Comment 24 Marc Véron 2016-08-07 19:50:29 UTC
Test passes now.
Comment 25 Marc Véron 2016-08-07 19:55:06 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2016-08-07 19:56:18 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2016-08-07 19:57:19 UTC Comment hidden (obsolete)
Comment 28 Marc Véron 2016-08-07 19:58:06 UTC Comment hidden (obsolete)
Comment 29 Nick Clemens 2016-08-12 17:48:49 UTC
Created attachment 54392 [details] [review]
Bug 16686: Fix "Item in transit from since" in Holds tab

POC, tests needed.

Alternative patch works for me.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens 2016-08-12 17:48:54 UTC
Created attachment 54393 [details] [review]
Bug 16686: Rename the transfer method to get_transfer

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens 2016-08-12 17:49:03 UTC
Created attachment 54394 [details] [review]
Bug 16686: Add Koha::Object based class for Koha::Item::Transfer[s]

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Nick Clemens 2016-08-12 17:49:09 UTC
Created attachment 54395 [details] [review]
Bug 16686: Add test for Koha::Item::Transfer[s] and Koha::Item->get_transfer

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Kyle M Hall 2016-08-18 15:47:09 UTC
Items.t fails if SearchEngine is set to Elastic:

prove t/db_dependent/Koha/Items.t
t/db_dependent/Koha/Items.t .. 1/5  problem with :564 : :1: parser error : Start tag expected, '<' not found
tAw
^

tAw at /home/vagrant/kohaclone/C4/Biblio.pm line 1339.
    # Looks like you planned 3 tests but ran 1.
Can't call method "field" on an undefined value at /home/vagrant/kohaclone/Koha/ElasticSearch/Indexer.pm line 180.
# Looks like your test exited with 255 just after 4.
t/db_dependent/Koha/Items.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/5 subtests

Test Summary Report
-------------------
t/db_dependent/Koha/Items.t (Wstat: 65280 Tests: 4 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 5 tests but ran 4.
Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.30 cusr  0.13 csys =  1.45 CPU)
Result: FAIL
Comment 34 Kyle M Hall 2016-08-18 16:32:09 UTC
Found the bug, unrelated. Fixed on followup to bug 16272.
Comment 35 Kyle M Hall 2016-09-02 14:55:47 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 36 Frédéric Demians 2016-09-08 19:26:47 UTC
Pushed in 16.05. Will be in 16.05.04.