Bug 8836

Summary: Resurrect Rotating Collections
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: new feature    
Priority: P5 - low CC: chris, cmurdock, dcook, gmcharlt, jean-manuel.broust, jsasse, jseppal, katrin.fischer, kyle.m.hall, magnus, nengard, nick, olli-antti.kivilahti, paul.poulain, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7981
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 9605, 11595, 12807    
Attachments: Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections - QA Followup
Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
Bug 8836 - Resurrect Rotating Collections - Followup 3 - Fix bad TT tag
Bug 8836 - Resurrect Rotating Collections - QA Followup 3
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections - QA Followup
Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
Bug 8836 - Resurrect Rotating Collections - Followup 3
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections - QA Followup
Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
Bug 8836 - Resurrect Rotating Collections - Followup 3
Bug 8836 - Resurrect Rotating Collections - Followup 3
Bug 8836 - Resurrect Rotating Collections - Followup 4 - Autofocus on barcode field
Bug 8836 - Resurrect Rotating Collections - Followup 5 - Don't transfer issued and waiting items
Bug 8836 - Resurrect Rotating Collections - Followup 5 - Link collections.colBranchcode to branches.branchcode
Bug 8836 - Resurrect Rotating Collections - Followup 5 - Link collections.colBranchcode to branches.branchcode
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections: additions
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 - Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 [QA Followup 2]
Bug 8836 [Template follow-up] Resurrect Rotating Collections
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 [QA Followup 2]
Bug 8836 [Template follow-up] Resurrect Rotating Collections
Bug 8836 [QA Followup] - Allow collection description to be optional
Bug 8836 [QA Followup] - Add confirmation dialog for deleting collections
Bug 8836 [QA Followup] - Prevent multiple collections from having the same title
Bug 8836 [QA Followup] - Sort transfer branches alphabetically
[SIGNED-OFF] Bug 8836 [QA Followup] - Allow collection description to be optional
[SIGNED-OFF] Bug 8836 [QA Followup] - Sort transfer branches alphabetically
Bug 8836 - Resurrect Rotating Collections
Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
Bug 8836 [QA Followup 2]
Bug 8836 [Template follow-up] Resurrect Rotating Collections
Bug 8836 [QA Followup] - Allow collection description to be optional
Bug 8836 [QA Followup] - Sort transfer branches alphabetically
Bug 8836 [QA Followup] - Add confirmation dialog for deleting collections
Bug 8836 [QA Followup] - Prevent multiple collections from having the same title
Bug 8836 [QA Followup] - Don't transfer in items already in transit
Bug 8836 [Alt QA Followup] - Add confirmation dialog for deleting collections
[SIGNED-OFF] Bug 8836 [QA Followup] - Prevent multiple collections from having the same title
[PASSED QA] Bug 8836 - Resurrect Rotating Collections
[PASSED QA] Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported
[PASSED QA] Bug 8836 [QA Followup 2]
[PASSED QA] Bug 8836 [Template follow-up] Resurrect Rotating Collections
[PASSED QA] Bug 8836 [QA Followup] - Allow collection description to be optional
[PASSED QA] Bug 8836 [QA Followup] - Sort transfer branches alphabetically
[PASSED QA] Bug 8836 [QA Followup] - Don't transfer in items already in transit
[PASSED QA] Bug 8836 [Alt QA Followup] - Add confirmation dialog for deleting collections
[PASSED QA] Bug 8836 [QA Followup] - Prevent multiple collections from having the same title
Bug 8836: (RM followup) fix updatedatabase.pl rebase problem
Bug 8836: Update DBIx
Bug 8836: (RM followup) unit tests should use the new API

Description Kyle M Hall 2012-09-27 14:29:47 UTC
I think it's time to bring back the Rotating Collections feature. The security issue related to it was fixed, but the feature was never re-enabled.
Comment 1 Magnus Enger 2012-09-27 14:36:36 UTC
Some relevant links:

Initial commit adding the feature in January 2010: http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=f650aad34b00da6fd44d974b04975b2987cdaaef

Fixed: Bug 6653 - [security] Rotating collections pages can be accessed without being logged in

See relevant comments at the bottom here: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=circ/returns.pl;hb=HEAD#l615
Comment 2 Kyle M Hall 2012-09-27 14:40:10 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2013-01-13 00:46:05 UTC
Apart from the security concern, the major concern is how this would play with holds initiated transfers.

So that must be tested thoroughly when testing this patch
Comment 4 Kyle M Hall 2013-01-17 16:48:12 UTC
The good news is that the rotating collections feature doesn't do anything except tell a library that the item is part of a rotating collection and that the collection is currently at the given library. So, there should be no worries about it messing with transfers.
Comment 5 Paul Poulain 2013-03-22 10:07:08 UTC
Patch tested with a sandbox, by jmbroust <jean-manuel.broust@univ-lyon2.fr>
Comment 6 Paul Poulain 2013-03-22 10:07:25 UTC Comment hidden (obsolete)
Comment 7 jmbroust 2013-03-22 10:17:38 UTC
Note that if you have syspref "automaticitemreturn" ON (DO automatic return...) and HomeOrHoldingBranchReturn turned to : "the library the item is from", then when the library you transfer items receives items (doing a check in) the items will go back to homelibrary automaticaly. There should be a way to avoid that (specific syspref ? to allow to keep items in the holdingbranch if they make part of a rotating collection).
Comment 8 Chris Cormack 2013-06-18 07:57:58 UTC
I note that while this patch is fairly harmless, 

rotating_collections/rotatingCollections.pl still has at least one problem

require Exporter;

I think the other scripts have this problem too.
Comment 9 Kyle M Hall 2013-06-18 14:46:32 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-06-18 14:46:40 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-06-18 14:49:29 UTC
Chris, is this QA follow up what you are looking for?


(In reply to comment #8)
> I note that while this patch is fairly harmless, 
> 
> rotating_collections/rotatingCollections.pl still has at least one problem
> 
> require Exporter;
> 
> I think the other scripts have this problem too.
Comment 12 Katrin Fischer 2013-07-01 20:41:15 UTC
Hi Kyle, I noticed a problem in the rotating collections templates while working on the po files: "[ editColDescription %]" in editCollections.tt.
Comment 13 David Cook 2013-07-01 23:51:57 UTC
Hi Kyle:

Any recent work on this one? I recently got a question about rotating collections, so it would be great to see this feature make its way back into Koha. 

Kyle: Do you have a test plan for this patch?

Others: What are the current reservations about this patch?
Comment 14 Kyle M Hall 2013-07-02 11:45:39 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-07-02 13:09:03 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2013-09-12 08:16:53 UTC
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8836 - Resurrect Rotating Collections
Applying: Bug 8836 - Resurrect Rotating Collections - QA Followup
Applying: Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
fatal: sha1 information is lacking or useless (C4/RotatingCollections.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
Comment 17 Kyle M Hall 2013-09-12 12:06:11 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-09-12 12:06:27 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-09-12 12:06:35 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-09-12 12:06:50 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2013-09-29 21:05:46 UTC
Hi Kyle, the QA script found something:
 FAIL	circ/returns.pl
   OK	  pod
   OK	  forbidden patterns
   FAIL	  valid
   "my" variable $itemnumber masks earlier declaration in same scope 
   OK	  critic

Could you check?
Comment 22 Olli-Antti Kivilahti 2013-10-30 10:12:32 UTC
We need this functionality as well. Is there some way I can help?
Comment 23 Magnus Enger 2013-10-30 10:15:43 UTC
(In reply to Olli-Antti Kivilahti from comment #22)
> We need this functionality as well. Is there some way I can help?

More testing and another signoff never hurts. ;-)
Comment 24 Katrin Fischer 2013-10-30 10:18:26 UTC
You could test the feature some more and add an additional sign-off :)
Comment 25 Olli-Antti Kivilahti 2013-10-30 10:29:00 UTC
Could I have a test plan for this feature?
I have no idea where to start looking or how to operate this?
Comment 26 Olli-Antti Kivilahti 2013-10-30 11:57:05 UTC
Rotating collections test case.

In "Tools" -> "Rotating Collections" -> "Add/Remove items":
When adding item barcodes to the collection, the input field
  <input type="text" id="barcode" name="barcode">
should become active
automatically so it is easy to add multiple barcodes rapidly without touching the
mouse or keyboard.
Enter-press is dealt by the barcode reader so automatic form submittal should be handled
by the barcode reader.

In "Rotating collections" -> "Transfer Collection":
When the collection is initially transferred, items are set to trasfer correctly.
When the collection is transferred while items are still being transferred, the transfer 
destination library doesn't change from the original one.
The holding library changes for all items in the collection to the destination library on
retransfers as well.
This is tricky if a user accidentally places the wrong destination.
When I try to checkin these items to their new retransfer location, I get the following messages:
-"This item is part of a rotating collection and needs to be transferred to <original transfer destination>"
-"Please return Valkoinen ihmissyöjä / to <original transfer destination>"
-"Print slip or Cancel transfer"

When I checkin a Item to a arbitrary branch, I get the following messages:
"This item is part of a rotating collection and needs to be transferred to <retransfer destination>"
"Please return Valkoinen ihmissyöjä / to <original trasfer destination>"
Comment 27 Olli-Antti Kivilahti 2013-10-31 09:57:16 UTC
Also unit tests are required.
Comment 28 Kyle M Hall 2014-01-06 16:16:53 UTC
This problem is actually an issue with Koha's transfer implementation, and is not directly related to this feature.

(In reply to Olli-Antti Kivilahti from comment #26)
> Rotating collections test case.
> 
> In "Tools" -> "Rotating Collections" -> "Add/Remove items":
> When adding item barcodes to the collection, the input field
>   <input type="text" id="barcode" name="barcode">
> should become active
> automatically so it is easy to add multiple barcodes rapidly without
> touching the
> mouse or keyboard.
> Enter-press is dealt by the barcode reader so automatic form submittal
> should be handled
> by the barcode reader.
> 
> In "Rotating collections" -> "Transfer Collection":
> When the collection is initially transferred, items are set to trasfer
> correctly.
> When the collection is transferred while items are still being transferred,
> the transfer 
> destination library doesn't change from the original one.
> The holding library changes for all items in the collection to the
> destination library on
> retransfers as well.
> This is tricky if a user accidentally places the wrong destination.
> When I try to checkin these items to their new retransfer location, I get
> the following messages:
> -"This item is part of a rotating collection and needs to be transferred to
> <original transfer destination>"
> -"Please return Valkoinen ihmissyöjä / to <original transfer destination>"
> -"Print slip or Cancel transfer"
> 
> When I checkin a Item to a arbitrary branch, I get the following messages:
> "This item is part of a rotating collection and needs to be transferred to
> <retransfer destination>"
> "Please return Valkoinen ihmissyöjä / to <original trasfer destination>"
Comment 29 Kyle M Hall 2014-01-06 16:18:54 UTC
While I agree that we should try to add unit tests, these patches do not add any new perl subroutines. There are many existing modules that do not have unit tests.

> Also unit tests are required.
Comment 30 Kyle M Hall 2014-01-06 16:38:57 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2014-01-06 16:39:02 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2014-01-06 16:39:08 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2014-01-06 16:39:12 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2014-01-06 16:53:04 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2014-01-06 16:56:41 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2014-01-06 17:39:54 UTC Comment hidden (obsolete)
Comment 37 Chris Cormack 2014-03-10 22:24:45 UTC
(In reply to Kyle M Hall from comment #29)
> While I agree that we should try to add unit tests, these patches do not add
> any new perl subroutines. There are many existing modules that do not have
> unit tests.
> 
> > Also unit tests are required.

Course you said that as a developer who didn't want to write tests, not a member of the QA team eh ? :)
Comment 38 Galen Charlton 2014-03-10 22:25:59 UTC
I think it is stretching just a bit too far to have 1 out of six patches being signed off resulting in an overall bug status of signed off.  Also, follow-up 5 doesn't seem to apply.

More to the point, the collections.colbranchcode column is too narrow.  It should be varchar(10) and a FK linking to the branches table.  Failing QA on that basis; I further request that you change the status to needs signoff, not signed off, when you submit a follow-up.

Also, given the age of the original code and the fact that it has not be in general use, there's even more call to write unit tests.
Comment 39 Kyle M Hall 2014-05-09 15:01:02 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2014-05-09 15:02:53 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2014-05-09 15:37:09 UTC
(In reply to Galen Charlton from comment #38)

> More to the point, the collections.colbranchcode column is too narrow.  It
> should be varchar(10) and a FK linking to the branches table.  Failing QA on
> that basis; I further request that you change the status to needs signoff,
> not signed off, when you submit a follow-up.

Fixed!

> Also, given the age of the original code and the fact that it has not be in
> general use, there's even more call to write unit tests.

While this is true, the Crawford County Federated Library system has been using this feature for since 2007 with no issues. I also find the existing set of rotating collections unit tests to be quite thorough.
Comment 42 Juhani Seppälä 2014-06-18 10:46:35 UTC
Hello, we're looking at bringing this feature into production and I've been going through this component - a trivial issue in additems.tt -template:

[% IF ( previousActionRemove ) %]
  [% IF ( removeSuccess ) %]
    <div>Item with barcode '[% addedBarcode %]' Removed successfully!</div>
  [% ELSE %]
    <div>Failed to remove item with barcode '[% removedBarcode %]'!</div>
    <div>Reason: <strong>[% failureMessage %]</strong></div>
  [% END %]
[% END %]

The parameter addedBarcode is used here when it probably was supposed to be removedBarcode - this leads to the barcode not being displayed after a succesful item removal.
Comment 43 Nick Clemens 2014-08-02 16:31:42 UTC
Applying: Bug 8836 - Resurrect Rotating Collections
Applying: Bug 8836 - Resurrect Rotating Collections - QA Followup
Applying: Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
Using index info to reconstruct a base tree...
<stdin>:59: trailing whitespace.
        "INSERT INTO collections ( colId, colTitle, colDesc ) 
<stdin>:211: trailing whitespace.
        "SELECT 
<stdin>:373: trailing whitespace.
        "DELETE FROM collections_tracking 
<stdin>:424: trailing whitespace.
        "SELECT barcode FROM items, collections_tracking 
<stdin>:471: trailing whitespace.
"SELECT holdingbranch, colBranchcode FROM items, collections, collections_tracking 
warning: 5 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging rotating_collections/transferCollection.pl
CONFLICT (content): Merge conflict in rotating_collections/transferCollection.pl
Auto-merging rotating_collections/rotatingCollections.pl
CONFLICT (content): Merge conflict in rotating_collections/rotatingCollections.pl
Auto-merging rotating_collections/editCollections.pl
CONFLICT (content): Merge conflict in rotating_collections/editCollections.pl
Auto-merging rotating_collections/addItems.pl
CONFLICT (content): Merge conflict in rotating_collections/addItems.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
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-8836---Resurrect-Rotating-Collections---Follow-DX51L_.patch
Comment 44 Juhani Seppälä 2014-08-15 13:58:23 UTC Comment hidden (obsolete)
Comment 45 Nick Clemens 2014-08-15 18:54:24 UTC
Could not apply on VM or sandbox

Applying: Bug 8836 - Resurrect Rotating Collections - QA Followup
Applying: Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
/home/nick/koha/.git/rebase-apply/patch:59: trailing whitespace.
        "INSERT INTO collections ( colId, colTitle, colDesc ) 
/home/nick/koha/.git/rebase-apply/patch:211: trailing whitespace.
        "SELECT 
/home/nick/koha/.git/rebase-apply/patch:373: trailing whitespace.
        "DELETE FROM collections_tracking 
/home/nick/koha/.git/rebase-apply/patch:424: trailing whitespace.
        "SELECT barcode FROM items, collections_tracking 
/home/nick/koha/.git/rebase-apply/patch:471: trailing whitespace.
"SELECT holdingbranch, colBranchcode FROM items, collections, collections_tracking 
fatal: sha1 information is lacking or useless (rotating_collections/addItems.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts
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-8836---Resurrect-Rotating-Collections---Follow-hvjidv.patch
Comment 46 Juhani Seppälä 2014-08-18 10:12:31 UTC
(In reply to Nick Clemens from comment #45)
> Could not apply on VM or sandbox
> 
> Applying: Bug 8836 - Resurrect Rotating Collections - QA Followup
> Applying: Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy
> rotating collections scripts
> /home/nick/koha/.git/rebase-apply/patch:59: trailing whitespace.
>         "INSERT INTO collections ( colId, colTitle, colDesc ) 
> /home/nick/koha/.git/rebase-apply/patch:211: trailing whitespace.
>         "SELECT 
> /home/nick/koha/.git/rebase-apply/patch:373: trailing whitespace.
>         "DELETE FROM collections_tracking 
> /home/nick/koha/.git/rebase-apply/patch:424: trailing whitespace.
>         "SELECT barcode FROM items, collections_tracking 
> /home/nick/koha/.git/rebase-apply/patch:471: trailing whitespace.
> "SELECT holdingbranch, colBranchcode FROM items, collections,
> collections_tracking 
> fatal: sha1 information is lacking or useless
> (rotating_collections/addItems.pl).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 8836 - Resurrect Rotating Collections - Followup 2
> - Perltidy rotating collections scripts
> 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-8836---Resurrect-Rotating-Collections---Follow-hvjidv.patch

Did you mean to test the squashed patch? I didn't set anything obsolete because I'm unsure as to whether squashing these old patches like this is actually welcome at all - I was hoping for comments and reactions. I guess I shouldn't have a set "needs signoff" - sorry. I can fix the old separate patches if that's how they should be kept.
Comment 47 Juhani Seppälä 2014-08-18 10:14:46 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2014-08-22 12:01:25 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2014-08-22 12:10:06 UTC
The squashing was fine, except you removed my authorship from the patches! I've uploaded a fixed version. In addition, your patch makes large changes, so I've moved it to a separate bug and made this one a blocker for it.

The biggest problems with your patch are
1) There is no test plan
2) The unit tests for rotating collections fail with it applied

(In reply to Juhani Seppälä from comment #46)
> Did you mean to test the squashed patch? I didn't set anything obsolete
> because I'm unsure as to whether squashing these old patches like this is
> actually welcome at all - I was hoping for comments and reactions. I guess I
> shouldn't have a set "needs signoff" - sorry. I can fix the old separate
> patches if that's how they should be kept.
Comment 50 Juhani Seppälä 2014-08-22 12:28:35 UTC
(In reply to Kyle M Hall from comment #49)
> The squashing was fine, except you removed my authorship from the patches!
> I've uploaded a fixed version. In addition, your patch makes large changes,
> so I've moved it to a separate bug and made this one a blocker for it.
> 
> The biggest problems with your patch are
> 1) There is no test plan
> 2) The unit tests for rotating collections fail with it applied
> 
> (In reply to Juhani Seppälä from comment #46)
> > Did you mean to test the squashed patch? I didn't set anything obsolete
> > because I'm unsure as to whether squashing these old patches like this is
> > actually welcome at all - I was hoping for comments and reactions. I guess I
> > shouldn't have a set "needs signoff" - sorry. I can fix the old separate
> > patches if that's how they should be kept.

The patch authorship thing was, probably among other stuff, something I completely missed at the time - sorry about that. I agree with the separate bug and am aware of the missing test stuff.
Comment 51 Katrin Fischer 2014-10-09 23:24:28 UTC
Hi Kyle, there have been lots of follow-ups on this, would you be ok with asking for another sign-off? This is a bigger one and I'd feel better with some additional testing.
Comment 52 Katrin Fischer 2014-10-10 02:24:53 UTC
There are also a couple of problems pointed out by the QA script at this point:

FAIL	circ/returns.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
		"my" variable $itemnumber masks earlier declaration in same scope 

 FAIL	rotating_collections/addItems.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: You should not use a .tmpl extension for the template name (see bug 11349) (line 33)
   OK	  pod
   OK	  valid

 FAIL	rotating_collections/editCollections.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: You should not use a .tmpl extension for the template name (see bug 11349) (line 33)
   OK	  pod
   OK	  valid

 FAIL	rotating_collections/rotatingCollections.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: You should not use a .tmpl extension for the template name (see bug 11349) (line 32)
   OK	  pod
   OK	  valid

 FAIL	rotating_collections/transferCollection.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: You should not use a .tmpl extension for the template name (see bug 11349) (line 36)
   OK	  pod
   OK	  valid
Comment 53 Kyle M Hall 2014-10-15 15:58:17 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2014-10-15 15:58:35 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2014-10-15 15:59:41 UTC
(In reply to Katrin Fischer from comment #51)
> Hi Kyle, there have been lots of follow-ups on this, would you be ok with
> asking for another sign-off? This is a bigger one and I'd feel better with
> some additional testing.

That's fine by me! Reseting status to "Needs Signoff".
Comment 56 Cindy Murdock Ames 2014-10-17 15:39:59 UTC
I tried applying attachment 32361 [details] [review], and I got this error:

Applying: Bug 8836 - Resurrect Rotating Collections
error: patch failed: installer/data/mysql/updatedatabase.pl:8800
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Bug 8836 - Resurrect Rotating Collections
Comment 57 Kyle M Hall 2014-10-22 16:33:43 UTC Comment hidden (obsolete)
Comment 58 Kyle M Hall 2014-10-22 16:34:13 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2014-10-24 19:00:23 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2014-10-24 19:00:34 UTC Comment hidden (obsolete)
Comment 61 Chris Cormack 2014-10-24 20:18:39 UTC Comment hidden (obsolete)
Comment 62 Cindy Murdock Ames 2014-10-24 20:40:49 UTC
I was just about to sign off on them both--Chris you beat me to the punch!
Comment 63 Katrin Fischer 2014-10-24 20:42:01 UTC
Hi Cindy, if you want, we can add your sign off line in QA.
Comment 64 Cindy Murdock Ames 2014-10-24 20:54:49 UTC
Yes, sounds good!  Do I need to do it?  Sorry, my git-fu is weak!  Still getting used to the signoff process.
Comment 65 Katrin Fischer 2014-10-24 20:55:54 UTC
I can do it - thx for testing :)
Comment 66 Katrin Fischer 2014-10-26 11:00:04 UTC
Starting work on this now - is there a good description of the Rotating collections module somewhere?
Comment 67 Katrin Fischer 2014-10-26 11:03:20 UTC Comment hidden (obsolete)
Comment 68 Katrin Fischer 2014-10-26 11:03:36 UTC Comment hidden (obsolete)
Comment 69 Katrin Fischer 2014-10-26 11:04:12 UTC
Just adding Cindy's sign-off line so we don't forget it :)
Comment 70 Katrin Fischer 2014-10-26 12:19:35 UTC
Hi Kyle, 

starting with a code review of the new patches. 

1. CODE REVIEW

1.1 QA script
Problem (normal) We got a problem with perl critic:
 FAIL	C4/Circulation.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  

1.2 Code
Note: Be careful with GetReserveStatus, it's not always working correctly.
'waiting' works, but other status are problematic, see bug 10697.
+        transferbook( $colBranchcode, $item->{barcode},
+            my $ignore_reserves = 1 )
+          unless ( GetReserveStatus( $item->{itemnumber} ) eq "Waiting" );
GetReserveStatus works for 'Waiting' but is problematic for any other 
status (see bug 10697)

1.3 Database structure 
Question: Why is the constraint in kohastructure.sql added by a 
separate statement?
+-- Constraints for table `collections`
+--
+ALTER TABLE `collections`
+  ADD CONSTRAINT `collections_ibfk_1` FOREIGN KEY (`colBranchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE


2. FEATURE TEST

As this brings the Rotating collections feature back to life, I think we
also need to test the feature here. 

I know that the code has been written a while ago, so I will add notes
on what I think NEED to be fixed in order for this to go back in at
the bottom, while also noting other things I see while testing this.


2.1 Documentation

2.1.1 Question: Is there a good documentation about the feature 
somewhere? Feeling a bit like flying blind here :)

2.1.2 Problem (normal, separate bug)
We have no help pages for the Rotating collections


2.2 Navigation

2.2.1 Problem (normal)
There is no entry for "Rotating collections" on the side navigation when
you are on a tool's page.

2.2.2 Problem (normal) 
The main page of the Rotating collection page is missing the side
navigation altogether.

2.2.3 Hm.
In several places we have a button to return to the Rotating collections
start page, maybe this could be replaced by the side navigation link?


2.3 Adding a rotating collection

2.3.1 Problem (minor)
The 'new collection' form should have 'Title' marked as required.

2.3.2 Problem (normal)
The error messages stem from RotatingCollections.pm and are not
translatable (see bug 11595). Please fix!

2.3.3 Problem (trivial)
The table shows the branch code instead of the description name, 
easy to fix today, with TT!

2.3.3 Hm.
It feels a bit weird, that I have to use a button to get to a page
where I can edit the collections. Why not have this functionality
accessible from the table on the start page? (Adding "Edit" and "Delete"
links to the table and a "New collection" button at the top? This way
it would fit in a bit better with Koha's usual way of doing things.


2.4 Rotating collections home

2.4.1 Problem (trivial)
Several capitalization 'problems' on rotatingCollections.pl.

2.4.1 Problem (trivial)
The table shows the branch code instead of the description name, 
easy to fix today, with TT!


2.5 Adding/removing items

2.5.1 Problem (normal)
The error messages stem from RotatingCollections.pm and are not
translatable (see also bug 11595). Please fix!

2.5.2 Problem (trivial) 
Error messages should use the usual styling
to stand out a bit more. Example: remove a barcode that is not in the
collection.

2.5.3 Problem (trivial)
I am not sure this error makes sense to the user: 
Reason: No Itemnumber Given
I think it would be better to say: Barcode doesn't exist (or similar)

2.5.4 Enhancement idea
Removing an item from the collection currently requires that you enter
the barcode. It would be nice to also have a 'Delete' link in the 
table.


2.6 Transfers

Collection goes to Troy.

2.6.1 OK Checking in an item that is "waiting" in Centerville
The rotating collection message shows, but also the hold message.
I confirm the hold message.
The item is still 'waiting' for the patron.
No transfer is generated.

2.6.2 OK Check-out the waiting item in Centerville to the borrower.

2.6.3 OK Check-in item in Centerville.
There is another hold in Springfield for another patron. 
Confirm transfer.
Transfer is generated.

2.6.4 OK Check-in of the item in Springfield.
Confirm hold.
Transfer is completed.

2.6.5 OK Check-out to the patron the item is waiting for.

2.6.6 OK? Check-in of the item in _Centerville_
Patron returns the item at another branch. 
I still see the dialog, that the item shoudl go to "Troy", but
no transfer message is generated, is this the way it's supposed to work?

Note: not sure, but should the rotating collections message show up, when there is also a hold/other transfer request?


SUMMARY

Please check everything noted as a 'Problem' specifically the perl critic 
and the translation issues. 
Some of the other things are easy to fix too :)

Can you also summarize for me, what the suspected behaviour is (2.6.6)?
Comment 71 Cindy Murdock Ames 2014-10-29 19:58:19 UTC
To address 2.1.1, I can write some documentation for it.  Our libraries have been using Rotating Collections for years.
Comment 72 Katrin Fischer 2014-10-29 20:52:05 UTC
Hi Cindy, thanks for the offer! Maybe you can also help with 2.6? I am not sure there how it is intended to work.
Comment 73 Kyle M Hall 2014-10-30 14:38:00 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2014-10-30 14:48:50 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2014-10-30 14:49:00 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2014-10-30 14:49:04 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2014-10-30 14:52:10 UTC
I believe this followup should fix most all your issues. There is a small visual bug with the alert divs that I'm hoping Owen can take a look at.

As far as the your functionality test, I think Cindy can speak more to its accuracy, but it helps to think of the the rotating collections as something of a floating collection. For this reason, automatic transfers are disabled for rotating items. If you need to transfer an item, you must use the transfer tool from the circulation page. Does that make sense?

Kyle
Comment 78 Owen Leonard 2014-10-30 16:46:09 UTC
Some issues I see:

- The list of collections should show a count of the
  items in that collection

- Should collections require unique titles?
  I was able to add multiple collections with the same title.

- Why are descriptions required?

- Transfer library list isn't in alphabetical order

- No confirmation of deletion operation, including 
  for collections which contain items.

- Shouldn't the detail page for an item in a collection show
  that it is in a collection?

- Add/edit/delete/transfer operations should redirect sensibly
  instead of showing a blank page with a confirmation message.

  For instance: After a successful collection edit the user
  should be redirected to the collections home page and a
  message displayed there.

- Templates are a mess. I'm preparing a follow-up.
Comment 79 Owen Leonard 2014-10-30 16:50:18 UTC
Another issue:

I happened to add to a collection an item which was already in mid-transfer between two libraries. When I transferred that collection to another library the in-progress transfer was still active.

When I tried checking in that item at another library (not the collection's library, not the library to which the item had originally been transferred) I got conflicting messages: Both a message telling me where the collection belonged and also a message about the original transfer.

If I understand the feature correctly the original in-progress transfer should have been cancelled when I initiated the transfer of the collection.
Comment 80 Olli-Antti Kivilahti 2014-10-30 17:11:22 UTC
Canceling an in-progress transport causes issues in real life.
When a book is being moved between cities, it is essential to know that the item is on the move. When the Item in the rotating collection reaches the transfer destination, it is automatically sent back to the "new" home library (depending on the sysprefs).

Initially the idea back here was to be able to create these Rotating Collections by utilizing the normal consortial transportation features. So we could put items from multiple branches to one rotating collection destined to one place. For example if we want to make a collection of a special topic from all the books available throughout the consortium.

Ofcourse the items need to be checked-in to activate the transfer and this needs some manual work in notifying each branch about the desired items which are waiting in shelves. Items on normal circulation tend to move towards the rotating collection "home" after they are checked-in.
I guess there are still some issues with the idea, but I don't think canceling transfers is the way to go. It would be better to force items in a rotating collection to be sent to the rotating collection "home" after they reach their destination.
Regarding holds queue for items on a rotating collection. We thought that it will be something the librarian needs to consider, maybe as a follow up feature to block placing holds on rotating collections via syspref or via a option in the rotating collection itself (better!).

I just don't feel like missing the opportunity to providing a tool to do cool things with our material, is worth the price of assuming that all the Items are immediately available when they are put to the collection. Even if that would be the easy way out.
Comment 81 Olli-Antti Kivilahti 2014-10-30 17:14:02 UTC
(In reply to Owen Leonard from comment #78)
> Some issues I see:
> 
> - The list of collections should show a count of the
>   items in that collection
> 
true

> - Should collections require unique titles?
>   I was able to add multiple collections with the same title.
> 
a good idea

> - Why are descriptions required?
If we have a special rotating collection we want to tell about. Or maybe to tell others to not touch the material in here until 14.4.2015? Its handy to have a freeword place to leave messages and extra info.
> 
> - Transfer library list isn't in alphabetical order
> 
> - No confirmation of deletion operation, including 
>   for collections which contain items.
> 
we got complaints of that as well, I thought Juhani already fixed that?

> - Shouldn't the detail page for an item in a collection show
>   that it is in a collection?
> 
It should.

> - Add/edit/delete/transfer operations should redirect sensibly
>   instead of showing a blank page with a confirmation message.
> 
>   For instance: After a successful collection edit the user
>   should be redirected to the collections home page and a
>   message displayed there.
> 
> - Templates are a mess. I'm preparing a follow-up.

I'll ask Juhani to comment on these.
Comment 82 Owen Leonard 2014-10-30 17:48:30 UTC
(In reply to Olli-Antti Kivilahti from comment #81)
> > - Why are descriptions required?
> If we have a special rotating collection we want to tell about.

Yes, I can see why a description is a useful option. I just don't understand why the description would be a required field.
Comment 83 Owen Leonard 2014-10-30 18:37:00 UTC Comment hidden (obsolete)
Comment 84 Brendan Gallagher 2014-10-30 20:23:55 UTC Comment hidden (obsolete)
Comment 85 Brendan Gallagher 2014-10-30 20:24:03 UTC Comment hidden (obsolete)
Comment 86 Brendan Gallagher 2014-10-30 20:24:12 UTC Comment hidden (obsolete)
Comment 87 Brendan Gallagher 2014-10-30 20:24:27 UTC Comment hidden (obsolete)
Comment 88 Katrin Fischer 2014-10-30 20:59:37 UTC
Kivilahti, please keep in mind that Juhani's work is on a separate bug 12807 and not included here. I'd suggest to break it into well documented easily testable parts when rebasing for the changes made here.
Comment 89 Katrin Fischer 2014-10-30 23:35:36 UTC
I have gathered some information from reading through the comments on this bug, could you confirm if the following assumptions are correct?

1) If I create a collection and transfer it, all available items will get automatically moved to the new library?

2) If I am using AutomaticItemReturn, it will not send a book back to its home library, but will display a note where it should go for Rotating collections?

3) All transfers for books missed in 1) have to be done manually and there will only be a note shown on checkin?
Comment 90 Olli-Antti Kivilahti 2014-10-31 07:00:50 UTC
(In reply to Katrin Fischer from comment #88)
> Kivilahti, please keep in mind that Juhani's work is on a separate bug 12807
> and not included here. I'd suggest to break it into well documented easily
> testable parts when rebasing for the changes made here.

Cool! So this means I can sign off this patch.
Comment 91 Katrin Fischer 2014-10-31 07:01:12 UTC
Hi Kyle, 

I went through my notes again and I think the templates, and
adding/editing/deleting rotating collections is now ok. 
There is only one minor thing about the branch list not being alphabetical
(2.3.5).

Running out of time now, please answer my questions about transfer 
behaviour in comment #89 and check comment #79 from Owen, about the behaviour 
for an item that was 'in transit' before the rotating collections transfer.

I will try to continue there tonight.

1. CODE REVIEW

1.1 QA script
- OK  

1.2 Code
- OK, no more problems spotted.

1.3 Database structure 
- OK, changes are consistent.

2. FEATURE TEST

As this brings the Rotating collections feature back to life, I think we
also need to test the feature here. 

2.1 Documentation

2.1.1 Question: Is there a good documentation about the feature 
somewhere? Feeling a bit like flying blind here :)

2.1.2 Problem - no blocker
I have filed bug 13177 for the missing help pages.


2.2 Navigation

2.2.1 OK - FIXED
There is no entry for "Rotating collections" on the side navigation when
you are on a tool's page.

2.2.2 OK - FIXED
The main page of the Rotating collection page is missing the side
navigation altogether.

2.2.3 OK
In several places we have a button to return to the Rotating collections
start page, maybe this could be replaced by the side navigation link?


2.3 Adding a rotating collection

2.3.1 OK - FIXED
The 'new collection' form should have 'Title' marked as required.

2.3.2 OK - FIXED
The error messages stem from RotatingCollections.pm and are not
translatable (see bug 11595). Please fix!

2.3.3 OK - FIXED
The table shows the branch code instead of the description name, 
easy to fix today, with TT!

2.3.4 OK - FIXED with a new toolbar
It feels a bit weird, that I have to use a button to get to a page
where I can edit the collections. Why not have this functionality
accessible from the table on the start page? (Adding "Edit" and "Delete"
links to the table and a "New collection" button at the top? This way
it would fit in a bit better with Koha's usual way of doing things.

2.3.5 Problem - trivial
Branches in the list are not sorted alphabetically by description.

2.3.6 Question - could the description be optional?


2.4 Rotating collections home

2.4.1 OK - FIXED
Several capitalization 'problems' on rotatingCollections.pl.

2.4.1 OK - FIXED
The table shows the branch code instead of the description name, 
easy to fix today, with TT!


2.5 Adding/removing items

2.5.1 OK - FIXED
The error messages stem from RotatingCollections.pm and are not
translatable (see also bug 11595). Please fix!

2.5.2 OK - FIXED 
Error messages should use the usual styling
to stand out a bit more. Example: remove a barcode that is not in the
collection.

2.5.3 OK - FIXED
I am not sure this error makes sense to the user: 
Reason: No Itemnumber Given
I think it would be better to say: Barcode doesn't exist (or similar)

2.5.4 Enhancement idea - OK DONE!
Removing an item from the collection currently requires that you enter
the barcode. It would be nice to also have a 'Delete' link in the 
table.


2.6 Transfers 

Collection goes to Troy.

2.6.1 OK Checking in an item that is "waiting" in Centerville
The rotating collection message shows, but also the hold message.
I confirm the hold message.
The item is still 'waiting' for the patron.
No transfer is generated.

2.6.2 OK Check-out the waiting item in Centerville to the borrower.

2.6.3 OK Check-in item in Centerville.
There is another hold in Springfield for another patron. 
Confirm transfer.
Transfer is generated.

2.6.4 OK Check-in of the item in Springfield.
Confirm hold.
Transfer is completed.

2.6.5 OK Check-out to the patron the item is waiting for.

2.6.6 OK? Check-in of the item in _Centerville_
Patron returns the item at another branch. 
I still see the dialog, that the item shoudl go to "Troy", but
no transfer message is generated, is this the way it's supposed to work?

Note: I think it would be nicer if the rotating collection message
wouldn't show up, when there is a hold message at the same time.


2.7 Transferring a collection

2.7.1  OK
Verified that waiting and checked out items don't get transferred
automatically. 


X. Possible enhancements
- Show rotating collection information in staff detail pages
- Add redirect operations after Add/edit/delete/transfer operations
Comment 92 Juhani Seppälä 2014-10-31 10:24:04 UTC
Since there's a lot of significant changes both to the views and also to the core behaviour of some of the features in 12807, I agree with the idea of breaking things down (and perhaps making it more modular?).
Comment 93 Kyle M Hall 2014-10-31 11:02:41 UTC
(In reply to Katrin Fischer from comment #89)
> I have gathered some information from reading through the comments on this
> bug, could you confirm if the following assumptions are correct?
> 
> 1) If I create a collection and transfer it, all available items will get
> automatically moved to the new library?

Only those items that are available to transfer at the time.

> 2) If I am using AutomaticItemReturn, it will not send a book back to its
> home library, but will display a note where it should go for Rotating
> collections?

That is correct. Any item in a rotating collection bypasses AutomaticItemReturn, as it would be contrary to the feature.

> 
> 3) All transfers for books missed in 1) have to be done manually and there
> will only be a note shown on checkin?

That is correct.
Comment 94 Kyle M Hall 2014-10-31 11:14:47 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2014-10-31 11:25:57 UTC Comment hidden (obsolete)
Comment 96 Kyle M Hall 2014-10-31 11:37:09 UTC Comment hidden (obsolete)
Comment 97 Kyle M Hall 2014-10-31 11:40:50 UTC Comment hidden (obsolete)
Comment 98 Owen Leonard 2014-10-31 12:28:17 UTC Comment hidden (obsolete)
Comment 99 Owen Leonard 2014-10-31 12:28:25 UTC Comment hidden (obsolete)
Comment 100 Kyle M Hall 2014-10-31 13:09:32 UTC Comment hidden (obsolete)
Comment 101 Kyle M Hall 2014-10-31 13:09:44 UTC Comment hidden (obsolete)
Comment 102 Kyle M Hall 2014-10-31 13:09:47 UTC Comment hidden (obsolete)
Comment 103 Kyle M Hall 2014-10-31 13:09:50 UTC Comment hidden (obsolete)
Comment 104 Kyle M Hall 2014-10-31 13:09:54 UTC Comment hidden (obsolete)
Comment 105 Kyle M Hall 2014-10-31 13:09:57 UTC Comment hidden (obsolete)
Comment 106 Kyle M Hall 2014-10-31 13:10:01 UTC Comment hidden (obsolete)
Comment 107 Kyle M Hall 2014-10-31 13:10:05 UTC Comment hidden (obsolete)
Comment 108 Kyle M Hall 2014-10-31 13:10:09 UTC Comment hidden (obsolete)
Comment 109 Owen Leonard 2014-10-31 13:25:14 UTC Comment hidden (obsolete)
Comment 110 Owen Leonard 2014-10-31 13:44:53 UTC Comment hidden (obsolete)
Comment 111 Katrin Fischer 2014-10-31 19:26:47 UTC
Created attachment 33095 [details] [review]
[PASSED QA] Bug 8836 - Resurrect Rotating Collections

Test Plan:
In "Tools" -> "Rotating Collections" -> "Add/Remove items":
When adding item barcodes to the collection, the input field
  <input type="text" id="barcode" name="barcode">
should become active
automatically so it is easy to add multiple barcodes rapidly without touching the
mouse or keyboard.
Enter-press is dealt by the barcode reader so automatic form submittal should be handled
by the barcode reader.

In "Rotating collections" -> "Transfer Collection":
When the collection is initially transferred, items are set to trasfer correctly.
When the collection is transferred while items are still being transferred, the transfer
destination library doesn't change from the original one.
The holding library changes for all items in the collection to the destination library on
retransfers as well.
This is tricky if a user accidentally places the wrong destination.
When I try to checkin these items to their new retransfer location, I get the following messages:
-"This item is part of a rotating collection and needs to be transferred to <original transfer destination>"
-"Please return Valkoinen ihmissyj / to <original transfer destination>"
-"Print slip or Cancel transfer"

When I checkin a Item to a arbitrary branch, I get the following messages:
"This item is part of a rotating collection and needs to be transferred to <retransfer destination>"
"Please return Valkoinen ihmissyj / to <original trasfer destination>"

Bug 8836 - Resurrect Rotating Collections - QA Followup

Bug 8836 - Resurrect Rotating Collections - Followup 2 - Perltidy rotating collections scripts

Bug 8836 - Resurrect Rotating Collections - Followup 3

* Fix bad TT Tag
* Fix bad sql query
* Fix capitalization ( HTML4 )
* Allow a rotating collection's location to keep AutomaticItemReturn
  from sending it back to the branch of origin
* Fix bad query

Bug 8836 - Resurrect Rotating Collections - Followup 4 - Autofocus on barcode field

Bug 8836 - Resurrect Rotating Collections - Followup 5 - Don't transfer issued and waiting items

Items in a rotating collection are automatcially transferred when a
collection is transferred. This is a problem for currently checked out
items and items on hold marked as "Waiting".

This patch resolves this issue by skipping the transfer for those items.
When the items are then returned, the librarian will be alerted to
transfer the item to the library currently holding that rotating
collection.

Bug 8836 - Resurrect Rotating Collections - Followup 5 - Link collections.colBranchcode to branches.branchcode

Signed-off-by: jmbroust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Cindy Murdock Ames <cmurdock@ccfls.org>

http://bugs.koha-community.org/show_bug.cgi?id=8835

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 112 Katrin Fischer 2014-10-31 19:26:52 UTC
Created attachment 33096 [details] [review]
[PASSED QA] Bug 8836 [QA Followup] - Fix koha-qa.pl errors reported

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Trusting the sign off on the other page, I only tested the QA changes

Signed-off-by: Cindy Murdock Ames <cmurdock@ccfls.org>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 113 Katrin Fischer 2014-10-31 19:26:57 UTC
Created attachment 33097 [details] [review]
[PASSED QA] Bug 8836 [QA Followup 2]

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 114 Katrin Fischer 2014-10-31 19:27:02 UTC
Created attachment 33098 [details] [review]
[PASSED QA] Bug 8836 [Template follow-up] Resurrect Rotating Collections

This follow-up makes drastic changes to the templates in order to bring
them into compliance with established patterns and markup guidelines.
Only minor changes are made to perl scripts.

Changes:

- Add a toolbar include for displaying new, edit, transfer, and delete
  buttons.
- Improve title and breadcrumbs with collection titles and better
  specificity.
- Correct page structure which was inconsistent with the markup of
  similarly-structured pages.
- Correct styling of error and informational messages.
- Added detailed error messages for a couple of conditions which were
  not defined in the template.
- Add link to the detail page of titles which are in a collection using
  the view defined in the IntranetBiblioDefaultView preference.
- Add a link to remove an item from a collection directly without having
  to scan the barcode.
- Add client-side validation to collection creation form.
- In RotatingCollections.pm, add biblionumber to the list of columns
  returned by GetItemsInCollection.
- In rotating_collections/*.pl, remove obsolete declaration of system
  preference variables.

To test, perform all the operations associated with Rotating
Collections:

- Add a new collection
- Edit an existing collection
- Add items to a collection
- Remove items from a collection (via barcode and link)
- Test the behavior of all new toolbar buttons
- Verify that titles and breadcrumbs look correct and links work
  correctly.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 115 Katrin Fischer 2014-10-31 19:27:08 UTC
Created attachment 33099 [details] [review]
[PASSED QA] Bug 8836 [QA Followup] - Allow collection description to be optional

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Description is no longer made required by the template and an empty
description is saved correctly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 116 Katrin Fischer 2014-10-31 19:27:14 UTC
Created attachment 33100 [details] [review]
[PASSED QA] Bug 8836 [QA Followup] - Sort transfer branches alphabetically

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Libraries sort correctly now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 117 Katrin Fischer 2014-10-31 19:27:19 UTC
Created attachment 33101 [details] [review]
[PASSED QA] Bug 8836 [QA Followup] - Don't transfer in items already in transit

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 118 Katrin Fischer 2014-10-31 19:27:23 UTC
Created attachment 33102 [details] [review]
[PASSED QA] Bug 8836 [Alt QA Followup] - Add confirmation dialog for deleting collections

The previous patch for adding a confirmation dialog didn't work to make
the confirmation message translatable. This alternate patch uses a
progressive-enhancement method recycled from Guided Reports to trigger a
warning and highlight the row which was clicked.

To test, apply the patch and try to delete an existing rotating
collection. You should get a confirmation dialog, and the corresponding
row in the table should be highlighted red.

Clicking cancel should cancel the deletion. Clicking OK should complete
the deletion.

To test the translatability of the confirmation message:

Run perl translate update [a language code, e.g. 'en-GB']
Search [lang]-i-staff-t-prog-v-3006000.po for the message string

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 119 Katrin Fischer 2014-10-31 19:27:28 UTC
Created attachment 33103 [details] [review]
[PASSED QA] Bug 8836 [QA Followup] - Prevent multiple collections from having the same title

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

This works, and so I'll sign off, but I'm not crazy about the workflow.
Having the error message display on an otherwise empty page is not user
friendly. The entry form should be redisplayed so that the user can
modify the data they submitted.

That really should be changed in a follow-up.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 120 Katrin Fischer 2014-10-31 19:30:14 UTC
Hi Kyle and Owen,

thx for all the hard work on this!

1. CODE REVIEW

1.1 QA script
- OK  

1.2 Code
- OK, no more problems spotted.

1.3 Database structure 
- OK, changes are consistent.

2. FEATURE TEST

As this brings the Rotating collections feature back to life, I think we
also need to test the feature here. 

2.1 Documentation

2.1.1 Question: Is there a good documentation about the feature 
somewhere? Feeling a bit like flying blind here :)

2.1.2 Problem - no blocker
I have filed bug 13177 for the missing help pages.


2.2 Navigation

2.2.1 OK - FIXED
There is no entry for "Rotating collections" on the side navigation when
you are on a tool's page.

2.2.2 OK - FIXED
The main page of the Rotating collection page is missing the side
navigation altogether.

2.2.3 OK
In several places we have a button to return to the Rotating collections
start page, maybe this could be replaced by the side navigation link?


2.3 Adding a rotating collection

2.3.1 OK - FIXED
The 'new collection' form should have 'Title' marked as required.

2.3.2 OK - FIXED
The error messages stem from RotatingCollections.pm and are not
translatable (see bug 11595). Please fix!

2.3.3 OK - FIXED
The table shows the branch code instead of the description name, 
easy to fix today, with TT!

2.3.4 OK - FIXED with a new toolbar
It feels a bit weird, that I have to use a button to get to a page
where I can edit the collections. Why not have this functionality
accessible from the table on the start page? (Adding "Edit" and "Delete"
links to the table and a "New collection" button at the top? This way
it would fit in a bit better with Koha's usual way of doing things.

2.3.5 OK - FIXED
Branches in the list are not sorted alphabetically by description.

2.3.6 OK - FIXED
Description should be optional.


2.4 Rotating collections home

2.4.1 OK - FIXED
Several capitalization 'problems' on rotatingCollections.pl.

2.4.1 OK - FIXED
The table shows the branch code instead of the description name, 
easy to fix today, with TT!


2.5 Adding/removing items

2.5.1 OK - FIXED
The error messages stem from RotatingCollections.pm and are not
translatable (see also bug 11595). Please fix!

2.5.2 OK - FIXED 
Error messages should use the usual styling
to stand out a bit more. Example: remove a barcode that is not in the
collection.

2.5.3 OK - FIXED
I am not sure this error makes sense to the user: 
Reason: No Itemnumber Given
I think it would be better to say: Barcode doesn't exist (or similar)

2.5.4 Enhancement idea - OK DONE!
Removing an item from the collection currently requires that you enter
the barcode. It would be nice to also have a 'Delete' link in the 
table.


2.6 Transfers 

Collection goes to Troy.

2.6.1 OK Checking in an item that is "waiting" in Centerville
The rotating collection message shows, but also the hold message.
I confirm the hold message.
The item is still 'waiting' for the patron.
No transfer is generated.

2.6.2 OK Check-out the waiting item in Centerville to the borrower.

2.6.3 OK Check-in item in Centerville.
There is another hold in Springfield for another patron. 
Confirm transfer.
Transfer is generated.

2.6.4 OK Check-in of the item in Springfield.
Confirm hold.
Transfer is completed.

2.6.5 OK Check-out to the patron the item is waiting for.

2.6.6 OK? Check-in of the item in _Centerville_
Patron returns the item at another branch. 
I still see the dialog, that the item shoudl go to "Troy", but
no transfer message is generated, is this the way it's supposed to work?


2.7 Transferring a collection

2.7.1  OK
Verified that waiting and checked out items don't get transferred
automatically. 

2.7.2 OK
Verified that 'in transit' item doesn't get rerouted by transferring
the collection.

2.7.3 AutomaticItemReturn
... does not happen when your item is in a rotating collection.


X. Possible enhancements
- Show rotating collection information in staff detail pages
Comment 121 Tomás Cohen Arazi 2014-11-06 18:29:19 UTC
Created attachment 33330 [details] [review]
Bug 8836: (RM followup) fix updatedatabase.pl rebase problem

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 122 Tomás Cohen Arazi 2014-11-06 18:30:16 UTC
Created attachment 33331 [details] [review]
Bug 8836: Update DBIx

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 123 Tomás Cohen Arazi 2014-11-07 14:24:41 UTC
Created attachment 33374 [details] [review]
Bug 8836: (RM followup) unit tests should use the new API

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 124 Tomás Cohen Arazi 2014-11-07 14:26:41 UTC
Patches pushed to master.

Thanks Kyle!