Bug 12892 - Holds Waiting: not showing from check out screen
Summary: Holds Waiting: not showing from check out screen
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
: 13117 (view as bug list)
Depends on: 11703
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-08 19:24 UTC by Jesse Maseto
Modified: 2015-12-03 22:02 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Screen Shot from 3.14 (149.51 KB, image/png)
2014-09-08 19:24 UTC, Jesse Maseto
Details
Screen Shot from 3.16 (267.76 KB, image/png)
2014-09-08 19:25 UTC, Jesse Maseto
Details
Bug 12892 - Holds Waiting: not showing from check out screen (6.88 KB, patch)
2014-09-14 10:29 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (5.59 KB, patch)
2014-09-15 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (5.61 KB, patch)
2014-09-15 12:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Unit Tests (1.51 KB, patch)
2014-09-15 12:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (5.79 KB, patch)
2014-09-15 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Unit Tests (1.51 KB, patch)
2014-09-15 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12892 - Holds Waiting: not showing from check out screen (5.72 KB, patch)
2014-09-15 13:10 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12892 [QA Followup] - Encode printed vars as UTF8 (1.85 KB, patch)
2014-09-15 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug12892_multiple_waitingholds_utf8_in_branchname.png (159.35 KB, image/png)
2014-09-17 11:51 UTC, Olli-Antti Kivilahti
Details
Bug 12892 [QA Followup] - Encode printed vars as UTF8 (2.07 KB, patch)
2014-09-17 11:54 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 12892 [QA Followup] - Encode printed vars as UTF8 (2.53 KB, patch)
2014-09-17 12:02 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 12892 - Add Koha::Object, Koha::Objects and unit tests (19.69 KB, patch)
2014-10-01 09:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (17.75 KB, patch)
2014-10-01 09:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (17.76 KB, patch)
2014-10-06 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12892 - Holds Waiting: not showing from check out screen (17.82 KB, patch)
2014-10-06 17:07 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12892 - Holds Waiting: not showing from check out screen (17.83 KB, patch)
2014-10-07 15:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (17.83 KB, patch)
2014-10-08 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (17.83 KB, patch)
2014-10-08 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 - Holds Waiting: not showing from check out screen (17.66 KB, patch)
2014-10-08 16:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12892 : Alternate fix to restore the deleted code (2.72 KB, patch)
2014-11-05 07:22 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12892 : Alternate fix to restore the deleted code (2.79 KB, patch)
2014-11-05 08:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12892 : Alternate fix to restore the deleted code (2.83 KB, patch)
2014-11-05 08:59 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
[PASSED QA] Bug 12892 : Alternate fix to restore the deleted code (2.96 KB, patch)
2014-11-05 21:28 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Maseto 2014-09-08 19:24:43 UTC Comment hidden (obsolete)
Comment 1 Jesse Maseto 2014-09-08 19:25:07 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2014-09-14 10:29:31 UTC Comment hidden (obsolete)
Comment 3 Jesse Maseto 2014-09-14 14:58:36 UTC
I am not able to apply this patch.
==================================

Bug 12892 - Holds Waiting: not showing from check out screen

31581 - Bug 12892 - Holds Waiting: not showing from check out  screen

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12892 - Holds Waiting: not showing from check out screen
fatal: sha1 information is lacking or useless (Koha/Reserves.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12892 - Holds Waiting: not showing from check out screen
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12892---Holds-Waiting-not-showing-from-check-o-g6Guwy.patch

==================================
Comment 4 Katrin Fischer 2014-09-14 18:20:24 UTC
Hi Olli-Antti,
please submit your patches against master if the problem exists there too, this will make it much easier to get them tested and integrated. 

I have verifed this problem also appears on master.
Comment 5 Olli-Antti Kivilahti 2014-09-15 08:25:55 UTC
Thank you Katrin and Jesse.

This feature needs the Koha/Reserves.pm -file to be introduced to the git which happens in Bug 12917
Comment 6 Olli-Antti Kivilahti 2014-09-15 08:26:59 UTC
It prolly still fails to merge. I'll take a look into this when I have more time ;)
Comment 7 Katrin Fischer 2014-09-15 08:27:30 UTC
I tried with the dependency - it didn't apply yesterday.
Comment 8 Kyle M Hall 2014-09-15 12:27:34 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-09-15 12:31:41 UTC
There seems to be some urgency to this issue. Due to that, I've submitted an alternate solution. I hope that's ok!

(In reply to Olli-Antti Kivilahti from comment #6)
> It prolly still fails to merge. I'll take a look into this when I have more
> time ;)
Comment 10 Kyle M Hall 2014-09-15 12:36:59 UTC Comment hidden (obsolete)
Comment 11 Olli-Antti Kivilahti 2014-09-15 12:39:38 UTC
No problem, just sharing my code in hopes of it being helpful.
I also like your solution :)
Comment 12 Kyle M Hall 2014-09-15 12:46:30 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-09-15 13:01:25 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-09-15 13:01:34 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2014-09-15 13:10:42 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-09-15 13:15:40 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2014-09-15 13:20:29 UTC
Hm, not sure if the last patch needs to be undone when we push bug 11944 - creating a link.
Comment 18 Jonathan Druart 2014-09-15 13:39:59 UTC
This patch uses 2 "new" technical concepts:
- new Koha::Schema::ResultSet namespace
- use of Koha::Schema in pl file

They don't have been voted by the Koha dev community.
Note sure they should be introduced to fix a major bug.

I let someone else QA this patch but, IMO, it should not be pushed as it.
Comment 19 Katrin Fischer 2014-09-15 14:29:36 UTC
I can understand Jonathan's point and not sure how to proceed here - we got a dev meeting tomorrow - should we discuss this?
Comment 20 Olli-Antti Kivilahti 2014-09-17 11:51:00 UTC Comment hidden (obsolete)
Comment 21 Olli-Antti Kivilahti 2014-09-17 11:54:17 UTC Comment hidden (obsolete)
Comment 22 Olli-Antti Kivilahti 2014-09-17 12:02:41 UTC Comment hidden (obsolete)
Comment 23 Olli-Antti Kivilahti 2014-09-17 12:05:29 UTC
I also think it sucks super badly to put 

  [% VALUE | $EncodeUTF8 | html %]

to the templates. It is so wrong. As if the tempates aren't cluttered enough.
Comment 24 Kyle M Hall 2014-10-01 09:44:22 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-10-01 09:44:32 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2014-10-01 11:38:40 UTC
Comment on attachment 31962 [details] [review]
Bug 12892 - Add Koha::Object, Koha::Objects and unit tests

Moving the base class patch to it's own bug. Bug 13019.
Comment 27 Nick Clemens 2014-10-04 21:03:15 UTC
Applying: Bug 12892 - Holds Waiting: not showing from check out screen
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Reserves.t
CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging circ/circulation.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 12892 - Holds Waiting: not showing from check out screen
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12892---Holds-Waiting-not-showing-from-check-o-xSl9aQ.patch
Comment 28 Owen Leonard 2014-10-05 00:19:43 UTC
Did you apply the dependencies first?
Comment 29 Nick Clemens 2014-10-05 00:54:13 UTC
SMH, no

I got caught up reading through the discussion and forgot to go back up and note them.  

I can't apply 11518 so can't apply 11703 and so can't apply this, will retreat for simpler bugs
Comment 30 Kyle M Hall 2014-10-06 13:06:56 UTC Comment hidden (obsolete)
Comment 31 Nick Clemens 2014-10-06 17:07:33 UTC Comment hidden (obsolete)
Comment 32 Barton Chittenden 2014-10-06 18:02:11 UTC
There is an analogous problem on check-in. See bug 13033.
Comment 33 Kyle M Hall 2014-10-07 15:32:00 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2014-10-08 11:40:58 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2014-10-08 14:09:52 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2014-10-08 16:42:54 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2014-10-11 15:36:59 UTC
Since this is a major one, it could be good to have a fix not using bug 13019.
Comment 38 Rafal Kopaczka 2014-10-21 08:28:25 UTC
*** Bug 13117 has been marked as a duplicate of this bug. ***
Comment 39 Martin Renvoize 2014-10-23 11:59:37 UTC
I have been playing around allot with bug 13019 lately and my general feeling is that it's pretty workable already and a viable route to using proper objects in Koha. Whilst it has allot of further enhancements required to make it 'really' useful, it's in a good state as is for a first step.

I still feel these objects kinda of lock away allot of the advantages of dbic behind an abstraction that sometimes detracts rather than enhances; but with our debates going on and on I'de rather see Koha moving forward than stagnating in debate territory.

My real feeling after all this is that we're doing way too much logical work in our .pl's.. if it's logic based at all, it really belongs in a .pm where it can be fully tested ;-) !

With the above in mind, I would be happy to pass QA on this bug.. and sign off on bug 13019 on which it relies.

Is anyone watching this conversation able to also take a look at 13019 with either signoff or QA in mind as I obviously can't do both roles ;)
Comment 40 Katrin Fischer 2014-10-26 20:41:47 UTC
We urgently need to get bug 13019 moving in order to resolve this - please?
Comment 41 Katrin Fischer 2014-11-04 20:00:22 UTC
This feels like an evil bug stuck in a discussion about architecture - could we have a small and easy fix bringing the old functionality back and figure out the other stuff separately?
Comment 42 Chris Cormack 2014-11-05 07:22:15 UTC Comment hidden (obsolete)
Comment 43 Chris Cormack 2014-11-05 07:23:20 UTC
I have attached an alternate patch, which restores the code that was deleted when the ajax checkouts were added. 

This is not dependent on other patches.
Comment 44 Martin Renvoize 2014-11-05 08:38:06 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2014-11-05 08:43:22 UTC
Thanks for stepping into the breach Chris... alternate patch works as expected for me.. signing off.
Comment 46 Rafal Kopaczka 2014-11-05 08:59:28 UTC Comment hidden (obsolete)
Comment 47 Rafal Kopaczka 2014-11-05 09:01:23 UTC
This patch also fix bug 12971, and maybe some other bugs.
Comment 48 Rafal Kopaczka 2014-11-05 09:09:22 UTC
(In reply to Rafal Kopaczka from comment #47)
> This patch also fix bug 12971, and maybe some other bugs.

Nope, sorry this does not fix that issue. But, still this is good patch :)
Comment 49 Katrin Fischer 2014-11-05 21:28:41 UTC
Created attachment 33275 [details] [review]
[PASSED QA] Bug 12892 : Alternate fix to restore the deleted code

This doesn't depend on anything other patch.

1. Create a reservation for a Borrower with pickup branch
2. Check-in the corresponding Item in the pickup branch and confirm the hold
   for the Borrower
3. Go to circ/circulation.pl for the Borrower and verify that the reservation
   is "Waiting for pickup"
4. In the Borrower -> Check Out -tab (circ/circulation.pl) on the right of the
   big "Checking out to borrowername" should be a notification about holds ready
   for pickup.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Rafal Kopaczka <rkk0@poczta.onet.pl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script, restores the old code and behaviour.
Comment 50 Tomás Cohen Arazi 2014-11-06 13:28:56 UTC
Patch pushed to master.

Thanks Chris!
Comment 51 Mason James 2015-01-19 14:16:05 UTC
Pushed to 3.16.x, will be in 3.16.7
Comment 52 Fridolin SOMERS 2015-01-21 09:47:36 UTC
I set to pushed to stable since its in 3.16.x