Bug 5087

Hi Aleisha, 
please don't forget to change kohastructure.sql as well.

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

(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

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.

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>

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.

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.

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>

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!

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

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>

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>

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>

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>

Rebased the follow-up, boss.

(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?

(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.

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 :)

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

Pushed to master for 20.11, thanks to everybody involved!

backported to 20.05.x for 20.05.03

enhancement, not backported to 19.11.x