Bugzilla – Attachment 136737 Details for
Bug 23681
Make patron restrictions user definable
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23681: (QA follow-up) Proper handling of default option
Bug-23681-QA-follow-up-Proper-handling-of-default-.patch (text/plain), 20.22 KB, created by
Martin Renvoize (ashimema)
on 2022-06-29 15:12:59 UTC
(
hide
)
Description:
Bug 23681: (QA follow-up) Proper handling of default option
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2022-06-29 15:12:59 UTC
Size:
20.22 KB
patch
obsolete
>From 6a08df1208ca54a2d691fe97e94556e2ddd01d0c Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >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. >--- > Koha/RestrictionType.pm | 24 ++- > admin/restrictions.pl | 11 +- > .../data/mysql/atomicupdate/bug_23681.pl | 13 +- > .../en/mandatory/patron_restrictions.yml | 12 +- > 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, 159 insertions(+), 146 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..ea5c01669e 100644 >--- a/installer/data/mysql/en/mandatory/patron_restrictions.yml >+++ b/installer/data/mysql/en/mandatory/patron_restrictions.yml >@@ -28,23 +28,19 @@ tables: > - code: "MANUAL" > display_text: "Manual" > is_system: 1 >- default: 1 >- can_be_added_manually: 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 8c69f82121..d58ca30c17 100644 >--- a/installer/data/mysql/kohastructure.sql >+++ b/installer/data/mysql/kohastructure.sql >@@ -2032,8 +2032,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 @@ > <label for="debarred_type">Type:</label> > <select name="debarred_type"> > [% FOREACH code IN restriction_types.keys %] >- [% IF restriction_types.$code.can_be_added_manually %] >- <option value="[% code | html %]">[% PROCESS restriction_type_description restriction=restriction_types.$code %]</option> >+ [% IF !restriction_types.$code.is_system %] >+ [% IF restriction_types.$code.is_default %] >+ <option value="[% code | html %]" selected>[% PROCESS restriction_type_description restriction=restriction_types.$code %]</option> >+ [% ELSE %] >+ <option value="[% code | html %]">[% PROCESS restriction_type_description restriction=restriction_types.$code %]</option> >+ [% END %] > [% END %] > [% END %] > </select> >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 @@ > <input type="text" value="[% restriction.display_text | html %]" name="display_text" id="display_text" size="50" maxlength="50" class="required" required="required" /> > <span class="required">Required</span> > </li> >- <li> >- <label for="can_be_added_manually">Can be manually added?</label> >- [% IF restriction && restriction.is_system %] >- [% IF restriction.can_be_added_manually %]Yes[% ELSE %]No[% END %] >- [% ELSIF restriction.can_be_added_manually %] >- <input type="checkbox" name="can_be_added_manually" id="can_be_added_manually" checked="checked" value="1" /> >- [% ELSE %] >- <input type="checkbox" name="can_be_added_manually" id="can_be_added_manually" value="1" /> >- [% END %] >- </li> > [% ELSE %] > <li> > <label for="code" class="required">Code: </label> >@@ -124,16 +114,6 @@ > <input type="text" name="display_text" id="display_text" size="50" maxlength="50" class="required" required="required" /> > <span class="required">Required</span> > </li> >- <li> >- <label for="can_be_added_manually">Can be manually added?</label> >- [% IF restriction && restriction.is_system %] >- [% IF restriction.can_be_added_manually %]Yes[% ELSE %]No[% END %] >- [% ELSIF restriction.can_be_added_manually %] >- <input type="checkbox" name="can_be_added_manually" id="can_be_added_manually" checked="checked" value="1" /> >- [% ELSE %] >- <input type="checkbox" name="can_be_added_manually" id="can_be_added_manually" value="1" /> >- [% END %] >- </li> > [% END %] > </ol> > </fieldset> >@@ -180,7 +160,6 @@ > <tr> > <th scope="col">Code</th> > <th scope="col">Label</th> >- <th scope="col">Manual</th> > <th scope="col">Default</th> > <th scope="col">Actions</th> > </tr> >@@ -195,16 +174,16 @@ > [% PROCESS restriction_type_description %] > </td> > <td> >- [% IF restriction.can_be_added_manually %]Yes[% END %] >- </td> >- <td> >- [% IF restriction.default_value %]Yes[% END %] >+ [% IF restriction.is_default %]Yes[% END %] > </td> > <td class="actions"> > <a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/restrictions.pl?op=add_form&code=[% restriction.code | uri %]"><i class="fa fa-pencil"></i> Edit</a> >- [% IF !restriction.is_system %] >+ [% IF !restriction.is_system && !restriction.is_default %] > <a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/restrictions.pl?op=delete_confirm&code=[% restriction.code | uri %]"><i class="fa fa-trash"></i> Delete</a> > [% END %] >+ [% IF !restriction.is_system && !restriction.is_default %] >+ <a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/restrictions.pl?op=make_default&code=[% restriction.code | uri %]"><i class="fa fa-archive"></i> Make default</a> >+ [% END %] > </td> > </tr> > [% 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.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23681
:
93769
|
93770
|
93771
|
93772
|
93773
|
93774
|
93775
|
93776
|
93777
|
93778
|
93779
|
93780
|
93781
|
93782
|
93783
|
93784
|
93785
|
93786
|
94862
|
94863
|
94864
|
94865
|
94866
|
94867
|
94868
|
94869
|
94870
|
94896
|
94897
|
94898
|
94899
|
94900
|
94901
|
94902
|
94903
|
94904
|
95148
|
95255
|
95256
|
95257
|
95258
|
95259
|
95260
|
95261
|
95262
|
95263
|
95264
|
99893
|
99894
|
99895
|
99896
|
99897
|
99898
|
99899
|
99900
|
99901
|
99902
|
130373
|
130374
|
130375
|
130376
|
130377
|
130378
|
130379
|
130380
|
130381
|
130382
|
130383
|
130384
|
132302
|
132303
|
132304
|
132305
|
132306
|
132307
|
132308
|
132309
|
132310
|
132311
|
132312
|
132313
|
132314
|
132315
|
132316
|
132318
|
132319
|
132320
|
134512
|
134513
|
134514
|
134515
|
134516
|
134517
|
134518
|
134519
|
134520
|
134521
|
134522
|
134523
|
134524
|
134525
|
134526
|
134527
|
134528
|
134529
|
134580
|
134581
|
134582
|
134583
|
134584
|
134585
|
134586
|
134587
|
134588
|
134589
|
134590
|
134591
|
134592
|
134593
|
134594
|
134595
|
134596
|
134597
|
134598
|
134599
|
134600
|
134601
|
134602
|
134684
|
134685
|
134686
|
134687
|
134688
|
134689
|
134690
|
134691
|
134692
|
134693
|
134694
|
134696
|
134697
|
134698
|
134699
|
134700
|
134701
|
134702
|
134703
|
134704
|
134705
|
134706
|
134707
|
134708
|
136716
|
136717
|
136718
|
136719
|
136720
|
136721
|
136722
|
136723
|
136724
|
136725
|
136726
|
136727
|
136728
|
136729
|
136730
|
136731
|
136732
|
136733
|
136734
|
136735
|
136736
|
136737
|
136738
|
136739
|
137066
|
137067
|
137068
|
137069
|
137070
|
137071
|
137072
|
137073
|
137074
|
137075
|
137076
|
137077
|
137078
|
137079
|
137080
|
137081
|
137082
|
137083
|
137084
|
137085
|
137086
|
137087
|
137088
|
137089
|
137090
|
137091
|
137092
|
137093
|
137094
|
137102
|
137103
|
137104
|
137105
|
137106
|
137107
|
137108
|
137109
|
137110
|
137111
|
137112
|
137113
|
137114
|
137115
|
137116
|
137117
|
137118
|
137119
|
137120
|
137121
|
137122
|
137123
|
137124
|
137125
|
137126
|
137127
|
137128
|
137129
|
137130
|
139250
|
139251
|
139252
|
139253
|
139254
|
139255
|
139256
|
139257
|
139258
|
139259
|
139260
|
139261
|
139262
|
139263
|
139264
|
139265
|
139266
|
139267
|
139268
|
139269
|
139270
|
139271
|
139272
|
139273
|
139509
|
139512
|
139649
|
139717
|
139768
|
139769
|
139770
|
139771
|
139772
|
139773
|
139774
|
139775
|
139776
|
139777
|
139778
|
139779
|
139780
|
139781
|
139782
|
139783
|
139784
|
139785
|
139786
|
139787
|
139788
|
139789
|
139790
|
139791
|
139792
|
139793
|
139794
|
139795