Created attachment 164042 [details] [review]
Bug 36453: DB and data changes

- kohastructure.sql updates
- atomicupdate file
- Mandatory data installer updates

Created attachment 164043 [details] [review]
Bug 36453: [DO NOT PUSH] DBIC schema files

Created attachment 164044 [details] [review]
Bug 36453: effective_BlockExpiredPatronOpacActions_contains method

effective_BlockExpiredPatronOpacActions
is now
effective_BlockExpiredPatronOpacActions_contains

This method has been updated to now consider an action param.
Returns true if the given action is to be blocked to expired
patrons or not.

Created attachment 164045 [details] [review]
Bug 36453: Update old occurrences of effective_BlockExpiredPatronOpacActions

Check for 'renew' when appropriate
Check for 'hold' when appropriate

The following command must return nothing before this is pushed to
master:
git grep "\beffective_BlockExpiredPatronOpacActions\b"

Created attachment 164046 [details] [review]
Bug 36453: Categories admin page:

Account for multiple values param
Input is now multipleselect select
JavaScript
Staff CSS updates to account for multiselect dropdown in categories admin page

Created attachment 164047 [details] [review]
Bug 36453: BlockExpiredPatronOpacActions system preference

The sys pref now allows multipleselect values instead of YesNo

Created attachment 164048 [details] [review]
Bug 36453: Update tests

Address new new version of system pref
Address new version of categories DB table BlockExpiredPatronOpacActions column

Test plan, k-t-d, BEFORE applying patch:
1) Edit some patron categories, visit:
/cgi-bin/koha/admin/categories.pl
2) Set different values for the "Block expired patrons" input config
Set a category to block
Set a category to don't block
Set a category to follow the syspref
3) Take note of the current value of the BlockExpiredPatronOpacActions sys pref

Apply patch, then run the following commands
- koha-plack --restart kohadev
- yarn css:build
- run updatedatabase

4) Visit the patron categories you edited before, make sure they all have the correct values for the new config type
5) Check the BlockExpiredPatronOpacActions sys pref and make sure the new value is correct according to what was before
6) Add a new patron of one of the above categories, test that:
If 'hold' is a blocked action, patron is unable to place a hold in OPAC
If 'renew' is a blocked action, patron is unable to renew in OPAC
If 'follow sys pref' is the option, check that a patron may place a hold or renew an item according to the BlockExpiredPatronOpacActions sys pref value

prove t/db_dependent/Circulation.t
prove t/db_dependent/ILSDI_Services.t
prove t/db_dependent/Koha/Object.t
prove t/db_dependent/Koha/Patron/Categories.t

The following command must return nothing before this is pushed to
master:
git grep "\beffective_BlockExpiredPatronOpacActions\b"

2cents :
The "specific action" label on patrons category configuration is a bit ambiguous.
I wouldn't know if the selected actions are the blocked ones or allowed ones.
Might be better to call that "block specific actions".

Also the input field seems to have a slight CSS issue, it's not wide enough so we can read the selected value without clicking on it.
Tried both Firefox and Chromium with same results.
Choosing "Follow system preferences" make it wider than both of the other choices which is a bit weird but not much annoying actually.

Also, since the syspref is changed, the "comment" or label of the syspref makes no sense anymore.
Used to be "[block|don't block] expired patrons from blablabla"
Should now be something like "decide which actions are blocked for expired patrons"

tried with a patron the different values from the syspref, works.
the values from the category do actually override the syspref, works.

a couple of very small cosmetic things could be enhanced but still signing off because the functionality just works fine and is great :)

Created attachment 164762 [details] [review]
Bug 36453: DB and data changes

- kohastructure.sql updates
- atomicupdate file
- Mandatory data installer updates

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164763 [details] [review]
Bug 36453: [DO NOT PUSH] DBIC schema files

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164764 [details] [review]
Bug 36453: effective_BlockExpiredPatronOpacActions_contains method

effective_BlockExpiredPatronOpacActions
is now
effective_BlockExpiredPatronOpacActions_contains

This method has been updated to now consider an action param.
Returns true if the given action is to be blocked to expired
patrons or not.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164765 [details] [review]
Bug 36453: Update old occurrences of effective_BlockExpiredPatronOpacActions

Check for 'renew' when appropriate
Check for 'hold' when appropriate

The following command must return nothing before this is pushed to
master:
git grep "\beffective_BlockExpiredPatronOpacActions\b"

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164766 [details] [review]
Bug 36453: Categories admin page:

Account for multiple values param
Input is now multipleselect select
JavaScript
Staff CSS updates to account for multiselect dropdown in categories admin page

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164767 [details] [review]
Bug 36453: BlockExpiredPatronOpacActions system preference

The sys pref now allows multipleselect values instead of YesNo

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Created attachment 164768 [details] [review]
Bug 36453: Update tests

Address new new version of system pref
Address new version of categories DB table BlockExpiredPatronOpacActions column

Test plan, k-t-d, BEFORE applying patch:
1) Edit some patron categories, visit:
/cgi-bin/koha/admin/categories.pl
2) Set different values for the "Block expired patrons" input config
Set a category to block
Set a category to don't block
Set a category to follow the syspref
3) Take note of the current value of the BlockExpiredPatronOpacActions sys pref

Apply patch, then run the following commands
- koha-plack --restart kohadev
- yarn css:build
- run updatedatabase

4) Visit the patron categories you edited before, make sure they all have the correct values for the new config type
5) Check the BlockExpiredPatronOpacActions sys pref and make sure the new value is correct according to what was before
6) Add a new patron of one of the above categories, test that:
If 'hold' is a blocked action, patron is unable to place a hold in OPAC
If 'renew' is a blocked action, patron is unable to renew in OPAC
If 'follow sys pref' is the option, check that a patron may place a hold or renew an item according to the BlockExpiredPatronOpacActions sys pref value

prove t/db_dependent/Circulation.t
prove t/db_dependent/ILSDI_Services.t
prove t/db_dependent/Koha/Object.t
prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Thank you Arthur for testing and your review.
I agree with all your comments and I have followed up on the QA follow-up patch.
Thanks!

Created attachment 165414 [details] [review]
Bug 36453: (QA follow-up): Cosmetic fixes

This is a squash of several improvements:
- Override weird blue color being applied from MultipleSelect when all options from an optgroup are selected;
- Fix input width depending on selected value. Now it should always be same width regardless;
- Updated the system preference UI wording to reflect new behavior;
- Updated UI labels in the patron category edit page to enforce that the actions being selected are "actions to be blocked"
- Corrected sysprefs.sql entry

Adding bug 26297 as a dependency. Work here needs to update BlockExpiredPatronOpacActions in the patron categories REST API spec added by bug 26297.

Created attachment 165454 [details] [review]
Bug 36453: (QA follow-up): Update api spec

Access {staff_url}/api/v1/patron_categories and notice all is as expected
Update some patron category's blocked expired patron OPAC actions and access the endpoint again, notice it's okay.

Updated API tests to include to_api confirming attributes are rendered correctly:
prove t/db_dependent/api/v1/patron_categories

Created attachment 167461 [details] [review]
Bug 36453: DB and data changes

- kohastructure.sql updates
- atomicupdate file
- Mandatory data installer updates

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167462 [details] [review]
Bug 36453: [DO NOT PUSH] DBIC schema files

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167463 [details] [review]
Bug 36453: effective_BlockExpiredPatronOpacActions_contains method

effective_BlockExpiredPatronOpacActions
is now
effective_BlockExpiredPatronOpacActions_contains

This method has been updated to now consider an action param.
Returns true if the given action is to be blocked to expired
patrons or not.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167464 [details] [review]
Bug 36453: Update old occurrences of effective_BlockExpiredPatronOpacActions

Check for 'renew' when appropriate
Check for 'hold' when appropriate

The following command must return nothing before this is pushed to
master:
git grep "\beffective_BlockExpiredPatronOpacActions\b"

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167465 [details] [review]
Bug 36453: Categories admin page:

Account for multiple values param
Input is now multipleselect select
JavaScript
Staff CSS updates to account for multiselect dropdown in categories admin page

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167466 [details] [review]
Bug 36453: BlockExpiredPatronOpacActions system preference

The sys pref now allows multipleselect values instead of YesNo

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167467 [details] [review]
Bug 36453: Update tests

Address new new version of system pref
Address new version of categories DB table BlockExpiredPatronOpacActions column

Test plan, k-t-d, BEFORE applying patch:
1) Edit some patron categories, visit:
/cgi-bin/koha/admin/categories.pl
2) Set different values for the "Block expired patrons" input config
Set a category to block
Set a category to don't block
Set a category to follow the syspref
3) Take note of the current value of the BlockExpiredPatronOpacActions sys pref

Apply patch, then run the following commands
- koha-plack --restart kohadev
- yarn css:build
- run updatedatabase

4) Visit the patron categories you edited before, make sure they all have the correct values for the new config type
5) Check the BlockExpiredPatronOpacActions sys pref and make sure the new value is correct according to what was before
6) Add a new patron of one of the above categories, test that:
If 'hold' is a blocked action, patron is unable to place a hold in OPAC
If 'renew' is a blocked action, patron is unable to renew in OPAC
If 'follow sys pref' is the option, check that a patron may place a hold or renew an item according to the BlockExpiredPatronOpacActions sys pref value

prove t/db_dependent/Circulation.t
prove t/db_dependent/ILSDI_Services.t
prove t/db_dependent/Koha/Object.t
prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167468 [details] [review]
Bug 36453: (QA follow-up): Cosmetic fixes

This is a squash of several improvements:
- Override weird blue color being applied from MultipleSelect when all options from an optgroup are selected;
- Fix input width depending on selected value. Now it should always be same width regardless;
- Updated the system preference UI wording to reflect new behavior;
- Updated UI labels in the patron category edit page to enforce that the actions being selected are "actions to be blocked"
- Corrected sysprefs.sql entry

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167469 [details] [review]
Bug 36453: (QA follow-up): Update api spec

Access {staff_url}/api/v1/patron_categories and notice all is as expected
Update some patron category's blocked expired patron OPAC actions and access the endpoint again, notice it's okay.

Updated API tests to include to_api confirming attributes are rendered correctly:
prove t/db_dependent/api/v1/patron_categories

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167470 [details] [review]
Bug 36453: (QA follow-up) Modernise database update

This patch adds the colored output expected for 24.11.x onward database
updates.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167471 [details] [review]
Bug 36453: DB and data changes

- kohastructure.sql updates
- atomicupdate file
- Mandatory data installer updates

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167472 [details] [review]
Bug 36453: [DO NOT PUSH] DBIC schema files

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167473 [details] [review]
Bug 36453: effective_BlockExpiredPatronOpacActions_contains method

effective_BlockExpiredPatronOpacActions
is now
effective_BlockExpiredPatronOpacActions_contains

This method has been updated to now consider an action param.
Returns true if the given action is to be blocked to expired
patrons or not.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167474 [details] [review]
Bug 36453: Update old occurrences of effective_BlockExpiredPatronOpacActions

Check for 'renew' when appropriate
Check for 'hold' when appropriate

The following command must return nothing before this is pushed to
master:
git grep "\beffective_BlockExpiredPatronOpacActions\b"

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167475 [details] [review]
Bug 36453: Categories admin page:

Account for multiple values param
Input is now multipleselect select
JavaScript
Staff CSS updates to account for multiselect dropdown in categories admin page

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167476 [details] [review]
Bug 36453: BlockExpiredPatronOpacActions system preference

The sys pref now allows multipleselect values instead of YesNo

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167477 [details] [review]
Bug 36453: Update tests

Address new new version of system pref
Address new version of categories DB table BlockExpiredPatronOpacActions column

Test plan, k-t-d, BEFORE applying patch:
1) Edit some patron categories, visit:
/cgi-bin/koha/admin/categories.pl
2) Set different values for the "Block expired patrons" input config
Set a category to block
Set a category to don't block
Set a category to follow the syspref
3) Take note of the current value of the BlockExpiredPatronOpacActions sys pref

Apply patch, then run the following commands
- koha-plack --restart kohadev
- yarn css:build
- run updatedatabase

4) Visit the patron categories you edited before, make sure they all have the correct values for the new config type
5) Check the BlockExpiredPatronOpacActions sys pref and make sure the new value is correct according to what was before
6) Add a new patron of one of the above categories, test that:
If 'hold' is a blocked action, patron is unable to place a hold in OPAC
If 'renew' is a blocked action, patron is unable to renew in OPAC
If 'follow sys pref' is the option, check that a patron may place a hold or renew an item according to the BlockExpiredPatronOpacActions sys pref value

prove t/db_dependent/Circulation.t
prove t/db_dependent/ILSDI_Services.t
prove t/db_dependent/Koha/Object.t
prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167478 [details] [review]
Bug 36453: (QA follow-up) Cosmetic fixes

This is a squash of several improvements:
- Override weird blue color being applied from MultipleSelect when all options from an optgroup are selected;
- Fix input width depending on selected value. Now it should always be same width regardless;
- Updated the system preference UI wording to reflect new behavior;
- Updated UI labels in the patron category edit page to enforce that the actions being selected are "actions to be blocked"
- Corrected sysprefs.sql entry

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167479 [details] [review]
Bug 36453: (QA follow-up) Update api spec

Access {staff_url}/api/v1/patron_categories and notice all is as expected
Update some patron category's blocked expired patron OPAC actions and access the endpoint again, notice it's okay.

Updated API tests to include to_api confirming attributes are rendered correctly:
prove t/db_dependent/api/v1/patron_categories

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 167480 [details] [review]
Bug 36453: (QA follow-up) Modernise database update

This patch adds the colored output expected for 24.11.x onward database
updates.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Great work.. whilst the method name is a little scary I'm happy this is already a big improvement and I couldn't come up with a better name.

Down the line I'd love to see more cleanup like the two methods in Patron::Category merged and the method name somehow clarified/reduced... 

Passing QA

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

Hi all. This broke MySQL 8 support, as it is setting a default value for a MEDIUM column, which is not allowed.

Steps to reproduce:

```shell
ktd down
DB_IMAGE=mysql:8.0 ktd up
```

and you get:

```
C4::Installer::load_sql returned the following errors while attempting to load /kohadevbox/koha/installer/data/mysql/kohastructure.sql:
DBD::mysql::st execute failed: BLOB, TEXT, GEOMETRY or JSON column 'BlockExpiredPatronOpacActions' can't have a default value at /usr/share/perl5/DBIx/RunSQL.pm line 290, <$args{...}> line 1.
Something went wrong loading file /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE TABLE `categories` (
...
```

(In reply to Tomás Cohen Arazi from comment #44)
> Hi all. This broke MySQL 8 support, as it is setting a default value for a
> MEDIUM column, which is not allowed.

I propose we change it to NULL, and make the code take NULL => follow_syspref_BlockExpiredPatronOpacActions behavior.

Maybe we could also reconsider the datatype? Not sure what datatype would allow defaults, but since we only want to store a few things we do probably have a lot of other options.

Can try and test this in a bit, can't remember why the choice for mediumtext but I wonder if just changing it to varchar(255) would fix the issue?

Created attachment 168435 [details] [review]
Bug 36453: (QA follow-up) Address upgraded instances

To test:
1) Apply only this patch, skipping the next one (patch 'Use varchar instead of mediumtext')
2) Run ktd up with default DBMS mariadb:10.5: ktd up
3) Confirm the database state is wrong:
  DESC categories BlockExpiredPatronOpacActions;
  ^ This should return type = mediumtext
4) Run updatedatabase, make sure green success message shows
  Running the above query again will show type = varchar(128)
5) Run updatedatabase again, notice blue info message notifies its already varchar

Created attachment 168436 [details] [review]
Bug 36453: (QA follow-up) Use varchar instead of mediumtext

Test plan:
1) ktd down
2) DB_IMAGE=mysql:8.0 ktd up
3) Should now work with no issues

Please switch back to PQA when a follow-up is added - this way I can't miss it :)

I think we might have got away with changing the kohastructure and the first database update, as noone should be running on main... but this of course is the safest way of doing it. :)

Pushed follow-up patches to main.

(In reply to Katrin Fischer from comment #50)
> Please switch back to PQA when a follow-up is added - this way I can't miss
> it :)

Ooops.

(In reply to Katrin Fischer from comment #51)
> I think we might have got away with changing the kohastructure and the first
> database update, as noone should be running on main... but this of course is
> the safest way of doing it. :)

Agreed. But I think the additional atomic update covers all our basis and further helps ensure this does not come up again for anyone.
Removed 'additional_work_needed'.

This seems like an enhancement to me and I'm reluctant to backport it to 24.05.x. Feel free to tell me I'm wrong.

Multiple database changes and also some failing tests we need to look into - not for backporting.

(In reply to Katrin Fischer from comment #55)
> Multiple database changes and also some failing tests we need to look into -
> not for backporting.

Agreed. What tests are failing as a result of this? Let me know, I'll find the time to look into it.

.js file is using _("") when __("") must be used. I am suspecting some strings to not be picked by the translation script.

(In reply to Jonathan Druart from comment #57)
> .js file is using _("") when __("") must be used. I am suspecting some
> strings to not be picked by the translation script.

Opened bug 38492