Not 100% ready, but I need some feedback. Note that the transfers subtesto to RotatingCollections.t is not passsing. I don't know how to implement item transfering in Koha::RotatingCollection module yet.

Created attachment 66231 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

I've rebased this on top of master, squashed few commits, fix issues, now it should be working and ready to test.

Hi Josef

I have one test failing in t/db_dependent/Circulation.t. I've pasted the output here: http://paste.koha-community.org/621

All other tests pass.

Some notes from following the rest of your test plan:
When deleting a collection, two different confirmation boxes pop up. (i know this is the behaviour before your patches)
Deleting a collection with items was no different from deleting a collection without items (no error message to say the collection had items).
No difference when returning an item in an untransferred collection and returning an item in a transferred collection

I know the first point is the original behaviour before your patches. Unfortunately I can't test whether the second and third points are the expected behaviour because rotating collections appears to be broken for me without your patches! :) If the behaviour I've described is expected, then everything looks good.

Setting to Failed QA for the failed test.

(In reply to Aleisha Amohia from comment #54)
> Hi Josef
> 
> I have one test failing in t/db_dependent/Circulation.t. I've pasted the
> output here: http://paste.koha-community.org/621
> 
> All other tests pass.
> 
> Some notes from following the rest of your test plan:
> When deleting a collection, two different confirmation boxes pop up. (i know
> this is the behaviour before your patches)
> Deleting a collection with items was no different from deleting a collection
> without items (no error message to say the collection had items).
> No difference when returning an item in an untransferred collection and
> returning an item in a transferred collection
> 
> I know the first point is the original behaviour before your patches.
> Unfortunately I can't test whether the second and third points are the
> expected behaviour because rotating collections appears to be broken for me
> without your patches! :) If the behaviour I've described is expected, then
> everything looks good.
> 
> Setting to Failed QA for the failed test.

Thanks for testing!

I did another rebase today. 

Test t/db_dependent/Circulation.t is passing form me.

The double confirmation dialog was easy to fix, I am doing it in followup

Ability to delete a collection with items is current behavior in master, I will fix it on another report

Returning from transferred and untransferred collection is not diferrentiate in in current master too.

Setting back to needs signoff

> Ability to delete a collection with items is current behavior in master, I
> will fix it on another report

It is Bug 17202

Just rebased on master (conflict in C4/Circulation.pm because of bug 14826)

Created attachment 69709 [details] [review]
Bug 18606: Add RotationCollections object classes

Test plan:
1) Apply the patch
2) Update database
3) Go to tools -> Rotating collections and play with trying to break it:
    3a) Add some collections
    3b) Edit them
    3c) Add some items to collections
    3d) Remove some items
    3e) Transfer some collections
    3f) Try to remove collection without items
    3g) Try to remove collection with items
4) Go to returns and:
    4a) Try to return item from untransferred collection
    4b) Try to return item from transferred collection
5) prove t/db_dependent/Koha/RotatingCollections.t
t/db_dependent/Koha/Items.t t/db_dependent/Circulation.t
t/db_dependent/Circulation/Returns.t

Created attachment 69710 [details] [review]
Bug 18606: Easy ones - CRUD

Created attachment 69711 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

Created attachment 69712 [details] [review]
Bug 18606: Schema changes

Created attachment 69713 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

Created attachment 69714 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

Created attachment 69715 [details] [review]
Bug 18606: Get rid of AddItemToCollection

Created attachment 69716 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

Created attachment 69717 [details] [review]
Bug 18606: Get rid of TransferCollection

Created attachment 69718 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

Created attachment 69719 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

Created attachment 69720 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

Created attachment 69721 [details] [review]
Bug 18606: Rework tests for rotating collections

Created attachment 69722 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t

Created attachment 69723 [details] [review]
Bug 18606: Unify handling of success/error messages

Testing functionality goes without any problem, but running "prove t/db_dependent/Koha/Items.t t/db_dependent/Circu lation.t" gave this result:

t/db_dependent/Circulation.t (Wstat: 65280 Tests: 42 Failed: 1)
  Failed test:  41
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 112 tests but ran 42.

Also, running the QA tool highlighted some issues.

Created attachment 69759 [details] [review]
Bug 18606: (followup) Make QA tools happy

(In reply to Owen Leonard from comment #105)
> Testing functionality goes without any problem, but running "prove
> t/db_dependent/Koha/Items.t t/db_dependent/Circu lation.t" gave this result:
> 
> t/db_dependent/Circulation.t (Wstat: 65280 Tests: 42 Failed: 1)
>   Failed test:  41
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 112 tests but ran 42.
> 
> Also, running the QA tool highlighted some issues.

Thanks for testing!

I added a followup, test are passing now and QA tools return just one "warning" about pod coverage on Koha/RotatingCollection/Tracking.pm, but that is new file with no added methods, so there is nothing more to add to pod IMO.

Created attachment 72416 [details] [review]
Bug 18606: Add RotationCollections object classes

Test plan:
1) Apply the patch
2) Update database
3) Go to tools -> Rotating collections and play with trying to break it:
    3a) Add some collections
    3b) Edit them
    3c) Add some items to collections
    3d) Remove some items
    3e) Transfer some collections
    3f) Try to remove collection without items
    3g) Try to remove collection with items
4) Go to returns and:
    4a) Try to return item from untransferred collection
    4b) Try to return item from transferred collection
5) prove t/db_dependent/Koha/RotatingCollections.t
t/db_dependent/Koha/Items.t t/db_dependent/Circulation.t
t/db_dependent/Circulation/Returns.t

Created attachment 72417 [details] [review]
Bug 18606: Easy ones - CRUD

Created attachment 72418 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

Created attachment 72419 [details] [review]
Bug 18606: Schema changes

Created attachment 72420 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

Created attachment 72421 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

Created attachment 72422 [details] [review]
Bug 18606: Get rid of AddItemToCollection

Created attachment 72423 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

Created attachment 72424 [details] [review]
Bug 18606: Get rid of TransferCollection

Created attachment 72425 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

Created attachment 72426 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

Created attachment 72427 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

Created attachment 72428 [details] [review]
Bug 18606: Rework tests for rotating collections

Created attachment 72429 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t

Created attachment 72430 [details] [review]
Bug 18606: Unify handling of success/error messages

Created attachment 72431 [details] [review]
Bug 18606: (Follow-up) Make QA tools happy

Created attachment 72572 [details] [review]
Bug 18606: Add RotationCollections object classes

Test plan:
1) Apply the patch
2) Update database
3) Go to tools -> Rotating collections and play with trying to break it:
    3a) Add some collections
    3b) Edit them
    3c) Add some items to collections
    3d) Remove some items
    3e) Transfer some collections
    3f) Try to remove collection without items
    3g) Try to remove collection with items
4) Go to returns and:
    4a) Try to return item from untransferred collection
    4b) Try to return item from transferred collection
5) prove t/db_dependent/Koha/RotatingCollections.t
t/db_dependent/Koha/Items.t t/db_dependent/Circulation.t
t/db_dependent/Circulation/Returns.t

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

Created attachment 72573 [details] [review]
Bug 18606: Easy ones - CRUD

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

Created attachment 72574 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

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

Created attachment 72575 [details] [review]
Bug 18606: Schema changes

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

Created attachment 72576 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

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

Created attachment 72577 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

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

Created attachment 72578 [details] [review]
Bug 18606: Get rid of AddItemToCollection

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

Created attachment 72579 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

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

Created attachment 72580 [details] [review]
Bug 18606: Get rid of TransferCollection

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

Created attachment 72581 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

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

Created attachment 72582 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

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

Created attachment 72583 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

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

Created attachment 72584 [details] [review]
Bug 18606: Rework tests for rotating collections

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

Created attachment 72585 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t

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

Created attachment 72586 [details] [review]
Bug 18606: Unify handling of success/error messages

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

Created attachment 72587 [details] [review]
Bug 18606: (Follow-up) Make QA tools happy

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

QA question, should you not delete 'orphaned' collections_tracking rows before adding the foreign key constraints?

Created attachment 72816 [details] [review]
Bug 18606: (Follow-up) Remove lines from collections_tracking referencing non existant objects

(In reply to Kyle M Hall from comment #140)
> QA question, should you not delete 'orphaned' collections_tracking rows
> before adding the foreign key constraints?

Thank for testing Kyle!

I added a follow-up for this

Comment on attachment 72574 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

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

::: installer/data/mysql/kohastructure.sql
@@ +969,5 @@
> +  PRIMARY KEY (collections_tracking_id)
> +) ENGINE=InnoDB  DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
> +
> +ALTER TABLE `collections_tracking`
> +  ADD CONSTRAINT `collections_tracking_ibfk_1` FOREIGN KEY (`colId`) REFERENCES `collections` (`colId`) ON DELETE CASCADE ON UPDATE CASCADE;

Why did add constraints separatly from the table creation? It seems to me that it is less readable for people wanting to quickly get the structure of the table.

@@ +973,5 @@
> +  ADD CONSTRAINT `collections_tracking_ibfk_1` FOREIGN KEY (`colId`) REFERENCES `collections` (`colId`) ON DELETE CASCADE ON UPDATE CASCADE;
> +
> +ALTER TABLE `collections_tracking`
> +  ADD CONSTRAINT `collections_tracking_ibfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE CASCADE ON UPDATE CASCADE;
> +

idem

Also, we lost the "reason" in error messages when adding/removing items.
  - Failed to add item with barcode <barcode>: Item is already in this collection           => Failed to add item with barcode <barcode>
  - Failed to add item with barcode unknown_barcode: No item with matching barcode found    => Failed to add item with barcode unknown_barcode
  - Failed to remove item with barcode unknown_barcode: No item with matching barcode found => Failed to remove item with barcode unknown_barcode

Created attachment 79141 [details] [review]
Bug 18606: Add RotationCollections object classes

Test plan:
1) Apply the patch
2) Update database
3) Go to tools -> Rotating collections and play with trying to break it:
    3a) Add some collections
    3b) Edit them
    3c) Add some items to collections
    3d) Remove some items
    3e) Transfer some collections
    3f) Try to remove collection without items
    3g) Try to remove collection with items
4) Go to returns and:
    4a) Try to return item from untransferred collection
    4b) Try to return item from transferred collection
5) prove t/db_dependent/Koha/RotatingCollections.t
t/db_dependent/Koha/Items.t t/db_dependent/Circulation.t
t/db_dependent/Circulation/Returns.t

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

Created attachment 79142 [details] [review]
Bug 18606: Easy ones - CRUD

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

Created attachment 79143 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

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

Created attachment 79144 [details] [review]
Bug 18606: Schema changes

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

Created attachment 79145 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

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

Created attachment 79146 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

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

Created attachment 79147 [details] [review]
Bug 18606: Get rid of AddItemToCollection

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

Created attachment 79148 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

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

Created attachment 79149 [details] [review]
Bug 18606: Get rid of TransferCollection

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

Created attachment 79150 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

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

Created attachment 79151 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

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

Created attachment 79152 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

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

Created attachment 79153 [details] [review]
Bug 18606: Rework tests for rotating collections

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

Created attachment 79154 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t

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

Created attachment 79155 [details] [review]
Bug 18606: Unify handling of success/error messages

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

Created attachment 79156 [details] [review]
Bug 18606: (follow-up) Make QA tools happy

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

Created attachment 79157 [details] [review]
Bug 18606: (follow-up) Remove lines from collections_tracking referencing non existant objects

Created attachment 79158 [details] [review]
Bug 18606: (follow-up) Do not add contraints in kohastructure.sql separately

(In reply to Alex Arnaud from comment #143)
> Comment on attachment 72574 [details] [review] [review]
> Bug 18606: Database changes - Add foreign keys to collections_tracking
> 
> Review of attachment 72574 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +969,5 @@
> > +  PRIMARY KEY (collections_tracking_id)
> > +) ENGINE=InnoDB  DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
> > +
> > +ALTER TABLE `collections_tracking`
> > +  ADD CONSTRAINT `collections_tracking_ibfk_1` FOREIGN KEY (`colId`) REFERENCES `collections` (`colId`) ON DELETE CASCADE ON UPDATE CASCADE;
> 
> Why did add constraints separatly from the table creation? It seems to me
> that it is less readable for people wanting to quickly get the structure of
> the table.

No strong reason from me, fixed in follow-up.

(In reply to Alex Arnaud from comment #144)
> Also, we lost the "reason" in error messages when adding/removing items.
>   - Failed to add item with barcode <barcode>: Item is already in this
> collection           => Failed to add item with barcode <barcode>
>   - Failed to add item with barcode unknown_barcode: No item with matching
> barcode found    => Failed to add item with barcode unknown_barcode
>   - Failed to remove item with barcode unknown_barcode: No item with
> matching barcode found => Failed to remove item with barcode unknown_barcode

I rebased the patches on master and now I am working on the error messages, follow-up will come soon - hopefully

Created attachment 79161 [details] [review]
Bug 18606: Fix TT filters

Hi Josef, looking through some older bugs - should this still be FQA?

Still hoping to move this one forward Josef?