Bug 10851 introduced new missing status (codes 41,42,43,44), but in GetSerials and _update_missinglist, they are not take into account.
Created attachment 25047 [details] [review] 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.
Created attachment 25308 [details] [review] 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>
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.
(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.
(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.
> Understood. I would still consider that a bug fix rather than an enhancement > though. Ignore this, I was thinking of the wrong bug!
Created attachment 25537 [details] [review] Bug 11689 [QA Followup] - Use constants to describe statuses Make the code a bit more understandable by using constants to define the status values and reference those. Adds self-documentation.
Created attachment 25538 [details] [review] Bug 11689 [QA Followup] - Use constants to describe statuses Make the code a bit more understandable by using constants to define the status values and reference those. Adds self-documentation.
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.
And switch this one back in the need QA queue.
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.
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.
I filed bug 11843 - passing QA on this, the problems are not related.
Created attachment 25631 [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.
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.
Created attachment 25695 [details] [review] Bug 11689: Add unit tests for serials statuses prove t/db_dependent/Serials.t Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 25696 [details] [review] Bug 11689: Remove warning launching unit tests Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
2 last patches need QA.
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.
Created attachment 26389 [details] [review] Bug 11637: don't try to guess publication dates for irregular subscriptions Test plan: Create a subscription with an irregular numbering pattern. Verify the prediction pattern. With this patch, it should show "unknown" instead of the date of the first issue publication date. This patch does not fix the issue on generating the next serial. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> This is a partial fix for a small regression in the serials module. Now the prediction pattern doesn't predict dates when you test it, and shows 'Unknown' instead. We still need to fix the 'planned date' set for the expected serial. There should be no date, it should also be 'unknown'. Passes tests and QA script. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
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.
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>
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>
Pushed to master. Thanks, Jonathan!