From 20349d87379dc6f66290050c0b7c2beaf6763c06 Mon Sep 17 00:00:00 2001 From: Emmi Takkinen Date: Mon, 2 May 2022 15:40:35 +0300 Subject: [PATCH] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- Koha/RestrictionType.pm | 6 +-- admin/restrictions.pl | 9 +++- ...81_add_PatronRestrictionTypes_syspref.perl | 6 --- ...3681_add_PatronRestrictionTypes_syspref.pl | 14 ++++++ .../bug_23681_add_debarment_types.perl | 33 ------------ .../bug_23681_add_debarment_types.pl | 37 ++++++++++++++ ...1_add_manage_patron_restrictions_perm.perl | 9 ---- ...681_add_manage_patron_restrictions_perm.pl | 14 ++++++ .../en/mandatory/patron_restrictions.sql | 5 -- .../en/mandatory/patron_restrictions.txt | 1 - .../en/mandatory/patron_restrictions.yml | 50 +++++++++++++++++++ installer/data/mysql/kohastructure.sql | 5 +- .../prog/en/includes/borrower_debarments.inc | 4 +- .../modules/admin/preferences/accounting.pref | 6 +++ .../en/modules/admin/preferences/patrons.pref | 6 --- .../prog/en/modules/admin/restrictions.tt | 39 ++++++++++++--- .../prog/en/modules/members/memberentrygen.tt | 4 +- t/db_dependent/RestrictionTypes.t | 32 ++++++------ 18 files changed, 191 insertions(+), 89 deletions(-) delete mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.perl create mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.pl delete mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.perl create mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.pl delete mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.perl create mode 100644 installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.pl delete mode 100644 installer/data/mysql/en/mandatory/patron_restrictions.sql delete mode 100644 installer/data/mysql/en/mandatory/patron_restrictions.txt create mode 100644 installer/data/mysql/en/mandatory/patron_restrictions.yml diff --git a/Koha/RestrictionType.pm b/Koha/RestrictionType.pm index 70946a6ff4..4d43f20609 100644 --- a/Koha/RestrictionType.pm +++ b/Koha/RestrictionType.pm @@ -42,10 +42,10 @@ sub delete { my ( $self ) = @_; # Find out what the default is - my $default = Koha::RestrictionTypes->find({ is_system => 1 })->code; - # Ensure we're not trying to delete a readonly type (this includes + my $default = Koha::RestrictionTypes->find({ default_value => 1 })->code; + # Ensure we're not trying to delete a is_system type (this includes # the default type) - return 0 if $self->readonly == 1; + return 0 if $self->is_system == 1; # We can't use Koha objects here because Koha::Patron::Debarments # is not a Koha object. So we'll do it old skool my $rows = C4::Context->dbh->do( diff --git a/admin/restrictions.pl b/admin/restrictions.pl index 634cc065a7..d3449269a5 100755 --- a/admin/restrictions.pl +++ b/admin/restrictions.pl @@ -53,6 +53,7 @@ if ( $op eq 'add_form') { } elsif ( $op eq 'add_validate' ) { my $display_text = $input->param('display_text'); + my $can_be_added_manually = $input->param('can_be_added_manually') || 0; my $is_a_modif = $input->param("is_a_modif"); if ($is_a_modif) { @@ -66,7 +67,10 @@ if ( $op eq 'add_form') { }; } else { my $restriction = Koha::RestrictionTypes->find($code); - $restriction->display_text($display_text); + unless ($restriction->is_system) { + $restriction->display_text($display_text); + $restriction->can_be_added_manually($can_be_added_manually); + } $restriction->store; } } else { @@ -79,7 +83,8 @@ if ( $op eq 'add_form') { } else { my $restriction = Koha::RestrictionType->new({ code => $code, - display_text => $display_text + display_text => $display_text, + can_be_added_manually => $can_be_added_manually }); $restriction->store; } diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.perl b/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.perl deleted file mode 100644 index a4d6323554..0000000000 --- a/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.perl +++ /dev/null @@ -1,6 +0,0 @@ -$DBversion = 'XXX'; # will be replaced by the RM -if( CheckVersion( $DBversion ) ) { - $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('PatronRestrictionTypes', '0', 'If enabled, it is possible to specify the "type" of patron restriction being applied.', '', 'YesNo'); | ); - SetVersion( $DBversion ); - print "Upgrade to $DBversion done (Bug 23681 - Add PatronRestrictionTypes syspref)\n"; -} diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.pl b/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.pl new file mode 100644 index 0000000000..51940e5c65 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_23681_add_PatronRestrictionTypes_syspref.pl @@ -0,0 +1,14 @@ +use Modern::Perl; + +return { + bug_number => "23681", + description => "Add PatronRestrictionTypes syspref", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + # Do you stuffs here + $dbh->do(q{INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('PatronRestrictionTypes', '0', 'If enabled, it is possible to specify the "type" of patron restriction being applied.', '', 'YesNo');}); + # Print useful stuff here + say $out "Update is going well so far"; + }, +}; \ No newline at end of file diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.perl b/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.perl deleted file mode 100644 index 7380e141b2..0000000000 --- a/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.perl +++ /dev/null @@ -1,33 +0,0 @@ -$DBversion = 'XXX'; - -if ( CheckVersion( $DBversion ) ) { - - if ( !TableExists( 'debarment_types' ) ) { - $dbh->do( q| - CREATE TABLE debarment_types ( - code varchar(50) NOT NULL PRIMARY KEY, - display_text text NOT NULL, - readonly tinyint(1) NOT NULL DEFAULT 0, - system tinyint(1) NOT NULL DEFAULT 0 - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - | ); - $dbh->do( q| - INSERT INTO debarment_types (code, display_text, readonly, system) VALUES - ('MANUAL', 'Manual', 1, 1), - ('OVERDUES', 'Overdues', 1, 0), - ('SUSPENSION', 'Suspension', 1, 0), - ('DISCHARGE', 'Discharge', 1, 0); - |); - $dbh->do( q| - ALTER TABLE borrower_debarments - MODIFY COLUMN type varchar(50) NOT NULL - | ); - $dbh->do( q| - ALTER TABLE borrower_debarments - ADD CONSTRAINT borrower_debarments_ibfk_2 FOREIGN KEY (type) REFERENCES debarment_types(code) ON DELETE NO ACTION ON UPDATE CASCADE; - | ); - } - - SetVersion( $DBversion ); - print "Upgrade to $DBversion done (Bug 23681 - Add debarment_types)\n"; -} diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.pl b/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.pl new file mode 100644 index 0000000000..4aac79cb8c --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_23681_add_debarment_types.pl @@ -0,0 +1,37 @@ +use Modern::Perl; + +return { + bug_number => "23681", + description => "Add debarment_types", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + # Do you stuffs here + $dbh->do(q{ + CREATE TABLE IF NOT EXISTS debarment_types ( + code varchar(50) NOT NULL PRIMARY KEY, + display_text text NOT NULL, + is_system tinyint(1) NOT NULL DEFAULT 0, + default_value tinyint(1) NOT NULL DEFAULT 0, + can_be_added_manually tinyint(1) NOT NULL DEFAULT 0 + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; + }); + $dbh->do(q{ + INSERT IGNORE INTO debarment_types (code, display_text, is_system, default_value, can_be_added_manually) VALUES + ('MANUAL', 'Manual', 1, 1, 0), + ('OVERDUES', 'Overdues', 1, 0, 0), + ('SUSPENSION', 'Suspension', 1, 0, 0), + ('DISCHARGE', 'Discharge', 1, 0, 0); + }); + $dbh->do(q{ + ALTER TABLE borrower_debarments + MODIFY COLUMN type varchar(50) NOT NULL + }); + $dbh->do(q{ + ALTER TABLE borrower_debarments + ADD CONSTRAINT borrower_debarments_ibfk_2 FOREIGN KEY (type) REFERENCES debarment_types(code) ON DELETE NO ACTION ON UPDATE CASCADE; + }); + # Print useful stuff here + say $out "Update is going well so far"; + }, +}; \ No newline at end of file diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.perl b/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.perl deleted file mode 100644 index 283791ba8a..0000000000 --- a/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.perl +++ /dev/null @@ -1,9 +0,0 @@ -$DBversion = 'XXX'; -if( CheckVersion( $DBversion ) ) { - $dbh->do(q{ - INSERT IGNORE INTO permissions (module_bit, code, description) VALUES - ( 3, 'manage_patron_restrictions', 'Manage patron restrictions') - }); - SetVersion( $DBversion ); - print "Upgrade to $DBversion done (Bug 23681 - Add manage_patron_restrictions_permission)\n"; -} diff --git a/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.pl b/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.pl new file mode 100644 index 0000000000..396ca8f26b --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_23681_add_manage_patron_restrictions_perm.pl @@ -0,0 +1,14 @@ +use Modern::Perl; + +return { + bug_number => "23681", + description => "Add manage_patron_restrictions_permission", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + # Do you stuffs here + $dbh->do(q{ INSERT IGNORE INTO permissions (module_bit, code, description) VALUES ( 3, 'manage_patron_restrictions', 'Manage patron restrictions')}); + # Print useful stuff here + say $out "Update is going well so far"; + }, +}; \ No newline at end of file diff --git a/installer/data/mysql/en/mandatory/patron_restrictions.sql b/installer/data/mysql/en/mandatory/patron_restrictions.sql deleted file mode 100644 index c3e61322ad..0000000000 --- a/installer/data/mysql/en/mandatory/patron_restrictions.sql +++ /dev/null @@ -1,5 +0,0 @@ -INSERT INTO debarment_types (code, display_text, readonly, system) VALUES - ('MANUAL', 'Manual', 1, 1), - ('OVERDUES', 'Overdues', 1, 0), - ('SUSPENSION', 'Suspension', 1, 0), - ('DISCHARGE', 'Discharge', 1, 0); diff --git a/installer/data/mysql/en/mandatory/patron_restrictions.txt b/installer/data/mysql/en/mandatory/patron_restrictions.txt deleted file mode 100644 index b17a7c1270..0000000000 --- a/installer/data/mysql/en/mandatory/patron_restrictions.txt +++ /dev/null @@ -1 +0,0 @@ -Default Koha system patron restriction types diff --git a/installer/data/mysql/en/mandatory/patron_restrictions.yml b/installer/data/mysql/en/mandatory/patron_restrictions.yml new file mode 100644 index 0000000000..c0aeb7a396 --- /dev/null +++ b/installer/data/mysql/en/mandatory/patron_restrictions.yml @@ -0,0 +1,50 @@ +--- +# +# Copyright 2020 Koha Development Team +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +description: + - "Default Koha system patron restriction types" + +tables: + - debarment_types: + translatable: [ display_text ] + multiline: [] + rows: + - code: "MANUAL" + display_text: "Manual" + is_system: 1 + default: 1 + can_be_added_manually: 0 + + - code: "OVERDUES" + display_text: "Overdues" + is_system: 1 + default: 0 + can_be_added_manually: 0 + + - code: "SUSPENSION" + display_text: "Suspension" + is_system: 1 + default: 0 + can_be_added_manually: 0 + + - code: "DISCHARGE" + display_text: "Discharge" + is_system: 1 + default: 0 + can_be_added_manually: 0 \ No newline at end of file diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index f527b11fb3..d906d61fce 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -2085,8 +2085,9 @@ DROP TABLE IF EXISTS `debarment_types`; CREATE TABLE debarment_types ( code varchar(50) NOT NULL PRIMARY KEY, display_text text NOT NULL, - readonly tinyint(1) NOT NULL DEFAULT 0, - is_system tinyint(1) NOT NULL DEFAULT 0 + is_system tinyint(1) NOT NULL DEFAULT 0, + default_value tinyint(1) NOT NULL DEFAULT 0, + can_be_added_manually tinyint(1) NOT NULL DEFAULT 0 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; -- diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc index ebf9f0d538..b489c64123 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc @@ -64,7 +64,9 @@ diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref index cb79d1d379..7450736af1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref @@ -57,3 +57,9 @@ Accounting: branchyyyymmincr: 'Automatically generate credit numbers in the form yyyymm0001' incremental: 'Automatically generate credit numbers in the form 1, 2, 3' - Automatic generation also has to be enabled for each credit type (Configure credit types) + - + - pref: PatronRestrictionTypes + choices: + 1: Allow + 0: Don't allow + - "the type of patron restriction to be specified when applying manually." diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref index d3bf0e03d3..54f9945dfe 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref @@ -341,12 +341,6 @@ Patrons: 1: Allow 0: "Don't allow" - staff to set the ability for a patron's fines to be viewed by linked patrons in the OPAC. - - - - pref: PatronRestrictionTypes - choices: - 1: Allow - 0: Don't allow - - "the type of patron restriction to be specified when applying manually." Privacy: - diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt index ff7375cda2..82dcc485d7 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt @@ -21,19 +21,19 @@
  • Administration
    • -
  • - Patron restrictions -
    • [% IF op == 'list' %]
  • - All restrictions + Patron restrictions
    • [% END %] [% IF op == 'add_form' %] +
  • + Patron restrictions +
    • [% IF restriction %]
  • @@ -50,6 +50,9 @@ [% END %] [% IF op == 'delete_confirm' %] +
  • + Patron restrictions +
  • Delete restriction? @@ -99,6 +102,16 @@ Required
    • +
  • + + [% IF restriction && restriction.is_system %] + [% IF restriction.can_be_added_manually %]Yes[% ELSE %]No[% END %] + [% ELSIF restriction.can_be_added_manually %] + + [% ELSE %] + + [% END %] +
    • [% ELSE %]
  • @@ -110,6 +123,16 @@ Required
    • +
  • + + [% IF restriction && restriction.is_system %] + [% IF restriction.can_be_added_manually %]Yes[% ELSE %]No[% END %] + [% ELSIF restriction.can_be_added_manually %] + + [% ELSE %] + + [% END %] +
    • [% END %] @@ -156,6 +179,7 @@ Code Label + Manual Default Actions @@ -170,11 +194,14 @@ [% restriction.display_text | html %] - [% IF restriction.is_system %]Yes[% END %] + [% IF restriction.can_be_added_manually %]Yes[% END %] + + + [% IF restriction.default_value %]Yes[% END %]     Edit - [% IF !restriction.readonly %] + [% IF !restriction.is_system %] Delete [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt index ade7a102bc..f10ae9c4b4 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -1402,7 +1402,9 @@ legend:hover { diff --git a/t/db_dependent/RestrictionTypes.t b/t/db_dependent/RestrictionTypes.t index 1251846579..2c8b0519a4 100755 --- a/t/db_dependent/RestrictionTypes.t +++ b/t/db_dependent/RestrictionTypes.t @@ -17,15 +17,16 @@ use_ok('Koha::RestrictionType'); use_ok('Koha::RestrictionTypes'); $dbh->do(q|DELETE FROM borrower_debarments|); -Koha::RestrictionTypes->search->delete; +$dbh->do(q|DELETE FROM debarment_types|); $builder->build({ source => 'DebarmentType', value => { code => 'ONE', display_text => 'One', - readonly => 1, - is_system => 0 + is_system => 1, + default_value => 0, + can_be_added_manually => 0 } }); $builder->build({ @@ -33,8 +34,9 @@ $builder->build({ value => { code => 'TWO', display_text => 'Two', - readonly => 1, - is_system => 1 + is_system => 1, + default_value => 1, + can_be_added_manually => 0 } }); $builder->build({ @@ -42,8 +44,9 @@ $builder->build({ value => { code => 'THREE', display_text => 'Three', - readonly => 1, - is_system => 0 + is_system => 1, + default_value => 0, + can_be_added_manually => 0 } }); $builder->build({ @@ -51,8 +54,9 @@ $builder->build({ value => { code => 'FOUR', display_text => 'Four', - readonly => 0, - is_system => 0 + is_system => 0, + default_value => 0, + can_be_added_manually => 0 } }); $builder->build({ @@ -60,8 +64,9 @@ $builder->build({ value => { code => 'FIVE', display_text => 'Five', - readonly => 0, - is_system => 0 + is_system => 0, + default_value => 0, + can_be_added_manually => 0 } }); @@ -71,10 +76,9 @@ ok( $created->display_text eq 'One', 'Restrictions created'); # Can we delete RestrictionTypes, when appropriate my $deleted = Koha::RestrictionTypes->find({ code => 'FOUR' })->delete; -ok( $deleted == 1, 'Restriction deleted'); +ok( $deleted, 'Restriction deleted'); my $not_deleted = Koha::RestrictionTypes->find({ code => 'TWO' })->delete; -ok( $not_deleted == 0, 'Read only restriction not deleted'); - +ok( !$not_deleted, 'Read only restriction not deleted'); # Add a patron with a debarment my $library = $builder->build({ source => 'Branch' }); -- 2.39.5