Bug 5087

Summary: Option to not show CSV profiles in OPAC
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: ToolsAssignee: Aleisha Amohia <aleisha>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: aleisha, david, jonathan.druart, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch adds an option to show or not show a CSV profile in the OPAC cart and lists download formats list.
Version(s) released in:
20.11.00, 20.05.03
Bug Depends on: 25402    
Bug Blocks:    
Attachments: download option in opac's cart
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: (QA follow-up) Rename the attribute to fit later API usage
Bug 5087: (QA follow-up) used_for deserves an index
Bug 5087: (QA follow-up) Fix leakage in opac-downloadshelf.pl
Bug 5087: Ability to define whether CSV profile shows in OPAC
Bug 5087: (QA follow-up) Rename the attribute to fit later API usage
Bug 5087: (QA follow-up) used_for deserves an index
Bug 5087: (QA follow-up) Fix leakage in opac-downloadshelf.pl
Bug 5087: Add server-side check

Description Katrin Fischer 2010-08-04 11:27:03 UTC
Created attachment 2450 [details]
download option in opac's cart

Currently CSV profiles defined in staff are shown in the pull down of download options in opac's cart.

I would like an option to only show those CSV profiles in staff.

More complex but even better would be a system where I can define for each CSV profile wether it will be shown in OPAC or only in staff.
Comment 1 Aleisha Amohia 2018-12-20 22:59:25 UTC Comment hidden (obsolete)
Comment 2 Maryse Simard 2019-01-04 21:28:50 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2019-01-27 12:12:01 UTC
Hi Aleisha, 
please don't forget to change kohastructure.sql as well.
Comment 4 Aleisha Amohia 2020-02-12 21:34:52 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2020-02-14 18:08:10 UTC
I get this warning from the QA tool:

 FAIL   installer/data/mysql/kohastructure.sql
   OK     boolean_vs_tinyint
   OK     charset_collate
   OK     git manipulation
   FAIL   tinyint_has_boolean_flag
                The new column export_format.opac_option is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/ExportFormat.pm), see the SQL12 coding guideline
Comment 6 Aleisha Amohia 2020-02-17 01:47:02 UTC
(In reply to Owen Leonard from comment #5)
> I get this warning from the QA tool:
> 
>  FAIL   installer/data/mysql/kohastructure.sql
>    OK     boolean_vs_tinyint
>    OK     charset_collate
>    OK     git manipulation
>    FAIL   tinyint_has_boolean_flag
>                 The new column export_format.opac_option is using TINYINT(1)
> as type but is not defined as boolean in the schema file
> (Koha/Schema/Result/ExportFormat.pm), see the SQL12 coding guideline

Hi Owen, did you update the schema files? 

misc/devel/update_dbix_class_files.pl --db_name=koha_instance --db_user=koha_instance --db_passwd=thepassword
Comment 7 Aleisha Amohia 2020-04-22 02:16:36 UTC Comment hidden (obsolete)
Comment 8 David Nind 2020-04-22 09:42:35 UTC
Hi Aleisha.

I get this error when trying to do the database update:

root@eb7cfcd62ddf:koha(bz5087)$ perl installer/data/mysql/updatedatabase.pl
{UNKNOWN}: Can't locate object method "add_columns" via package "PACKAGE__" (perhaps you forgot to load "PACKAGE__"?) at /kohadevbox/koha/Koha/Schema/Result/ExportFormat.pm line 159.
Compilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155
Compilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50.
Comment 9 Aleisha Amohia 2020-04-22 21:13:19 UTC
Created attachment 103506 [details] [review]
Bug 5087: Ability to define whether CSV profile shows in OPAC

To test:
1) Update database
2) Update schema
3) In staff side, go to Tools -> CSV profiles
4) Make a CSV profile with profile type = MARC and usage = Export
records
5) Leave 'Show option in OPAC' unchecked and save
6) Go to OPAC and add an item to your cart
7) Click Cart, click Download, confirm that your newly created CSV
profile does not show as an option
8) Go back and edit CSV profile on staff side
9) Check the 'Show option in OPAC' checkbox and save
10) Go back to download cart in OPAC
11) Confirm the CSV profile now shows in the dropdown

Sponsored-by: Catalyst IT
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 10 David Nind 2020-04-23 11:46:50 UTC
Thanks Aleisha, the database update now works.

Everything now works as per the test plan.

However, when testing I tried to turn off the 'Show option in OPAC' setting and I get this error message:

  "An error occurred when updating this CSV profile. Perhaps it already exists."

This appears in red at the top of the CSV profiles page after pressing the submit button.

I can successfully make changes to any of the other form elements for the CSV profile.
Comment 11 Aleisha Amohia 2020-04-23 21:12:10 UTC
Thanks for testing David, but unfortunately I'm not getting that error! Perhaps a data problem?

Setting back to Needs signoff in case this is working as expected for someone else.
Comment 12 Owen Leonard 2020-04-29 14:53:33 UTC
Created attachment 103955 [details] [review]
Bug 5087: Ability to define whether CSV profile shows in OPAC

To test:
1) Update database
2) Update schema
3) In staff side, go to Tools -> CSV profiles
4) Make a CSV profile with profile type = MARC and usage = Export
records
5) Leave 'Show option in OPAC' unchecked and save
6) Go to OPAC and add an item to your cart
7) Click Cart, click Download, confirm that your newly created CSV
profile does not show as an option
8) Go back and edit CSV profile on staff side
9) Check the 'Show option in OPAC' checkbox and save
10) Go back to download cart in OPAC
11) Confirm the CSV profile now shows in the dropdown

Sponsored-by: Catalyst IT
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Katrin Fischer 2020-06-11 13:14:04 UTC
Hi Aleisha,

thanks a lot for taking these on!

1) With this patch we will hide any CSV profiles now shown in the OPAC. I think we should add an update for the existing CSV profiles to check the checkbox, so that we have no change of behaviour on update.

2) Can you please make the same change for the lists download? That should then cover all bases!
Comment 14 Tomás Cohen Arazi 2020-07-20 14:59:42 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2020-07-20 14:59:51 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2020-07-20 14:59:59 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2020-07-20 15:02:36 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2020-07-21 14:41:04 UTC
Applying: Bug 5087: (QA follow-up) Rename the attribute to fit later API usage
error: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql).
error: could not build fake ancestor
Comment 19 Tomás Cohen Arazi 2020-07-21 17:17:11 UTC
Created attachment 107166 [details] [review]
Bug 5087: Ability to define whether CSV profile shows in OPAC

To test:
1) Update database
2) Update schema
3) In staff side, go to Tools -> CSV profiles
4) Make a CSV profile with profile type = MARC and usage = Export
records
5) Leave 'Show option in OPAC' unchecked and save
6) Go to OPAC and add an item to your cart
7) Click Cart, click Download, confirm that your newly created CSV
profile does not show as an option
8) Go back and edit CSV profile on staff side
9) Check the 'Show option in OPAC' checkbox and save
10) Go back to download cart in OPAC
11) Confirm the CSV profile now shows in the dropdown

Sponsored-by: Catalyst IT
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2020-07-21 17:17:17 UTC
Created attachment 107167 [details] [review]
Bug 5087: (QA follow-up) Rename the attribute to fit later API usage

This patch renames the introduced attribute for later usage on the API,
and changes the logic to use it the other way around.

It also adds a KEY for the flag, as it will be used in WHERE statements.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2020-07-21 17:17:22 UTC
Created attachment 107168 [details] [review]
Bug 5087: (QA follow-up) used_for deserves an index

This patch introduces an index for the used_for column

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2020-07-21 17:17:28 UTC
Created attachment 107169 [details] [review]
Bug 5087: (QA follow-up) Fix leakage in opac-downloadshelf.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-07-21 17:18:26 UTC
Rebased the follow-up, boss.
Comment 24 Jonathan Druart 2020-07-22 08:30:51 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> Created attachment 107167 [details] [review] [review]
> Bug 5087: (QA follow-up) Rename the attribute to fit later API usage
> 
> This patch renames the introduced attribute for later usage on the API,
> and changes the logic to use it the other way around.
> 
> It also adds a KEY for the flag, as it will be used in WHERE statements.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Can you explain a bit more Tomas?
Should not we usually provide "OPAC only" by default, and explicitly pass the "staff" flag?
Comment 25 Tomás Cohen Arazi 2020-07-22 11:24:40 UTC
(In reply to Jonathan Druart from comment #24)
> 
> Can you explain a bit more Tomas?
> Should not we usually provide "OPAC only" by default, and explicitly pass
> the "staff" flag?

This is not 'opac only', this is like the 'public' flag on reports, i.e. 'staff only', and the author named it 'opac_display'. The original naming also required to pass it on the query in the OPAC controllers, I just thought it was better to name it different, and so the flipped value also. The default behavior is kept with my follow-up and DB default values.
Comment 26 Katrin Fischer 2020-07-22 20:26:49 UTC
Tomas is right, it makes more sense this way. We don't want 'opac only' we want 'staff only'. Usually you want the option with "less exposed" to be the default :)
Comment 27 Jonathan Druart 2020-07-23 09:17:13 UTC
Created attachment 107224 [details] [review]
Bug 5087: Add server-side check

If a user to access a CSV profile that is not available at the OPAC we
should redirect to 404.

It will also handle undefined csv_profiles

For instance:
kohadev.mydnsname.org:8080/cgi-bin/koha/opac-downloadshelf.pl?shelfnumber=1&format=3
kohadev.mydnsname.org:8080/cgi-bin/koha/opac-downloadcart.pl?bib_list=1/2/&format=3
Comment 28 Jonathan Druart 2020-07-23 09:49:05 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 29 Lucas Gass 2020-07-30 17:16:59 UTC
backported to 20.05.x for 20.05.03
Comment 30 Aleisha Amohia 2020-08-03 23:36:20 UTC
enhancement, not backported to 19.11.x