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.

(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.

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.

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.

(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.

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.

(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.

Seems to be causing Serials.t to fail.

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>

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>

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>

Created attachment 34570 [details] [review]
[PASSED QA] Bug 11814: Fix existing tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Patches pushed to master.

Thanks Kyle and Jonathan!