Bug 11814 - Serial statuses should be stored in constants
Summary: Serial statuses should be stored in constants
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 11689
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-21 15:01 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11814 - Use constants to describe statuses (3.34 KB, patch)
2014-02-21 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (10.45 KB, patch)
2014-02-21 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.59 KB, patch)
2014-04-25 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - Use constants to describe statuses (3.35 KB, patch)
2014-11-13 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.24 KB, patch)
2014-11-13 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.24 KB, patch)
2014-11-13 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.23 KB, patch)
2014-11-14 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814: Fix sql parameters order (790 bytes, patch)
2014-11-25 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - Use constants to describe statuses (3.50 KB, patch)
2014-11-27 11:19 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.59 KB, patch)
2014-11-27 11:20 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 11814: Fix sql parameters order (883 bytes, patch)
2014-11-27 11:20 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 11814: Fix existing tests (1.40 KB, patch)
2014-12-19 14:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11814 - Use constants to describe statuses (3.40 KB, patch)
2014-12-19 15:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11814 - (follow-up) Use constants to describe statuses (12.28 KB, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11814: Fix sql parameters order (850 bytes, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11814: Fix existing tests (1.41 KB, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11814 - Use constants to describe statuses (3.47 KB, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11814 - (follow-up) Use constants to describe statuses (12.35 KB, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11814: Fix sql parameters order (918 bytes, patch)
2014-12-19 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11814: Fix existing tests (1.47 KB, patch)
2014-12-19 15:24 UTC, Kyle M Hall
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-21 15:01:31 UTC
See bug 11689 comment 3
Comment 1 Jonathan Druart 2014-02-21 15:08:54 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-02-21 15:09:05 UTC Comment hidden (obsolete)
Comment 3 David Cook 2014-04-24 04:26:36 UTC
I really like the idea of this patch! It seems like it would make it a lot easier to do development on serials this way!

I'm curious about your follow-up. Are there are a few more places that the constants could be used in that SQL?

"AND (serial.STATUS IN ($missing_statuses_string) OR ((planneddate < now() AND serial.STATUS =1) OR serial.STATUS = 3 OR serial.STATUS = 7))"

Couldn't 1, 3, and 7 be exchanged a variable containing the value from the constant? 

I don't use serials very often, so I might let someone else with more experience with the workflow try this one out.
Comment 4 Jonathan Druart 2014-04-25 10:21:30 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-04-25 10:22:04 UTC
(In reply to David Cook from comment #3)
> Couldn't 1, 3, and 7 be exchanged a variable containing the value from the
> constant? 

Yes, they should.
Done in the last patch.
Comment 6 Paola Rossi 2014-11-12 16:52:10 UTC
I've tried to apply against master 3.17.00.054

Applying: Bug 11814 - Use constants to describe statuses
fatal: sha1 information is lacking or useless (C4/Serials.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11814 - Use constants to describe statuses

So I pass the patch to "Patch doesn't apply" status.
Comment 7 Jonathan Druart 2014-11-13 09:07:48 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-11-13 09:07:54 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-11-13 09:11:21 UTC Comment hidden (obsolete)
Comment 10 Paola Rossi 2014-11-13 16:43:35 UTC
I've applied the patch against master 3.17.00.054

I've created a subscription [length > 1 issues].
Selecting "Serial collection" of the subscription, I checked the EXPECTED issue and selected "Edit serials". 
I changed the issue from the EXPECTED status to the ARRIVED status.
A new issue in the EXPECTED status should have been created [the next issue], but it was not.

So I passed the patch to "Failed QA" status.
Comment 11 Jonathan Druart 2014-11-14 09:04:41 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-11-14 09:06:27 UTC
(In reply to Paola Rossi from comment #10)
> I've applied the patch against master 3.17.00.054
> 
> I've created a subscription [length > 1 issues].
> Selecting "Serial collection" of the subscription, I checked the EXPECTED
> issue and selected "Edit serials". 
> I changed the issue from the EXPECTED status to the ARRIVED status.
> A new issue in the EXPECTED status should have been created [the next
> issue], but it was not.


Paola,

Sorry for that, 1 replacement was wrong!
I checked all of them and it should be good now.
The new expected issue is now generated.

Changes are:

@@ -1245,7 +1245,7 @@ sub ModSerialStatus {
             }

             $missinglist .= "; $serialseq"
-                if ( ( grep { $_ == $status } ( MISSING_STATUSES, NOT_ISSUED ) ) && ( $missinglist !~/(^|;)\s*$serialseq(?=;|$)/ ) );
+                if ( ( grep { $_ == $status } ( MISSING_STATUSES ) ) && ( $missinglist !~/(^|;)\s*$serialseq(?=;|$)/ ) );
             $missinglist .= "; not issued $serialseq"
                 if ( $status == NOT_ISSUED && $missinglist !~ /(^|;)\s*$serialseq(?=;|$)/ );

@@ -1258,7 +1258,7 @@ sub ModSerialStatus {
     }

     # create new waited entry if needed (ie : was a "waited" and has changed)
-    if ( $oldstatus == ARRIVED && $status != ARRIVED ) {
+    if ( $oldstatus == EXPECTED && $status != EXPECTED ) {
         my $subscription = GetSubscription($subscriptionid);
         my $pattern = C4::Serials::Numberpattern::GetSubscriptionNumberpattern($subscription->{numberpattern});


The first one is a bad merge conflict.
Comment 13 Paola Rossi 2014-11-24 16:42:04 UTC
Thanks, Jonathan.

Today I've applied the patches against master 3.17.00.058 head 13297.

I think that the status codes in claims.pl had to be still patched.

On koha/serials/claims.pl:
Home › Serials › Claims

I've tried to claim 2 issues which were in late for a certain subscription. The mail was received correctly, but:
a) the "Claims counts" remained 0 for one of the issues (whose serial.serialid was X): ERROR 
b) the Claim date was not set for the same issue (whose serial.serialid was X): ERROR
c) the status of the other issue, whose Claim count and Claim date were set rightly, didn't become "Claimed" 7 (ERROR) [but X, I think].

So I pass the patch to "Failed QA" status.
Comment 14 Jonathan Druart 2014-11-25 09:02:44 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-11-25 09:03:58 UTC
(In reply to Paola Rossi from comment #13)
> Thanks, Jonathan.
> 
> Today I've applied the patches against master 3.17.00.058 head 13297.
> 
> I think that the status codes in claims.pl had to be still patched.

They are, but a sql query was not executed correctly.
The 3 issues you raised seem to be fixed now.
Comment 16 Paola Rossi 2014-11-27 11:19:48 UTC Comment hidden (obsolete)
Comment 17 Paola Rossi 2014-11-27 11:20:22 UTC Comment hidden (obsolete)
Comment 18 Paola Rossi 2014-11-27 11:20:58 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-12-19 14:40:38 UTC
Seems to be causing Serials.t to fail.
Comment 20 Jonathan Druart 2014-12-19 14:49:30 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2014-12-19 15:23:52 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2014-12-19 15:24:02 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2014-12-19 15:24:05 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-12-19 15:24:07 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-12-19 15:24:39 UTC
Created attachment 34567 [details] [review]
[PASSED QA] Bug 11814 - 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.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2014-12-19 15:24:46 UTC
Created attachment 34568 [details] [review]
[PASSED QA] Bug 11814 - (follow-up) Use constants to describe statuses

This patch deals with all hard-coded status codes in the C4::Serials
module.

Test plan:
Test a complete workflow in the serial module (create, order, receive,
generate next) trying to use all statuses.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2014-12-19 15:24:49 UTC
Created attachment 34569 [details] [review]
[PASSED QA] Bug 11814: Fix sql parameters order

Oops, bad paramaters passed on executing the sql query produced
unexpected behaviors.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2014-12-19 15:24:52 UTC
Created attachment 34570 [details] [review]
[PASSED QA] Bug 11814: Fix existing tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2015-04-30 18:35:03 UTC
Patches pushed to master.

Thanks Kyle and Jonathan!