When on the “Serial collection information” page for a serial (e.g. /cgi-bin/koha/serials/serials-collection.pl?subscriptionid=24) and “Generate next” is clicked, the issue that previously had a status of “Expected” changes to “Late” and a new issue is generated with a status of “Expected”. When the serial with the status of “Late” is selected, and click on “Edit serials” and change the late item's status to “Expected” then one ends up with 2 issues with the status “Expected”. If the status of either of these “Expected” issues is changed to something else (e.g. “Arrived” or “Delete”) it generates a new issue with an “Expected” status, one can never get back to just having one issue “Expected”.
Created attachment 41228 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted
Created attachment 41230 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted Signed-off-by: Liz Rea <liz@catalyst.net.nz> Tests pass, serials are now deleted correctly.
I could hug you.
Created attachment 41371 [details] [review] Bug 12748: QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref
Rémi, Could you please provide some tests for the changes you have done to ModSerialStatus?
Created attachment 41451 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref
(In reply to Jonathan Druart from comment #5) > Rémi, Could you please provide some tests for the changes you have done to > ModSerialStatus? Are those the tests you wanted to see?
Yes, thanks
Created attachment 44533 [details] [review] y# Attachment to Bug 12748 - Serials - two issues with status of "Expected" Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment on attachment 44533 [details] [review] y# Attachment to Bug 12748 - Serials - two issues with status of Review of attachment 44533 [details] [review]: ----------------------------------------------------------------- 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted Would be good to have a test for this step too. ::: C4/Serials.pm @@ +1172,5 @@ > } > } > > + # create new expected entry if needed (ie : was "expected" and has changed) > + # BUG 12748: Check if there are no other expected issues. No need to set the bug# in the code, we use git. @@ +2810,5 @@ > +=head2 findSerialByStatus > + > + @serials = findSerialByStatus($status, $subscriptionid); > + > + Returns an array of serials matching a given status and subscription id. findSerialsByStatus sounds more appropriate, do you agree?
Hi Rémi, thx a lot for your patch - can you please take a look at Jonathan's comment#10?
Created attachment 46091 [details] [review] Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
patch does not apply cleanly CONFLICT (content): Merge conflict in t/db_dependent/Serials.t
Created attachment 47770 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref Signed-off-by: Liz Rea <liz@catalyst.net.nz> https://bugs.koha-community.org/show_bug.cgi?id=12478
Created attachment 47771 [details] [review] Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus https://bugs.koha-community.org/show_bug.cgi?id=12478
CONFLICT (content): Merge conflict in t/db_dependent/Serials.t Patch failed at 0001 Bug12748 - Fixes duplicate serials with an "expected" status bug
Created attachment 47773 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 47774 [details] [review] Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus https://bugs.koha-community.org/show_bug.cgi?id=12478
Created attachment 47775 [details] [review] Bug12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Created attachment 47776 [details] [review] Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus https://bugs.koha-community.org/show_bug.cgi?id=12478
(In reply to Rémi Mayrand-Provencher from comment #20) > Created attachment 47776 [details] [review] [review] > Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus > > https://bugs.koha-community.org/show_bug.cgi?id=12478 Tested and works well, fails QA tools Koha qa tools problem: FAIL t/db_dependent/Serials.t "my" variable @serialsByStatus masks earlier declaration in same scope
Can someone address the qa issues here, this is a big deal to some of our libraries? Nicoel
library-koha@wingardium-leviosa:~/koha-src/koha$ prove t/db_dependent/Serials.t t/db_dependent/Serials.t .. 1/49 "my" variable @serialsByStatus masks earlier declaration in same scope at t/db_dependent/Serials.t line 256. t/db_dependent/Serials.t .. 2/49 No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. t/db_dependent/Serials.t .. ok All tests successful. Files=1, Tests=49, 3 wallclock secs ( 0.02 usr 0.00 sys + 0.99 cusr 0.05 csys = 1.06 CPU) Result: PASS These tests look like they are passing to me.
Oops I meant to do that with -v: library-koha@wingardium-leviosa:~/koha-src/koha$ prove -v t/db_dependent/Serials.t t/db_dependent/Serials.t .. 1..49 ok 1 - use C4::Serials; "my" variable @serialsByStatus masks earlier declaration in same scope at t/db_dependent/Serials.t line 256. ok 2 - NewSubscription should set notes ok 3 - NewSubscription should set internalnotes ok 4 - NewSubscription should not set subscriptionhistory opacnotes ok 5 - NewSubscription should not set subscriptionhistory librariannotes ok 6 - A reference of type 'ARRAY' isa 'ARRAY' ok 7 - A reference of type 'ARRAY' isa 'ARRAY' ok 8 - A reference of type 'ARRAY' isa 'ARRAY' ok 9 - A reference of type 'ARRAY' isa 'ARRAY' ok 10 - expiration date is not NULL ok 11 - test getting history from sub-scription ok 12 - Subscription has at least one serial ok 13 - test getting Serial Status From Serial Id ok 14 - 'test getting Serial Information' isa 'HASH' ok 15 - test adding item to serial ok 16 - test updating claim date ok 17 - test getting full subscription ok 18 - test preparing serial data ok 19 - test getting subscriptions form biblio number ok 20 - test getting serials when you enter nothing ok 21 - test getting serials when you enter nothing ok 22 - test getting lastest serials ok 23 - test getting distributed when nothing is entered ok 24 - test getting next seq when you enter nothing ok 25 - test getting seq when you enter nothing ok 26 - test counting subscription when nothing is entered ok 27 - test modding subscription history ok 28 - test modding serials ok 29 - test finding serial by status with no parameters ok 30 - test getting 0 when nothing is entered ok 31 - test if the subscriptions has expired ok 32 - test if the subscriptions has expired ok 33 - test getting last or missing issues ok 34 - test updating claim ok 35 - test getting supplier idea ok 36 - test checking route ok 37 - test adding route member ok 38 - NewSubscription created a first serial ok 39 - GetSerials returns the serial No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. No method update! at /home/liz/koha-src/koha/C4/Serials.pm line 1603. ok 40 - findSerialByStatus returns all serials with chosen status ok 41 - GetSerials returns total_issues ok 42 - GetSerials returns 5 arrived/missing by default ok 43 - GetSerials returns all serials not arrived and not missing ok 44 - GetSerials returns total_issues ok 45 - GetSerials returns all arrived/missing if count given ok 46 - GetSerials returns all serials not arrived and not missing if count given ok 47 - subscription missinglist is updated after ModSerialStatus ok 48 - ModSerialStatus delete corectly serial expected and create another if not exist ok 49 - ModSerialStatus delete corectly serial expected and not create another if exist ok All tests successful. Files=1, Tests=49, 2 wallclock secs ( 0.02 usr 0.00 sys + 0.98 cusr 0.06 csys = 1.06 CPU) Result: PASS
Created attachment 52356 [details] [review] Bug 12748 - Fixes duplicate serials with an "expected" status bug Added a new sub to Serials.pm to be able to get serials with their statuses. Now the sub ModSerialStatus checks for other serials with an "expected" status before doing anything. Also modified Serials.t to be able to test those changes. Test Plan 1) Apply patch 2) Run ./t/db_dependent/Serials.t 3) Validate that there are no errors 4) Go on "Serial collection information" page for a serial of your choice 5) Click on "Generate next" 6) Change the status of the original serial from "late" to "expected" 7) Change the newly generated serial from "expected" to "delete" 8) Validate that there are no new serials created by instruction 7 and that the serial was deleted 9) Run ./t/db_dependent/Serials.t With QA Fixes - Use the constant instead of the code (1 vs EXPECTED) - Avoid interpolation in query - use selectall_arrayref instead of fetchall_arrayref Signed-off-by: Liz Rea <liz@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52357 [details] [review] Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus https://bugs.koha-community.org/show_bug.cgi?id=12478 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52358 [details] [review] Bug 12748: Remove warning 'masks earlier declaration in same scope' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52359 [details] [review] Bug 12748: Code tidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52360 [details] [review] Bug 12748: findSerialByStatus has been renamed with findSerialsByStatus Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Our institutions are happy to see this issue is getting worked on! We're a consortium and it's pretty confusing to see 5 or 6 expected issues, all with different dates. Thanks for working on this! Karl
Pushed to master for Koha 16.11, thanks Remi, Jonathan!
Pushed in 16.05. Will be in 16.05.01.
Patches pushed to 3.22.x, will be in 3.22.9