Bugzilla – Attachment 124448 Details for
Bug 28935
No filtering on patron's data on member entry pages (OPAC, self registration, staff interface)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28935: Rename get_ui_fields, introduce add and remove
Bug-28935-Rename-getuifields-introduce-add-and-rem.patch (text/plain), 11.20 KB, created by
Marcel de Rooy
on 2021-09-03 08:26:19 UTC
(
hide
)
Description:
Bug 28935: Rename get_ui_fields, introduce add and remove
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2021-09-03 08:26:19 UTC
Size:
11.20 KB
patch
obsolete
>From b24a3d7d1992a2228295212c3e16cf87f92a9b9d Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Fri, 3 Sep 2021 08:23:31 +0000 >Subject: [PATCH] Bug 28935: Rename get_ui_fields, introduce add and remove >Content-Type: text/plain; charset=utf-8 > >--- > Koha/Allowlist.pm | 93 +++++++++++++----------- > Koha/Patron/Allowlist.pm | 65 ++--------------- > t/db_dependent/Koha/Patron/Allowlist.t | 97 ++++++-------------------- > 3 files changed, 82 insertions(+), 173 deletions(-) > >diff --git a/Koha/Allowlist.pm b/Koha/Allowlist.pm >index d0c67b7758..831f91709e 100644 >--- a/Koha/Allowlist.pm >+++ b/Koha/Allowlist.pm >@@ -1,7 +1,6 @@ > package Koha::Allowlist; > > use Modern::Perl; >-use Array::Utils qw( array_minus ); > > =head1 NAME > >@@ -11,13 +10,13 @@ Koha::Allowlist - Allowlist implementation base class > > =head2 Class Methods > >- my $allow_list Koha::Allowlist->new({ interface => 'staff' }); >- > =head3 new > >-Constructor. >+ my $allow_list = Koha::Allowlist->new({ interface => 'staff' }); >+ >+ Constructor. > >-Interface must be 'staff' or 'opac' - default to 'opac' >+ Interface must be 'staff' or 'opac' - defaults to 'opac' > > =cut > >@@ -29,65 +28,79 @@ sub new { > ( $args->{interface} && $args->{interface} eq 'staff' ) > ? 'staff' > : 'opac'; >+ $self->{_entries} = {}; >+ $self->_get_entries if $self->can('_get_entries'); > return $self; > } > >-=head3 apply >+=head3 add > >- my $ui_fields = Koha::Allowlist->new({ interface => 'staff' })->apply({ input => $hashref, additional_deny_list => [qw( list of fields )] }); >+ $list->add( 'column1', { column2 => { staff => 1 }} ); > >+=cut > >-Apply an allowlist to input data. >+sub add { >+ my ( $self, @args ) = @_; >+ foreach my $arg ( @args ) { >+ if( !ref($arg) ) { >+ $self->{_entries}->{$arg} = 1; >+ } elsif( ref($arg) eq 'HASH' ) { >+ $self->{_entries} = { %{$self->{_entries}}, %$arg }; >+ } >+ } >+} >+ >+=head3 remove >+ >+ $list->remove( 'column3', 'column4' ); > > =cut > >-sub apply { >- my ( $self, $params ) = @_; >+sub remove { >+ my ( $self, @args ) = @_; >+ foreach my $arg ( @args ) { >+ delete $self->{_entries}->{$arg}; >+ } >+} > >- my $input = $params->{input}; >- my $additional_deny_list = $params->{additional_deny_list} || []; >+=head3 apply > >- my $blocked = {}; >- my $ui_fields = { map { $_ => 1 } $self->get_ui_fields() }; >- return unless $ui_fields and %$ui_fields; >+ my $blocked = $allowlist->apply({ input => $hashref, verbose => 1 }); > >- delete $ui_fields->{$_} for @$additional_deny_list; >+ Apply an allowlist to input data. Note that we modify the $hashref argument! >+ The verbose flag controls the warn statement. > >- return unless $input and %$input; >+ Returns a hash of blocked entries. Might be useful for logging purposes? > >- my @keys = keys %$input; >- foreach my $key (@keys){ >- unless ( exists $ui_fields->{ $key } ) { >- #NOTE: We capture the deleted data so that it can be used for logging purposes >- $blocked->{$key} = delete $input->{ $key }; >- } >- } >+=cut > >- if ( %$blocked ) { >- while ( my ($k, $v)=each %$blocked){ >- # FIXME We should raise an exception from here >- my @c = caller; >- warn sprintf "Forbidden - Tried to modify '%s' with '%s' from %s", $k, $v, "@c"; >+sub apply { >+ my ( $self, $params ) = @_; >+ my $input = $params->{input} or return; >+ >+ my $blocked = {}; >+ foreach my $key ( keys %$input ){ >+ unless ( exists $self->{_entries}->{$key} ) { >+ $blocked->{$key} = delete $input->{ $key }; > } > } >- >+ warn 'Koha::Allowlist: apply blocked one or more keys' if keys %$blocked && $params->{verbose}; >+ # TODO Pass details and caller to Koha::Logger ? > return $blocked; > } > >-=head3 get_ui_fields >+=head3 _get_entries >+ >+ my $fields = $self->_get_entries; > >- my $ui_fields = $self->get_ui_fields(); >- my $ui_fields = Koha::Patron::Allowlist::Public->new->get_ui_fields >+ This routine should be overridden when subclassing. > > =cut > >-sub get_ui_fields { >- my ($self) = @_; >- my @all_fields = $self->_get_all_fields(); >- my @global_deny_list = $self->_global_deny_list(); >- my @deny_list = ( $self->{interface} eq 'staff' ) ? $self->_deny_list_staff() : $self->_deny_list_opac(); >- my @global_allow_list = array_minus @all_fields, @global_deny_list; >- return array_minus @global_allow_list, @deny_list; >+sub _get_entries { >+ my ( $self ) = @_; >+ # You could do something here like: >+ # $self->add( 'myfield' ); > } > > 1; >diff --git a/Koha/Patron/Allowlist.pm b/Koha/Patron/Allowlist.pm >index 259d61c928..7c9050b10c 100644 >--- a/Koha/Patron/Allowlist.pm >+++ b/Koha/Patron/Allowlist.pm >@@ -1,12 +1,11 @@ > package Koha::Patron::Allowlist; > >+use Modern::Perl; > use parent 'Koha::Allowlist'; > >-# This must not be replaced by Koha::Patrons->columns >-# We want developper to not forget to adjust it manually and add new attribute to the deny list if needed >-sub _get_all_fields { >- return qw( >- borrowernumber >+sub _get_entries { >+ my ( $self ) = @_; >+ my @allowed = qw( > cardnumber > surname > firstname >@@ -40,26 +39,13 @@ sub _get_all_fields { > dateofbirth > branchcode > categorycode >- dateenrolled >- dateexpiry >- date_renewed >- gonenoaddress >- lost >- debarred >- debarredcomment > contactname > contactfirstname > contacttitle >- borrowernotes >- relationship > sex > password >- flags > userid >- opacnote > contactnote >- sort1 >- sort2 > altcontactfirstname > altcontactsurname > altcontactaddress1 >@@ -70,49 +56,15 @@ sub _get_all_fields { > altcontactcountry > altcontactphone > smsalertnumber >- sms_provider_id >- privacy >- privacy_guarantor_fines >- privacy_guarantor_checkouts >- checkprevcheckout >- updated_on >- lastseen >- lang >- login_attempts >- overdrive_auth_token >- anonymized >- autorenew_checkouts > primary_contact_method > ); >-} >- >-sub _global_deny_list { >- return qw( >- borrowernumber >- date_renewed >- debarred >- debarredcomment >- flags >- privacy >- privacy_guarantor_fines >- privacy_guarantor_checkouts >- checkprevcheckout >- updated_on >- lastseen >- lang >- login_attempts >- overdrive_auth_token >- anonymized >- ); >-} >+ $self->add( @allowed ); > >-sub _deny_list_opac { >- return qw( >+ my @allowed_staff = qw( > dateenrolled > dateexpiry > gonenoaddress > lost >- borrowernotes > relationship > opacnote > sort1 >@@ -120,10 +72,7 @@ sub _deny_list_opac { > sms_provider_id > autorenew_checkouts > ); >-} >- >-sub _deny_list_staff { >- return qw(); >+ $self->add( @allowed_staff ) if $self->{interface} eq 'staff'; > } > > 1; >diff --git a/t/db_dependent/Koha/Patron/Allowlist.t b/t/db_dependent/Koha/Patron/Allowlist.t >index edec69c678..0083b9fd0d 100755 >--- a/t/db_dependent/Koha/Patron/Allowlist.t >+++ b/t/db_dependent/Koha/Patron/Allowlist.t >@@ -16,86 +16,33 @@ > # along with Koha; if not, see <http://www.gnu.org/licenses>. > > use Modern::Perl; >- >-use Test::More tests => 4; >- >-use Test::MockModule; >-use Test::Exception; >-use Test::Warn; >- >-use Koha::Patrons; >+use Test::More tests => 1; > use Koha::Patron::Allowlist; > >-use t::lib::Mocks; >+subtest 'Patron::Allowlist' => sub { >+ plan tests => 12; > >-subtest '_get_all_fields' => sub { >- my @columns = Koha::Patrons->columns; >- is_deeply( >- \@columns, >- [ Koha::Patron::Allowlist->_get_all_fields ], >- 'All DB columns must be listed in _get_all_fields' >- ); >-}; >- >-subtest 'Staff Allowlist' => sub { >- plan tests => 4; >- my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; >+ my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; > my $allowlist = Koha::Patron::Allowlist->new({ interface => 'staff' }); >- warning_like { >- $allowlist->apply({ input => $input, }); >- } qr{Forbidden - Tried to modify 'flags' with '1' from}; >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname preserved' ); >- is( $input->{flags}, undef, 'flags filtered' ); >-}; >- >-subtest 'Public Allowlist' => sub { >- plan tests => 4; >- my $input = { firstname => 'test firstname', surname => 'test surname', flags => '1' }; >- my $allowlist = Koha::Patron::Allowlist->new(); >- warning_like { >- $allowlist->apply({ input => $input, }); >- } qr{Forbidden - Tried to modify 'flags' with '1' from}; >+ my $blocked = $allowlist->apply({ input => $input }); > >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname preserved' ); >+ is( $blocked->{unexist}, 2, "Unexist got blocked" ); >+ is( $input->{unexist}, undef, 'unexist filtered' ); >+ is( $blocked->{flags}, 1, "Flags got blocked" ); > is( $input->{flags}, undef, 'flags filtered' ); >-}; >- >-subtest 'additional_deny_list' => sub { >- plan tests => 10; >- >- my $input = { firstname => 'test firstname', surname => 'test surname' }; >- my $allowlist = Koha::Patron::Allowlist->new(); >- >- $allowlist->apply({ >- input => $input, >- additional_deny_list => [], >- }); >- > is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname filtered' ); >- is( $input->{flags}, undef, 'flags filtered' ); >- >- $allowlist->apply({ >- input => $input, >- additional_deny_list => ['not_here'], >- }); >- >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, 'test surname', 'surname filtered' ); >- is( $input->{flags}, undef, 'flags filtered' ); >- >- warning_like { >- $allowlist->apply({ >- input => $input, >- additional_deny_list => ['surname'], >- }); >- } >- qr{Forbidden - Tried to modify 'surname' with 'test surname' from}; >- >- is( $input->{firstname}, 'test firstname', 'firstname preserved' ); >- is( $input->{surname}, undef, 'surname filtered' ); >- is( $input->{flags}, undef, 'flags filtered' ); >- >+ is( $input->{surname}, 'test surname', 'surname preserved' ); >+ is( $input->{gonenoaddress}, 1, 'gonenoaddress preserved on staff' ); >+ >+ $input = { firstname => 'test firstname', surname => 'test surname', flags => '1', gonenoaddress => 1, unexist => 2 }; >+ my $opacallowlist = Koha::Patron::Allowlist->new; >+ $blocked = $opacallowlist->apply({ input => $input }); >+ is( $blocked->{unexist}, 2, "Unexist got blocked on opac too"); >+ is( $blocked->{flags}, 1, "Flags got blocked on opac too" ); >+ is( $blocked->{gonenoaddress}, 1, "Gonenoaddress got blocked on opac now" ); >+ is( $input->{gonenoaddress}, undef, 'gonenoaddress filtered, opac' ); >+ is( $input->{surname}, 'test surname', 'surname preserved, opac' ); > }; >+ >+#subtest 'Additional adds or removes' => sub { >+# >-- >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 28935
:
124326
|
124327
|
124350
|
124416
|
124418
|
124422
|
124425
|
124446
|
124447
|
124448
|
124449
|
124450
|
124461
|
124462
|
124499
|
124500
|
124501
|
124502
|
124503
|
124523
|
124524
|
124525
|
124526
|
124527
|
124528
|
124529
|
124530
|
124531
|
124532
|
124640
|
124641
|
124642
|
124643
|
124644
|
124645
|
124646
|
124647
|
124648
|
124649
|
124650
|
124730
|
124731
|
124732
|
124733
|
124734
|
124735
|
124736
|
124737
|
124738
|
124739
|
124740
|
124769
|
124784
|
124832
|
124833
|
124837
|
124838
|
124839
|
124840