From 175eea0f0f8c9cd7c02f310a2fc81e6bceb9aeb4 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 9 Jun 2016 09:47:53 -0400 Subject: [PATCH] Bug 14576 - (QA followups squashed) Fix message for use of value not in authorized values use mock for tests use a separate variable for yaml fix indentation Remove other instances of ReturnToShelvingCart/InprocessingToShelvingCart Deletes Returns.t (it only tested those two prefs and new tests are in issue.t) Fix undelcared use of $item Move strings from js files Delete duplicated lines Remove C4:::Items::ShelfToCart Fix issue.t Fix Returns.t - Two warnings, not one Fix warnings in tests --- C4/Circulation.pm | 22 +++------ C4/Items.pm | 25 +--------- C4/Reserves.pm | 9 ++-- C4/UsageStats.pm | 2 - installer/data/mysql/sysprefs.sql | 2 - .../prog/en/modules/admin/preferences.tt | 2 + .../en/modules/help/admin/authorised_values.tt | 4 +- .../intranet-tmpl/prog/js/pages/preferences.js | 8 ++-- svc/checkin | 14 ------ t/db_dependent/Circulation/Returns.t | 54 +++------------------- t/db_dependent/Circulation/issue.t | 30 ++++++------ t/db_dependent/UsageStats.t | 2 - 12 files changed, 44 insertions(+), 130 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 9efe42a..67a57df 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1383,19 +1383,11 @@ sub AddIssue { } ); - if ( $item->{'location'} eq 'CART' && $item->{'permanent_location'} ne 'CART' ) { - ## Item was moved to cart via UpdateItemLocationOnCheckin, anything issued should be taken off the cart. - CartToShelf( $item->{'itemnumber'} ); - } - $item->{'issues'}++; - if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) { - UpdateTotalIssues($item->{'biblionumber'}, 1); - } - - if ( C4::Context->preference('ReturnToShelvingCart') ) { - # ReturnToShelvingCart is on, anything issued should be taken off the cart. + if ( $item->{'location'} eq 'CART' && $item->{'permanent_location'} ne 'CART' ) { + ## Item was moved to cart via UpdateItemLocationOnCheckin, anything issued should be taken off the cart. CartToShelf( $item->{'itemnumber'} ); } + $item->{'issues'}++; if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) { UpdateTotalIssues( $item->{'biblionumber'}, 1 ); @@ -1872,11 +1864,11 @@ sub AddReturn { my $borrowernumber = $borrower->{'borrowernumber'} || undef; # we don't know if we had a borrower or not - my $yaml = C4::Context->preference('UpdateItemLocationOnCheckin'); - if ($yaml) { - $yaml = "$yaml\n\n"; # YAML is strict on ending \n. Surplus does not hurt + my $yaml_loc = C4::Context->preference('UpdateItemLocationOnCheckin'); + if ($yaml_loc) { + $yaml_loc = "$yaml_loc\n\n"; # YAML is strict on ending \n. Surplus does not hurt my $rules; - eval { $rules = YAML::Load($yaml); }; + eval { $rules = YAML::Load($yaml_loc); }; if ($@) { warn "Unable to parse UpdateItemLocationOnCheckin syspref : $@"; } diff --git a/C4/Items.pm b/C4/Items.pm index 26c14a1..13fa62a 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -84,7 +84,6 @@ BEGIN { GetLatestAcquisitions CartToShelf - ShelfToCart GetAnalyticsCount @@ -199,27 +198,6 @@ sub CartToShelf { } } -=head2 ShelfToCart - - ShelfToCart($itemnumber); - -Set the current shelving location of the item -to shelving cart ('CART'). - -=cut - -sub ShelfToCart { - my ( $itemnumber ) = @_; - - unless ( $itemnumber ) { - croak "FAILED ShelfToCart() - no itemnumber supplied"; - } - - my $item = GetItem($itemnumber); - $item->{'location'} = 'CART'; - ModItem($item, undef, $itemnumber); -} - =head2 AddItemFromMarc my ($biblionumber, $biblioitemnumber, $itemnumber) @@ -619,9 +597,10 @@ sub ModItemTransfer { my ( $itemnumber, $frombranch, $tobranch ) = @_; my $dbh = C4::Context->dbh; + my $item = GetItem( $itemnumber ); # Remove the 'shelving cart' location status if it is being used. - CartToShelf( $itemnumber ) if ( C4::Context->preference("ReturnToShelvingCart") ); + CartToShelf( $itemnumber ) if ( $item->{'location'} eq 'CART' && $item->{'permanent_location'} ne 'CART' ); #new entry in branchtransfers.... my $sth = $dbh->prepare( diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 6f245c3..9516dc7 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1158,7 +1158,8 @@ sub ModReserveStatus { my $sth_set = $dbh->prepare($query); $sth_set->execute( $newstatus, $itemnumber ); - if ( C4::Context->preference("ReturnToShelvingCart") && $newstatus ) { + my $item = GetItem($itemnumber); + if ( ( $item->{'location'} eq 'CART' && $item->{'permanent_location'} ne 'CART' ) && $newstatus ) { CartToShelf( $itemnumber ); } } @@ -1210,9 +1211,9 @@ sub ModReserveAffect { if ( !$transferToDo && !$already_on_shelf ); _FixPriority( { biblionumber => $biblionumber } ); - - if ( C4::Context->preference("ReturnToShelvingCart") ) { - CartToShelf($itemnumber); + my $item = GetItem($itemnumber); + if ( ( $item->{'location'} eq 'CART' && $item->{'permanent_location'} ne 'CART' ) ) { + CartToShelf( $itemnumber ); } return; diff --git a/C4/UsageStats.pm b/C4/UsageStats.pm index 96e79fa..1a52919 100644 --- a/C4/UsageStats.pm +++ b/C4/UsageStats.pm @@ -143,7 +143,6 @@ sub BuildReport { AutoRemoveOverduesRestrictions CircControl HomeOrHoldingBranch - InProcessingToShelvingCart IssueLostItem IssuingInProcess ManInvInNoissuesCharge @@ -152,7 +151,6 @@ sub BuildReport { RenewalSendNotice RentalsInNoissuesCharge ReturnBeforeExpiry - ReturnToShelvingCart TransfersMaxDaysWarning UseBranchTransferLimits useDaysMode diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index ce84c3c..43f607d 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -204,7 +204,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('IntranetCatalogSearchPulldown','0', NULL, 'Show a search field pulldown for \"Search the catalog\" boxes','YesNo'), ('OnSiteCheckouts','0','','Enable/Disable the on-site checkouts feature','YesNo'), ('OnSiteCheckoutsForce','0','','Enable/Disable the on-site for all cases (Even if a user is debarred, etc.)','YesNo'), -('InProcessingToShelvingCart','0','','If set, when any item with a location code of PROC is \'checked in\', it\'s location code will be changed to CART.','YesNo'), ('INTRAdidyoumean','',NULL,'Did you mean? configuration for the Intranet. Do not change, as this is controlled by /cgi-bin/koha/admin/didyoumean.pl.','Free'), ('IntranetBiblioDefaultView','normal','normal|marc|isbd|labeled_marc','Choose the default detail view in the staff interface; choose between normal, labeled_marc, marc or isbd','Choice'), ('intranetbookbag','1','','If ON, enables display of Cart feature in the intranet','YesNo'), @@ -456,7 +455,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('ReturnBeforeExpiry','0',NULL,'If ON, checkout will be prevented if returndate is after patron card expiry','YesNo'), ('ReturnLog','1',NULL,'If ON, enables the circulation (returns) log','YesNo'), ('ReturnpathDefault','',NULL,'Use this email address as return path or bounce address for undeliverable emails','Free'), -('ReturnToShelvingCart','0','','If set, when any item is \'checked in\', it\'s location code will be changed to CART.','YesNo'), ('reviewson','1','','If ON, enables patron reviews of bibliographic records in the OPAC','YesNo'), ('RisExportAdditionalFields', '', NULL , 'Define additional RIS tags to export from MARC records in YAML format as an associative array with either a marc tag/subfield combination as the value, or a list of tag/subfield combinations.', 'textarea'), ('RoutingListAddReserves','1','','If ON the patrons on routing lists are automatically added to holds on the issue.','YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt index 28ccddc..4d1c052 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt @@ -44,6 +44,8 @@ var MSG_SESSION_TIMED_OUT = _( "You need to log in again, your session has timed out" ); var MSG_DATA_NOT_SAVED = _( "Error; your data might not have been saved" ); var MSG_LOADING = _( "Loading..." ); + var MSG_ALL_VALUE_WARN = _("Note: _ALL_ value will override all other values"); + var MSG_UPD_LOC_FORMAT_WARN = _("The following values are not formatted correctly:"); //]]> diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt index 2d75968..163074f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/authorised_values.tt @@ -16,7 +16,7 @@
  • 'BOR_NOTES' is for values for custom patron notes that appear on the circulation screen and the OPAC.
  • 'Bsort1' is for patron statistical purposes.
  • 'Bsort2' is used for patron statistical purposes.
  • -
  • 'CART' is the shelving cart location, used by InProcessingToShelvingCart and ReturnToShelvingCart
  • +
  • 'CART' is the shelving cart location, used by UpdateItemLocationOnCheckin
  • 'CCODE' is for collection codes (appears when cataloging and working with items).
  • 'DAMAGED' is for descriptions of damaged items, and appears when cataloging and working with items.
  • 'HINGS_AS' is for General Holdings: Acquisition Status Designator, a data element that specifies the acquisition status for the unit at the time of the holdings report.
  • @@ -32,7 +32,7 @@
  • 'MANUAL_INV' includes values for manual invoicing.
  • 'NOT_LOAN' is used to list reasons why a title is not for loan.
  • -
  • 'PROC' is for the location to be used for NewItemsDefaultLocation (change description as desired), also the location expected by InProcessingToShelvingCart.
  • +
  • 'PROC' is for the location to be used for NewItemsDefaultLocation (change description as desired), also a special location for UpdateItemLocationOnCheckin.
  • 'REPORT_GROUP' provides a way to sort and filter your reports, the default values in this category include the Koha modules (Accounts, Acquisitions, Catalog, Circulation, Patrons)
  • 'REPORT_SUBGROUP' can be used to further sort and filter your reports. This category is empty by default. Values here need to include the authorized value code from REPORT_GROUP in the Description (OPAC) field to link the subgroup to the appropriate group.
  • 'RESTRICTED' is used for the restricted status of an item
  • diff --git a/koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js b/koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js index 4db6750..f86abee 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js +++ b/koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js @@ -141,18 +141,18 @@ $( document ).ready( function () { $("#pref_UpdateItemLocationOnCheckin").change(function(){ var the_text = $(this).val(); var alert_text = ''; - if ( the_text.indexOf('_ALL_:') != -1 ) alert_text= _("Note: _ALL_ value will override all other values") + '\n'; + if ( the_text.indexOf('_ALL_:') != -1 ) alert_text = MSG_ALL_VALUE_WARN + '\n'; var split_text =the_text.split("\n"); var alert_issues = ''; var issue_count = 0; var reg_check = /.*:\s.*/; for (var i=0; i < split_text.length; i++){ if ( !split_text[i].match(reg_check) && split_text[i].length ) { - alert_issues+=split_text[i]+"\n"; - issue_count++; + alert_issues+=split_text[i]+"\n"; + issue_count++; } } - if (issue_count) alert_text += "\n"+_("The following values are not formatted correctly:")+"\n"+alert_issues; + if (issue_count) alert_text += "\n"+ MSG_UPD_LOC_FORMAT_WARN +"\n"+alert_issues; if ( alert_text.length ) alert(alert_text); }); diff --git a/svc/checkin b/svc/checkin index 78db9d9..caf6686 100755 --- a/svc/checkin +++ b/svc/checkin @@ -60,20 +60,6 @@ $data->{itemnumber} = $itemnumber; $data->{borrowernumber} = $borrowernumber; $data->{branchcode} = $branchcode; -if ( C4::Context->preference("InProcessingToShelvingCart") ) { - my $item = GetItem($itemnumber); - if ( $item->{'location'} eq 'PROC' ) { - $item->{'location'} = 'CART'; - ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'} ); - } -} - -if ( C4::Context->preference("ReturnToShelvingCart") ) { - my $item = GetItem($itemnumber); - $item->{'location'} = 'CART'; - ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'} ); -} - my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber }); $data->{patronnote} = $checkout ? $checkout->note : q||; diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t index 19e6313..5f650ec 100644 --- a/t/db_dependent/Circulation/Returns.t +++ b/t/db_dependent/Circulation/Returns.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 3; use Test::MockModule; use Test::Warn; @@ -59,47 +59,6 @@ my $rule = Koha::IssuingRule->new( ); $rule->store(); -subtest "InProcessingToShelvingCart tests" => sub { - - plan tests => 2; - - $branch = $builder->build({ source => 'Branch' })->{ branchcode }; - my $permanent_location = 'TEST'; - my $location = 'PROC'; - - # Create a biblio record with biblio-level itemtype - my $record = MARC::Record->new(); - my ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record, '' ); - my $built_item = $builder->build({ - source => 'Item', - value => { - biblionumber => $biblionumber, - homebranch => $branch, - holdingbranch => $branch, - location => $location, - permanent_location => $permanent_location - } - }); - my $barcode = $built_item->{ barcode }; - my $itemnumber = $built_item->{ itemnumber }; - my $item; - - t::lib::Mocks::mock_preference( "InProcessingToShelvingCart", 1 ); - AddReturn( $barcode, $branch ); - $item = GetItem( $itemnumber ); - is( $item->{location}, 'CART', - "InProcessingToShelvingCart functions as intended" ); - - $item->{location} = $location; - ModItem( $item, undef, $itemnumber ); - - t::lib::Mocks::mock_preference( "InProcessingToShelvingCart", 0 ); - AddReturn( $barcode, $branch ); - $item = GetItem( $itemnumber ); - is( $item->{location}, $permanent_location, - "InProcessingToShelvingCart functions as intended" ); -}; - subtest "AddReturn logging on statistics table (item-level_itypes=1)" => sub { @@ -174,12 +133,12 @@ subtest "AddReturn logging on statistics table (item-level_itypes=1)" => sub { is( $stat->itemtype, $ilevel_itemtype, "item-level itype recorded on statistics for return"); - warning_like { AddIssue( $borrower, $item_without_itemtype->{ barcode } ) } - qr/^item-level_itypes set but no itemtype set for item/, + warnings_like { AddIssue( $borrower, $item_without_itemtype->{ barcode } ) } + [qr/^item-level_itypes set but no itemtype set for item/,qr/^item-level_itypes set but no itemtype set for item/,qr/^item-level_itypes set but no itemtype set for item/], 'Item without itemtype set raises warning on AddIssue'; - warning_like { AddReturn( $item_without_itemtype->{ barcode }, $branch ) } - qr/^item-level_itypes set but no itemtype set for item/, - 'Item without itemtype set raises warning on AddReturn'; + warnings_like { AddReturn( $item_without_itemtype->{ barcode }, $branch ) } + [qr/^item-level_itypes set but no itemtype set for item/,qr/^item-level_itypes set but no itemtype set for item/,qr/^item-level_itypes set but no itemtype set for item/], + 'Item without itemtype set raises warnings on AddReturn'; # Test biblio-level itemtype was recorded on the 'statistics' table $stat = $schema->resultset('Statistic')->search({ branch => $branch, @@ -316,5 +275,6 @@ subtest 'Handle ids duplication' => sub { isnt( $original_checkout->issue_id, $new_checkout->{issue_id}, 'AddReturn should return the issue with the new issue_id' ); isnt( $old_checkout->itemnumber, $item->{itemnumber}, 'If an item is checked-in, it should be moved to old_issues even if the issue_id already existed in the table' ); }; +$schema->storage->txn_rollback; 1; diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t index e3d3207..d1158fc 100644 --- a/t/db_dependent/Circulation/issue.t +++ b/t/db_dependent/Circulation/issue.t @@ -17,7 +17,6 @@ use Modern::Perl; -use Test::More tests => 33; use DateTime::Duration; use t::lib::Mocks; @@ -33,7 +32,7 @@ use Koha::Database; use Koha::DateUtils; use Koha::Library; -use Test::More tests => 44; +use Test::More tests => 45; BEGIN { require_ok('C4::Circulation'); @@ -370,29 +369,30 @@ my $itemnumber2; { barcode => 'barcode_4', itemcallnumber => 'callnumber4', - homebranch => $samplebranch1->{branchcode}, - holdingbranch => $samplebranch1->{branchcode}, + homebranch => $branchcode_1, + holdingbranch => $branchcode_1, location => 'FIC', + itype => $itemtype }, $biblionumber ); -C4::Context->set_preference( 'UpdateItemLocationOnCheckin', q{} ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckin', q{} ); +AddReturn( 'barcode_4', $branchcode_1 ); my $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'FIC', 'UpdateItemLocationOnCheckin does not modify value when not enabled' ); -C4::Context->set_preference( 'UpdateItemLocationOnCheckin', 'FIC: GEN' ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckin', 'FIC: GEN' ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'GEN', q{UpdateItemLocationOnCheckin updates location value from 'FIC' to 'GEN' with setting "FIC: GEN"} ); ok( $item2->{permanent_location} eq 'GEN', q{UpdateItemLocationOnCheckin updates permanent_location value from 'FIC' to 'GEN' with setting "FIC: GEN"} ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'GEN', q{UpdateItemLocationOnCheckin does not update location value from 'GEN' with setting "FIC: GEN"} ); -C4::Context->set_preference( 'UpdateItemLocationOnCheckin', '_ALL_: CART' ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckin', '_ALL_: CART' ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'CART', q{UpdateItemLocationOnCheckin updates location value from 'GEN' with setting "_ALL_: CART"} ); ok( $item2->{permanent_location} eq 'GEN', q{UpdateItemLocationOnCheckin does not update permanent_location value from 'GEN' with setting "_ALL_: CART"} ); @@ -400,15 +400,15 @@ AddIssue( $borrower_1, 'barcode_4', $daysago10,0, $today, '' ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'GEN', q{Location updates from 'CART' to permanent location on issue} ); -C4::Context->set_preference( 'UpdateItemLocationOnCheckin', "GEN: _BLANK_\n_BLANK_: PROC\nPROC: _PERM_" ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +t::lib::Mocks::mock_preference( 'UpdateItemLocationOnCheckin', "GEN: _BLANK_\n_BLANK_: PROC\nPROC: _PERM_" ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq '', q{UpdateItemLocationOnCheckin updates location value from 'GEN' to '' with setting "GEN: _BLANK_"} ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq 'PROC' , q{UpdateItemLocationOnCheckin updates location value from '' to 'PROC' with setting "_BLANK_: PROC"} ); ok( $item2->{permanent_location} eq '' , q{UpdateItemLocationOnCheckin does not update permanent_location value from '' to 'PROC' with setting "_BLANK_: PROC"} ); -AddReturn( 'barcode_4', $samplebranch1->{branchcode} ); +AddReturn( 'barcode_4', $branchcode_1 ); $item2 = GetItem( $itemnumber2 ); ok( $item2->{location} eq '' , q{UpdateItemLocationOnCheckin updates location value from 'PROC' to '' with setting "PROC: _PERM_" } ); ok( $item2->{permanent_location} eq '' , q{UpdateItemLocationOnCheckin does not update permanent_location from '' with setting "PROC: _PERM_" } ); diff --git a/t/db_dependent/UsageStats.t b/t/db_dependent/UsageStats.t index 5f4dfad..3282c29 100644 --- a/t/db_dependent/UsageStats.t +++ b/t/db_dependent/UsageStats.t @@ -396,7 +396,6 @@ sub mocking_systempreferences_to_a_set_value { CircControl HomeOrHoldingBranch HomeOrHoldingBranchReturn - InProcessingToShelvingCart IssueLostItem IssuingInProcess ManInvInNoissuesCharge @@ -405,7 +404,6 @@ sub mocking_systempreferences_to_a_set_value { RenewalSendNotice RentalsInNoissuesCharge ReturnBeforeExpiry - ReturnToShelvingCart TransfersMaxDaysWarning UseBranchTransferLimits useDaysMode -- 2.1.4