From db14ede44907fb8861ed8fae35d622f0ad65da1c Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 27 Mar 2017 13:07:26 -0300 Subject: [PATCH] Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute Trying to simplify the implementation of 13757, I replaced some legacy code in favour of a Koha::Object-based implementation for the OPAC controller scripts. On doing so I went too far with this methods, as what was needed was a way to retrieve the type of the attribute and ask it about such properties. This patch removes those methods, its tests, and replaces them for new methods in the Koha::Patron::Attributes class: - K:P:A->search_opac_displayable - K:P:A->search_opac_editable To test: - Run $ prove t/db_dependent/Koha/Patron/Attributes.t => SUCCESS: Tests still pass - Try changing the opac_editable and opac_display properties and play in the OPAC with those attributes. Verify that everything works as expected. - Sign off :-D --- Koha/Patron/Attribute.pm | 28 ------ Koha/Patron/Attributes.pm | 38 +++++++++ opac/opac-memberentry.pl | 23 +++-- t/db_dependent/Koha/Patron/Attributes.t | 145 +++++++++++++++----------------- 4 files changed, 120 insertions(+), 114 deletions(-) diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index 906fc6d..4c880cb 100644 --- a/Koha/Patron/Attribute.pm +++ b/Koha/Patron/Attribute.pm @@ -51,34 +51,6 @@ sub store { return $self->SUPER::store(); } -=head3 opac_display - - my $attribute = Koha::Patron::Attribute->new({ code => 'a_code', ... }); - if ( $attribute->opac_display ) { ... } - -=cut - -sub opac_display { - - my $self = shift; - - return Koha::Patron::Attribute::Types->find( $self->code )->opac_display; -} - -=head3 opac_editable - - my $attribute = Koha::Patron::Attribute->new({ code => 'a_code', ... }); - if ( $attribute->is_opac_editable ) { ... } - -=cut - -sub opac_editable { - - my $self = shift; - - return Koha::Patron::Attribute::Types->find( $self->code )->opac_editable; -} - =head3 type my $attribute_type = $attribute->type; diff --git a/Koha/Patron/Attributes.pm b/Koha/Patron/Attributes.pm index 4264e06..8b30965 100644 --- a/Koha/Patron/Attributes.pm +++ b/Koha/Patron/Attributes.pm @@ -31,6 +31,44 @@ Koha::Patron::Attributes - Koha Patron Attributes Object set class =cut +=head3 search_opac_editable + +search_opac_editable will accept the same parameters as the search method, but will only +return attributes that are editable by patrons via the opac. + +=cut + +sub search_opac_editable { + my ( $self, $params, $attributes ) = @_; + + my $join = { join => 'code' }; + $attributes //= {}; + $attributes = { %$attributes, %$join }; + + my $opac_editable = { 'code.opac_editable' => 1 }; + + return $self->SUPER::search( { %$params, %$opac_editable }, $attributes ); +} + +=head3 search_opac_displayable + +search_opac_displayable will accept the same parameters as the search method, but will only +return attributes that are displayable for patrons via the opac. + +=cut + +sub search_opac_displayable { + my ( $self, $params, $attributes ) = @_; + + my $join = { join => 'code' }; + $attributes //= {}; + $attributes = { %$attributes, %$join }; + + my $opac_displayable = { 'code.opac_display' => 1 }; + + return $self->SUPER::search( { %$params, %$opac_displayable }, $attributes ); +} + =head3 _type =cut diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 64f9e03..d66fb7a 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -473,7 +473,8 @@ sub DelEmptyFields { sub FilterUnchangedAttributes { my ( $borrowernumber, $entered_attributes ) = @_; - my @patron_attributes = grep {$_->opac_editable} Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list; + my @patron_attributes = Koha::Patron::Attributes->search_opac_editable( + { borrowernumber => $borrowernumber } ); my $patron_attribute_types; foreach my $attr (@patron_attributes) { @@ -530,14 +531,20 @@ sub GeneratePatronAttributesForm { my ( $borrowernumber, $entered_attributes ) = @_; # Get all attribute types and the values for this patron (if applicable) - my @types = grep { $_->opac_editable() or $_->opac_display } - Koha::Patron::Attribute::Types->search()->as_list(); + my @types = Koha::Patron::Attribute::Types->search( + { -or => [ + opac_editable => 1, + opac_display => 1 + ] + } + ); if ( scalar(@types) == 0 ) { return []; } - my @displayable_attributes = grep { $_->opac_display } - Koha::Patron::Attributes->search({ borrowernumber => $borrowernumber })->as_list; + my @displayable_attributes + = Koha::Patron::Attributes->search_opac_displayable( + { borrowernumber => $borrowernumber } ); my %attr_values = (); @@ -549,14 +556,16 @@ sub GeneratePatronAttributesForm { } } elsif ( defined $borrowernumber ) { - my @editable_attributes = grep { $_->opac_editable } @displayable_attributes; + my @editable_attributes + = Koha::Patron::Attributes->search_opac_editable( + { borrowernumber => $borrowernumber } ); foreach my $attr (@editable_attributes) { push @{ $attr_values{ $attr->code } }, $attr->attribute; } } # Add the non-editable attributes (that don't come from the form) - foreach my $attr ( grep { !$_->opac_editable } @displayable_attributes ) { + foreach my $attr ( grep { !$_->type->opac_editable } @displayable_attributes ) { push @{ $attr_values{ $attr->code } }, $attr->attribute; } diff --git a/t/db_dependent/Koha/Patron/Attributes.t b/t/db_dependent/Koha/Patron/Attributes.t index cbb02b9..d330a64 100644 --- a/t/db_dependent/Koha/Patron/Attributes.t +++ b/t/db_dependent/Koha/Patron/Attributes.t @@ -19,13 +19,12 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 4; use t::lib::TestBuilder; use Test::Exception; use Koha::Database; -use Koha::Patron::Attribute; use Koha::Patron::Attributes; my $schema = Koha::Database->new->schema; @@ -157,120 +156,108 @@ subtest 'store() unique_id attributes tests' => sub { $schema->storage->txn_rollback; }; -subtest 'opac_display() tests' => sub { +subtest 'type() tests' => sub { - plan tests => 2; + plan tests => 4; $schema->storage->txn_begin; my $patron = $builder->build( { source => 'Borrower' } )->{borrowernumber}; - my $attribute_type_1 = $builder->build( - { source => 'BorrowerAttributeType', - value => { opac_display => 1 } - } - ); - - my $attribute_1 = Koha::Patron::Attribute->new( + my $attr_type = $builder->build( { source => 'BorrowerAttributeType' } ); + my $attribute = Koha::Patron::Attribute->new( { borrowernumber => $patron, - code => $attribute_type_1->{code}, + code => $attr_type->{code}, attribute => $patron } ); - is( $attribute_1->opac_display, 1, '->opac_display returns 1' ); - my $attribute_type_2 = $builder->build( - { source => 'BorrowerAttributeType', - value => { opac_display => 0 } - } + my $attribute_type = $attribute->type; + + is( ref($attribute_type), + 'Koha::Patron::Attribute::Type', + '->type returns a Koha::Patron::Attribute::Type object' ); - my $attribute_2 = Koha::Patron::Attribute->new( - { borrowernumber => $patron, - code => $attribute_type_2->{code}, - attribute => $patron - } + is( $attribute_type->code, + $attr_type->{code}, + '->type returns the right Koha::Patron::Attribute::Type object' ); + + is( $attribute_type->opac_editable, + $attr_type->{opac_editable}, + '->type returns the right Koha::Patron::Attribute::Type object' + ); + + is( $attribute_type->opac_display, + $attr_type->{opac_display}, + '->type returns the right Koha::Patron::Attribute::Type object' ); - is( $attribute_2->opac_display, 0, '->opac_display returns 0' ); $schema->storage->txn_rollback; }; -subtest 'opac_editable() tests' => sub { +subtest 'search_opac_editable() and search_opac_displayable() tests' => sub { - plan tests => 2; + plan tests => 3; $schema->storage->txn_begin; - my $patron - = $builder->build( { source => 'Borrower' } )->{borrowernumber}; - my $attribute_type_1 = $builder->build( - { source => 'BorrowerAttributeType', - value => { opac_editable => 1 } + my $editable_attr_type = $builder->build_object( + { class => 'Koha::Patron::Attribute::Types', + value => { opac_editable => 1, opac_display => 1 } } ); - - my $attribute_1 = Koha::Patron::Attribute->new( - { borrowernumber => $patron, - code => $attribute_type_1->{code}, - attribute => $patron + my $displayable_attr_type = $builder->build_object( + { class => 'Koha::Patron::Attribute::Types', + value => { opac_editable => 0, opac_display => 1 } } ); - is( $attribute_1->opac_editable, 1, '->opac_editable returns 1' ); - - my $attribute_type_2 = $builder->build( - { source => 'BorrowerAttributeType', - value => { opac_editable => 0 } + my $staff_only_attr_type = $builder->build_object( + { class => 'Koha::Patron::Attribute::Types', + value => { opac_editable => 0, opac_display => 0 } } ); - - my $attribute_2 = Koha::Patron::Attribute->new( - { borrowernumber => $patron, - code => $attribute_type_2->{code}, - attribute => $patron + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $editable_attr = $builder->build_object( + { class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron->borrowernumber, + code => $editable_attr_type->code + } } ); - is( $attribute_2->opac_editable, 0, '->opac_editable returns 0' ); - - $schema->storage->txn_rollback; -}; - -subtest 'type() tests' => sub { - - plan tests => 4; - - $schema->storage->txn_begin; - - my $patron - = $builder->build( { source => 'Borrower' } )->{borrowernumber}; - my $attr_type = $builder->build( { source => 'BorrowerAttributeType' } ); - my $attribute = Koha::Patron::Attribute->new( - { borrowernumber => $patron, - code => $attr_type->{code}, - attribute => $patron + my $displayable_attr = $builder->build_object( + { class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron->borrowernumber, + code => $displayable_attr_type->code + } + } + ); + my $staff_only_attr = $builder->build_object( + { class => 'Koha::Patron::Attributes', + value => { + borrowernumber => $patron->borrowernumber, + code => $staff_only_attr_type->code + } } ); - my $attribute_type = $attribute->type; + my $search_results = Koha::Patron::Attributes->search( + { borrowernumber => $patron->borrowernumber } ); + is( $search_results->count, 3, '3 attributes present for patron' ); - is( ref($attribute_type), - 'Koha::Patron::Attribute::Type', - '->type returns a Koha::Patron::Attribute::Type object' - ); + $search_results = Koha::Patron::Attributes->search_opac_displayable( + { borrowernumber => $patron->borrowernumber } ); - is( $attribute_type->code, - $attr_type->{code}, - '->type returns the right Koha::Patron::Attribute::Type object' ); + is( $search_results->count, 2, + '2 attributes are opac displayable for patron' ); - is( $attribute_type->opac_editable, - $attr_type->{opac_editable}, - '->type returns the right Koha::Patron::Attribute::Type object' - ); + $search_results = Koha::Patron::Attributes->search_opac_editable( + { borrowernumber => $patron->borrowernumber } ); - is( $attribute_type->opac_display, - $attr_type->{opac_display}, - '->type returns the right Koha::Patron::Attribute::Type object' - ); + is( $search_results->count, 1, + '1 attribute is opac editable for patron' ); $schema->storage->txn_rollback; }; -- 2.7.4