From 5411250b9c34160e262a9dc0a7719ddc383b8ad2 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 16 Sep 2025 17:41:31 +0100 Subject: [PATCH] Bug 40445: Implement two-phase cashup workflow for point of sale This commit introduces a two-phase cashup system that allows staff to start a cashup session, remove cash for counting, and complete the cashup later with reconciliation against the actual counted amount. Key changes: 1. **New cashup workflow methods**: - start_cashup(): Creates CASHUP_START action to begin counting session - cashup_in_progress(): Checks if a cashup session is active - Enhanced add_cashup(): Supports both legacy 'Quick cashup' and new two-phase modes 2. **Improved session boundary calculation**: - _get_session_start_timestamp(): Handles mixed quick/two-phase workflows - outstanding_accountlines(): Uses session boundaries instead of last CASHUP - Session boundaries correctly account for CASHUP_START timestamps 3. **Enhanced reconciliation logic**: - Reconciliation lines are backdated appropriately for each mode - Two-phase mode: Backdate to before CASHUP_START timestamp - Legacy mode: Backdate to before current time 4. **Updated cashup summary calculations**: - _get_session_boundaries(): Properly calculates session start/end - accountlines(): Returns session-specific accountlines - summary(): Uses correct session boundaries for transaction grouping 5. **Register interface updates**: - Dynamic toolbar shows "Start cashup" vs "Complete cashup" - Status indicator when cashup is in progress - Dual-workflow modal with quick and two-phase options 6. **Comprehensive test coverage**: - Two-phase workflow scenarios - Session boundary calculations - Mixed workflow compatibility - Error handling for duplicate operations This implementation maintains full backward compatibility while enabling libraries to separate cash counting from register operation, supporting real-world workflows where counting takes time. --- Koha/Cash/Register.pm | 302 ++++++- Koha/Cash/Register/Cashup.pm | 144 +++- .../prog/en/modules/pos/register.tt | 105 ++- pos/register.pl | 67 +- t/db_dependent/Koha/Cash/Register.t | 767 ++++++++++++++++-- t/db_dependent/Koha/Cash/Register/Cashup.t | 277 ++++++- 6 files changed, 1516 insertions(+), 146 deletions(-) diff --git a/Koha/Cash/Register.pm b/Koha/Cash/Register.pm index b67d91ad6af..293021b0ef9 100644 --- a/Koha/Cash/Register.pm +++ b/Koha/Cash/Register.pm @@ -16,6 +16,8 @@ package Koha::Cash::Register; # along with Koha; if not, see . use Modern::Perl; +use DateTime; +use Scalar::Util qw( looks_like_number ); use Koha::Account; use Koha::Account::Lines; @@ -23,6 +25,7 @@ use Koha::Account::Offsets; use Koha::Cash::Register::Actions; use Koha::Cash::Register::Cashups; use Koha::Database; +use Koha::DateUtils qw( dt_from_string ); use base qw(Koha::Object); @@ -113,35 +116,14 @@ Return a set of accountlines linked to this cash register since the last cashup sub outstanding_accountlines { my ( $self, $conditions, $attrs ) = @_; - my $since = $self->_result->search_related( - 'cash_register_actions', - { 'code' => 'CASHUP' }, - { - order_by => { '-desc' => [ 'timestamp', 'id' ] }, - rows => 1 - } - ); + # Find the start timestamp for the current "open" session + my $start_timestamp = $self->_get_session_start_timestamp; my $local_conditions = - $since->count - ? { 'date' => { '>' => $since->get_column('timestamp')->as_query } } + $start_timestamp + ? { 'date' => { '>' => $start_timestamp } } : {}; - # Exclude reconciliation accountlines from outstanding accountlines - $local_conditions->{'-and'} = [ - { - '-or' => [ - { 'credit_type_code' => { '!=' => 'CASHUP_SURPLUS' } }, - { 'credit_type_code' => undef } - ] - }, - { - '-or' => [ - { 'debit_type_code' => { '!=' => 'CASHUP_DEFICIT' } }, - { 'debit_type_code' => undef } - ] - } - ]; my $merged_conditions = $conditions ? { %{$conditions}, %{$local_conditions} } @@ -155,6 +137,40 @@ sub outstanding_accountlines { return Koha::Account::Lines->_new_from_dbic($rs); } +=head3 cashup_in_progress + +Check if there is currently a cashup in progress (CASHUP_START without corresponding CASHUP). +Returns the CASHUP_START action if in progress, undef otherwise. + +=cut + +sub cashup_in_progress { + my ($self) = @_; + + my $last_start = $self->_result->search_related( + 'cash_register_actions', + { 'code' => 'CASHUP_START' }, + { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 } + )->single; + + return unless $last_start; + + my $last_completion = $self->cashups( + {}, + { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 } + )->single; + + # If we have a start but no completion, or the start is more recent than completion + if ( !$last_completion + || DateTime->compare( dt_from_string( $last_start->timestamp ), dt_from_string( $last_completion->timestamp ) ) + > 0 ) + { + return Koha::Cash::Register::Action->_new_from_dbic($last_start); + } + + return; +} + =head3 store Local store method to prevent direct manipulation of the 'branch_default' field @@ -221,6 +237,72 @@ sub drop_default { return $self; } +=head3 start_cashup + + my $cashup_start = $cash_register->start_cashup( + { + manager_id => $logged_in_user->id, + } + ); + +Start a new cashup period. This marks the beginning of the cash counting process +and creates a snapshot point for calculating outstanding amounts. Returns the +CASHUP_START action. + +=cut + +sub start_cashup { + my ( $self, $params ) = @_; + + # check for mandatory params + my @mandatory = ('manager_id'); + for my $param (@mandatory) { + unless ( defined( $params->{$param} ) ) { + Koha::Exceptions::MissingParameter->throw( error => "The $param parameter is mandatory" ); + } + } + my $manager_id = $params->{manager_id}; + + # Check if there's already a cashup in progress + my $last_cashup_start_rs = $self->_result->search_related( + 'cash_register_actions', + { 'code' => 'CASHUP_START' }, + { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 } + )->single; + + my $last_cashup_completed = $self->cashups( + {}, + { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 } + )->single; + + # If we have a CASHUP_START that's more recent than the last CASHUP, there's already an active cashup + if ( + $last_cashup_start_rs + && ( + !$last_cashup_completed || DateTime->compare( + dt_from_string( $last_cashup_start_rs->timestamp ), + dt_from_string( $last_cashup_completed->timestamp ) + ) > 0 + ) + ) + { + Koha::Exceptions::Object::DuplicateID->throw( error => "A cashup is already in progress for this register" ); + } + + my $expected_amount = abs( $self->outstanding_accountlines->total( { payment_type => [ 'CASH', 'SIP00' ] } ) ); + + # Create the CASHUP_START action + my $rs = $self->_result->add_to_cash_register_actions( + { + code => 'CASHUP_START', + manager_id => $manager_id, + amount => $expected_amount + } + )->discard_changes; + + return Koha::Cash::Register::Cashup->_new_from_dbic($rs); +} + =head3 add_cashup my $cashup = $cash_register->add_cashup( @@ -231,33 +313,73 @@ sub drop_default { } ); -Add a new cashup action to the till, returns the added action. -If amount differs from expected amount, creates surplus/deficit accountlines. +Complete a cashup period started with start_cashup(). This performs the actual +reconciliation against the amount counted and creates surplus/deficit accountlines +if needed. Returns the completed CASHUP action. =cut sub add_cashup { my ( $self, $params ) = @_; - my $manager_id = $params->{manager_id}; - my $amount = $params->{amount}; - my $reconciliation_note = $params->{reconciliation_note}; + # check for mandatory params + my @mandatory = ( 'manager_id', 'amount' ); + for my $param (@mandatory) { + unless ( defined( $params->{$param} ) ) { + Koha::Exceptions::MissingParameter->throw( error => "The $param parameter is mandatory" ); + } + } + my $manager_id = $params->{manager_id}; + + # Validate amount should always be a positive value + my $amount = $params->{amount}; + unless ( looks_like_number($amount) && $amount > 0 ) { + Koha::Exceptions::Account::AmountNotPositive->throw( error => 'Cashup amount passed is not positive number' ); + } # Sanitize reconciliation note - treat empty/whitespace-only as undef + my $reconciliation_note = $params->{reconciliation_note}; if ( defined $reconciliation_note ) { $reconciliation_note = substr( $reconciliation_note, 0, 1000 ); # Limit length $reconciliation_note =~ s/^\s+|\s+$//g; # Trim whitespace $reconciliation_note = undef if $reconciliation_note eq ''; # Empty after trim = undef } - # Calculate expected amount from outstanding accountlines - my $expected_amount = $self->outstanding_accountlines->total; + # Find the most recent CASHUP_START to determine if we're in two-phase mode + my $cashup_start; + my $cashup_start_rs = $self->_result->search_related( + 'cash_register_actions', + { 'code' => 'CASHUP_START' }, + { order_by => { '-desc' => [ 'timestamp', 'id' ] }, rows => 1 } + )->single; + + if ($cashup_start_rs) { - # For backward compatibility, if no actual amount is specified, use expected amount - $amount //= abs($expected_amount); + # Two-phase mode: Check if this CASHUP_START has already been completed + my $existing_completion = $self->_result->search_related( + 'cash_register_actions', + { + 'code' => 'CASHUP', + 'timestamp' => { '>' => $cashup_start_rs->timestamp } + }, + { rows => 1 } + )->single; + + if ( !$existing_completion ) { + $cashup_start = Koha::Cash::Register::Cashup->_new_from_dbic($cashup_start_rs); + } + + } + + # Calculate expected amount from session accountlines + my $expected_amount = ( + $cashup_start + ? $cashup_start->accountlines->total( { payment_type => [ 'CASH', 'SIP00' ] } ) + : $self->outstanding_accountlines->total( { payment_type => [ 'CASH', 'SIP00' ] } ) + ) * -1; # Calculate difference (actual - expected) - my $difference = $amount - abs($expected_amount); + my $difference = $amount - $expected_amount; # Use database transaction to ensure consistency my $schema = $self->_result->result_source->schema; @@ -279,14 +401,27 @@ sub add_cashup { # Create reconciliation accountline if there's a difference if ( $difference != 0 ) { + # Determine reconciliation date based on mode + my $reconciliation_date; + if ($cashup_start) { + + # Two-phase mode: Backdate reconciliation lines to just before the CASHUP_START timestamp + # This ensures they belong to the previous session, not the current one + my $timestamp_str = "DATE_SUB('" . $cashup_start->timestamp . "', INTERVAL 1 SECOND)"; + $reconciliation_date = \$timestamp_str; + } else { + + # Legacy mode: Use the original backdating approach + $reconciliation_date = \'DATE_SUB(NOW(), INTERVAL 1 SECOND)'; + } + if ( $difference > 0 ) { # Surplus: more cash found than expected (credits are negative amounts) my $surplus = Koha::Account::Line->new( { - date => \'DATE_SUB(NOW(), INTERVAL 1 SECOND)', - amount => -abs($difference), # Credits are negative - description => 'Cash register surplus found during cashup', + date => $reconciliation_date, + amount => -abs($difference), # Credits are negative credit_type_code => 'CASHUP_SURPLUS', manager_id => $manager_id, interface => 'intranet', @@ -309,9 +444,8 @@ sub add_cashup { # Deficit: less cash found than expected my $deficit = Koha::Account::Line->new( { - date => \'DATE_SUB(NOW(), INTERVAL 1 SECOND)', + date => $reconciliation_date, amount => abs($difference), - description => 'Cash register deficit found during cashup', debit_type_code => 'CASHUP_DEFICIT', manager_id => $manager_id, interface => 'intranet', @@ -335,6 +469,94 @@ sub add_cashup { return $cashup; } +=head3 _get_session_start_timestamp + +Internal method to determine the start timestamp for the current "open" session. +This handles the following cashup scenarios: + +=over 4 + +=item 1. No cashups ever → undef (returns all accountlines) + +=item 2. Quick cashup completed → Uses CASHUP timestamp + +=item 3. Two-phase started → Uses CASHUP_START timestamp + +=item 4. Two-phase completed → Uses the CASHUP_START timestamp that led to the last CASHUP + +=item 5. Mixed workflows → Correctly distinguishes between quick and two-phase cashups + +=back + +=cut + +sub _get_session_start_timestamp { + my ($self) = @_; + + # Check if there's a cashup in progress (CASHUP_START without corresponding CASHUP) + my $cashup_in_progress = $self->cashup_in_progress; + + if ($cashup_in_progress) { + + # Scenario 3: Two-phase cashup started - return accountlines since CASHUP_START + return $cashup_in_progress->timestamp; + } + + # No cashup in progress - find the most recent cashup completion + my $last_cashup = $self->cashups( + {}, + { + order_by => { '-desc' => [ 'timestamp', 'id' ] }, + rows => 1 + } + )->single; + + if ( !$last_cashup ) { + + # Scenario 1: No cashups have ever taken place - return all accountlines + return; + } + + # Find if this CASHUP was part of a two-phase workflow + my $corresponding_start = $self->_result->search_related( + 'cash_register_actions', + { + 'code' => 'CASHUP_START', + 'timestamp' => { '<' => $last_cashup->timestamp } + }, + { + order_by => { '-desc' => [ 'timestamp', 'id' ] }, + rows => 1 + } + )->single; + + if ($corresponding_start) { + + # Check if this CASHUP_START was completed by this CASHUP + # (no other CASHUP between them) + my $intervening_cashup = $self->_result->search_related( + 'cash_register_actions', + { + 'code' => 'CASHUP', + 'timestamp' => { + '>' => $corresponding_start->timestamp, + '<' => $last_cashup->timestamp + } + }, + { rows => 1 } + )->single; + + if ( !$intervening_cashup ) { + + # Scenario 4: Two-phase cashup completed - return accountlines since the CASHUP_START + return $corresponding_start->timestamp; + } + } + + # Scenarios 2 & 5: Quick cashup (or orphaned CASHUP) - return accountlines since CASHUP + return $last_cashup->timestamp; +} + =head3 to_api_mapping This method returns the mapping for representing a Koha::Cash::Register object diff --git a/Koha/Cash/Register/Cashup.pm b/Koha/Cash/Register/Cashup.pm index 819bcff29f1..8627a64b360 100644 --- a/Koha/Cash/Register/Cashup.pm +++ b/Koha/Cash/Register/Cashup.pm @@ -61,23 +61,29 @@ Return a hashref containing a summary of transactions that make up this cashup. sub summary { my ($self) = @_; my $summary; - my $prior_cashup = Koha::Cash::Register::Cashups->search( - { - 'timestamp' => { '<' => $self->timestamp }, - register_id => $self->register_id - }, - { - order_by => { '-desc' => [ 'timestamp', 'id' ] }, - rows => 1 - } - ); - my $previous = $prior_cashup->single; + # Get the session boundaries for this cashup + my ( $session_start, $session_end ) = $self->_get_session_boundaries; - my $conditions = - $previous - ? { 'date' => { '-between' => [ $previous->_result->get_column('timestamp'), $self->timestamp ] } } - : { 'date' => { '<' => $self->timestamp } }; + my $conditions; + if ( $session_start && $session_end ) { + + # Complete session: between start and end (exclusive) + $conditions = { + 'date' => { + '>' => $session_start, + '<' => $session_end + } + }; + } elsif ($session_end) { + + # Session from beginning to end + $conditions = { 'date' => { '<' => $session_end } }; + } else { + + # Shouldn't happen for a completed cashup, but fallback + $conditions = { 'date' => { '<' => $self->timestamp } }; + } my $payout_transactions = $self->register->accountlines->search( { @@ -191,8 +197,8 @@ sub summary { my $deficit_total = $deficit_lines->count ? $deficit_lines->total : undef; $summary = { - from_date => $previous ? $previous->timestamp : undef, - to_date => $self->timestamp, + from_date => $session_start, + to_date => $session_end, income_grouped => \@income, income_total => abs($income_total), payout_grouped => \@payout, @@ -208,6 +214,110 @@ sub summary { return $summary; } +=head3 accountlines + +Fetch the accountlines associated with this cashup + +=cut + +sub accountlines { + my ($self) = @_; + + # Get the session boundaries for this cashup + my ( $session_start, $session_end ) = $self->_get_session_boundaries; + + my $conditions; + if ( $session_start && $session_end ) { + + # Complete session: between start and end (exclusive) + $conditions = { + 'date' => { + '>' => $session_start, + '<' => $session_end + } + }; + } elsif ($session_end) { + + # Session from beginning to end + $conditions = { 'date' => { '<' => $session_end } }; + } else { + + # Shouldn't happen for a completed cashup, but fallback + $conditions = { 'date' => { '<' => $self->timestamp } }; + } + + return $self->register->accountlines->search($conditions); +} + +=head3 _get_session_boundaries + +Internal method to determine the session boundaries for this cashup. +Returns ($session_start, $session_end) timestamps. + +=cut + +sub _get_session_boundaries { + my ($self) = @_; + + my $session_end = $self->_get_session_end; + + # Find the previous CASHUP + my $session_start; + my $previous_cashup = $self->register->cashups( + { 'timestamp' => { '<' => $session_end } }, + { + order_by => { '-desc' => [ 'timestamp', 'id' ] }, + rows => 1 + } + )->single; + + $session_start = $previous_cashup ? $previous_cashup->_get_session_end : undef; + + return ( $session_start, $session_end ); +} + +sub _get_session_end { + my ($self) = @_; + + my $session_end = $self->timestamp; + + # Find if this CASHUP was part of a two-phase workflow + my $nearest_start = $self->register->_result->search_related( + 'cash_register_actions', + { + 'code' => 'CASHUP_START', + 'timestamp' => { '<' => $session_end } + }, + { + order_by => { '-desc' => [ 'timestamp', 'id' ] }, + rows => 1 + } + )->single; + + if ($nearest_start) { + + # Check if this CASHUP_START was completed by this CASHUP + # (no other CASHUP between them) + my $intervening_cashup = $self->register->cashups( + { + 'timestamp' => { + '>' => $nearest_start->timestamp, + '<' => $session_end + } + }, + { rows => 1 } + )->single; + + if ( !$intervening_cashup ) { + + # Two-phase workflow: session runs to CASHUP_START + $session_end = $nearest_start->timestamp; + } + } + + return $session_end; +} + =head3 to_api_mapping This method returns the mapping for representing a Koha::Cash::Register::Cashup object diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt index 44fc6a6ae8e..2f012023fd1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt @@ -52,13 +52,44 @@
Invalid amount entered for cashup. Please enter a valid monetary amount.
[% END %] + [% IF ( error_cashup_in_progress ) %] +
A cashup is already in progress for this register.
+ [% END %] + + [% IF ( error_no_cashup_start ) %] +
No cashup session has been started. Please start a cashup before attempting to complete it.
+ [% END %] + + [% IF ( error_cashup_already_completed ) %] +
This cashup session has already been completed.
+ [% END %] + + [% IF ( error_cashup_start ) %] +
Failed to start cashup. Please try again.
+ [% END %] + + [% IF ( error_cashup_complete ) %] +
Failed to complete cashup. Please try again.
+ [% END %] + [% IF ( error_refund_permission ) %]
You do not have permission to perform refund actions.
[% END %] + [% IF cashup_in_progress %] +
+ + Cashup in progress - started [% cashup_in_progress.timestamp | $KohaDates with_hours => 1 %]. You can continue to make transactions while counting cash. +
+ [% END %] + [% IF ( CAN_user_cash_management_cashup ) %]
- + [% IF cashup_in_progress %] + + [% ELSE %] + + [% END %]
[% END %] @@ -375,22 +406,36 @@