Bug 12748

Summary: Serials - two issues with status of "Expected"
Product: Koha Reporter: Liz Rea <liz>
Component: SerialsAssignee: Bouzid Fergani <bouzid.fergani>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: aleisha, bouzid.fergani, colin.campbell, ellermaugustus, f.demians, janet.mcgowan, jonathan.druart, jschmidt, julian.maurice, katrin.fischer, kholten, kyle, marjorie.barry-vila, nick, remi.mayrand-provencher
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug12748 - Fixes duplicate serials with an "expected" status bug
Bug12748 - Fixes duplicate serials with an "expected" status bug
Bug 12748: QA Fixes
Bug12748 - Fixes duplicate serials with an "expected" status bug
y# Attachment to Bug 12748 - Serials - two issues with status of
Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
Bug12748 - Fixes duplicate serials with an "expected" status bug
Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
Bug12748 - Fixes duplicate serials with an "expected" status bug
Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
Bug12748 - Fixes duplicate serials with an "expected" status bug
Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
Bug 12748 - Fixes duplicate serials with an "expected" status bug
Bug 12748 - Add test for step 7 and 8 and rename findSerialByStatus
Bug 12748: Remove warning 'masks earlier declaration in same scope'
Bug 12748: Code tidy
Bug 12748: findSerialByStatus has been renamed with findSerialsByStatus

Description Liz Rea 2014-08-11 22:43:59 UTC
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”.
Comment 1 Rémi Mayrand-Provencher 2015-07-29 16:01:51 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2015-07-29 21:21:50 UTC Comment hidden (obsolete)
Comment 3 Liz Rea 2015-07-29 21:22:06 UTC
I could hug you.
Comment 4 Jonathan Druart 2015-08-05 08:44:55 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-08-05 08:45:47 UTC
Rémi, Could you please provide some tests for the changes you have done to ModSerialStatus?
Comment 6 Rémi Mayrand-Provencher 2015-08-11 15:16:32 UTC Comment hidden (obsolete)
Comment 7 Rémi Mayrand-Provencher 2015-08-11 15:23:52 UTC
(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?
Comment 8 Jonathan Druart 2015-08-19 13:33:44 UTC
Yes, thanks
Comment 9 Liz Rea 2015-11-06 01:59:31 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-11-13 09:54:31 UTC
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?
Comment 11 Katrin Fischer 2015-11-16 21:28:59 UTC
Hi Rémi, thx a lot for your patch - can you please take a look at Jonathan's comment#10?
Comment 12 Bouzid Fergani 2015-12-30 15:49:45 UTC Comment hidden (obsolete)
Comment 13 Gus Ellerm 2016-01-20 03:13:03 UTC
patch does not apply cleanly 
CONFLICT (content): Merge conflict in t/db_dependent/Serials.t
Comment 14 Rémi Mayrand-Provencher 2016-02-08 20:11:53 UTC Comment hidden (obsolete)
Comment 15 Rémi Mayrand-Provencher 2016-02-08 20:11:56 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2016-02-08 21:10:39 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Serials.t
Patch failed at 0001 Bug12748 - Fixes duplicate serials with an "expected" status bug
Comment 17 Rémi Mayrand-Provencher 2016-02-08 21:16:04 UTC Comment hidden (obsolete)
Comment 18 Rémi Mayrand-Provencher 2016-02-08 21:16:07 UTC Comment hidden (obsolete)
Comment 19 Rémi Mayrand-Provencher 2016-02-08 21:26:20 UTC Comment hidden (obsolete)
Comment 20 Rémi Mayrand-Provencher 2016-02-08 21:26:33 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2016-04-05 13:12:31 UTC
(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
Comment 22 Nicole C. Engard 2016-06-13 18:57:35 UTC
Can someone address the qa issues here, this is a big deal to some of our libraries?

Nicoel
Comment 23 Liz Rea 2016-06-13 23:24:28 UTC
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.
Comment 24 Liz Rea 2016-06-13 23:25:22 UTC
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
Comment 25 Jonathan Druart 2016-06-14 09:53:36 UTC
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>
Comment 26 Jonathan Druart 2016-06-14 09:53:42 UTC
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>
Comment 27 Jonathan Druart 2016-06-14 09:53:47 UTC
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>
Comment 28 Jonathan Druart 2016-06-14 09:53:52 UTC
Created attachment 52359 [details] [review]
Bug 12748: Code tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2016-06-14 09:53:56 UTC
Created attachment 52360 [details] [review]
Bug 12748: findSerialByStatus has been renamed with findSerialsByStatus

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Karl Holten 2016-06-14 20:02:07 UTC
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
Comment 31 Kyle M Hall 2016-06-17 14:27:17 UTC
Pushed to master for Koha 16.11, thanks Remi, Jonathan!
Comment 32 Frédéric Demians 2016-06-24 05:43:36 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 33 Julian Maurice 2016-07-01 06:27:16 UTC
Patches pushed to 3.22.x, will be in 3.22.9