The 'cashup' process introduced in bug 23355 assumes that once set, the timestamp of an accountline is fixed. This assumption is incorrect as the timestamp is a 'last updated' time and as such is transient. We should be using the 'date' field for comparisons instead as this field is a fixed 'created on' record.
Created attachment 100265 [details] [review] Bug 24820: Use 'date' instead of 'timestamp' for cashup Prior to this patch we were comparing cash register action timestamp to accountlines date fields to fetch the list of outstanding accountlines. accountlines.date is a transient field used as 'updated_on' and so this was a logical mistake. We should be using the 'date' field which is fixed 'created_on' time.
Test plan?
This really just corrects an error in the original bug 23355. Because we were using 'timestamp', which is the 'last updated' date, if a credit action was later modified somehow.. partial application to a second debit perhaps, then the cashup report would display it again. Using the 'created on' date instead, means the data is more consistently displayed in the correct cashup period. I'm not really sure how to prove this in a test plan however... hmm.. perhaps a new unit test.
Patch no longer applies - was going to just check that the cashup process worked. Applying: Bug 24820: Use 'date' instead of 'timestamp' for cashup Using index info to reconstruct a base tree... M Koha/Cash/Register.pm Falling back to patching base and 3-way merge... Auto-merging Koha/Cash/Register.pm CONFLICT (content): Merge conflict in Koha/Cash/Register.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 24820: Use 'date' instead of 'timestamp' for cashup
Created attachment 102080 [details] [review] Bug 24820: Use 'date' instead of 'timestamp' for cashup Prior to this patch we were comparing cash register action timestamp to accountlines date fields to fetch the list of outstanding accountlines. accountlines.date is a transient field used as 'updated_on' and so this was a logical mistake. We should be using the 'date' field which is fixed 'created_on' time.
Rebased
Thanks Martin! I'm happy to sign off, however I'm getting a database error if I do the cashup from the Point of sale > Branch details screen (any of the buttons), but I don't get it when doing a cashup from the Point of sale > Register details screen. I get this before the patch applies, so it is not related to this bug. I'm also getting an error when I try to display older transactions on the Point of sale > Register details screen as well. Should this be logged as a separate bug(s), or is there a way to fix it?
The bug with cashup on the branch details page has now been corrected in master. Well spotted, it was a rebase error.
Created attachment 102465 [details] [review] Bug 24820: Use 'date' instead of 'timestamp' for cashup Prior to this patch we were comparing cash register action timestamp to accountlines date fields to fetch the list of outstanding accountlines. accountlines.date is a transient field used as 'updated_on' and so this was a logical mistake. We should be using the 'date' field which is fixed 'created_on' time. To test, make sure the cash register and record cashup features works as expected. Test plan: 1) Enable EnablePointOfSale and UseCashRegisters system preferences. 2) Add a cash register (Koha Administration > Accounting > Cash registers > New cash register). 3) From the Point of sale screen add two new items for purchase (Point of sale > Administration > Configure items for purchase > add new debit types). 4) Add some transactions. 5) Record a cashup (Register details > Record cashup). 6) Repeat steps 4-5 at least once. 7) Everyting should work as expected. 8) Sign off! Signed-off-by: David Nind <david@davidnind.com>
Thanks Martin for fixing the cashup error! > I'm also getting an error when I try to display older transactions on the > Point of sale > Register details screen as well. There is still an error occurring in the 'Older transactions' section for the point of sale system on the register details screen - this error occurs when you select a date range for transactions: Template process failed: undef error - The given date (04/06/2020) does not match the date format (iso) at /kohadevbox/koha/Koha/DateUtils.pm line 168. at /kohadevbox/koha/C4/Templates.pm line 122 in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 122 119: 120: my $data; 121: binmode( STDOUT, ":utf8" ); 122: $template->process( $self->filename, $vars, \$data ) 123: || die "Template process failed: ", $template->error(); 124: return $data; 125: } Show function arguments in (eval) at /kohadevbox/koha/pos/register.pl line 146 143: } 144: } 145: 146: output_html_with_http_headers( $input, $cookie, $template->output ); in CGI::Compile::ROOT::kohadevbox_koha_pos_register_2epl::__ANON__ at /kohadevbox/koha/pos/register.pl line 2 1: #!/usr/bin/perl .....
(In reply to David Nind from comment #10) > Thanks Martin for fixing the cashup error! > > > I'm also getting an error when I try to display older transactions on the > > Point of sale > Register details screen as well. > > There is still an error occurring in the 'Older transactions' section for > the point of sale system on the register details screen - this error occurs > when you select a date range for transactions: > > Template process failed: undef error - The given date (04/06/2020) does not > match the date format (iso) at /kohadevbox/koha/Koha/DateUtils.pm line 168. > at /kohadevbox/koha/C4/Templates.pm line 122 > in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 122 > 119: > 120: my $data; > 121: binmode( STDOUT, ":utf8" ); > 122: $template->process( $self->filename, $vars, \$data ) > 123: || die "Template process failed: ", $template->error(); > 124: return $data; > 125: } > Show function arguments > > in (eval) at /kohadevbox/koha/pos/register.pl line 146 > 143: } > 144: } > 145: > 146: output_html_with_http_headers( $input, $cookie, $template->output ); > in CGI::Compile::ROOT::kohadevbox_koha_pos_register_2epl::__ANON__ at > /kohadevbox/koha/pos/register.pl line 2 > 1: #!/usr/bin/perl > ..... I believe this is a separate bug - I have the same broken behaviour without the patch. I filed a new bug report: new Bug 25139 - POS explodes in error when trying to display older transactions
Ok, it might be late, but why only change one timestamp to date? - ? { 'timestamp' => { '>' => $since->get_column('timestamp')->as_query } } + ? { 'date' => { '>' => $since->get_column('timestamp')->as_query } }
(In reply to Katrin Fischer from comment #12) > Ok, it might be late, but why only change one timestamp to date? > > - ? { 'timestamp' => { '>' => $since->get_column('timestamp')->as_query > } } > + ? { 'date' => { '>' => $since->get_column('timestamp')->as_query } } Sorry Katrin, not sure I understand your question there? 'date' is the field in the accountlines table and 'timestamp' is the field in the cash_register_actions table. Are you suggesting I update the cash_register_actions table to have a field name of 'date' to mate the accountlines 'date' field name? If we're thinking of doing that I'd actually prefer to change both tables to 'created' or similar to denote exactly their use but I felt that's outside of scope for this bug
Patch now attached to bug 25139 for the aforementioned previous transactions issue.
(In reply to Martin Renvoize from comment #13) > (In reply to Katrin Fischer from comment #12) > > Ok, it might be late, but why only change one timestamp to date? > > > > - ? { 'timestamp' => { '>' => $since->get_column('timestamp')->as_query > > } } > > + ? { 'date' => { '>' => $since->get_column('timestamp')->as_query } } > > Sorry Katrin, not sure I understand your question there? > > 'date' is the field in the accountlines table and 'timestamp' is the field > in the cash_register_actions table. > > Are you suggesting I update the cash_register_actions table to have a field > name of 'date' to mate the accountlines 'date' field name? If we're > thinking of doing that I'd actually prefer to change both tables to > 'created' or similar to denote exactly their use but I felt that's outside > of scope for this bug Just me being confused - it didn't occur to me we were looking at values from 2 different tables here...
Created attachment 102950 [details] [review] Bug 24820: Use 'date' instead of 'timestamp' for cashup Prior to this patch we were comparing cash register action timestamp to accountlines date fields to fetch the list of outstanding accountlines. accountlines.date is a transient field used as 'updated_on' and so this was a logical mistake. We should be using the 'date' field which is fixed 'created_on' time. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
Created attachment 102986 [details] [review] Bug 24820: (RM follow-up) Fix test after switch to 'date' field Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
missing dependencies - not backported to 19.11.x