Bug 5347 - late order management
Summary: late order management
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.8
Hardware: All All
: P2 enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Paul Poulain
URL: http://wiki.koha-community.org/wiki/L...
Keywords:
: 7296 7297 7481 (view as bug list)
Depends on:
Blocks: 7163 7100 7101 7102 7103 7104 7165 7756
  Show dependency treegraph
 
Reported: 2010-11-02 03:17 UTC by Paul Poulain
Modified: 2014-08-29 12:55 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (41.63 KB, patch)
2011-12-02 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (41.63 KB, patch)
2011-12-05 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (41.60 KB, patch)
2011-12-07 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
screenshot (83.04 KB, image/png)
2011-12-08 07:12 UTC, Katrin Fischer
Details
Proposed patch (54.36 KB, patch)
2011-12-08 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (54.39 KB, patch)
2011-12-14 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Proposed patch (55.63 KB, patch)
2011-12-15 11:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Followup patch (1.21 KB, patch)
2011-12-28 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5347: Adds columns claims_count and claimed_date in aqorders (55.68 KB, patch)
2011-12-30 01:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5347: Followup Adds columns claims_count and claimed_date in aqorders (1.34 KB, patch)
2011-12-30 01:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5347: Follow up - Fix warnings, check/uncheck all and makes error message translatable (5.37 KB, patch)
2011-12-30 01:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Follow up - Fix warnings, check/uncheck all and makes error message translatable (5.38 KB, patch)
2011-12-30 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Follow up - makes error message "no email" translatable (2.98 KB, patch)
2011-12-30 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5347: Set translatable the "no email" error (3.17 KB, patch)
2011-12-31 07:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5347: Follow up - Fix warnings, check/uncheck all and makes error message translatable (5.45 KB, patch)
2012-01-02 08:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5347: Followup change class for message + Unit test (2.51 KB, patch)
2012-01-17 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5374 Followup: adds conditional check to unit test for late orders (1.70 KB, patch)
2012-03-19 23:19 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 5347: Followup: FIX 2 bugs in lateorders (1.26 KB, patch)
2012-04-13 07:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5374 [SIGNED-OFF] Followup: adds conditional check to unit test for late orders (2.11 KB, patch)
2012-04-13 07:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5347: Followup: FIX 2 bugs in lateorders (975 bytes, patch)
2012-04-15 17:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5347 Follow up, fix for broken test (825 bytes, patch)
2012-04-19 09:34 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-02 03:17:47 UTC
For lateorders, we will add fields for date and comment coming from the library.
For claims, we will add
* a claim counter, to know how many times an order has been claimed
* a claim table, containing all the claims an order has, with the date of the claim and the content of the claim
Comment 1 Katrin Fischer 2011-10-27 10:00:32 UTC
Hi Paul, 
can you give a short update on this development?
Comment 2 Paul Poulain 2011-10-27 10:14:23 UTC
Katrin, see our ggdocs "biblibre developments"
https://docs.google.com/a/biblibre.com/spreadsheet/ccc?key=0AuZF5Y_c4pIxdHE3S0RXMjJqSzZ3d1BVUmxpRnRVUUE&hl=en_US#gid=0

(line 43 at this time)

This is done (but on a 3.2, so with HTML::Template), but not yet submitted mainstream.

Note for anyone reading this : anyone with the link can look at the document. If you need/want write access to comment, ask me
Comment 3 Paul Poulain 2011-11-05 10:56:18 UTC
If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 6541
Comment 4 claire.hernandez@biblibre.com 2011-12-02 13:21:49 UTC
*** Bug 7296 has been marked as a duplicate of this bug. ***
Comment 5 claire.hernandez@biblibre.com 2011-12-02 13:22:17 UTC
*** Bug 7297 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2011-12-02 15:35:06 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2011-12-02 15:39:38 UTC
ref biblibre MT6541, MT6543, MT6547
Comment 8 Katrin Fischer 2011-12-03 19:58:19 UTC
Hi Jonathan, 

your patch applies and counter and claim date get populated in my tests. Also the notice is created and sent out. Some questons:

1)
You write that it's possible to use <item> and fields from the item table in the acqclaim notice. This seems not to work for me and the example you gave seems to be taken from a totally different notice?
Did you mean a list of titles? (biblio level information?)
A list of titles works for me.

2) 
For some languages the sample acqclaim notice is not corrected. Could you add the missing corrections?

3) 
Would it be possible/desirable to make the message go through the message queue? I see that the full notice is logged in action_logs if the logs are turned on.
Comment 9 Katrin Fischer 2011-12-03 20:04:13 UTC
My ACQCLAIM looks like this:

<<aqbooksellers.name>>
<<aqbooksellers.address1>>
<<aqbooksellers.address2>>
<<aqbooksellers.address3>>
<<aqbooksellers.address4>>
<<aqbooksellers.phone>>

Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered)($<<aqorders.listprice>> each) has not been received.

<item>
<<items.homebranch>>
<<biblio.author>>. <<biblio.title>>
<<biblioitems.publishercode>>, <<biblioitems.publicationyear>>
Callnumber: <<items.itemcallnumber>>
Itemtype:  <<items.itype>>
Barcode: <<items.barcode>>
</item>
Comment 10 Jonathan Druart 2011-12-05 10:30:15 UTC
Hi Katrin,

1/ Exactly, my example is not good :-/

A good example : 
<<aqbooksellers.name>>
<<aqbooksellers.address1>>
<<aqbooksellers.address2>>
<<aqbooksellers.address3>>
<<aqbooksellers.address4>>
<<aqbooksellers.phone>>

<item>Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered) ($<<aqorders.listprice>> each) has not been received.</item>

For the ACQCLAIM notice, we can select fields from tables branches, aqbooksellers, aqorders, biblio and biblioitems.

2/ Ok, I'm going to create a new patch containing modifications for all languages

3/ Yes, it's desirable but it's not the goal of this development
Comment 11 Katrin Fischer 2011-12-05 10:36:37 UTC
Hi Jonathan, thx for your comments :)
Comment 12 Jonathan Druart 2011-12-05 10:38:37 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2011-12-06 19:51:17 UTC
Hi Jonathan,
thx for your work on this. I tried to be fast, but was not fast enought - there is conflict in the css file:

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 5347: Adds columns claims_count and claimed_date in aqorders
Using index info to reconstruct a base tree...
<stdin>:324: trailing whitespace.
    # send an "account details" notice to a newly created user 
<stdin>:486: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
Failed to merge in the changes.
Patch failed at 0001 Bug 5347: Adds columns claims_count and claimed_date in aqorders
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Can you please fix and add a new patch? Will try to test asap.
Comment 14 Jonathan Druart 2011-12-07 08:13:52 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2011-12-08 07:12:48 UTC
Created attachment 6661 [details]
screenshot

Hi Jonathan, 

found 2 small things:

1) very small:
Could you make the column headers start with Capitals to match the others?

2) more important:
I noticed that the claim date is not formatted like the order date is. See screenshot attached. It should be formatted according to the DateFormat preference.
Comment 16 Katrin Fischer 2011-12-08 07:21:31 UTC
3) The corrected default notice will not output a list:

ACQCLAIM
Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered)($<<aqorders.listprice>> each) has not been received.

Email:
Ordernumber 10 (City of Fallen Angels) (1 ordered)($10.00 each) has not been received.

Using that works better:
ACQCLAIM
<item>Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered)($<<aqorders.listprice>> each) has not been received.</item>

I think the <item> tags (or another tag, see 4) ) should be added to the notices too.

Email:
Ordernumber 10 (City of Fallen Angels) (1 ordered)($10.00 each) has not been received.
Ordernumber 11 (Die Staatl. Ingenieurschule in Konstanz /) (1 ordered)($3.00 each) has not been received.

4) 
I think using the <item> tag here might be confusing, because people expect a list of items instead of records. Perhaps it would be better to use <title> or <record> or <biblio>?

Sorry for not spotting those things earlier :(
Comment 17 Jonathan Druart 2011-12-08 11:41:18 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2011-12-08 12:00:55 UTC
:)

Let me explain 4)
ODUE notices gives you item information. You can also display the title information, but it's one line per item. The ACQCLAIM gives you one line per ordered title and you can't display item information in there yet. (And it would only make sense for libraries using AcqCreateItem = on order anyway) But if we keep the "1 line per title" and "1 line per item" in separate tags, we could perhaps later have a construct like:

<record>
<<biblio.title>>
<item><<item.callnumber>>.</item>
</record>

And it would be always clear if you get a list of records, or a list of items.
Comment 19 Francois Charbonnier 2011-12-08 14:24:41 UTC
Hi Katrin,

To me, your proposition introduce complexity. 

It looks like you are focusing on the idea of "item" vs "biblios" but actually, to me, it's a distinction that we can do because we know the database structure.

But on a librarian perspective, I order a book and I claim a book. Either way, I buy or claim a physical document and i don't make any difference if the information comes from the biblio or the item table.

At the end, it's just a list of documents (items) that i want. It can be books that I bought or overdue books... 

So, I think it's easier to handle only one tag.
Comment 20 Chris Cormack 2011-12-08 21:53:48 UTC
Hmm, I disagree, I think being consistent is not being more complex, if we use the <item> to mean different things in 2 places in the notices, I think that actually introduces complexity.
Comment 21 Liz Rea 2011-12-08 22:00:17 UTC
I'm with Katrin and Chris, I think the <item> tag behavior should be consistent across all notices. Making it different = making it confusing.
Comment 22 Francois Charbonnier 2011-12-09 10:02:30 UTC
well, the thing is, to me, it does the same thing...

whether it's the acquisition claim notice or the overdue notice, the <item> tag allow you to display a list of books. 

For the acquisition claim, you will have one line for a book with the quantity ordered, and for the overdue notice, one line for one overdue book... At the end, it just a list of books. 

And as a librarian, I used to buy books, not biblio records. So, today, I understand the logic as a "techie" but before working on koha, I think it would have been more confusing having two tags if we keep the <item> tag the way it works today.

Because, the <item> tag can already display biblio info...

So imagine, we have two tags, what are going to say to librarians? 

- how can i display info about overdue books in the odue notice? 
- well, you can use the item tag to display item information.
- but, what about the title and the author?
- you can put these information in the item tag.
- great! ;^)
- and now how can I display info about books I ordered and didn't receive in the acqclaim notice?
- for this one, you can use the record tag  because what you want for is only the biblio information
- why not the <item> tag? i can use it to display the title and the author in the odue notice...
- ...

What I become to realise, writing all this, is that you are maybe right but it should be another development. That's not the purpose of this one. 

The use of the <item> tag for this develpment is still relevent _today_ because :
* it's already in koha and it works,
* it displays a list of books and that what we want
* it can show item and biblio info as well and that what we need

So if we want to introduce a new way to display item and biblio info. Why not! But, i think it should be another development.

My 2cts ;^)
Comment 23 Francois Charbonnier 2011-12-09 10:12:17 UTC
Sorry, just to sum it up :

The way it works today, the <item> tag behavior is consistent across all notices.
* it displays a list of books
* it can show item and biblio info as well
If it's not what we want, this behavior should be change within another development, it' not the purpose of this one...

5 lines... woooo! \0/
Comment 24 Francois Charbonnier 2011-12-09 10:25:16 UTC
OK, Katrin just told me that :
<kf> francharb: the <item> in claims CAN NOT display item information :)
<kf> that's the difference
<kf> it can only display title level information at the moment
<kf> and you get a list of books - but not of item information

Something i didn't understand at the first place.

So now, what do you think we should do? 

* use this devlopement to change the behavior of the notice tags and use two tags instead of one (one for the record level info and one for item level info) ?

* or just fix the item tag behabior in the acqclaim notice for now ? and introduce a new tag later, with a specific devlopment?
Comment 25 Katrin Fischer 2011-12-09 10:36:42 UTC
I think different tags is the way to go. 

ACQCLAIM can not display item information when you are using AcqCreateItem = receive or AcqCreateItem = in cataloging. All it has and can display is biblio level information from a list of biblionumbers.

I think the main difference is how the information is build. From a list of itemnumbers (ODUE) or from a list of biblionumbers (ACQCLAIM).
First should be <item>, second should be <somethingelse>.
When you have an itemnumber, you can also pull in information from the levels above (biblioitems, biblio).
Comment 26 Katrin Fischer 2011-12-09 10:41:55 UTC
Also, I was hoping to open the door for something like this:

<record>
<<biblio.title>>
<item><<item.callnumber>>.</item>
</record>

Where I would only display the Title (or other information that is the same for every item) once, with a list of callnumbers (or other item related information like branch for which it was ordered) below. 

At the moment this would not work for several reasons and in ODUE notices it does not make so much sense, because you check out one specific item. But I think it could make sense for notices in serials, acquisisions and other new things. For example if we had notices for cart and list information send out to the user.
Comment 27 Francois Charbonnier 2011-12-09 12:58:06 UTC
I was thinking about it.

In the end, if we have this :
<record>
<<biblio.title>>
<item><<item.callnumber>>.</item>
</record>

How do we manage <<aqorders.quantity>> for the ACQCLAIM notice? 

Moreover, I don't think it's relevant to manage item level information within ACQCLAIM notice (even if libraries create item at this level)... Because I don't think vendors care about itemcallnumber, barcode or location for a specific library...

So, before doing something like that, maybe we should do a RFC and dig this up with librarians who order books, etc. I've been into that position years ago but I'm interested about what other librarians could say.

On the IRC, Katrin said that for the moment, we could just rename this tag and it would enough. 

Katrin, did you mean "rename the tag for every notice" or "rename the tag for the ACQCLAIM notice only"?

If we rename the tag, i suggest <list> :
* within the odue notice, it's a list of books where we can get biblio and item level information.
* within the acqclaim notice, it's also a list of books where we can get only biblio level information.

(PS : if you know situations where the vendor will be interested knowing item level information, let me know, I'm interested!)
Comment 28 Katrin Fischer 2011-12-13 07:37:15 UTC
We talked about it on IRC already: I suggest compromising on <order> for the new tag? :)
Comment 29 Jonathan Druart 2011-12-14 14:13:40 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2011-12-14 14:14:39 UTC
Thx, Jonathan! Will try to test asap.
Comment 31 Katrin Fischer 2011-12-15 07:45:35 UTC
Hi Jonathan,
I have problems with the email now. It used to work with your earlier patches. Perhaps something went wrong when you added the nice eedback?

I have set up:
- KohaAdminEmailAddress
- Vendor's email
- Staff patron's email

1 )When I had no email for the vendor the system complained:

a) This bookseller have no email 
> Little correction: This vendor has no email address ?

b) I added an email address but have problems sending the mail:
(I changed the full email address to ...)

katrin@...
SMTP error from remote mail server after RCPT TO:<katrin@...>:
host smtp.... [82.100.220.166]: 504 5.5.2 <root@localhost>:
Sender address rejected: need fully-qualified address

I have no idea why it's trying to send from root@localhost as I can't spot that in my configuration now.

Could you take a look?

2) Also: The message content is a bit short for my notice template, the order information seems to be missing?

----- This is a copy of the message, including all the headers. ------

Return-path: <root@localhost>
Received: from localhost ([127.0.0.1] helo=bumblebee)
by bumblebee with esmtp (Exim 4.76)
(envelope-from <root@localhost>)
id 1Rb5is-0001bI-5o
for katrin@...; Thu, 15 Dec 2011 08:27:22 +0100
Subject: Item Not Received
Date: Thu, 15 Dec 2011 08:27:22 +0100
Mime-Version: 1.0
Content-Type: text/plain; charset="utf8"
To: katrin@...
Content-Transfer-Encoding: quoted-printable
From: root@localhost
Message-Id: <E1Rb5is-0001bI-5o@bumblebee>

B=FCcherschiff

<<aqbooksellers.name>>
<<aqbooksellers.address1>>
<<aqbooksellers.address2>>
<<aqbooksellers.address3>>
<<aqbooksellers.address4>>
<<aqbooksellers.phone>>

<order>
Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered) ($<<aqorders.listprice>> each) has not been received.
</order>

3) One last thing that would not keep me from signing off but is a little bit annoying: After trying to send out the mail, you get redirected to the start page of 'late orders'. It should bring me back to my list of orders that I have searched for so I can see the new value for the counter and the date.

4) I see that your patch also changes the ACCTDETAILS message? Not sure if I am misreading that:

-   # send an "account details" notice to a newly created user 
+
+        warn "sending to From $userenv->{emailaddress} subj $innerletter->{title} Mess $innerletter->{content}";
+    }
+
+    # send an "account details" notice to a newly created user 
     elsif ( $type eq 'members' ) {
Comment 32 Jonathan Druart 2011-12-15 11:08:08 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2011-12-15 13:04:20 UTC
Hi Jonathan, 
you are right about 1)! I had to rebuild my database from scratch after I accidently deleted my dumped database this morning (don't ask...) and forgot to change my login. I will test again and thank you for fixing everything else!
Comment 34 Katrin Fischer 2011-12-16 06:40:15 UTC
1) - 4) resolved - thx again! This gets more beautiful every time I look at it and it converts the late orders into a really useful feature.

There is only one more problem I spotted and I hope it can be resolved easily. The diacritics in my email are broken. I used titles and a vendor name with umlauts. They display is wrong in my web client and in Thunderbird.


B�cherschiff


Ordernumber 1 (Handbuch der Religionsp�dagogik.) (1 ordered) ($10.00 each) has not been received.


Ordernumber 2 (Das Sch�ne Konstanz am Bodensee und Rhein, die alte Stadt im deutschen S�den  (1 ordered) ($10.00 each) has not been received.
Comment 35 Jonathan Druart 2011-12-28 08:50:53 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2011-12-28 08:56:00 UTC
Hi Jonathan - thx a lot! Will try to test asap :)
Comment 37 Katrin Fischer 2011-12-30 01:05:55 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2011-12-30 01:06:24 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2011-12-30 01:06:33 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2011-12-30 09:13:43 UTC
(In reply to comment #39)
I can't apply your patch. Have you created it after applying 2 others patchs ?

I got :
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.

You're right about translation of "no email" error.
We can replace in SendAlerts l.402
die "This bookseller has no email\n";
with 
return { error => "no_email" };

and in acqui/lateorders.pl
replacing
    eval {
        SendAlerts( 'claimacquisition', \@ordernums, $input->param("letter_code") );
        AddClaim ( $_ ) for @ordernums;
    };
    if ( $@ ) {
        $template->param(error_claim => $@);
    } else {
        $template->param(info_claim => "Emails have been sent");
    }
with something like :
    eval {
        my $err = SendAlerts( 'claimacquisition', \@ordernums, $input->param("letter_code") );
        AddClaim ( $_ ) for @ordernums;
    };
    if ( $@ ) {
        $template->param(error_claim => $@);
    } elsif ( defined $err{error} ) {
        $template->{VARS}->{'error_claim'} = 'no_email';
    } else {
        $template->{VARS}->{'info_claim'} = 1;
    }

I could propose a patch but without applying your patch, it's not useful :-/
Comment 41 Katrin Fischer 2011-12-30 09:18:47 UTC
Hi Jonathan, thx for your comment - my patch goes on top of your 2, it's a follow up fixing some things I found while testing. I can add your suggestions to my follow up - or you can sign off on this and I will on yours :)
Comment 42 Jonathan Druart 2011-12-30 09:40:27 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2011-12-30 09:41:32 UTC Comment hidden (obsolete)
Comment 44 Katrin Fischer 2011-12-30 11:05:13 UTC
Hi Jonathan, can you sign off on my patch if it's ok? Will try to do so for yours later.
Comment 45 Katrin Fischer 2011-12-31 07:32:18 UTC Comment hidden (obsolete)
Comment 46 Katrin Fischer 2011-12-31 07:34:17 UTC
This is only missing a sign-off for the third patch from me now. 
Patch 1 and 2 must be applied before testing the third.
Comment 47 Jonathan Druart 2012-01-02 08:55:57 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2012-01-02 09:01:47 UTC
Yay!

Note for QA: Order of patches is 1-3, 5, 4
Comment 49 Paul Poulain 2012-01-13 15:12:10 UTC
QA comments:
1 what's the use of div.info in staff.css ? couldn't we use an existing class ?
2 there is a new sub in C4/Acquisitions.pm (AddClaim), but no unit test. I've already pushed a patch with a new sub here and no unit test, so I won't refuse this one, but that would be a plus.
* Other than this = good code, fixes some crapy things in previously existing code, perlcritic OK, except for C4/Letter.pm that was not perlcritic-compliant before the patch, so no regression here

not marking passed QA because or point 1 & 2
Comment 50 Jonathan Druart 2012-01-17 10:43:23 UTC Comment hidden (obsolete)
Comment 51 Paul Poulain 2012-01-18 16:58:14 UTC
this time, passed QA !!!
I will do a little bit last minut checks tomorrow, as it's a large patch, and will push just after !
Comment 52 Paul Poulain 2012-01-20 13:17:58 UTC
Patch pushed, nice feature, thx St Etienne university for sponsoring it !

A last comment : it would be nice to have the claim counter on the basket.pl page, displayed if there is at least one claim of something like that. But that could be another improvement. Katrin, if you think it's a good idea, file a new bug !
Comment 53 Adrien SAURAT 2012-01-31 15:45:22 UTC
*** Bug 7481 has been marked as a duplicate of this bug. ***
Comment 54 Ian Walls 2012-03-19 23:19:46 UTC Comment hidden (obsolete)
Comment 55 Ian Walls 2012-03-19 23:27:45 UTC
Adding patch to handle testing better in cases where there are no late orders in the system.
Comment 56 Katrin Fischer 2012-04-08 12:02:12 UTC
check/uncheck all and makes error message translatable
Comment 57 Katrin Fischer 2012-04-08 12:06:03 UTC
I checked git and all patches except Ian's last patch were pushed to master.

The bad news is: I get a nasty perl error when trying to claim some late orders in master now. Marking this blocker.

Can't use string ("1") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/acqui/lateorders.pl line 88.

Errors/warnings from the logs are:
[Sun Apr 08 14:00:27 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:27 2012] lateorders.pl: Bad or missing From address: '', referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
[Sun Apr 08 14:00:27 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:27 2012] lateorders.pl:  at /home/katrin/kohaclone/acqui/lateorders.pl line 83, referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
[Sun Apr 08 14:00:27 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:27 2012] lateorders.pl: Can't use string ("1") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/acqui/lateorders.pl line 88., referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
[Sun Apr 08 14:00:58 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:58 2012] lateorders.pl: Bad or missing From address: '', referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
[Sun Apr 08 14:00:58 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:58 2012] lateorders.pl:  at /home/katrin/kohaclone/acqui/lateorders.pl line 83, referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
[Sun Apr 08 14:00:58 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:58 2012] lateorders.pl: Can't use string ("1") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/acqui/lateorders.pl line 88., referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
Comment 58 Katrin Fischer 2012-04-08 12:08:53 UTC
I fixed the missing email address in my staff patron causing the error below, but the perl error remains:

[Sun Apr 08 14:00:27 2012] [error] [client 127.0.0.1] [Sun Apr  8 14:00:27 2012] lateorders.pl: Bad or missing From address: '', referer: http://localhost:8080/cgi-bin/koha/acqui/lateorders.pl
Comment 59 Katrin Fischer 2012-04-10 19:54:34 UTC
I think the patch on bug 7756 fixes the problems noted here.
Comment 60 Jonathan Druart 2012-04-13 07:18:50 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2012-04-13 07:51:04 UTC Comment hidden (obsolete)
Comment 62 Katrin Fischer 2012-04-15 17:04:19 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2012-04-15 17:10:04 UTC
Both patches are signed off now but I found some smaller problems in the mail:

Output:
<order>Ordernumber 21 (Wissenschaft und Bildung  (3 bestellt) (je EUR ).</order>
<order>Ordernumber 22 (...) (1 bestellt) (je EUR )</order>

Notice template:
<order>Ordernumber <<aqorders.ordernumber>> (<<aqorders.title>>) (<<aqorders.quantity>> bestellt) (je <<aqorders.listprice>> )</order>

1) The <order></order> tags are not removed.
2) aqorders.listprice doesn't work. I have checked that the field was not empty in the database.
Comment 64 Ian Walls 2012-04-16 14:39:17 UTC
First patch fixes busted unit test (thank you!)

Second patch changes 'defined $delay' check to just '$delay', implying $delay may not be zero. I would think that this makes sense, since a delay of 0 is no delay at all.

Marking both attached as Passed QA.
Comment 65 Paul Poulain 2012-04-18 15:06:04 UTC
the 2 follow-up have been pushed. The test has been pushed on the branch bug_5347 while the other has been pushed on master because it does not apply on bug_5347 anymore

(switching back to ENH, as the bug describe an ENH)
Comment 66 Chris Cormack 2012-04-19 09:29:47 UTC
This is causing the tests to fail

specifically

-                             gst
+                             gstrate

But in the kohastructure.sql

The column is gst, (gstrate is aqbooksellers).

Patch to fix the test on its way.
Comment 67 Chris Cormack 2012-04-19 09:34:04 UTC
Created attachment 9254 [details] [review]
Bug 5347 Follow up, fix for broken test
Comment 68 Paul Poulain 2012-04-19 10:06:15 UTC
fix for broken test pushed
Comment 69 Jared Camins-Esakov 2012-12-31 00:56:09 UTC
There have been no further reports of problems so I am marking this bug resolved.