Description
Maxime Pelletier
2012-08-09 21:33:40 UTC
Created attachment 11514 [details] [review] patch that adds new syspref and uses it This needs to be completly redone because the basket csv export is now in a template. Hi Maxime, maybe you could consider doing this similar to the new option for exporting late serial issues, where the export format is defined as a CSV profile. This could also allow to have multiple formats instead of only one (see bug 10654) Just a quick update: you can use CSV profiles now to define the export format for late serials - it would be nice if this could work similarly, so we don't add too many different ways of how to configure the CSV exports. (bug 10854) Created attachment 36860 [details] [review] Bug 8612 - System preference to have custom fields in exort csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: I - Apply the patch; II - Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno III - Create a new basket and add an order to the basket IV - Validate: the text "Select CSV profile:" and a dropdown V - Select the CSV export profile created VI - Validate: the CSV file. Created attachment 37289 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Comment on attachment 36860 [details] [review] Bug 8612 - System preference to have custom fields in exort csv basket The left panel was removed by this patch, and it when used on a closed basket it caused a crash. These are fixed by the latest version. Hello Biou, thanks for this pattch. I tested it in sandbox but get some messs with some character for example "í". But not for all characters in natioanl alphabet. We need more tests. I confirm that patch works with open or close basket. Mike Created attachment 37293 [details] [review] [SIGNED-OFF] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me *** Bug 10654 has been marked as a duplicate of this bug. *** Comment on attachment 37293 [details] [review] [SIGNED-OFF] Bug 8612 - System preference to have custom fields in export csv basket Review of attachment 37293 [details] [review]: ----------------------------------------------------------------- ::: acqui/basket.pl @@ +197,5 @@ > -type => 'text/csv', > -attachment => 'basket' . $basket->{'basketno'} . '.csv', > ); > + if ( $query->param('csv_profile') eq 'default'){ > + print GetBasketAsCSV($query->param('basketno'), $query); Actually this subroutine should take the csv profile in parameter, don't you think? @@ +531,4 @@ > return \%line; > } > > +sub get_basket_DB_info{ Don't you have something in the module to do that? (I didn't check). In all cases, you cannot add it here in the pl. Yes that means tests. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt @@ +190,5 @@ > [% END %] > + <div class="btn-group"> > + <fieldset class="action"> > + <label for="csv_code">Select CSV profile:</label> > + <select id="csv_profile_for_export"> This block is c/p twice, should be in a TT BLOCK. Hi Jonathan, > ::: acqui/basket.pl > @@ +197,5 @@ > > -type => 'text/csv', > > -attachment => 'basket' . $basket->{'basketno'} . '.csv', > > ); > > + if ( $query->param('csv_profile') eq 'default'){ > > + print GetBasketAsCSV($query->param('basketno'), $query); > > Actually this subroutine should take the csv profile in parameter, don't you > think? Could you be more specific about what are the changes you would like to see here? Thank you, (In reply to Jonathan Druart from comment #11) > Comment on attachment 37293 [details] [review] [review] > [SIGNED-OFF] Bug 8612 - System preference to have custom fields in export > csv basket > > Review of attachment 37293 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: acqui/basket.pl > @@ +197,5 @@ > > -type => 'text/csv', > > -attachment => 'basket' . $basket->{'basketno'} . '.csv', > > ); > > + if ( $query->param('csv_profile') eq 'default'){ > > + print GetBasketAsCSV($query->param('basketno'), $query); > > Actually this subroutine should take the csv profile in parameter, don't you > think? > > @@ +531,4 @@ > > return \%line; > > } > > > > +sub get_basket_DB_info{ > > Don't you have something in the module to do that? (I didn't check). > In all cases, you cannot add it here in the pl. Yes that means tests. > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt > @@ +190,5 @@ > > [% END %] > > + <div class="btn-group"> > > + <fieldset class="action"> > > + <label for="csv_code">Select CSV profile:</label> > > + <select id="csv_profile_for_export"> > > This block is c/p twice, should be in a TT BLOCK. Created attachment 41224 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Created attachment 41225 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Created attachment 41226 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Created attachment 41227 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks (In reply to Rémi Mayrand-Provencher from comment #12) > Hi Jonathan, > > > ::: acqui/basket.pl > > @@ +197,5 @@ > > > -type => 'text/csv', > > > -attachment => 'basket' . $basket->{'basketno'} . '.csv', > > > ); > > > + if ( $query->param('csv_profile') eq 'default'){ > > > + print GetBasketAsCSV($query->param('basketno'), $query); > > > > Actually this subroutine should take the csv profile in parameter, don't you > > think? > > Could you be more specific about what are the changes you would like to see > here? You added 2 subroutines to the script acqui/basket.pl to build a CSV file. But the subroutine GetBasketAsCSV already does it. I think you should modify this subroutine to do what you want to do: export a basket as CSV given a specific CSV profile. The call would be: my $csv = GetBasketAsCSV($basketno, $query, $csv_profile_id); Yes, it's a little bit more work, but more robust and reusable :) Created attachment 42532 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Created attachment 42533 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Created attachment 42534 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Could you please rebase and change the icon to Font Awesome icon? See http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13696 Created attachment 47591 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Created attachment 47592 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Created attachment 47593 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Fixed conflicts and changed icons to font awesome This didn't work properly for me - I selected the pipe | as my CSV separator and copied your data from step 2 but the CSV file did not separate my files on pipes and separated on commas instead. Created attachment 56222 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56223 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56224 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 56225 [details] [review] Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug 15451) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I've rebased patch set on master. Aleisha, I tried the pipe as separator too, and it worked for me well, so I changed status back to signed off. If it's still not working for you, could you please add more details? No tests provided. Did a quick code review: 1) Could we make this a split button maybe (like the one in cataloguing for z39.50 import)? (no blocker) 2) The button itself is labelled differently on a closed basket and an open basket. Should be the same label for both. I'd suggest something in the middle between both versions "Export as CSV"? 2) Please provide tests for changes to GetBasketAsCSV. Created attachment 57012 [details] [review] Bug 8612: QA followup - Make export button split button, unify button labels Created attachment 57013 [details] [review] Bug 8612: Add tests for GetBasketAsCSV (In reply to Katrin Fischer from comment #33) > Did a quick code review: > > 1) Could we make this a split button maybe (like the one in cataloguing for > z39.50 import)? (no blocker) Good point, I've done it > > 2) The button itself is labelled differently on a closed basket and an open > basket. Should be the same label for both. I'd suggest something in the > middle between both versions "Export as CSV"? Fixed > > 2) Please provide tests for changes to GetBasketAsCSV. Test added As have little experiences with writing test, feel free to point me to the right direction if it's not written as it should be. Thanks Apply? [(y)es, (n)o, (i)nteractive] Applying: Bug 8612 - System preference to have custom fields in export csv basket Using index info to reconstruct a base tree... M acqui/basket.pl M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging acqui/basket.pl CONFLICT (content): Merge conflict in acqui/basket.pl Created attachment 57024 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 57025 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 57026 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 57027 [details] [review] Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug 15451) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 57028 [details] [review] Bug 8612: QA followup - Make export button split button, unify button labels Created attachment 57029 [details] [review] Bug 8612: Add tests for GetBasketAsCSV Sorry, rebased on master I really like the new split button! Tests pass. Sorry, but noticed this now for the first time: When you have a CSV profile defined for your late issues export, it will also show up as export option for the CSV export in acquisitions. I think this could get confusing as both will require different fields from the database. I'd really like a way to indicate what the CSV profile is to be used for. I am hesitating to pass this as is, as I think it will be harder to do this later on. Once the libraries have added profiles for both, there will be no way to assign them automatically to the correct module. If we do it now, we can safely assume that all SQL profiles are used for exporting late issues. Hope that makes sense? Created attachment 57079 [details] [review] Bug 8612: QA follow-up - distinguish between late issues and basket export csv profiles tatus: Signed Off (In reply to Katrin Fischer from comment #45) > > I'd really like a way to indicate what the CSV profile is to be used for. I > am hesitating to pass this as is, as I think it will be harder to do this > later on. Once the libraries have added profiles for both, there will be no > way to assign them automatically to the correct module. If we do it now, we > can safely assume that all SQL profiles are used for exporting late issues. > Hope that makes sense? It absolutely makes sense, I agree with you. I've submitted follow-up, to you think I'm on right way, or should I take another approach to this problem? Created attachment 58073 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58074 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58075 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58076 [details] [review] Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug 15451) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58077 [details] [review] Bug 8612: QA followup - Make export button split button, unify button labels Created attachment 58078 [details] [review] Bug 8612: Add tests for GetBasketAsCSV Created attachment 58079 [details] [review] Bug 8612: QA follow-up - distinguish between late issues and basket export csv profiles Just rebased on master Created attachment 58985 [details] [review] Bug 8612 - System preference to have custom fields in export csv basket This patch allows the user use a CSV export profile to create the fields to export the basket as CSV in a basket page. To test: 1) Apply the patch; 2) Create a CSV export profile - type sql - Home › Tools › CSV export profiles ex: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) Create a new basket and add an order to the basket 4) Validate: the text "Select CSV profile:" and a dropdown in the basket view 5) In the new dropdown, select the CSV export profile created 6) Validate the CSV file. 7) Do the same validation with a closed basket. a) close the basket b) View the basket c) validate that there is a dropdown d) test it with an export Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Works as expected for me Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58986 [details] [review] Bug 8612 - Cleans basket.tt by using TT blocks Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58987 [details] [review] Bug 8612 - Clean basket.pl getting rid of subroutines with sql query and use GetBasketAsCSV instead GetBasketAsCSV now has a new argument the profile_csv_id, when this param is defined, it loads the profile_content, searches the value of the predefined fields and assembles the data to create the output of the csv file. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58988 [details] [review] Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug 15451) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 58989 [details] [review] Bug 8612: QA followup - Make export button split button, unify button labels Created attachment 58990 [details] [review] Bug 8612: Add tests for GetBasketAsCSV Created attachment 58991 [details] [review] Bug 8612: QA follow-up - distinguish between late issues and basket export csv profiles Created attachment 58992 [details] [review] Bug 8612: Fix test due to bug 13726 Rebased on master, added another followup. Asking for new sign off - too many followups and conflicts with new work pushed to master. Also note, that the test plan should be amended. New test plan: 1) Apply the patches 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Josef, I'd be happy to have someone in our team test and sign this, but right now I get a Applying: Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug 15451) fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Damn sha1... Making one big patch out of it would probably limit future rebases, btw, since a lot of these do-undo-redo the same code... Reminder for the initial work: **** Sponsored by : CCSR **** Created attachment 59757 [details] [review] Bug 8612: Use CSV profile for exporting basket This patch allows the user to use a CSV export profile to create the fields to export the basket as CSV in a basket page. Test plan: 1) Apply the patch 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Initial work: Sponsored by: CCSR Signed-off-by: Josef Moravec <josef.moravec@gmail.com> (In reply to Blou from comment #65) > Josef, > I'd be happy to have someone in our team test and sign this, but right now I > get a > Applying: Bug 8612 - Followup - use Koha::CsvProfile[s] module (see bug > 15451) > fatal: sha1 information is lacking or useless (C4/Acquisition.pm). > > Damn sha1... > Making one big patch out of it would probably limit future rebases, btw, > since a lot of these do-undo-redo the same code... > > Reminder for the initial work: > > **** > Sponsored by : CCSR > **** I squashed the patches. Created attachment 60480 [details] [review] Bug 8612: Use CSV profile for exporting basket This patch allows the user to use a CSV export profile to create the fields to export the basket as CSV in a basket page. Test plan: 1) Apply the patch 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Initial work: Sponsored by: CCSR Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: mehdi <mehdi.hamidi@inlibro.com> I have the feeling that there is a design issue. I'd have let the type="sql" but add another DB field ('usage' or similar) to identify how the profile can be used. (In reply to Jonathan Druart from comment #69) > I have the feeling that there is a design issue. I'd have let the type="sql" > but add another DB field ('usage' or similar) to identify how the profile > can be used. I agree with Jonathan. Although I acknowledge that this patch already has quite a history already. Export format is the name of the table for the CSV profiles (this is already confusing btw). Actually, csv profiles would be a better name. The column type should be either marc or sql: we refer to marc fields or database columns. But yes, we need an additional column where to use which profile. Probably we can start here with the simple list we have now. Could be expanded later.. About t/db_dependent/Acquisition/GetBasketAsCSV.t: Remove the dbh statements. You do not need them. You use schema txn.. You pass a new CGI object in your test. You probably can; just wondering if you should mock C4::Languages::getlanguage and not use CGI here. Typo defautl Created attachment 61264 [details] [review] Bug 8612: Use CSV profile for exporting basket This patch allows the user to use a CSV export profile to create the fields to export the basket as CSV in a basket page. Test plan: 1) Apply the patch 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Initial work: Sponsored by: CCSR Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: mehdi <mehdi.hamidi@inlibro.com> Created attachment 61265 [details] [review] Bug 8612: QA-followup - Fix test Created attachment 61266 [details] [review] Bug 8612: QA followup - make usage and type different columns in db Created attachment 61267 [details] [review] Bug 8612: QA followup - update dbix schema (In reply to Marcel de Rooy from comment #70) > (In reply to Jonathan Druart from comment #69) > > I have the feeling that there is a design issue. I'd have let the type="sql" > > but add another DB field ('usage' or similar) to identify how the profile > > can be used. > > I agree with Jonathan. > Although I acknowledge that this patch already has quite a history already. > > Export format is the name of the table for the CSV profiles (this is already > confusing btw). Actually, csv profiles would be a better name. > The column type should be either marc or sql: we refer to marc fields or > database columns. > But yes, we need an additional column where to use which profile. Probably > we can start here with the simple list we have now. Could be expanded later.. > > About t/db_dependent/Acquisition/GetBasketAsCSV.t: > Remove the dbh statements. You do not need them. You use schema txn.. > You pass a new CGI object in your test. You probably can; just wondering if > you should mock C4::Languages::getlanguage and not use CGI here. > Typo defautl So, I rebased the patch on top of current master and added some followups. Changes made - - revert the changes in csv profile types so there are again only two - marc and sql - added new db column used_for, which should say where the profile could be used - fix typo and remove dbh from test GetBasketAsCSV.t How do you think the mocked getlanguage could be used? The soubroutine GetBasketAsCSV does need the CGI object as a param... used_for => 'export_records' BUT in kohastructure: `usage` varchar(255) DEFAULT 'export_records' You have to choose ;) Typo: [% ELSE %] Uknown type Created attachment 61405 [details] [review] Bug 8612 - QA followup, fix kohastructure.sql, fix typo in template (In reply to Marcel de Rooy from comment #76) > used_for => 'export_records' > BUT in kohastructure: `usage` varchar(255) DEFAULT 'export_records' > You have to choose ;) > > Typo: [% ELSE %] Uknown type I wanted to use "usage" first, but it is mysql keyword Both fixed in followup. QA: Looking here now Created attachment 61520 [details] [review] Bug 8612: [QA Follow-up] Just a cosmetic space Could not resist to add it :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> See two things so far: [1] Regex for field does not work: $field =~ s/[^\.]*\.?//; # Remove the table name if exists. Does not work well if there is no table in front. Try this: $field="title"; print "$field\n"; $field =~ s/[^\.]*\.?//; # Remove the table name if exists. print "$field\n"; [2] Regex for header and fieldname makes header no longer optional. while ( $csv_profile_content =~ / ([^=]+) # header = ([^\|]+) # fieldname (table.row or row) \|? /gxms ) { But if I understand correctly from this feature, the header should be optional. === Quote You have to define which fields you want to export, separated by pipes. You can also use your own headers (instead of the ones from Koha) by prefixing the field name with an header, followed by the equal sign. === If I remove the headers now, I will have an empty csv file. Hi Marcel, I was wondering what you want to show in your first subparagraph, because it seems normal to me that it will not work if there is no table in front of the field. Koha need to know the table where the field come from. (Sorry for my english :)) (In reply to Philippe Audet-Fortin from comment #82) > Hi Marcel, I was wondering what you want to show in your first subparagraph, > because it seems normal to me that it will not work if there is no table in > front of the field. Koha need to know the table where the field come from. > (Sorry for my english :)) Hmm. Maybe a misunderstanding. Comment81 refers to some lines in the code including the comment "# Remove the table name if exists." Created attachment 63048 [details] [review] Bug 8612: Use CSV profile for exporting basket This patch allows the user to use a CSV export profile to create the fields to export the basket as CSV in a basket page. Test plan: 1) Apply the patch 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Initial work: Sponsored by: CCSR Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: mehdi <mehdi.hamidi@inlibro.com> Created attachment 63049 [details] [review] Bug 8612: QA-followup - Fix test Created attachment 63050 [details] [review] Bug 8612: QA followup - make usage and type different columns in db Created attachment 63051 [details] [review] Bug 8612: QA followup - update dbix schema Created attachment 63052 [details] [review] Bug 8612 - QA followup, fix kohastructure.sql, fix typo in template Created attachment 63053 [details] [review] Bug 8612: [QA Follow-up] Just a cosmetic space Could not resist to add it :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Just a rebase first... Created attachment 63054 [details] [review] Bug 8612: QA folowup - fix regular expression to do what is described in the comment Created attachment 63055 [details] [review] Bug 8612: QA followup - make header in CSV profile definition optional Created attachment 63056 [details] [review] Bug 8612: QA followup - expand tests Test plan: prove t/db_dependent/Acquisition/GetBasketAsCSV.t (In reply to Marcel de Rooy from comment #81) > See two things so far: > > [1] Regex for field does not work: > $field =~ s/[^\.]*\.?//; # Remove the table name if exists. > Does not work well if there is no table in front. > Try this: > $field="title"; > print "$field\n"; > $field =~ s/[^\.]*\.?//; # Remove the table name if exists. > print "$field\n"; Fixed by first today's followup > [2] Regex for header and fieldname makes header no longer optional. > while ( $csv_profile_content =~ / > ([^=]+) # header > = > ([^\|]+) # fieldname (table.row or row) > \|? /gxms > ) { > But if I understand correctly from this feature, the header should be > optional. > === Quote > You have to define which fields you want to export, separated by pipes. > You can also use your own headers (instead of the ones from Koha) by > prefixing the field name with an header, followed by the equal sign. > === > If I remove the headers now, I will have an empty csv file. Fixed by second today's followup Third followup is adding one more test case I tested with a trival csv: biblio.title | some_text=aqorders.quantity The title is not shown, the quantity is. Have the impression that the regex might still be the cause. What did you test? Created attachment 63104 [details] [review] Bug 8612: QA followup - expand tests Test plan: prove t/db_dependent/Acquisition/GetBasketAsCSV.t (In reply to Marcel de Rooy from comment #95) > I tested with a trival csv: > biblio.title | some_text=aqorders.quantity > > The title is not shown, the quantity is. Have the impression that the regex > might still be the cause. > What did you test? I copy paste your csv and I got some blank fields too. I try by removing white spaces and it worked. (I had not seen your comment and I signoff too early. Sorry.) Created attachment 63158 [details] [review] Bug 8612: QA followup - strip white chars from csv profile definition (In reply to Marcel de Rooy from comment #95) > I tested with a trival csv: > biblio.title | some_text=aqorders.quantity > > The title is not shown, the quantity is. Have the impression that the regex > might still be the cause. > What did you test? The reason it doesn't work for you are the spaces between fields in your definition. The help paragraph on csv profile page doesn't say anything about spaces, so i decided to strip the white chars from definition before generating csv. It doesn't hurt and could help to avoid unexpected issues. Created attachment 63279 [details] [review] Bug 8612: QA followup - strip white chars from csv profile definition, expand test Ok, it was not good solution, now should be better ;) Rearranging extensive number of (qa) follow-ups according to this plan: Patch 1 Original start Patch 3 + 5 + 6 Change table design Patch 4 DBIx Schema Patch 7 + Patch 8 + Patch 10a Module fixes Patch 2 + Patch 9 + Patch 10b Unit test Patch 11 New QA follow-up Created attachment 63910 [details] [review] Bug 8612: Use CSV profile for exporting basket This patch allows the user to use a CSV export profile to create the fields to export the basket as CSV in a basket page. Test plan: 1) Apply the patch 2) Go to Tools › CSV export profiles and create a profile of type "SQL for basket export in acquisition" example: biblionumber=biblio.biblionumber|auteur=biblio.author|titre=biblio.title|date=biblioitems.copyrightdate|editeur=biblioitems.publishercode|isbn=biblioitems.isbn|quantite=aqorders.quantity|prix=aqorders.rrp|panier=aqorders.basketno 3) In acquisition module, create a new basket and add an order to the basket 4) On basket detail page, there should be the split button labelled "Export to CSV" 5) Try to use the button and export CSV with your CSV profile you defined in step 2 6) Validate the CSV file. 7) Repeat 4-6 with a closed basket. a) close the basket b) View the basket c) validate that there is an export button d) test it with an export 8) prove t/db_dependent/Acquisition/GetBasketAsCSV.t t/db_dependent/Koha/CsvProfiles.t Initial work: Sponsored by: CCSR Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: mehdi <mehdi.hamidi@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63911 [details] [review] Bug 8612: [Follow-up] Make usage and type different columns in table Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63912 [details] [review] Bug 8612: [Follow-up] Update DBIx schema Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63913 [details] [review] Bug 8612: [Follow-up] Fix regular expression Fix regular expression to do what is described in the comment Make header in CSV profile definition optional Strip white chars from csv profile definition Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63914 [details] [review] Bug 8612: [Follow-up] Fix unit test GetBasketAsCSV.t Test plan: Run t/db_dependent/Acquisition/GetBasketAsCSV.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63915 [details] [review] Bug 8612: [QA Follow-up] Remove two newlines from template output When using the Default profile from the basket form, the resulting csv file has an additional newline after the headers and at the end. This patch removes them. Unit test adjusted accordingly. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Another report may still deal with this warn: Use of uninitialized value in string eq at C4/Acquisition.pm line 318. And: die "There is no valid csv profile given" unless $csv_profile; Being liberal now. But we should actually use Koha::Exceptions now according to a new rule in the coding guidelines. (In reply to Marcel de Rooy from comment #109) > Another report may still deal with this warn: > Use of uninitialized value in string eq at C4/Acquisition.pm line 318. I filled new bug 18717 for this (In reply to Marcel de Rooy from comment #110) > And: > die "There is no valid csv profile given" unless $csv_profile; > Being liberal now. But we should actually use Koha::Exceptions now according > to a new rule in the coding guidelines. I filled new bug 18720 Created attachment 63964 [details] [review] Bug 8612: Remove warnings from tests Created attachment 63966 [details] [review] Bug 8612: Update installer files for late_issues Pushed to master for 17.11, thanks to everybody involved! *** Bug 18717 has been marked as a duplicate of this bug. *** Comment on attachment 63910 [details] [review] Bug 8612: Use CSV profile for exporting basket Review of attachment 63910 [details] [review]: ----------------------------------------------------------------- ::: C4/Acquisition.pm @@ +310,5 @@ > + my $bd = GetBiblioData( $order->{'biblionumber'} ); > + my @biblioitems = GetBiblioItemByBiblioNumber( $order->{'biblionumber'}); > + for my $biblioitem (@biblioitems) { > + if ($biblioitem->{isbn} eq $order->{isbn}) { > + $order = {%$order, %$biblioitem}; Could you explain these 3 lines? Enhancement not pushed to 17.05.x |