Bug 11689 - New missing statuses is not well managed
Summary: New missing statuses is not well managed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 10851
Blocks: 11814
  Show dependency treegraph
 
Reported: 2014-02-05 10:26 UTC by Jonathan Druart
Modified: 2014-12-07 20:07 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11689: New missing statuses are not well managed (2.84 KB, patch)
2014-02-05 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11689: New missing statuses are not well managed (2.90 KB, patch)
2014-02-16 05:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11689 [QA Followup] - Use constants to describe statuses (3.36 KB, patch)
2014-02-21 14:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11689 [QA Followup] - Use constants to describe statuses (3.36 KB, patch)
2014-02-21 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11689: New missing statuses are not well managed (3.01 KB, patch)
2014-02-26 06:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11689: Add unit tests for serials statuses (4.11 KB, patch)
2014-02-27 12:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11689: Remove warning launching unit tests (2.15 KB, patch)
2014-02-27 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11637: don't try to guess publication dates for irregular subscriptions (2.55 KB, patch)
2014-03-16 21:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11689: New missing statuses are not well managed (3.01 KB, patch)
2014-03-16 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11689: Add unit tests for serials statuses (4.18 KB, patch)
2014-03-16 21:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11689: Remove warning launching unit tests (2.21 KB, patch)
2014-03-16 21:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-02-05 10:26:28 UTC
Bug 10851 introduced new missing status (codes 41,42,43,44), but in GetSerials and _update_missinglist, they are not take into account.
Comment 1 Jonathan Druart 2014-02-05 10:34:38 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-02-16 05:03:36 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-02-21 13:15:56 UTC
Jonathan, I wonder if it would be best to define the list of statuses in a package constant in C4::Serials, and to replace all the references to the array of statuses with this constant to make future additions simpler.

use constant MISSING_STATUSES => [ 2, 4, 41, 42, 43, 44, 5 ];

If you think this isn't an idea worth pursuing just reset the status to signed off.
Comment 4 Jonathan Druart 2014-02-21 13:49:43 UTC
(In reply to Kyle M Hall from comment #3)
> Jonathan, I wonder if it would be best to define the list of statuses in a
> package constant in C4::Serials, and to replace all the references to the
> array of statuses with this constant to make future additions simpler.

Kyle,
Yes of course, it is a good idea. But this one is major. I think that your suggestion is an enhancement, not a bugfix.
I tried to fix the issue by making as few changes as possible.
Comment 5 Kyle M Hall 2014-02-21 13:52:45 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Kyle M Hall from comment #3)
> > Jonathan, I wonder if it would be best to define the list of statuses in a
> > package constant in C4::Serials, and to replace all the references to the
> > array of statuses with this constant to make future additions simpler.
> 
> Kyle,
> Yes of course, it is a good idea. But this one is major. I think that your
> suggestion is an enhancement, not a bugfix.
> I tried to fix the issue by making as few changes as possible.

Understood. I would still consider that a bug fix rather than an enhancement though.
Comment 6 Kyle M Hall 2014-02-21 13:54:47 UTC
> Understood. I would still consider that a bug fix rather than an enhancement
> though.

Ignore this, I was thinking of the wrong bug!
Comment 7 Kyle M Hall 2014-02-21 14:20:23 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2014-02-21 14:22:13 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-02-21 15:03:08 UTC
Comment on attachment 25538 [details] [review]
Bug 11689 [QA Followup] - Use constants to describe statuses

In agreement with Kyle, I move this patch to another report, see bug 11814.
Comment 10 Jonathan Druart 2014-02-21 15:03:40 UTC
And switch this one back in the need QA queue.
Comment 11 Katrin Fischer 2014-02-26 06:37:16 UTC
I applied the patch, then added 'generate next' some more issues to an existing subscription. Then I edited them all in one go, setting different missing status. The summary tab of the subscription shows up completely empty for me - I am not sure yet what's going wrong.
Comment 12 Katrin Fischer 2014-02-26 06:45:04 UTC
I think the problem is the behaviour of the manual history checkbox. If it's set in the subscription, the summary seems no longer to be updated. This seems not the right behaviour, as the page states:

Hint : you can update the serial history manually. This can be useful for an old subscription or to clean the existing history. Modify those fields with care, as future serial recieve will continue to update them automatically.

I think it used to be, that the new issues were added at the end, but the old information was kept and it should not regenerate the history in that case? But I am not totally sure here.
Comment 13 Katrin Fischer 2014-02-26 06:55:06 UTC
I filed bug 11843 - passing QA on this, the problems are not related.
Comment 14 Katrin Fischer 2014-02-26 06:56:00 UTC Comment hidden (obsolete)
Comment 15 Galen Charlton 2014-02-26 16:20:16 UTC
Particularly in light of the planned use of a constant (+1 to that, BTW), for this one I'm going to insist on that a regression test be written before I will push this to master.
Comment 16 Jonathan Druart 2014-02-27 12:18:51 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-02-27 12:19:04 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-02-27 12:20:26 UTC
2 last patches need QA.
Comment 19 Katrin Fischer 2014-03-16 21:17:25 UTC
I think this change could create a problem with Jenkins - depending on which dateformat the instance uses. I seem to remember it was MM/DD/YYYY:

-    budget_period_enddate   => '12-31-2015',
+    budget_period_enddate   => '31-12-2015',

But not sure, I am going to pass, as the tests pass even with MM/DD/YYYY set.
Comment 20 Katrin Fischer 2014-03-16 21:20:29 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-03-16 21:21:49 UTC
Created attachment 26390 [details] [review]
[PASSED QA] Bug 11689: New missing statuses are not well managed

Bug 10851 introduced new missing status (codes 41,42,43,44), but in
GetSerials and _update_missinglist, they are not take into account.

To reproduce:
1/ Create a serial with 10 issues.
2/ Set different statuses on each one, with at least 6 missing statuses
(not only "Missing").
3/ Go on the subscription detail page, tab "Summary", the issues with a
new missing status are not listed in the missing issues list.
4/ On the "Issues" tab, all missing are listed (normally only 5 should
be listed.
5/ Apply the patch.
6/ Edit serial (to rewrite the missing list).
6/ Verify that steps 3 and 4 have now correct behavior.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, passes QA script and tests.
Comment 22 Katrin Fischer 2014-03-16 21:22:04 UTC
Created attachment 26391 [details] [review]
[PASSED QA] Bug 11689: Add unit tests for serials statuses

prove t/db_dependent/Serials.t

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 23 Katrin Fischer 2014-03-16 21:22:16 UTC
Created attachment 26392 [details] [review]
[PASSED QA] Bug 11689: Remove warning launching unit tests

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 24 Galen Charlton 2014-04-18 21:13:17 UTC
Pushed to master.  Thanks, Jonathan!