From 3b1c271f78bb339800fac3f4c2b11ce4900f8e6a Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 4 May 2022 12:36:17 +0100 Subject: [PATCH] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- Koha/RestrictionType.pm | 24 ++- admin/restrictions.pl | 11 +- .../data/mysql/atomicupdate/bug_23681.pl | 13 +- .../en/mandatory/patron_restrictions.yml | 14 +- installer/data/mysql/kohastructure.sql | 3 +- .../prog/en/includes/borrower_debarments.inc | 8 +- .../prog/en/modules/admin/restrictions.tt | 31 +--- t/db_dependent/RestrictionType.t | 52 +++--- t/db_dependent/RestrictionTypes.t | 151 ++++++++++-------- 9 files changed, 160 insertions(+), 147 deletions(-) mode change 100644 => 100755 installer/data/mysql/atomicupdate/bug_23681.pl diff --git a/Koha/RestrictionType.pm b/Koha/RestrictionType.pm index 4d43f20609..d6f6690369 100644 --- a/Koha/RestrictionType.pm +++ b/Koha/RestrictionType.pm @@ -42,7 +42,7 @@ sub delete { my ( $self ) = @_; # Find out what the default is - my $default = Koha::RestrictionTypes->find({ default_value => 1 })->code; + my $default = Koha::RestrictionTypes->find({ is_default => 1 })->code; # Ensure we're not trying to delete a is_system type (this includes # the default type) return 0 if $self->is_system == 1; @@ -63,6 +63,28 @@ sub delete { return 0; } +=head3 make_default + +Set the current restriction type as the default for manual restrictions + +=cut + +sub make_default { + my ( $self ) = @_; + + $self->_result->result_source->schema->txn_do( + sub { + my $types = + Koha::RestrictionTypes->search( { code => { '!=' => $self->code } } ); + $types->update( { is_default => 0 } ); + $self->set( { is_default => 1 } ); + $self->SUPER::store; + } + ); + + return $self; +} + =head2 Internal methods =head3 type diff --git a/admin/restrictions.pl b/admin/restrictions.pl index b41fd3f6cd..6344e6ab92 100755 --- a/admin/restrictions.pl +++ b/admin/restrictions.pl @@ -53,7 +53,6 @@ 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) { @@ -68,9 +67,6 @@ if ( $op eq 'add_form') { } else { my $restriction = Koha::RestrictionTypes->find($code); $restriction->display_text($display_text); - unless ($restriction->is_system) { - $restriction->can_be_added_manually($can_be_added_manually); - } $restriction->store; } } else { @@ -83,13 +79,16 @@ if ( $op eq 'add_form') { } else { my $restriction = Koha::RestrictionType->new({ code => $code, - display_text => $display_text, - can_be_added_manually => $can_be_added_manually + display_text => $display_text }); $restriction->store; } } $op = 'list'; +} elsif ( $op eq 'make_default' ) { + my $restriction = Koha::RestrictionTypes->find($code); + $restriction->make_default; + $op = 'list'; } elsif ( $op eq 'delete_confirm' ) { $template->param( restriction => scalar Koha::RestrictionTypes->find($code) diff --git a/installer/data/mysql/atomicupdate/bug_23681.pl b/installer/data/mysql/atomicupdate/bug_23681.pl old mode 100644 new mode 100755 index 375f5a1459..422de181bc --- a/installer/data/mysql/atomicupdate/bug_23681.pl +++ b/installer/data/mysql/atomicupdate/bug_23681.pl @@ -12,18 +12,17 @@ return { 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 + is_default tinyint(1) NOT NULL DEFAULT 0 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; }); say $out "Added debarment_types table"; $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); + INSERT IGNORE INTO debarment_types (code, display_text, is_system, is_default) VALUES + ('MANUAL', 'Manual', 0, 1), + ('OVERDUES', 'Overdues', 1, 0), + ('SUSPENSION', 'Suspension', 1, 0), + ('DISCHARGE', 'Discharge', 1, 0); }); say $out "Added system debarment_types"; diff --git a/installer/data/mysql/en/mandatory/patron_restrictions.yml b/installer/data/mysql/en/mandatory/patron_restrictions.yml index 26216504bd..634803469c 100644 --- a/installer/data/mysql/en/mandatory/patron_restrictions.yml +++ b/installer/data/mysql/en/mandatory/patron_restrictions.yml @@ -27,24 +27,20 @@ tables: rows: - code: "MANUAL" display_text: "Manual" - is_system: 1 - default: 1 - can_be_added_manually: 0 + is_system: 0 + is_default: 1 - code: "OVERDUES" display_text: "Overdues" is_system: 1 - default: 0 - can_be_added_manually: 0 + is_default: 0 - code: "SUSPENSION" display_text: "Suspension" is_system: 1 - default: 0 - can_be_added_manually: 0 + is_default: 0 - code: "DISCHARGE" display_text: "Discharge" is_system: 1 - default: 0 - can_be_added_manually: 0 + is_default: 0 diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index d906d61fce..b19d86bb79 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -2086,8 +2086,7 @@ CREATE TABLE 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 + is_default 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 e5ff9639dc..c70073db7a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc @@ -58,8 +58,12 @@ 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 4b9767e46e..a8dd141271 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt @@ -103,16 +103,6 @@ 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 %]
  • @@ -124,16 +114,6 @@ 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 %] @@ -180,7 +160,6 @@ Code Label - Manual Default Actions @@ -195,16 +174,16 @@ [% PROCESS restriction_type_description %] - [% IF restriction.can_be_added_manually %]Yes[% END %] - - - [% IF restriction.default_value %]Yes[% END %] + [% IF restriction.is_default %]Yes[% END %] Edit - [% IF !restriction.is_system %] + [% IF !restriction.is_system && !restriction.is_default %] Delete [% END %] + [% IF !restriction.is_system && !restriction.is_default %] + Make default + [% END %] [% END %] diff --git a/t/db_dependent/RestrictionType.t b/t/db_dependent/RestrictionType.t index aea33a5800..0e74c96e1e 100755 --- a/t/db_dependent/RestrictionType.t +++ b/t/db_dependent/RestrictionType.t @@ -10,51 +10,55 @@ use Test::More tests => 3; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; -my $dbh = C4::Context->dbh; +my $dbh = C4::Context->dbh; my $builder = t::lib::TestBuilder->new; 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 +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'ONE', + display_text => 'One', + is_system => 1, + is_default => 0 + } } -}); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'TWO', - display_text => 'Two', - readonly => 1, - is_system => 1 +); +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'TWO', + display_text => 'Two', + is_system => 1, + is_default => 1 + } } -}); +); # keyed_on_code -my $keyed = Koha::RestrictionTypes->keyed_on_code; +my $keyed = Koha::RestrictionTypes->keyed_on_code; my $expecting = { ONE => { code => 'ONE', display_text => 'One', - readonly => 1, - is_system => 0 + is_system => 1, + is_default => 0 }, TWO => { code => 'TWO', display_text => 'Two', - readonly => 1, - is_system => 1 + is_system => 1, + is_default => 1 } }; -is_deeply($keyed, $expecting, 'keyed_on_code returns correctly'); +is_deeply( $keyed, $expecting, 'keyed_on_code returns correctly' ); $schema->storage->txn_rollback; diff --git a/t/db_dependent/RestrictionTypes.t b/t/db_dependent/RestrictionTypes.t index 2c8b0519a4..1b9f4e1e68 100755 --- a/t/db_dependent/RestrictionTypes.t +++ b/t/db_dependent/RestrictionTypes.t @@ -10,7 +10,7 @@ use Test::More tests => 6; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; -my $dbh = C4::Context->dbh; +my $dbh = C4::Context->dbh; my $builder = t::lib::TestBuilder->new; use_ok('Koha::RestrictionType'); @@ -19,91 +19,102 @@ use_ok('Koha::RestrictionTypes'); $dbh->do(q|DELETE FROM borrower_debarments|); $dbh->do(q|DELETE FROM debarment_types|); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'ONE', - display_text => 'One', - is_system => 1, - default_value => 0, - can_be_added_manually => 0 +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'ONE', + display_text => 'One', + is_system => 1, + is_default => 0, + } } -}); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'TWO', - display_text => 'Two', - is_system => 1, - default_value => 1, - can_be_added_manually => 0 +); +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'TWO', + display_text => 'Two', + is_system => 1, + is_default => 1, + } } -}); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'THREE', - display_text => 'Three', - is_system => 1, - default_value => 0, - can_be_added_manually => 0 +); +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'THREE', + display_text => 'Three', + is_system => 1, + is_default => 0, + } } -}); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'FOUR', - display_text => 'Four', - is_system => 0, - default_value => 0, - can_be_added_manually => 0 +); +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'FOUR', + display_text => 'Four', + is_system => 0, + is_default => 0, + } } -}); -$builder->build({ - source => 'DebarmentType', - value => { - code => 'FIVE', - display_text => 'Five', - is_system => 0, - default_value => 0, - can_be_added_manually => 0 +); +$builder->build( + { + source => 'DebarmentType', + value => { + code => 'FIVE', + display_text => 'Five', + is_system => 0, + is_default => 0, + } } -}); +); # Can we create RestrictionTypes -my $created = Koha::RestrictionTypes->find({ code => 'ONE' }); -ok( $created->display_text eq 'One', 'Restrictions created'); +my $created = Koha::RestrictionTypes->find( { code => 'ONE' } ); +ok( $created->display_text eq 'One', 'Restrictions created' ); # Can we delete RestrictionTypes, when appropriate -my $deleted = Koha::RestrictionTypes->find({ code => 'FOUR' })->delete; -ok( $deleted, 'Restriction deleted'); -my $not_deleted = Koha::RestrictionTypes->find({ code => 'TWO' })->delete; -ok( !$not_deleted, 'Read only restriction not deleted'); +my $deleted = Koha::RestrictionTypes->find( { code => 'FOUR' } )->delete; +ok( $deleted, 'Restriction deleted' ); +my $not_deleted = Koha::RestrictionTypes->find( { code => 'TWO' } )->delete; +ok( !$not_deleted, 'Read only restriction not deleted' ); # Add a patron with a debarment -my $library = $builder->build({ source => 'Branch' }); +my $library = $builder->build( { source => 'Branch' } ); -my $patron_category = $builder->build({ source => 'Category' }); -my $borrowernumber = Koha::Patron->new({ - firstname => 'my firstname', - surname => 'my surname', - categorycode => $patron_category->{categorycode}, - branchcode => $library->{branchcode}, -})->store->borrowernumber; +my $patron_category = $builder->build( { source => 'Category' } ); +my $borrowernumber = Koha::Patron->new( + { + firstname => 'my firstname', + surname => 'my surname', + categorycode => $patron_category->{categorycode}, + branchcode => $library->{branchcode}, + } +)->store->borrowernumber; -Koha::Patron::Debarments::AddDebarment({ - borrowernumber => $borrowernumber, - expiration => '9999-06-10', - type => 'FIVE', - comment => 'Test 1', -}); +Koha::Patron::Debarments::AddDebarment( + { + borrowernumber => $borrowernumber, + expiration => '9999-06-10', + type => 'FIVE', + comment => 'Test 1', + } +); # Now delete a code and check debarments using that code switch to # using the default -my $to_delete = Koha::RestrictionTypes->find({ code => 'FIVE' })->delete; -my $debarments = Koha::Patron::Debarments::GetDebarments({ - borrowernumber => $borrowernumber -}); +my $to_delete = Koha::RestrictionTypes->find( { code => 'FIVE' } )->delete; +my $debarments = Koha::Patron::Debarments::GetDebarments( + { + borrowernumber => $borrowernumber + } +); is( $debarments->[0]->{type}, 'TWO', 'Debarments update with restrictions' ); $schema->storage->txn_rollback; -- 2.39.5