Bug 7298 - export late orders, with # of claims and claim dates
Summary: export late orders, with # of claims and claim dates
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 5356
Blocks: 7163 14264
  Show dependency treegraph
 
Reported: 2011-12-02 09:38 UTC by claire.hernandez@biblibre.com
Modified: 2015-05-24 16:07 UTC (History)
12 users (show)

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


Attachments
Bug 7298 Export late orders as csv (10.62 KB, patch)
2012-02-22 16:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
CSV file contains no order data (45.35 KB, image/png)
2012-02-26 15:27 UTC, Katrin Fischer
Details
Bug 7298 Followup rename export scripts (16.65 KB, patch)
2012-02-29 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
csv file (178 bytes, text/csv)
2012-03-20 09:23 UTC, Koha Team University Lyon 3
Details
Bug 7298 Export late orders as csv (12.76 KB, patch)
2012-06-22 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
bottom row misaligned (169.18 KB, image/png)
2012-08-20 20:03 UTC, Nicole C. Engard
Details
Bug 7298: Followup, FIX table bottom (1.19 KB, patch)
2012-08-21 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298 Export late orders as csv (12.84 KB, patch)
2012-08-21 15:11 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298: Followup, FIX table bottom (1.25 KB, patch)
2012-08-21 15:11 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7298 fixing 2 QA errors (2.84 KB, patch)
2012-10-01 17:40 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7298: Followup FIX the field list to return in GetOrder (3.75 KB, patch)
2012-10-15 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: Followup FIX the field list to return in GetOrder (3.75 KB, patch)
2012-10-15 15:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298 fixing 3 QA errors (3.60 KB, patch)
2013-02-04 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: Followup FIX the field list to return in GetOrder (3.75 KB, patch)
2013-02-04 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298 Export late orders as csv (12.94 KB, patch)
2013-02-04 16:51 UTC, mathieu saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298: Followup, FIX table bottom (1.35 KB, patch)
2013-02-04 16:51 UTC, mathieu saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298 fixing 3 QA errors (3.70 KB, patch)
2013-02-04 16:51 UTC, mathieu saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7298: Followup FIX the field list to return in GetOrder (3.87 KB, patch)
2013-02-04 16:51 UTC, mathieu saby
Details | Diff | Splinter Review
[PASSED QA] Bug 7298 Export late orders as csv (12.95 KB, patch)
2013-03-21 22:28 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: Followup, FIX table bottom (1.37 KB, patch)
2013-03-21 22:29 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 7298 fixing 3 QA errors (3.71 KB, patch)
2013-03-21 22:29 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: Followup FIX the field list to return in GetOrder (3.88 KB, patch)
2013-03-21 22:29 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] bug 7298 QA follow-up, fix POD (889 bytes, patch)
2013-03-21 22:29 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7298 Export late orders as csv (16.56 KB, patch)
2013-03-28 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: Follow-up fixing capitalization and moving supplier id (3.58 KB, patch)
2013-07-20 17:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7298: Follow-up fixing capitalization and moving supplier id (3.66 KB, patch)
2013-08-13 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: FIX checkboxes binding on load (1.02 KB, patch)
2013-08-13 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: CSV strings are translatables. (4.46 KB, patch)
2013-08-13 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298 Export late orders as csv (16.60 KB, patch)
2013-08-19 15:01 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 7298: Follow-up fixing capitalization and moving supplier id (3.68 KB, patch)
2013-08-19 15:01 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 7298: FIX checkboxes binding on load (1.07 KB, patch)
2013-08-19 15:01 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 7298: CSV strings are translatables. (4.50 KB, patch)
2013-08-19 15:01 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 7298: QA Followup (5.92 KB, patch)
2013-09-23 08:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 7298 Export late orders as csv (16.72 KB, patch)
2013-09-23 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: Follow-up fixing capitalization and moving supplier id (3.75 KB, patch)
2013-09-23 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: FIX checkboxes binding on load (1.12 KB, patch)
2013-09-23 20:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: CSV strings are translatables. (4.56 KB, patch)
2013-09-23 20:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: QA Followup (7.36 KB, patch)
2013-09-23 20:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7298: Followup: CSV strings are translatables. (2.04 KB, patch)
2013-09-24 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 7298: Followup: CSV strings are translatables. (2.14 KB, patch)
2013-09-27 20:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7298 Export late orders as csv (14.77 KB, patch)
2013-10-31 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: Follow-up fixing capitalization and moving supplier id (3.75 KB, patch)
2013-10-31 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: FIX checkboxes binding on load (1.12 KB, patch)
2013-10-31 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: CSV strings are translatables. (4.55 KB, patch)
2013-10-31 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: QA Followup (7.37 KB, patch)
2013-10-31 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7298: Followup: CSV strings are translatables. (2.15 KB, patch)
2013-10-31 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2011-12-02 09:38:25 UTC
let the librarian export late order listing, and see how many claims have been made, as well as claim dates(BibLibre MT6844)
Comment 1 Jonathan Druart 2012-02-22 16:27:53 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-02-23 11:58:54 UTC
I am not so happy about the filename lateorders-excel.pl. Perhaps we could choose something more generic? export_lateorders perhaps?
Comment 3 Jonathan Druart 2012-02-23 13:17:53 UTC
Hi Katrin, 

I don't like this name neither, however a file named ' alserials/lateissues-excel.pl' already exists and have the same purpose :-/
Comment 4 Katrin Fischer 2012-02-23 13:47:18 UTC
I think in this case I would vote against consistency and for a nicer file name. Perhaps someone else has an opinion on this?
Comment 5 Magnus Enger 2012-02-23 14:02:40 UTC
(In reply to comment #4)
> I think in this case I would vote against consistency and for a nicer file
> name. Perhaps someone else has an opinion on this?

I agree with using something more generic!
Comment 6 Chris Cormack 2012-02-23 20:24:03 UTC
If we are exporting it as csv not .xls why not lateissues-csv.pl ?
Comment 7 Katrin Fischer 2012-02-26 15:27:14 UTC
Created attachment 7891 [details]
CSV file contains no order data

Hi Jonathan, 

while testing this I spotted a another problem, that might explain why my csv didn't have the data I expected it to have. When you forget to add an email address to the vendor there will be an error message, but it still saves a claim data and a claim count to the database. I think this should not happen.

Problems I found:

1) I claimed 2 orders successfully, the table shows the count and the date. But when I mark them and click on export, there is no useful information in the csv file. (see attached screenshot)

2) Perhaps make the link a button 'Export as CSV' next to 'Claim orders'? Similar to the button for exporting CSV data on the basket page.
Comment 8 Marcel de Rooy 2012-02-27 10:38:52 UTC
Just a comment on the name: I agree with Magnus to use a more generic name. Why not something like late_orders_export? Note that someone in the future may add an option to your script that makes it export to csv as well as ...
Comment 9 Jonathan Druart 2012-02-29 16:04:58 UTC Comment hidden (obsolete)
Comment 10 jmbroust 2012-03-19 15:58:34 UTC
I have the same problem as Katrin in comment 7 : no data in the csv file (tested whith openoffice 3.1).
Furthermore, in the biblibre sandbox 1, I tried to claim  a late order (because the number and dates of claims might appear in the csv) and had this message : "No letter_code at /home/koha/src/acqui/lateorders.pl line 83"
Comment 11 Jonathan Druart 2012-03-20 08:18:27 UTC
Can you verify you are a "Claim acquisition" defined in yours letters ?
Comment 12 Koha Team University Lyon 3 2012-03-20 09:19:13 UTC
I have an error when I try "claim order" :
"Software error:
Can't use string ("1") as a HASH ref while "strict refs" in use at /home/koha/src/acqui/lateorders.pl line 91.
For help, please send mail to the webmaster (webmaster@test4-sandbox), giving this error message and the time and date of the error. "                           
I have a "Claim acquisition" defined in the letters.The csv file is quite empty. (cf. attachment)
Comment 13 Koha Team University Lyon 3 2012-03-20 09:23:01 UTC
Created attachment 8333 [details]
csv file
Comment 14 Jonathan Druart 2012-03-22 13:50:17 UTC
Blocked by Bug 5356
Comment 15 Jonathan Druart 2012-06-22 09:11:04 UTC Comment hidden (obsolete)
Comment 16 Nicole C. Engard 2012-08-20 20:03:40 UTC
Created attachment 11713 [details]
bottom row misaligned

The function seems to work fine, but the moving of the checkboxes to the left has made it so that the bottom row is no longer lined up right. Also there is no need for 2 claim order buttons. 

These are minor design fixes.
Comment 17 Jonathan Druart 2012-08-21 08:20:36 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2012-08-21 08:21:45 UTC
Hi Nicole,
This followup fixes the 2 problems in the table bottom.
Comment 19 Nicole C. Engard 2012-08-21 15:11:27 UTC Comment hidden (obsolete)
Comment 20 Nicole C. Engard 2012-08-21 15:11:55 UTC Comment hidden (obsolete)
Comment 21 Paul Poulain 2012-10-01 17:39:37 UTC
QA comment
 1- bug signed-off since more than 1 month, dealing with QA myself
 2- qa.pl script shows some errors, easy to solve (use COALESCE instead of IFNULL & use spaces for indent)
 3- replaces a SQL query in GetOrder in an unsafe way imo:
   - add many table.* to an already very long query (was biblio+biblioitems+aqorders, and adds aqbudgets+aqbasket) Would be better not to use * (including biblioitems, that contains marcxml field !!! was already here before the patch, but if you can fix it...)
   - does some changes that look strange : why DATE(aqbasket.closedate)  AS orderdate ? why aqorders.quantity - IFNULL(aqorders.quantityreceived,0) AS quantity -quantity is a field from aqorders ? 
 4- the form $$order{estimateddeliverydate} must be discarded in favor of $order->{estimateddeliverydate}

Failing QA for #4, need info (and maybe fix) for #3
Comment 22 Paul Poulain 2012-10-01 17:40:11 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2012-10-15 15:19:00 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2012-10-15 15:20:19 UTC
I don't know if the list of field is good but grep says me I am good.
Comment 25 Jonathan Druart 2012-10-15 15:23:25 UTC Comment hidden (obsolete)
Comment 26 mathieu saby 2013-02-02 21:46:15 UTC
Tested on a VM.
I think it does not work : 
I had 8 documents late, I created the csv, and here is its content :

ORDER DATE	ESTIMATED DELIVERY DATE	VENDOR	INFORMATION	TOTAL COST	BASKET	CLAIMS COUNT	CLAIMED DATE
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
()  ( days)				x =  ()	 ()		
		Total Number Late	8				




M. Saby
Rennes 2 university
Comment 27 Jonathan Druart 2013-02-04 09:10:12 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-02-04 09:10:20 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-02-04 09:11:17 UTC
(In reply to comment #26)
> Tested on a VM.
> I think it does not work : 

The last patch produced a mysql error.
Comment 30 mathieu saby 2013-02-04 16:51:29 UTC Comment hidden (obsolete)
Comment 31 mathieu saby 2013-02-04 16:51:40 UTC Comment hidden (obsolete)
Comment 32 mathieu saby 2013-02-04 16:51:48 UTC Comment hidden (obsolete)
Comment 33 mathieu saby 2013-02-04 16:51:53 UTC Comment hidden (obsolete)
Comment 34 mathieu saby 2013-02-04 16:54:43 UTC
My first sign-off using git bz. I hope everything is ok...

I signed off but maybe a little followup would be necessary :
In the last line of the csv, there is two , before "Total Number Late". I am not sure it is normal.

M. Saby
Comment 35 mathieu saby 2013-02-04 16:55:54 UTC
Oh, I missed that point "Uncomment the # before each patch you would like to obsolete before saving." :(
M. Saby
Comment 36 Jonathan Druart 2013-02-05 08:29:33 UTC
(In reply to comment #34)
> My first sign-off using git bz. I hope everything is ok...
> 
> I signed off but maybe a little followup would be necessary :
> In the last line of the csv, there is two , before "Total Number Late". I am
> not sure it is normal.
> 
> M. Saby

The last line of the CSV contains : "Total Number Late" in the third column and "x" (number of orders in the CSV) in the 4th column.
Comment 37 mathieu saby 2013-02-05 08:34:17 UTC
Yes. I thought it will be in column 1 and 2.
But is a matter of taste, so I am fine with that...
M. Saby
Comment 38 Paul Poulain 2013-03-21 22:26:56 UTC
QA comment
 * a tiny POD mistake, fixed in a follow-up
 * no other comments, work fine

passed QA
Comment 39 Paul Poulain 2013-03-21 22:28:54 UTC Comment hidden (obsolete)
Comment 40 Paul Poulain 2013-03-21 22:29:01 UTC Comment hidden (obsolete)
Comment 41 Paul Poulain 2013-03-21 22:29:05 UTC Comment hidden (obsolete)
Comment 42 Paul Poulain 2013-03-21 22:29:10 UTC Comment hidden (obsolete)
Comment 43 Paul Poulain 2013-03-21 22:29:18 UTC Comment hidden (obsolete)
Comment 44 Jared Camins-Esakov 2013-03-22 02:56:51 UTC
I get a nasty error from this:
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'o
ORDER BY title' at line 12 at /home/jcamins/kohaclone/C4/Serials.pm line 1906. at /home/jcamins/kohaclone/serials/lateissues-export.pl line 134        

The query that is generating is, sure enough, wrong:
SELECT
serialid,      aqbooksellerid,        name,
biblio.title,  planneddate,           serialseq,
serial.status, serial.subscriptionid, claimdate,
subscription.branchcode
FROM      serial
LEFT JOIN subscription  ON serial.subscriptionid=subscription.subscriptionid 
LEFT JOIN biblio        ON subscription.biblionumber=biblio.biblionumber
LEFT JOIN aqbooksellers ON subscription.aqbooksellerid = aqbooksellers.id
WHERE subscription.subscriptionid = serial.subscriptionid
AND (serial.STATUS = 4 OR ((planneddate < now() AND serial.STATUS =1) OR serial.STATUS = 3 OR serial.STATUS = 7))
AND subscription.aqbooksellerid=
and serialid = on
ORDER BY title

Also, there needs to be a test plan.
Comment 45 Jonathan Druart 2013-03-28 09:57:38 UTC
Jared,
This development does not modify the export for serials. I just renamed the script in order to be consistent (mv /serials/lateissues-excel.pl /serials/lateissues-export.pl).
So your issue certainly exists on master.
Comment 46 Jonathan Druart 2013-03-28 09:57:59 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2013-03-28 10:00:47 UTC
The attachment 16956 [details] [review] is a rebased and squashed patch.
I added a test plan.

An issue is fixed: the user is not allow to select orders from different vendor. This behavior was broken by datatable.

Switch back to Needs Signoff.
Comment 48 Jonathan Druart 2013-06-27 10:42:51 UTC
Since this patch has 3 signoffs and I cannot recreate the issue described by Jared, I switch the status to Signed Off. The issue is not relevant, as I said, I don't affect the lateissues-export.pl script.

I retested feature and it works.
Comment 49 Katrin Fischer 2013-07-20 17:54:17 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2013-07-20 17:57:34 UTC
Hi Jonathan, 

I have tested this and it works fine. I have fixed some things in a follow up patch, but I think I can't pass this as it is. The CSV has lots of untranslatable strings while it seems we have found some solution for that problem for the CSV export of baskets and basket groups. Could you take a look there if if this can be adapted?

Also I think it would be nicer to have the author and publisher in separate columns in the CSV, which would also remove some more translation problems.
Comment 51 Katrin Fischer 2013-07-20 18:02:54 UTC
There is also a bug in how the checkboxed get blocked so you can only select titles from one vendor. I have more than 10 late orders, when I load the late orders page, the checkboxes don't lock. When I change the number of late orders shown in the table, it starts to work - hope you can reproduce this.

Also the late order emails are broken... filing a bug.
Comment 52 Jonathan Druart 2013-08-13 14:25:42 UTC
(In reply to Katrin Fischer from comment #50)

Katrin, thank you for your patch!
I will submit 2 patchs. 1 fixes the checkboxes issues, the other allow the translation for csv headers.
Comment 53 Jonathan Druart 2013-08-13 14:26:17 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2013-08-13 14:26:26 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2013-08-13 14:26:33 UTC Comment hidden (obsolete)
Comment 56 Koha Team University Lyon 3 2013-08-19 14:05:54 UTC
Hello,
I test this patch with a sandbox. Everything seems to work as define in the test plan. I try only with 3 orders.
But, as katrin mentionned, in the csv file, the header wasn't translate in french...
Can you verify before I change the status in "signed off"
Sonia
Comment 57 Jonathan Druart 2013-08-19 14:24:32 UTC
(In reply to Koha Team Lyon 3 from comment #56)
> Hello,
> I test this patch with a sandbox. Everything seems to work as define in the
> test plan. I try only with 3 orders.
> But, as katrin mentionned, in the csv file, the header wasn't translate in
> french...
> Can you verify before I change the status in "signed off"
> Sonia

Hello Sonia,
Headers are new strings, so they are not translatable on sandboxes without a manual modification of po files.
Comment 58 sandboxes@biblibre.com 2013-08-19 15:00:18 UTC
Patch tested with a sandbox, by sonia <koha@univ-lyon3.fr>
Comment 59 sandboxes@biblibre.com 2013-08-19 15:01:16 UTC Comment hidden (obsolete)
Comment 60 sandboxes@biblibre.com 2013-08-19 15:01:20 UTC Comment hidden (obsolete)
Comment 61 sandboxes@biblibre.com 2013-08-19 15:01:23 UTC Comment hidden (obsolete)
Comment 62 sandboxes@biblibre.com 2013-08-19 15:01:27 UTC Comment hidden (obsolete)
Comment 63 Koha Team University Lyon 3 2013-08-19 15:02:57 UTC
Hello Jonathan. So for me everything works fine. I change in Signed off.
Comment 64 Jonathan Druart 2013-08-19 15:18:11 UTC
(In reply to Koha Team Lyon 3 from comment #63)
> Hello Jonathan. So for me everything works fine. I change in Signed off.

Thanks for testing Sonia!
Comment 65 Katrin Fischer 2013-09-22 09:35:22 UTC
Hi Jonathan, 

the QA script points out some things, I would have fixed the tabs, but I think deciding about the license is up to you:

 FAIL	serials/lateissues-export.pl
   OK	  pod
   FAIL	  forbidden patterns
		forbidden pattern: Koha is now under the GPLv3 license (line 7)
   OK	  valid
   OK	  critic

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv/lateorders.tt
   OK	  forbidden patterns
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 220)
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/serials/claims.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 45)
   OK	  tt_valid
   OK	  valid_template

Could you take a look? I will continue testing the functionality.
Comment 66 Katrin Fischer 2013-09-22 09:52:34 UTC
Some additional notes from code review:
- GetBasketGroupAsCSV could use some unit tests as the SQL 
  statement has been altered quite a bit.
- lateissues-export.pl could use Modern::Perl
Comment 67 Katrin Fischer 2013-09-22 10:23:13 UTC
Hi Jonathan,

we are almost there - it works really nicely, but we got one bigger problems with translating the CSV. Posting my testing notes here:

Testing notes:
- CSV header row is now translatable.
  Tested by updating the German po files and checking for the line.
- Tested that claiming for late serial issues still works as
  expected, as one file has been renamed. Filed 10931 for
  untranslatable CSV contents.
- Tested that claiming late orders still works:
  * Table searching and sorting
    Works nicely, but some columns could be split up for better
    searching and sorting:
      * Order date: 20/01/2013 (245 days)
      * Total cost:     10.00x1 = 10.00 Books
        => item type should be separate
      * Basket:  10 MPL
        => Library and basket number could be separate columns
  * Filters
     * Combined various filters, search results look correct.
  * Selecting order for claiming
      * Limiting by vendor makes it possible to check/uncheck all
      * With no vendor limit, entries for other vendors will be
        locked after the first checkbox is checked for one vendor
  * Exporting as CSV
     * Exported single line > CSV appears correct.
     * Exporting multiple lines > CSV appears correct.
     PROBLEM: Translated CSV don't work correctly, as line
     breaks are lost in the translation process.
  * Sending serial claim email
     * No regressions found - there are some problems with the
       email contents noted on bug 7298.

In order to reproduce the problem with translated CSV files make sure you test with updated po files containing a translated version of the header row or a copy of it. I think we already ran into this problem with the templates for the cart email, which are plain text and not HTML. Not sure how to solve this, but currently it seems not to work for translated templates at all as you will get
all data on one single line.
Comment 68 Jonathan Druart 2013-09-23 08:58:58 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2013-09-23 09:00:35 UTC
(In reply to Katrin Fischer from comment #65)
> Hi Jonathan,
Hi Katrin,
 
> [...]
> Could you take a look? I will continue testing the functionality.

Will be fixed.

(In reply to Katrin Fischer from comment #66)
> Some additional notes from code review:
> - GetBasketGroupAsCSV could use some unit tests as the SQL 
>   statement has been altered quite a bit.

The modified routine is GetOrder. It already has a unit test (not really good but exists).

> - lateissues-export.pl could use Modern::Perl
Will be done.

(In reply to Katrin Fischer from comment #67)
> Hi Jonathan,
>   * Exporting as CSV
>      * Exported single line > CSV appears correct.
>      * Exporting multiple lines > CSV appears correct.
>      PROBLEM: Translated CSV don't work correctly, as line
>      breaks are lost in the translation process.

>   * Sending serial claim email
>      * No regressions found - there are some problems with the
>        email contents noted on bug 7298.

We are on bug 7298 :)


> In order to reproduce the problem with translated CSV files make sure you
> test with updated po files containing a translated version of the header row
> or a copy of it. I think we already ran into this problem with the templates
> for the cart email, which are plain text and not HTML. Not sure how to solve
> this, but currently it seems not to work for translated templates at all as
> you will get
> all data on one single line.

I will have a look at bug 10931. But the problem does not occur for others csv export (basket and basketgroup)?
Comment 70 Katrin Fischer 2013-09-23 09:15:14 UTC
> (In reply to Katrin Fischer from comment #67)
> > Hi Jonathan,
> >   * Exporting as CSV
> >      * Exported single line > CSV appears correct.
> >      * Exporting multiple lines > CSV appears correct.
> >      PROBLEM: Translated CSV don't work correctly, as line
> >      breaks are lost in the translation process.
> 
> >   * Sending serial claim email
> >      * No regressions found - there are some problems with the
> >        email contents noted on bug 7298.
> 
> We are on bug 7298 :)

Bug 10618 - sorry. And was not failing the bug for this - just thought it would be worth noting.

> I will have a look at bug 10931. But the problem does not occur for others
> csv export (basket and basketgroup)?

I didn't have time to check - sorry. But I assume it will happen everywhere where we try to translate plain text (not HTML) with the translation scripts and line breaks are essential for functionality.

Maybe we need to have it 'non translatable' for now, but i would prefer a different solution. We found a fix for the cart mails, but it's a bit ugly. The template is HTML and then we strip the tags out (iirc).
Comment 71 Katrin Fischer 2013-09-23 20:50:51 UTC
Hi Jonathan, this is all good, but I think we still need to fix the translated CSV for exportig late orders? Serials CSV will be fixed by the new feature (hopefully) and there is bug 10935 for baskets and basketgroups which I will test next.

I will go ahead and attach my tested patches here but not switch the status to passed QA until I have a follow-up for fixing the translations :)
Comment 72 Katrin Fischer 2013-09-23 20:51:35 UTC Comment hidden (obsolete)
Comment 73 Katrin Fischer 2013-09-23 20:51:54 UTC Comment hidden (obsolete)
Comment 74 Katrin Fischer 2013-09-23 20:52:05 UTC Comment hidden (obsolete)
Comment 75 Katrin Fischer 2013-09-23 20:52:52 UTC Comment hidden (obsolete)
Comment 76 Katrin Fischer 2013-09-23 20:53:05 UTC Comment hidden (obsolete)
Comment 77 Katrin Fischer 2013-09-23 22:09:45 UTC
Tested bug 10935 and added a follow-up - please take a look and provide a follow up :)
Comment 78 Jonathan Druart 2013-09-24 08:21:34 UTC Comment hidden (obsolete)
Comment 79 Katrin Fischer 2013-09-27 06:44:09 UTC
Working on this.
Comment 80 Katrin Fischer 2013-09-27 20:29:46 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2013-09-27 20:30:22 UTC
This has taken a while... thx Jonathan for your patience!
Comment 82 Jonathan Druart 2013-09-30 09:41:12 UTC
(In reply to Katrin Fischer from comment #81)
> This has taken a while... thx Jonathan for your patience!

Thanks for your patience too :)
Comment 83 Galen Charlton 2013-10-31 02:25:02 UTC
Patch doesn't apply on account of the renaming of the serial claims export script -- tossing back for the patch submitter to rebase.  Feel free to set back to passed QA afterwards so that I can try to make this in before the string freeze window closes.

General note for the future -- it would have been better if the patch for renaming the existing script had stayed separate; in fact, that kind of refactoring should have been a separate bug since the name of the serials claim export script has no direct bearing on the functionality of the feature proposed here.
Comment 84 Jonathan Druart 2013-10-31 08:25:32 UTC
Created attachment 22617 [details] [review]
Bug 7298 Export late orders as csv

This patch allows to export late orders as csv.

Test plan:
- Go on the late orders page (acqui/lateorders.pl)
- Select one or more order and click on the button "export as CSV".
- The generated file should contains some information on the orders
  (order date, estimated delivery date, vendor name, information field,
  cost, basket name (and basketid), claims count and the claimed date)
  The last line of the file is the total of orders.
- You are not allow to select order from different vendor.
- The check/uncheck all links appears only if a vendor is selected.
- Check that the check/uncheck works for all pages of the table.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Testing comments on last patch in this series.
Comment 85 Jonathan Druart 2013-10-31 08:25:42 UTC
Created attachment 22618 [details] [review]
Bug 7298: Follow-up fixing capitalization and moving supplier id

Fixes a few capitalization errors on the late orders page like
- Claim orders
- filter
- Search results

Also moves the supplierid from the order date column to the
vendor column.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 86 Jonathan Druart 2013-10-31 08:25:48 UTC
Created attachment 22619 [details] [review]
Bug 7298: FIX checkboxes binding on load

Click on checkboxes should be bound on load.

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 87 Jonathan Druart 2013-10-31 08:25:55 UTC
Created attachment 22620 [details] [review]
Bug 7298: CSV strings are translatables.

Using a template file, the CSV headers become translatable.

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 88 Jonathan Druart 2013-10-31 08:26:01 UTC
Created attachment 22621 [details] [review]
Bug 7298: QA Followup

- use Modern::Perl;
- GPL version
- tabs

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and all tests.

Testing notes:
- CSV header row is now translatable.
  Tested by updating the German po files and checking for the line.
- Tested that claiming for late serial issues still works as
  expected, as one file has been renamed. Filed 10931 for
  untranslatable CSV contents.
- Tested that claiming late orders still works:
  * Table searching and sorting
    Works nicely, but some columns could be split up for better
    searching and sorting:
      * Order date: 20/01/2013 (245 days)
      * Total cost:     10.00x1 = 10.00 Books
        => item type should be separate
      * Basket:  10 MPL
        => Library and basket number could be separate columns
  * Filters
     * Combined various filters, search results look correct.
  * Selecting order for claiming
      * Limiting by vendor makes it possible to check/uncheck all
      * With no vendor limit, entries for other vendors will be
        locked after the first checkbox is checked for one vendor
  * Exporting as CSV
     * Exported single line > CSV appears correct.
     * Exporting multiple lines > CSV appears correct.
     PROBLEM: Translated CSV don't work correctly, as line
     breaks are lost in the translation process.
     Needs to be fixed in a follow-up.
  * Sending serial claim email
     * No regressions found - there are some problems with the
       email contents noted on bug 7298.
Comment 89 Jonathan Druart 2013-10-31 08:26:08 UTC
Created attachment 22622 [details] [review]
Bug 7298: Followup: CSV strings are translatables.

Following the same way as bug 10935, the headers are in an include file.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Translatability tested successfully.
Passes all tests.
Comment 90 Jonathan Druart 2013-10-31 08:27:28 UTC
Galen,
All patches rebased.
The serial claims export does not work, see bug 11168.
Comment 91 Galen Charlton 2013-10-31 15:59:26 UTC
Pushed to master.  Thanks, Jonathan!
Comment 92 mathieu saby 2013-11-03 20:51:39 UTC
Hi Jonathan

I am trying to resolve conflict on Bug 10758, so I came to the changes introduced in GetOrder by commit fd2c751a86fad7a388c57fcd51746dbc4592c807 (the main patch for bug 7298)

Don't you think the resulting routine could be make simpler ?

I don't see the use for:
aqorders.claims_count     AS claims_count,
aqorders.claimed_date     AS claimed_date,

For
aqorders.rrp              AS unitpricesupplier,
aqorders.ecost            AS unitpricelib,
and
biblioitems.publishercode AS publisher
aqbudgets.budget_name     AS budget,

Are these renaming really needed? Are they made for avoiding confusion somewhere in code?

By the way, I was trying to factorize some UT in Acquisition.t (not yet done), and one of my assumptions was that the fields returned by GetOrder were the same as each orders of GetOrderS and GetCancelledOrders. It was working, but of course it is no more the case. So I'm wondering if the fields added in the hashe returned by GetOrder could be usefull too in GetOrderS and GetCancelledOrders.

Mathieu
Comment 93 Jonathan Druart 2013-11-04 09:04:04 UTC
(In reply to mathieu saby from comment #92)
> Are these renaming really needed? Are they made for avoiding confusion
> somewhere in code?

I wrote this patch 18 months ago, so I don't know why I renamed these fields. Certainly I preferred to use "unitpricesupplier" rather than "rrp". But "unitpricelib" is not a good choice...