Bugzilla – Attachment 142485 Details for
Bug 23012
Possibility to mark processing fee by default refund when item is found
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23012: (QA follow-up) Combine method to get both values
Bug-23012-QA-follow-up-Combine-method-to-get-both-.patch (text/plain), 18.95 KB, created by
Nick Clemens (kidclamp)
on 2022-10-24 11:23:47 UTC
(
hide
)
Description:
Bug 23012: (QA follow-up) Combine method to get both values
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2022-10-24 11:23:47 UTC
Size:
18.95 KB
patch
obsolete
>From 3ba461253e861925265c5c5cf6f723ade486cce1 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Mon, 24 Oct 2022 11:13:32 +0000 >Subject: [PATCH] Bug 23012: (QA follow-up) Combine method to get both values > >The code for get_processingreturn_policy was very similar to >get_lostreturn_policy. Combining the two methods allows for use of >get_effective_rules which uses get_effective_rule_value which is cached. >This should reduce lines of code and improve performance > >Tests updated and adjusted as well > >Signed-off-by: Nick Clemens <nick@bywatersolutions.com> >--- > Koha/CirculationRules.pm | 47 ++---- > Koha/Item.pm | 12 +- > t/db_dependent/Koha/CirculationRules.t | 221 +++++++++---------------- > 3 files changed, 95 insertions(+), 185 deletions(-) > >diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm >index 64999533b5..f96df496fb 100644 >--- a/Koha/CirculationRules.pm >+++ b/Koha/CirculationRules.pm >@@ -577,9 +577,9 @@ sub get_onshelfholds_policy { > > =head3 get_lostreturn_policy > >- my $lostrefund_policy = Koha::CirculationRules->get_lostreturn_policy( { return_branch => $return_branch, item => $item } ); >+ my $lost_proc_refund_policy = Koha::CirculationRules->get_lostreturn_policy( { return_branch => $return_branch, item => $item } ); > >-Return values are: >+lostreturn return values are: > > =over 2 > >@@ -593,37 +593,7 @@ Return values are: > > =back > >-=cut >- >-sub get_lostreturn_policy { >- my ( $class, $params ) = @_; >- >- my $item = $params->{item}; >- >- my $behaviour = C4::Context->preference( 'RefundLostOnReturnControl' ) // 'CheckinLibrary'; >- my $behaviour_mapping = { >- CheckinLibrary => $params->{'return_branch'} // $item->homebranch, >- ItemHomeBranch => $item->homebranch, >- ItemHoldingBranch => $item->holdingbranch >- }; >- >- my $branch = $behaviour_mapping->{ $behaviour }; >- >- my $rule = Koha::CirculationRules->get_effective_rule( >- { >- branchcode => $branch, >- rule_name => 'lostreturn', >- } >- ); >- >- return $rule ? $rule->rule_value : 'refund'; >-} >- >-=head3 get_processingreturn_policy >- >- my $processingrefund_policy = Koha::CirculationRules->get_processingreturn_policy( { return_branch => $return_branch, item => $item } ); >- >-Return values are: >+processing return return values are: > > =over 2 > >@@ -637,9 +607,10 @@ Return values are: > > =back > >+ > =cut > >-sub get_processingreturn_policy { >+sub get_lostreturn_policy { > my ( $class, $params ) = @_; > > my $item = $params->{item}; >@@ -653,14 +624,16 @@ sub get_processingreturn_policy { > > my $branch = $behaviour_mapping->{ $behaviour }; > >- my $rule = Koha::CirculationRules->get_effective_rule( >+ my $rules = Koha::CirculationRules->get_effective_rules( > { > branchcode => $branch, >- rule_name => 'processingreturn', >+ rules => ['lostreturn','processingreturn'] > } > ); > >- return $rule ? $rule->rule_value : 'refund'; >+ $rules->{lostreturn} //= 'refund'; >+ $rules->{processingreturn} //= 'refund'; >+ return $rules; > } > > =head3 article_requestable_rules >diff --git a/Koha/Item.pm b/Koha/Item.pm >index 37d2ee343c..25b4e54dc3 100644 >--- a/Koha/Item.pm >+++ b/Koha/Item.pm >@@ -1159,7 +1159,7 @@ sub _set_found_trigger { > return $self unless $lost_age_in_days < $no_refund_after_days; > } > >- my $lostreturn_policy = Koha::CirculationRules->get_lostreturn_policy( >+ my $lost_proc_return_policy = Koha::CirculationRules->get_lostreturn_policy( > { > item => $self, > return_branch => C4::Context->userenv >@@ -1167,6 +1167,7 @@ sub _set_found_trigger { > : undef, > } > ); >+ my $lostreturn_policy = $lost_proc_return_policy->{lostreturn}; > > if ( $lostreturn_policy ) { > >@@ -1324,14 +1325,7 @@ sub _set_found_trigger { > } > } > >- my $processingreturn_policy = Koha::CirculationRules->get_processingreturn_policy( >- { >- item => $self, >- return_branch => C4::Context->userenv >- ? C4::Context->userenv->{'branch'} >- : undef, >- } >- ); >+ my $processingreturn_policy = $lost_proc_return_policy->{lostreturn}; > > if ( $processingreturn_policy ) { > >diff --git a/t/db_dependent/Koha/CirculationRules.t b/t/db_dependent/Koha/CirculationRules.t >index 8664505524..c4009c6a96 100755 >--- a/t/db_dependent/Koha/CirculationRules.t >+++ b/t/db_dependent/Koha/CirculationRules.t >@@ -20,7 +20,7 @@ > use Modern::Perl; > > use Benchmark; >-use Test::More tests => 8; >+use Test::More tests => 7; > use Test::Deep qw( cmp_methods ); > use Test::Exception; > >@@ -29,6 +29,7 @@ use Koha::Database; > > use t::lib::Mocks; > use t::lib::TestBuilder; >+use Koha::Cache::Memory::Lite; > > my $schema = Koha::Database->new->schema; > my $builder = t::lib::TestBuilder->new; >@@ -687,65 +688,61 @@ subtest 'get_lostreturn_policy() tests' => sub { > > $schema->resultset('CirculationRule')->search()->delete; > >- my $default_rule_charge = $builder->build( >+ my $default_proc_rule_charge = $builder->build( > { > source => 'CirculationRule', > value => { > branchcode => undef, > categorycode => undef, > itemtype => undef, >- rule_name => 'lostreturn', >+ rule_name => 'processingreturn', > rule_value => 'charge' > } > } > ); >- my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_false = $builder->build( >+ my $default_lost_rule_charge = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode, >+ branchcode => undef, > categorycode => undef, > itemtype => undef, > rule_name => 'lostreturn', >- rule_value => 0 >+ rule_value => 'charge' > } > } > ); >- my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_refund = $builder->build( >+ my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; >+ my $specific_lost_rule_false = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode2, >+ branchcode => $branchcode, > categorycode => undef, > itemtype => undef, > rule_name => 'lostreturn', >- rule_value => 'refund' >+ rule_value => 0 > } > } > ); >- my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_restore = $builder->build( >+ my $specific_proc_rule_false = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode3, >+ branchcode => $branchcode, > categorycode => undef, > itemtype => undef, >- rule_name => 'lostreturn', >- rule_value => 'restore' >+ rule_name => 'processingreturn', >+ rule_value => 0 > } > } > ); >- >- # Make sure we have an unused branchcode >- my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_dummy = $builder->build( >+ my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; >+ my $specific_lost_rule_refund = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode4, >+ branchcode => $branchcode2, > categorycode => undef, > itemtype => undef, > rule_name => 'lostreturn', >@@ -753,140 +750,59 @@ subtest 'get_lostreturn_policy() tests' => sub { > } > } > ); >- my $branch_without_rule = $specific_rule_dummy->{ branchcode }; >- Koha::CirculationRules >- ->search( >- { >- branchcode => $branch_without_rule, >- categorycode => undef, >- itemtype => undef, >- rule_name => 'lostreturn', >- rule_value => 'refund' >- } >- ) >- ->next >- ->delete; >- >- my $item = $builder->build_sample_item( >- { >- homebranch => $specific_rule_restore->{branchcode}, >- holdingbranch => $specific_rule_false->{branchcode} >- } >- ); >- my $params = { >- return_branch => $specific_rule_refund->{ branchcode }, >- item => $item >- }; >- >- # Specific rules >- t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 'refund','Specific rule for checkin branch is applied (refund)'); >- >- t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 'restore','Specific rule for home branch is applied (restore)'); >- >- t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 0,'Specific rule for holding branch is applied (false)'); >- >- # Default rule check >- t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); >- $params->{return_branch} = $branch_without_rule; >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 'charge','No rule for branch, global rule applied (charge)'); >- >- # Change the default value just to try >- Koha::CirculationRules->search({ branchcode => undef, rule_name => 'lostreturn' })->next->rule_value(0)->store; >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 0,'No rule for branch, global rule applied (false)'); >- >- # No default rule defined check >- Koha::CirculationRules >- ->search( >- { >- branchcode => undef, >- categorycode => undef, >- itemtype => undef, >- rule_name => 'lostreturn' >- } >- ) >- ->next >- ->delete; >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 'refund','No rule for branch, no default rule, fallback default (refund)'); >- >- # Fallback to ItemHoldBranch if CheckinLibrary is undefined >- $params->{return_branch} = undef; >- is( Koha::CirculationRules->get_lostreturn_policy( $params ), >- 'restore','return_branch undefined, fallback to ItemHomeBranch rule (restore)'); >- >- $schema->storage->txn_rollback; >-}; >- >-subtest 'get_processingreturn_policy() tests' => sub { >- plan tests => 7; >- >- $schema->storage->txn_begin; >- >- $schema->resultset('CirculationRule')->search()->delete; >- >- my $default_rule_charge = $builder->build( >+ my $specific_proc_rule_refund = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => undef, >+ branchcode => $branchcode2, > categorycode => undef, > itemtype => undef, > rule_name => 'processingreturn', >- rule_value => 'charge' >+ rule_value => 'refund' > } > } > ); >- my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_false = $builder->build( >+ my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; >+ my $specific_lost_rule_restore = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode, >+ branchcode => $branchcode3, > categorycode => undef, > itemtype => undef, >- rule_name => 'processingreturn', >- rule_value => 0 >+ rule_name => 'lostreturn', >+ rule_value => 'restore' > } > } > ); >- my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_refund = $builder->build( >+ my $specific_proc_rule_restore = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode2, >+ branchcode => $branchcode3, > categorycode => undef, > itemtype => undef, > rule_name => 'processingreturn', >- rule_value => 'refund' >+ rule_value => 'restore' > } > } > ); >- my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_restore = $builder->build( >+ >+ # Make sure we have an unused branchcode >+ my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; >+ my $specific_lost_rule_dummy = $builder->build( > { > source => 'CirculationRule', > value => { >- branchcode => $branchcode3, >+ branchcode => $branchcode4, > categorycode => undef, > itemtype => undef, >- rule_name => 'processingreturn', >- rule_value => 'restore' >+ rule_name => 'lostreturn', >+ rule_value => 'refund' > } > } > ); >- >- # Make sure we have an unused branchcode >- my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; >- my $specific_rule_dummy = $builder->build( >+ my $specific_proc_rule_dummy = $builder->build( > { > source => 'CirculationRule', > value => { >@@ -898,7 +814,19 @@ subtest 'get_processingreturn_policy() tests' => sub { > } > } > ); >- my $branch_without_rule = $specific_rule_dummy->{ branchcode }; >+ my $branch_without_rule = $specific_lost_rule_dummy->{ branchcode }; >+ Koha::CirculationRules >+ ->search( >+ { >+ branchcode => $branch_without_rule, >+ categorycode => undef, >+ itemtype => undef, >+ rule_name => 'lostreturn', >+ rule_value => 'refund' >+ } >+ ) >+ ->next >+ ->delete; > Koha::CirculationRules > ->search( > { >@@ -914,40 +842,55 @@ subtest 'get_processingreturn_policy() tests' => sub { > > my $item = $builder->build_sample_item( > { >- homebranch => $specific_rule_restore->{branchcode}, >- holdingbranch => $specific_rule_false->{branchcode} >+ homebranch => $specific_lost_rule_restore->{branchcode}, >+ holdingbranch => $specific_lost_rule_false->{branchcode} > } > ); > my $params = { >- return_branch => $specific_rule_refund->{ branchcode }, >+ return_branch => $specific_lost_rule_refund->{ branchcode }, > item => $item > }; > > # Specific rules > t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 'refund','Specific rule for checkin branch is applied (refund)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 'refund', processingreturn => 'refund' },'Specific rule for checkin branch is applied (refund)'); > > t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 'restore','Specific rule for home branch is applied (restore)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 'restore', processingreturn => 'restore' },'Specific rule for home branch is applied (restore)'); > > t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 0,'Specific rule for holding branch is applied (false)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 0, processingreturn => 0 },'Specific rule for holding branch is applied (false)'); > > # Default rule check > t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); > $params->{return_branch} = $branch_without_rule; >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 'charge','No rule for branch, global rule applied (charge)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 'charge', processingreturn => 'charge' },'No rule for branch, global rule applied (charge)'); > > # Change the default value just to try >+ Koha::CirculationRules->search({ branchcode => undef, rule_name => 'lostreturn' })->next->rule_value(0)->store; > Koha::CirculationRules->search({ branchcode => undef, rule_name => 'processingreturn' })->next->rule_value(0)->store; >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 0,'No rule for branch, global rule applied (false)'); >+ my $memory_cache = Koha::Cache::Memory::Lite->get_instance; >+ $memory_cache->flush(); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 0, processingreturn => 0 },'No rule for branch, global rule applied (false)'); > > # No default rule defined check >+ Koha::CirculationRules >+ ->search( >+ { >+ branchcode => undef, >+ categorycode => undef, >+ itemtype => undef, >+ rule_name => 'lostreturn' >+ } >+ ) >+ ->next >+ ->delete; >+ # No default rule defined check > Koha::CirculationRules > ->search( > { >@@ -959,13 +902,13 @@ subtest 'get_processingreturn_policy() tests' => sub { > ) > ->next > ->delete; >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 'refund','No rule for branch, no default rule, fallback default (refund)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 'refund', processingreturn => 'refund' },'No rule for branch, no default rule, fallback default (refund)'); > > # Fallback to ItemHoldBranch if CheckinLibrary is undefined > $params->{return_branch} = undef; >- is( Koha::CirculationRules->get_processingreturn_policy( $params ), >- 'restore','return_branch undefined, fallback to ItemHomeBranch rule (restore)'); >+ is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), >+ { lostreturn => 'restore', processingreturn => 'restore' },'return_branch undefined, fallback to ItemHomeBranch rule (restore)'); > > $schema->storage->txn_rollback; > }; >-- >2.30.2
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 23012
:
141627
|
141628
|
141629
|
141630
|
142046
|
142047
|
142048
|
142049
|
142113
|
142123
|
142124
|
142125
|
142126
|
142481
|
142482
|
142483
|
142484
| 142485 |
142486
|
142487
|
142575
|
142652
|
142693
|
142736
|
142737