Bug 19928

Summary: Acquisitions' CSV exports should honor syspref "delimiter"
Product: Koha Reporter: Victor Grousset/tuxayo <victor.grousset>
Component: AcquisitionsAssignee: Victor Grousset/tuxayo <victor.grousset>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, mtompset, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17031
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"

Description Victor Grousset/tuxayo 2018-01-09 09:05:53 UTC
1. Set syspref "delimiter" to "tabs"

2. Create a vendor with minimal info
3. Create a basket with minimal info
4. Go to the basket. URL should be /cgi-bin/koha/acqui/basket.pl?basketno=XXXXX
5. "Export as CSV"
6. You should see that the file contains commas "," This is the bug.

7. Create a basket group with minimal info
8. Close this basket group
9. Go to the "Closed" tab to see your basket group
10. "Export as CSV"
11. You should see that the file contains commas "," This is the bug.

12. Create a basket with minimal info
13. Add a item to the basket
14. Close the basket
15. Go to /cgi-bin/koha/acqui/lateorders.pl
16. "Export as CSV"
17. You should see that the file contains commas "," This is the bug.
Comment 1 Victor Grousset/tuxayo 2018-01-09 10:14:53 UTC
Everything come from here:
http://git.koha-community.org/gitweb/?p=koha.git;a=tree;f=koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv;hb=94cd27677d8febd2979f9f618f1e8cd1bd2d68d7

3 templates with commas hardcoded
Comment 2 Victor Grousset/tuxayo 2018-01-11 11:33:37 UTC Comment hidden (obsolete)
Comment 3 Victor Grousset/tuxayo 2018-01-11 11:37:03 UTC Comment hidden (obsolete)
Comment 4 Victor Grousset/tuxayo 2018-01-11 11:37:50 UTC
Thanks Joubu for this explanation:
http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=8403799c7212349bee70090249e3425afd62423b
About the part "in order to make it a have-to-read for next changes."
How would the next person find this? As it won't be the last commit.
Comment 5 Jonathan Druart 2018-01-11 12:56:52 UTC
(In reply to Victor Grousset/tuxayo from comment #4)
> Thanks Joubu for this explanation:
> http://git.koha-community.org/gitweb/?p=koha.git;a=commit;
> h=8403799c7212349bee70090249e3425afd62423b
> About the part "in order to make it a have-to-read for next changes."
> How would the next person find this? As it won't be the last commit.

(It could be a have-to-read if I fix all the grammar errors!)

At least you, Katrin and me are aware of this commit and the problem. So we can guide people to this commit message when they need it :)
Feel free to create a wiki page if you think it is a good idea (and you have time).
Comment 6 Jonathan Druart 2018-01-11 12:58:41 UTC
About your patches, it should not be needed but it is the behaviour of other occurrences: you do not default to ';', ',' or '\t' if the pref 'delimiter' does not exist.

Maybe ',' is better here as it was the value before your changes.
Comment 7 Victor Grousset/tuxayo 2018-01-11 13:32:45 UTC
(In reply to Jonathan Druart from comment #6)
> [...] you do not default to ';', ',' or '\t' if the pref 'delimiter'
> does not exist.
> 
> Maybe ',' is better here as it was the value before your changes.

You're right, I though that the pref 'delimiter' always existed.

Is giving 'delimiter' a default value in the DB[1] also an option?

[1] INSERT in updatedatabase.pl
Comment 8 Victor Grousset/tuxayo 2018-01-11 15:24:53 UTC
Created attachment 70426 [details] [review]
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"

Bonus fixes:
- Basketgroup: there was an unnecessary space.
  " RRP tax excluded" replaced by "RRP tax excluded"
- Late orders:
  - leading spaces before every line
  - surrounding spaces for [% orders.size %]

Other changes:
- quoted "Total orders in late" to no worry about additional spaces
- Late orders: additional spaces which shouldn't be a problem because
  all the fields are enclosed by quotes.

Test plan:
1. Set syspref "delimiter" to "#'s"
2. Change the language e.g. FR-fr

3. Create a vendor with minimal info
4. Create a basket with two items
5. Go to the basket. URL should be
   /cgi-bin/koha/acqui/basket.pl?basketno=XXXXX
6. Close this basket
7. "Export as CSV"
8. You should see that the file contains commas "," This is the bug.
   Leave the file open for future comparison to ensure that there are
   no regressions.

9. Create a basket group with two baskets
   (tick the close this basket group check box)
10. Go to the "Closed" tab to see your basket group
11. "Export as CSV"
12. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

13. Go to /cgi-bin/koha/acqui/lateorders.pl
14. Tick two orders. Of the same vendor, otherwise Koha won't let you tick
    orders of different vendors.
15. "Export as CSV"
16. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

17. Apply this patch
18. Translation stuff
    1. cd misc/translator
    2. perl translate update YOUR_LANG
    3. manually edit the po file, remove the fuzzy tags.
       And translate the string. You can just copy the original and
       replace some parts by gibberish.
    4. perl translate install YOUR_LANG

19. Re-export the same CSVs and compare with the original version to check that
    - delimiter syspref is honored
    - the headers are translatable
    - there is no whitespace regression (additional newlines or spaces)
Comment 9 Victor Grousset/tuxayo 2018-01-11 15:26:26 UTC
"," is now used by default if the syspref is missing.
Comment 10 M. Tompsett 2018-01-12 17:03:15 UTC
basket.tt and basketgroup.tt aren't [%- delimiter -%]. I believe the hyphens are important to the formatting of the file. Please correct.
Comment 11 Victor Grousset/tuxayo 2018-01-12 17:10:17 UTC
(In reply to M. Tompsett from comment #10)
> basket.tt and basketgroup.tt aren't [%- delimiter -%]. I believe the hyphens
> are important to the formatting of the file. Please correct.

I remember removing the post/pre-chomp after having checked that the output was still correct. Is there another caveat?
Comment 12 M. Tompsett 2018-01-12 17:45:35 UTC
(In reply to Victor Grousset/tuxayo from comment #11)
> I remember removing the post/pre-chomp after having checked that the output
> was still correct. Is there another caveat?

If you say it is the same, I'll test it. But it just didn't eyeball well. It if works, I'll set to signed off. If not, I'll confirm that it didn't.
Comment 13 M. Tompsett 2018-01-12 22:35:46 UTC
Failed QA test tools.
Comment 14 Katrin Fischer 2018-01-13 08:57:07 UTC
Please check that the translated export still works nicely after this change.
Comment 15 Katrin Fischer 2018-01-13 08:59:27 UTC
See bug 18331 - we have broken the translations of the CSV files more times than I can count - should not repeat that.
Comment 16 M. Tompsett 2018-01-13 13:45:08 UTC
(In reply to Katrin Fischer from comment #14)
> Please check that the translated export still works nicely after this change.

That's his test plan in comment #8.
Comment 17 Katrin Fischer 2018-01-13 17:21:51 UTC
It's missing steps.

- you need to update the po files after the patch was applied
- you have to check that the string got not marked as 'fuzzy'
- you have to reinstall the language

Only then the test gives you a correct result.
Comment 18 M. Tompsett 2018-01-16 01:24:55 UTC
(In reply to Katrin Fischer from comment #17)
> It's missing steps.
> 
> - you need to update the po files after the patch was applied
> - you have to check that the string got not marked as 'fuzzy'
> - you have to reinstall the language
> 
> Only then the test gives you a correct result.

So in short, the test plan in comment #8 combined with this should work. However, this is still Failed QA for not passing the koha qa test tools.
Comment 19 Victor Grousset/tuxayo 2018-01-18 15:38:57 UTC
(In reply to M. Tompsett from comment #13)
> Failed QA test tools.

It passed for me. Did I make any mistake?

##########################################################
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ git show HEAD
commit 50d3119a32ce0f8ecb0ab88404975f9cfab03b43
Author: Victor Grousset <victor.grousset@biblibre.com>
Date:   Wed Jan 10 16:07:36 2018 +0100

    Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"
#################################################################
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ qa -v 1 -c 1
testing 1 commit(s) (applied to 94cd276 'Bug 19918: Close span tag in opac-reg')

Processing files before patches
|========================>| 6 / 6 (100.00%)
Processing files after patches
|========================>| 6 / 6 (100.00%)

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/acqui/basket.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/acqui/basketgroup.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/acqui/lateorders.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv/basket.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv/basketgroup.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/csv/lateorders.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template
#################################################################
Comment 20 Victor Grousset/tuxayo 2018-01-18 15:41:29 UTC
Tested also with current master without error.

commit 129ffee791da9f8ab4ad85a4473a138fc81ccbef
Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date:   Mon Jan 15 10:10:41 2018 -0300

    Bug 19968: Add missing use Date::Calc statement
Comment 21 Victor Grousset/tuxayo 2018-01-18 15:46:33 UTC
(In reply to Katrin Fischer from comment #17)
> It's missing steps.
> 
> - you need to update the po files after the patch was applied
> - you have to check that the string got not marked as 'fuzzy'
> - you have to reinstall the language
> 
> Only then the test gives you a correct result.

I don't get, isn't this covered in the steps 18?
Comment 22 M. Tompsett 2018-01-18 16:23:25 UTC
(In reply to Victor Grousset/tuxayo from comment #19)
> (In reply to M. Tompsett from comment #13)
> > Failed QA test tools.
> 
> It passed for me. Did I make any mistake?

Nope. It would seem I did.
It passed after:
sudo koha-shell -c bash kohadev
~vagrant/qa-test-tools/koha-qa.pl -v 2 -c 1

But running it from vagrant directly does not
exit
qa -v 2 -c 1


As for comment #21, I will leave QA to pass or fail it. I'm signing off.
Comment 23 M. Tompsett 2018-01-18 16:24:22 UTC
Created attachment 70698 [details] [review]
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"

Bonus fixes:
- Basketgroup: there was an unnecessary space.
  " RRP tax excluded" replaced by "RRP tax excluded"
- Late orders:
  - leading spaces before every line
  - surrounding spaces for [% orders.size %]

Other changes:
- quoted "Total orders in late" to no worry about additional spaces
- Late orders: additional spaces which shouldn't be a problem because
  all the fields are enclosed by quotes.

Test plan:
1. Set syspref "delimiter" to "#'s"
2. Change the language e.g. FR-fr

3. Create a vendor with minimal info
4. Create a basket with two items
5. Go to the basket. URL should be
   /cgi-bin/koha/acqui/basket.pl?basketno=XXXXX
6. Close this basket
7. "Export as CSV"
8. You should see that the file contains commas "," This is the bug.
   Leave the file open for future comparison to ensure that there are
   no regressions.

9. Create a basket group with two baskets
   (tick the close this basket group check box)
10. Go to the "Closed" tab to see your basket group
11. "Export as CSV"
12. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

13. Go to /cgi-bin/koha/acqui/lateorders.pl
14. Tick two orders. Of the same vendor, otherwise Koha won't let you tick
    orders of different vendors.
15. "Export as CSV"
16. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

17. Apply this patch
18. Translation stuff
    1. cd misc/translator
    2. perl translate update YOUR_LANG
    3. manually edit the po file, remove the fuzzy tags.
       And translate the string. You can just copy the original and
       replace some parts by gibberish.
    4. perl translate install YOUR_LANG

19. Re-export the same CSVs and compare with the original version to check that
    - delimiter syspref is honored
    - the headers are translatable
    - there is no whitespace regression (additional newlines or spaces)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 24 Katrin Fischer 2018-01-19 05:07:41 UTC
(In reply to Victor Grousset/tuxayo from comment #21)
> (In reply to Katrin Fischer from comment #17)
> > It's missing steps.
> > 
> > - you need to update the po files after the patch was applied
> > - you have to check that the string got not marked as 'fuzzy'
> > - you have to reinstall the language
> > 
> > Only then the test gives you a correct result.
> 
> I don't get, isn't this covered in the steps 18?

Sorry Viktor, it appears I didn't read to the end of the test plan...
Comment 25 Nick Clemens 2018-01-19 14:48:29 UTC
This works as advertised, however see bug 19910

If we are going to honor the delimiter pref, I think we need to use it in displaying what format will be downloaded.

Want another QA opinion here
Comment 26 Nick Clemens 2018-01-19 15:10:05 UTC
Created attachment 70750 [details] [review]
Bug 19928: Acquisitions' CSV exports now honors syspref "delimiter"

Bonus fixes:
- Basketgroup: there was an unnecessary space.
  " RRP tax excluded" replaced by "RRP tax excluded"
- Late orders:
  - leading spaces before every line
  - surrounding spaces for [% orders.size %]

Other changes:
- quoted "Total orders in late" to no worry about additional spaces
- Late orders: additional spaces which shouldn't be a problem because
  all the fields are enclosed by quotes.

Test plan:
1. Set syspref "delimiter" to "#'s"
2. Change the language e.g. FR-fr

3. Create a vendor with minimal info
4. Create a basket with two items
5. Go to the basket. URL should be
   /cgi-bin/koha/acqui/basket.pl?basketno=XXXXX
6. Close this basket
7. "Export as CSV"
8. You should see that the file contains commas "," This is the bug.
   Leave the file open for future comparison to ensure that there are
   no regressions.

9. Create a basket group with two baskets
   (tick the close this basket group check box)
10. Go to the "Closed" tab to see your basket group
11. "Export as CSV"
12. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

13. Go to /cgi-bin/koha/acqui/lateorders.pl
14. Tick two orders. Of the same vendor, otherwise Koha won't let you tick
    orders of different vendors.
15. "Export as CSV"
16. You should see that the file contains commas "," This is the bug.
    Leave the file open for future comparison to ensure that there are
    no regressions.

17. Apply this patch
18. Translation stuff
    1. cd misc/translator
    2. perl translate update YOUR_LANG
    3. manually edit the po file, remove the fuzzy tags.
       And translate the string. You can just copy the original and
       replace some parts by gibberish.
    4. perl translate install YOUR_LANG

19. Re-export the same CSVs and compare with the original version to check that
    - delimiter syspref is honored
    - the headers are translatable
    - there is no whitespace regression (additional newlines or spaces)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Jonathan Druart 2018-01-19 15:13:39 UTC
(In reply to Nick Clemens from comment #25)
> This works as advertised, however see bug 19910
> 
> If we are going to honor the delimiter pref, I think we need to use it in
> displaying what format will be downloaded.
> 
> Want another QA opinion here

Agreed, please see the changes made on bug 19910 and add the info here too.
You can then switch back to Signed Off.