Bug 18606

Summary: Move rotating collections code to Koha::Object
Product: Koha Reporter: Josef Moravec <josef.moravec>
Component: Architecture, internals, and plumbingAssignee: Josef Moravec <josef.moravec>
Status: Failed QA --- QA Contact: Alex Arnaud <alex.arnaud>
Severity: enhancement    
Priority: P5 - low CC: aleisha, alex.arnaud, jonathan.druart, katrin.fischer, kyle, marjorie.barry-vila, martin.renvoize, patrick.robitaille
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17202
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 29842, 17202    
Bug Blocks: 15449, 19470, 19519    
Attachments: Bug 18606: Add Koha::RotatingCollection[s] object classes
Bug 18606: Get rid of DeleteCollection
Bug 18606: Get rid of GetCollection
Bug 18606: Get rid off GetCollections
Bug 18606: Get rid of CreateCollection
Bug 18606: Get rid of UpdateCollection
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Add RotatingCollectionTracking objects
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Fix issues to pass tests
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (followup) Fix double delete confirmation
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (followup) Fix double delete confirmation
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (followup) Make QA tools happy
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (Follow-up) Make QA tools happy
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (Follow-up) Make QA tools happy
Bug 18606: (Follow-up) Remove lines from collections_tracking referencing non existant objects
Bug 18606: Add RotationCollections object classes
Bug 18606: Easy ones - CRUD
Bug 18606: Database changes - Add foreign keys to collections_tracking
Bug 18606: Schema changes
Bug 18606: Get rid of GetItemsInCollection
Bug 18606: Get rid of GetCollectionItemBranches
Bug 18606: Get rid of AddItemToCollection
Bug 18606: Get rid of RemoveItemFromCollection
Bug 18606: Get rid of TransferCollection
Bug 18606: Get rid of isItemInThisCollection
Bug 18606: Get rid of isItemInAnyCollection
Bug 18606: Remove lefover of C4::RotatingCollections
Bug 18606: Rework tests for rotating collections
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Bug 18606: Unify handling of success/error messages
Bug 18606: (follow-up) Make QA tools happy
Bug 18606: (follow-up) Remove lines from collections_tracking referencing non existant objects
Bug 18606: (follow-up) Do not add contraints in kohastructure.sql separately
Bug 18606: Fix TT filters

Description Josef Moravec 2017-05-15 11:47:39 UTC

    
Comment 1 Josef Moravec 2017-05-18 09:21:12 UTC Comment hidden (obsolete)
Comment 2 Josef Moravec 2017-05-18 09:21:17 UTC Comment hidden (obsolete)
Comment 3 Josef Moravec 2017-05-18 09:21:21 UTC Comment hidden (obsolete)
Comment 4 Josef Moravec 2017-05-18 09:21:25 UTC Comment hidden (obsolete)
Comment 5 Josef Moravec 2017-05-18 09:21:29 UTC Comment hidden (obsolete)
Comment 6 Josef Moravec 2017-05-18 09:21:33 UTC Comment hidden (obsolete)
Comment 7 Josef Moravec 2017-05-18 09:21:37 UTC Comment hidden (obsolete)
Comment 8 Josef Moravec 2017-05-18 09:21:41 UTC Comment hidden (obsolete)
Comment 9 Josef Moravec 2017-05-18 09:21:46 UTC Comment hidden (obsolete)
Comment 10 Josef Moravec 2017-05-18 09:21:50 UTC Comment hidden (obsolete)
Comment 11 Josef Moravec 2017-05-18 09:21:54 UTC Comment hidden (obsolete)
Comment 12 Josef Moravec 2017-05-18 09:21:58 UTC Comment hidden (obsolete)
Comment 13 Josef Moravec 2017-05-18 09:22:02 UTC Comment hidden (obsolete)
Comment 14 Josef Moravec 2017-05-18 09:22:09 UTC Comment hidden (obsolete)
Comment 15 Josef Moravec 2017-05-18 09:22:13 UTC Comment hidden (obsolete)
Comment 16 Josef Moravec 2017-05-18 09:22:17 UTC Comment hidden (obsolete)
Comment 17 Josef Moravec 2017-05-18 09:22:22 UTC Comment hidden (obsolete)
Comment 18 Josef Moravec 2017-05-18 09:22:26 UTC Comment hidden (obsolete)
Comment 19 Josef Moravec 2017-05-18 09:22:30 UTC Comment hidden (obsolete)
Comment 20 Josef Moravec 2017-05-18 09:22:34 UTC Comment hidden (obsolete)
Comment 21 Josef Moravec 2017-05-18 09:22:38 UTC Comment hidden (obsolete)
Comment 22 Josef Moravec 2017-05-18 09:24:31 UTC
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.
Comment 23 Josef Moravec 2017-08-20 08:12:37 UTC Comment hidden (obsolete)
Comment 24 Josef Moravec 2017-08-20 08:12:42 UTC Comment hidden (obsolete)
Comment 25 Josef Moravec 2017-08-20 08:12:47 UTC Comment hidden (obsolete)
Comment 26 Josef Moravec 2017-08-20 08:12:51 UTC Comment hidden (obsolete)
Comment 27 Josef Moravec 2017-08-20 08:12:56 UTC
Created attachment 66231 [details] [review]
Bug 18606: Get rid of GetItemsInCollection
Comment 28 Josef Moravec 2017-08-20 08:13:01 UTC Comment hidden (obsolete)
Comment 29 Josef Moravec 2017-08-20 08:13:05 UTC Comment hidden (obsolete)
Comment 30 Josef Moravec 2017-08-20 08:13:10 UTC Comment hidden (obsolete)
Comment 31 Josef Moravec 2017-08-20 08:13:15 UTC Comment hidden (obsolete)
Comment 32 Josef Moravec 2017-08-20 08:13:20 UTC Comment hidden (obsolete)
Comment 33 Josef Moravec 2017-08-20 08:13:25 UTC Comment hidden (obsolete)
Comment 34 Josef Moravec 2017-08-20 08:13:29 UTC Comment hidden (obsolete)
Comment 35 Josef Moravec 2017-08-20 08:13:33 UTC Comment hidden (obsolete)
Comment 36 Josef Moravec 2017-08-20 08:13:37 UTC Comment hidden (obsolete)
Comment 37 Josef Moravec 2017-08-20 08:13:42 UTC Comment hidden (obsolete)
Comment 38 Josef Moravec 2017-08-21 20:24:54 UTC Comment hidden (obsolete)
Comment 39 Josef Moravec 2017-08-21 20:25:02 UTC Comment hidden (obsolete)
Comment 40 Josef Moravec 2017-08-21 20:25:11 UTC Comment hidden (obsolete)
Comment 41 Josef Moravec 2017-08-21 20:25:20 UTC Comment hidden (obsolete)
Comment 42 Josef Moravec 2017-08-21 20:25:29 UTC Comment hidden (obsolete)
Comment 43 Josef Moravec 2017-08-21 20:25:34 UTC Comment hidden (obsolete)
Comment 44 Josef Moravec 2017-08-21 20:25:41 UTC Comment hidden (obsolete)
Comment 45 Josef Moravec 2017-08-21 20:25:47 UTC Comment hidden (obsolete)
Comment 46 Josef Moravec 2017-08-21 20:25:54 UTC Comment hidden (obsolete)
Comment 47 Josef Moravec 2017-08-21 20:26:00 UTC Comment hidden (obsolete)
Comment 48 Josef Moravec 2017-08-21 20:26:06 UTC Comment hidden (obsolete)
Comment 49 Josef Moravec 2017-08-21 20:26:13 UTC Comment hidden (obsolete)
Comment 50 Josef Moravec 2017-08-21 20:26:19 UTC Comment hidden (obsolete)
Comment 51 Josef Moravec 2017-08-21 20:26:26 UTC Comment hidden (obsolete)
Comment 52 Josef Moravec 2017-08-21 20:26:32 UTC Comment hidden (obsolete)
Comment 53 Josef Moravec 2017-08-21 20:27:42 UTC
I've rebased this on top of master, squashed few commits, fix issues, now it should be working and ready to test.
Comment 54 Aleisha Amohia 2017-08-27 22:14:03 UTC
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.
Comment 55 Josef Moravec 2017-10-06 17:14:18 UTC Comment hidden (obsolete)
Comment 56 Josef Moravec 2017-10-06 17:14:22 UTC Comment hidden (obsolete)
Comment 57 Josef Moravec 2017-10-06 17:14:26 UTC Comment hidden (obsolete)
Comment 58 Josef Moravec 2017-10-06 17:14:29 UTC Comment hidden (obsolete)
Comment 59 Josef Moravec 2017-10-06 17:14:33 UTC Comment hidden (obsolete)
Comment 60 Josef Moravec 2017-10-06 17:14:37 UTC Comment hidden (obsolete)
Comment 61 Josef Moravec 2017-10-06 17:14:41 UTC Comment hidden (obsolete)
Comment 62 Josef Moravec 2017-10-06 17:14:45 UTC Comment hidden (obsolete)
Comment 63 Josef Moravec 2017-10-06 17:14:48 UTC Comment hidden (obsolete)
Comment 64 Josef Moravec 2017-10-06 17:14:52 UTC Comment hidden (obsolete)
Comment 65 Josef Moravec 2017-10-06 17:14:55 UTC Comment hidden (obsolete)
Comment 66 Josef Moravec 2017-10-06 17:14:59 UTC Comment hidden (obsolete)
Comment 67 Josef Moravec 2017-10-06 17:15:03 UTC Comment hidden (obsolete)
Comment 68 Josef Moravec 2017-10-06 17:15:06 UTC Comment hidden (obsolete)
Comment 69 Josef Moravec 2017-10-06 17:15:10 UTC Comment hidden (obsolete)
Comment 70 Josef Moravec 2017-10-06 17:15:14 UTC Comment hidden (obsolete)
Comment 71 Josef Moravec 2017-10-06 17:15:32 UTC
(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
Comment 72 Josef Moravec 2017-10-06 17:19:00 UTC
> Ability to delete a collection with items is current behavior in master, I
> will fix it on another report

It is Bug 17202
Comment 73 Josef Moravec 2017-10-23 18:18:21 UTC Comment hidden (obsolete)
Comment 74 Josef Moravec 2017-10-23 18:18:31 UTC Comment hidden (obsolete)
Comment 75 Josef Moravec 2017-10-23 18:18:36 UTC Comment hidden (obsolete)
Comment 76 Josef Moravec 2017-10-23 18:18:42 UTC Comment hidden (obsolete)
Comment 77 Josef Moravec 2017-10-23 18:18:48 UTC Comment hidden (obsolete)
Comment 78 Josef Moravec 2017-10-23 18:18:53 UTC Comment hidden (obsolete)
Comment 79 Josef Moravec 2017-10-23 18:18:58 UTC Comment hidden (obsolete)
Comment 80 Josef Moravec 2017-10-23 18:19:03 UTC Comment hidden (obsolete)
Comment 81 Josef Moravec 2017-10-23 18:19:10 UTC Comment hidden (obsolete)
Comment 82 Josef Moravec 2017-10-23 18:19:16 UTC Comment hidden (obsolete)
Comment 83 Josef Moravec 2017-10-23 18:19:22 UTC Comment hidden (obsolete)
Comment 84 Josef Moravec 2017-10-23 18:19:27 UTC Comment hidden (obsolete)
Comment 85 Josef Moravec 2017-10-23 18:19:33 UTC Comment hidden (obsolete)
Comment 86 Josef Moravec 2017-10-23 18:19:42 UTC Comment hidden (obsolete)
Comment 87 Josef Moravec 2017-10-23 18:19:49 UTC Comment hidden (obsolete)
Comment 88 Josef Moravec 2017-10-23 18:19:56 UTC Comment hidden (obsolete)
Comment 89 Josef Moravec 2017-10-23 18:21:26 UTC
Just rebased on master (conflict in C4/Circulation.pm because of bug 14826)
Comment 90 Josef Moravec 2017-12-12 08:54:38 UTC
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
Comment 91 Josef Moravec 2017-12-12 08:54:42 UTC
Created attachment 69710 [details] [review]
Bug 18606: Easy ones - CRUD
Comment 92 Josef Moravec 2017-12-12 08:54:46 UTC
Created attachment 69711 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking
Comment 93 Josef Moravec 2017-12-12 08:54:50 UTC
Created attachment 69712 [details] [review]
Bug 18606: Schema changes
Comment 94 Josef Moravec 2017-12-12 08:54:53 UTC
Created attachment 69713 [details] [review]
Bug 18606: Get rid of GetItemsInCollection
Comment 95 Josef Moravec 2017-12-12 08:54:57 UTC
Created attachment 69714 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches
Comment 96 Josef Moravec 2017-12-12 08:55:01 UTC
Created attachment 69715 [details] [review]
Bug 18606: Get rid of AddItemToCollection
Comment 97 Josef Moravec 2017-12-12 08:55:05 UTC
Created attachment 69716 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection
Comment 98 Josef Moravec 2017-12-12 08:55:09 UTC
Created attachment 69717 [details] [review]
Bug 18606: Get rid of TransferCollection
Comment 99 Josef Moravec 2017-12-12 08:55:12 UTC
Created attachment 69718 [details] [review]
Bug 18606: Get rid of isItemInThisCollection
Comment 100 Josef Moravec 2017-12-12 08:55:16 UTC
Created attachment 69719 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection
Comment 101 Josef Moravec 2017-12-12 08:55:20 UTC
Created attachment 69720 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections
Comment 102 Josef Moravec 2017-12-12 08:55:24 UTC
Created attachment 69721 [details] [review]
Bug 18606: Rework tests for rotating collections
Comment 103 Josef Moravec 2017-12-12 08:55:28 UTC
Created attachment 69722 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Comment 104 Josef Moravec 2017-12-12 08:55:32 UTC
Created attachment 69723 [details] [review]
Bug 18606: Unify handling of success/error messages
Comment 105 Owen Leonard 2017-12-12 17:22:30 UTC
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.
Comment 106 Josef Moravec 2017-12-13 06:44:12 UTC
Created attachment 69759 [details] [review]
Bug 18606: (followup) Make QA tools happy
Comment 107 Josef Moravec 2017-12-13 06:48:38 UTC
(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.
Comment 108 Josef Moravec 2018-03-05 11:17:54 UTC
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
Comment 109 Josef Moravec 2018-03-05 11:17:58 UTC
Created attachment 72417 [details] [review]
Bug 18606: Easy ones - CRUD
Comment 110 Josef Moravec 2018-03-05 11:18:02 UTC
Created attachment 72418 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking
Comment 111 Josef Moravec 2018-03-05 11:18:06 UTC
Created attachment 72419 [details] [review]
Bug 18606: Schema changes
Comment 112 Josef Moravec 2018-03-05 11:18:10 UTC
Created attachment 72420 [details] [review]
Bug 18606: Get rid of GetItemsInCollection
Comment 113 Josef Moravec 2018-03-05 11:18:14 UTC
Created attachment 72421 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches
Comment 114 Josef Moravec 2018-03-05 11:18:19 UTC
Created attachment 72422 [details] [review]
Bug 18606: Get rid of AddItemToCollection
Comment 115 Josef Moravec 2018-03-05 11:18:23 UTC
Created attachment 72423 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection
Comment 116 Josef Moravec 2018-03-05 11:18:27 UTC
Created attachment 72424 [details] [review]
Bug 18606: Get rid of TransferCollection
Comment 117 Josef Moravec 2018-03-05 11:18:31 UTC
Created attachment 72425 [details] [review]
Bug 18606: Get rid of isItemInThisCollection
Comment 118 Josef Moravec 2018-03-05 11:18:35 UTC
Created attachment 72426 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection
Comment 119 Josef Moravec 2018-03-05 11:18:39 UTC
Created attachment 72427 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections
Comment 120 Josef Moravec 2018-03-05 11:18:43 UTC
Created attachment 72428 [details] [review]
Bug 18606: Rework tests for rotating collections
Comment 121 Josef Moravec 2018-03-05 11:18:47 UTC
Created attachment 72429 [details] [review]
Bug 18606: Add test to t/db_dependent/Koha/Items.t
Comment 122 Josef Moravec 2018-03-05 11:18:51 UTC
Created attachment 72430 [details] [review]
Bug 18606: Unify handling of success/error messages
Comment 123 Josef Moravec 2018-03-05 11:18:55 UTC
Created attachment 72431 [details] [review]
Bug 18606: (Follow-up) Make QA tools happy
Comment 124 Kyle M Hall 2018-03-09 13:02:26 UTC
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>
Comment 125 Kyle M Hall 2018-03-09 13:02:36 UTC
Created attachment 72573 [details] [review]
Bug 18606: Easy ones - CRUD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 126 Kyle M Hall 2018-03-09 13:02:40 UTC
Created attachment 72574 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 127 Kyle M Hall 2018-03-09 13:02:44 UTC
Created attachment 72575 [details] [review]
Bug 18606: Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 128 Kyle M Hall 2018-03-09 13:02:48 UTC
Created attachment 72576 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 129 Kyle M Hall 2018-03-09 13:02:53 UTC
Created attachment 72577 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 130 Kyle M Hall 2018-03-09 13:02:58 UTC
Created attachment 72578 [details] [review]
Bug 18606: Get rid of AddItemToCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 131 Kyle M Hall 2018-03-09 13:03:03 UTC
Created attachment 72579 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 132 Kyle M Hall 2018-03-09 13:03:07 UTC
Created attachment 72580 [details] [review]
Bug 18606: Get rid of TransferCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 133 Kyle M Hall 2018-03-09 13:03:11 UTC
Created attachment 72581 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 134 Kyle M Hall 2018-03-09 13:03:17 UTC
Created attachment 72582 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 135 Kyle M Hall 2018-03-09 13:03:21 UTC
Created attachment 72583 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 136 Kyle M Hall 2018-03-09 13:03:26 UTC
Created attachment 72584 [details] [review]
Bug 18606: Rework tests for rotating collections

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 137 Kyle M Hall 2018-03-09 13:03:30 UTC
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>
Comment 138 Kyle M Hall 2018-03-09 13:03:34 UTC
Created attachment 72586 [details] [review]
Bug 18606: Unify handling of success/error messages

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 139 Kyle M Hall 2018-03-09 13:03:38 UTC
Created attachment 72587 [details] [review]
Bug 18606: (Follow-up) Make QA tools happy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 140 Kyle M Hall 2018-03-09 13:04:56 UTC
QA question, should you not delete 'orphaned' collections_tracking rows before adding the foreign key constraints?
Comment 141 Josef Moravec 2018-03-13 20:52:35 UTC
Created attachment 72816 [details] [review]
Bug 18606: (Follow-up) Remove lines from collections_tracking referencing non existant objects
Comment 142 Josef Moravec 2018-03-13 20:53:25 UTC
(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 143 Alex Arnaud 2018-04-06 14:54:00 UTC
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
Comment 144 Alex Arnaud 2018-04-06 14:59:11 UTC
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
Comment 145 Josef Moravec 2018-09-20 11:17:47 UTC
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>
Comment 146 Josef Moravec 2018-09-20 11:17:53 UTC
Created attachment 79142 [details] [review]
Bug 18606: Easy ones - CRUD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 147 Josef Moravec 2018-09-20 11:17:58 UTC
Created attachment 79143 [details] [review]
Bug 18606: Database changes - Add foreign keys to collections_tracking

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 148 Josef Moravec 2018-09-20 11:18:03 UTC
Created attachment 79144 [details] [review]
Bug 18606: Schema changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 149 Josef Moravec 2018-09-20 11:18:08 UTC
Created attachment 79145 [details] [review]
Bug 18606: Get rid of GetItemsInCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 150 Josef Moravec 2018-09-20 11:18:13 UTC
Created attachment 79146 [details] [review]
Bug 18606: Get rid of GetCollectionItemBranches

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 151 Josef Moravec 2018-09-20 11:18:19 UTC
Created attachment 79147 [details] [review]
Bug 18606: Get rid of AddItemToCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 152 Josef Moravec 2018-09-20 11:18:24 UTC
Created attachment 79148 [details] [review]
Bug 18606: Get rid of RemoveItemFromCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 153 Josef Moravec 2018-09-20 11:18:29 UTC
Created attachment 79149 [details] [review]
Bug 18606: Get rid of TransferCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 154 Josef Moravec 2018-09-20 11:18:34 UTC
Created attachment 79150 [details] [review]
Bug 18606: Get rid of isItemInThisCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 155 Josef Moravec 2018-09-20 11:18:40 UTC
Created attachment 79151 [details] [review]
Bug 18606: Get rid of isItemInAnyCollection

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 156 Josef Moravec 2018-09-20 11:18:45 UTC
Created attachment 79152 [details] [review]
Bug 18606: Remove lefover of C4::RotatingCollections

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 157 Josef Moravec 2018-09-20 11:18:51 UTC
Created attachment 79153 [details] [review]
Bug 18606: Rework tests for rotating collections

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 158 Josef Moravec 2018-09-20 11:18:56 UTC
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>
Comment 159 Josef Moravec 2018-09-20 11:19:01 UTC
Created attachment 79155 [details] [review]
Bug 18606: Unify handling of success/error messages

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 160 Josef Moravec 2018-09-20 11:19:06 UTC
Created attachment 79156 [details] [review]
Bug 18606: (follow-up) Make QA tools happy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 161 Josef Moravec 2018-09-20 11:19:11 UTC
Created attachment 79157 [details] [review]
Bug 18606: (follow-up) Remove lines from collections_tracking referencing non existant objects
Comment 162 Josef Moravec 2018-09-20 11:19:17 UTC
Created attachment 79158 [details] [review]
Bug 18606: (follow-up) Do not add contraints in kohastructure.sql separately
Comment 163 Josef Moravec 2018-09-20 11:19:56 UTC
(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.
Comment 164 Josef Moravec 2018-09-20 11:20:50 UTC
(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
Comment 165 Josef Moravec 2018-09-20 11:58:21 UTC
Created attachment 79161 [details] [review]
Bug 18606: Fix TT filters
Comment 166 Katrin Fischer 2018-10-27 12:08:06 UTC
Hi Josef, looking through some older bugs - should this still be FQA?
Comment 167 Martin Renvoize 2019-12-23 09:28:34 UTC
Still hoping to move this one forward Josef?