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
Hi Paul, can you give a short update on this development?
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
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
*** Bug 7296 has been marked as a duplicate of this bug. ***
*** Bug 7297 has been marked as a duplicate of this bug. ***
Created attachment 6538 [details] [review] Proposed patch This patch adds 2 columns in the aqorders table : - claims_count : number of claims for an orders - claimed_date : date of the lastest claim In the lateorders.pl table, you can not select orders from different supplier because there is just one letter sent after clicking the "Claim order" button. So, it's logic that you want to select only orders from this supplier. Modification in C4/Letters.pm: refactoring code for claimacquisition and claimissues letter type. Now, fields for theses letters check the table name. It's not possible to chooce aqorders.title, this field doesn't exist ! Furthermore, you can add a <item> tag around your item fields, like this : -- Begin example <<LibrarianFirstname>> <<LibrarianSurname>> <<aqbooksellers.contact>> <<aqbooksellers.address1>> <<aqbooksellers.phone>> <<aqbasket.basketno>> <<aqbooksellers.phone>> <item>Library : <<items.homebranch>> In your possesssion : <<biblio.author>>. <<biblio.title>>. <<biblioitems.publishercode>>, <<biblioitems.publicationyear>>. Callnumber : <<items.itemcallnumber>>. doc type : <<items.itype>> Barcode : <<items.barcode>> Date for the return : <<items.onloan>>.</item> <<LibrarianSurname>> -- End example Needs signedoff
ref biblibre MT6541, MT6543, MT6547
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.
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>
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
Hi Jonathan, thx for your comments :)
Created attachment 6578 [details] [review] Proposed patch Adds sql modifications for all languages (contains de-DE, es-ES, it-IT, nb-NO)
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.
Created attachment 6634 [details] [review] Proposed patch rebase
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.
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 :(
Created attachment 6664 [details] [review] Proposed patch Hi Katrin, 1/ 2/ Ok, it's fixed 3/ Ok, I modified */sample_notices.sql by adding <item> tag 4/ I use <item> tag because this tag already exists in koha (misc/cronjobs/overdue_notices.pl and 'ODUE' notices). I think it's better to keep a general term to simplify things. > Sorry for not spotting those things earlier :( No problem, I can do everything for chocolate ;-)
:) 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.
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.
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.
I'm with Katrin and Chris, I think the <item> tag behavior should be consistent across all notices. Making it different = making it confusing.
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 ;^)
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/
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?
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).
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.
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!)
We talked about it on IRC already: I suggest compromising on <order> for the new tag? :)
Created attachment 6782 [details] [review] Proposed patch Replace item tag with order tag for claimacquisition and claimissues
Thx, Jonathan! Will try to test asap.
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' ) {
Created attachment 6808 [details] [review] Proposed patch Hi Katrin, 1a/ Corrected (my english is very poor as you see :-/) 1b/ Normally, the from field is created with the email key of C4::Context->userenv. This one get the email field from database for the logged user. Maybe you're logged as root ? I tried, it works for me. 2/ I forgot a replacement :-/ 3/ Ok; it's done 4/ No, I didn't modifify this section. I just add a warning to display the sent message. I added biblio and biblioitems in the field list when you modify a notice template.
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!
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.
Created attachment 6943 [details] [review] Followup patch Hi Katrin, Sorry for the delay :-/ This followup patch should be fix your encoding problem. Jonathan
Hi Jonathan - thx a lot! Will try to test asap :)
Created attachment 6961 [details] [review] [SIGNED-OFF] Bug 5347: Adds columns claims_count and claimed_date in aqorders This patch adds 2 columns in the aqorders table : - claims_count : number of claims for an orders - claimed_date : date of the lastest claim In the lateorders.pl table, you can not select orders from different supplier because there is just one letter sent after clicking the "Claim order" button. So, it's logic that you want to select only orders from this supplier. Modification in C4/Letters.pm: refactoring code for claimacquisition and claimissues letter type. Now, fields for theses letters check the table name. It's not possible to chooce aqorders.title, this field doesn't exist ! Furthermore, you can add a <order> tag around your item fields, like this : -- Begin example <<LibrarianFirstname>> <<LibrarianSurname>> <<aqbooksellers.contact>> <<aqbooksellers.address1>> <<aqbooksellers.phone>> <<aqbasket.basketno>> <<aqbooksellers.phone>> <order>Library : <<items.homebranch>> In your possesssion : <<biblio.author>>. <<biblio.title>>. <<biblioitems.publishercode>>, <<biblioitems.publicationyear>>. Callnumber : <<items.itemcallnumber>>. doc type : <<items.itype>> Barcode : <<items.barcode>> Date for the return : <<items.onloan>>.</order> <<LibrarianSurname>> -- End example Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 6962 [details] [review] [SIGNED-OFF] Bug 5347: Followup Adds columns claims_count and claimed_date in aqorders FIX encoding problem Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Umlauts and diacritics now show up correctly in generated claims notices.
Created attachment 6963 [details] [review] Bug 5347: Follow up - Fix warnings, check/uncheck all and makes error message translatable This patch fixes some minor problems found in late order management: 1) Silences 2 warns in Letters.pm After patch is applied no more warns should show up in the logs. 2) Fixes check/uncheck all When limiting on one vendor the checkbox in the last header column was doing nothing. I changed the checkbox to 2 links 'check all' and 'uncheck all' as it's done in other templates. 3) Email has been sent. The message was hardcoded into the lateorders.pl file and not translatable. I moved it to the template and changed the wording slightly. Note: The error message 'The bookseller has no email' comes from Letters.pm. I didn't change that, because I was not sure where it is used. The error message as is can not be translated and should be moved into the templates too.
(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 :-/
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 :)
Created attachment 6969 [details] [review] Follow up - Fix warnings, check/uncheck all and makes error message translatable It seems "1)" at the beginning of a line break the patch (???). I recreated your patch and it seems applied.
Created attachment 6970 [details] [review] Follow up - makes error message "no email" translatable
Hi Jonathan, can you sign off on my patch if it's ok? Will try to do so for yours later.
Created attachment 6991 [details] [review] [SIGNED-OFF] Bug 5347: Set translatable the "no email" error Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> This bookseller has no email shows up correctly, when vendor has no email address. Small change made: Changed bookseller to vendor.
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.
Created attachment 7008 [details] [review] [SIGNED-OFF] Bug 5347: Follow up - Fix warnings, check/uncheck all and makes error message translatable
Yay! Note for QA: Order of patches is 1-3, 5, 4
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
Created attachment 7196 [details] [review] Bug 5347: Followup change class for message + Unit test This patch modify the div class for "dialog message" and adds an unit test for the AddClaim function.
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 !
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 !
*** Bug 7481 has been marked as a duplicate of this bug. ***
Created attachment 8318 [details] [review] Bug 5374 Followup: adds conditional check to unit test for late orders If there are no late orders in the database, the 5th unit test for C4/Acquisition.pm fails, since no date is returned (and today's date is expected) This patch adds a conditional check around whether any late orders are actually returned by GetLateOrders. If so, tests proceed, if not, they're skipped. This seems to quiet the failures in the automated test suite.
Adding patch to handle testing better in cases where there are no late orders in the system.
check/uncheck all and makes error message translatable
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
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
I think the patch on bug 7756 fixes the problems noted here.
Created attachment 9165 [details] [review] Bug 5347: Followup: FIX 2 bugs in lateorders 1/ Failed to claim late orders (Can't use string as a HASH ref) 2/ Don't show the red error message after submit the form for claims
Created attachment 9166 [details] [review] Bug 5374 [SIGNED-OFF] Followup: adds conditional check to unit test for late orders Signed-off. I replace a line (gst with gstrate). Else, the test does not pass.
Created attachment 9208 [details] [review] [SIGNED-OFF] Bug 5347: Followup: FIX 2 bugs in lateorders 1/ Failed to claim late orders (Can't use string as a HASH ref) 2/ Don't show the red error message after submit the form for claims Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 1/ Fixed. 2/ Also fixed.
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.
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.
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)
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.
Created attachment 9254 [details] [review] Bug 5347 Follow up, fix for broken test
fix for broken test pushed
There have been no further reports of problems so I am marking this bug resolved.