Bug 24820 - The cashup workflow should use the static 'date' field rather than the transient 'timestamp' field in accountlines
Summary: The cashup workflow should use the static 'date' field rather than the transi...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 23355 24818
Blocks: 13985
  Show dependency treegraph
 
Reported: 2020-03-06 09:19 UTC by Martin Renvoize
Modified: 2020-08-07 08:11 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24820: Use 'date' instead of 'timestamp' for cashup (1.09 KB, patch)
2020-03-06 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24820: Use 'date' instead of 'timestamp' for cashup (1.08 KB, patch)
2020-03-30 11:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24820: Use 'date' instead of 'timestamp' for cashup (1.13 KB, patch)
2020-04-06 17:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 24820: Use 'date' instead of 'timestamp' for cashup (1.19 KB, patch)
2020-04-14 21:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24820: (RM follow-up) Fix test after switch to 'date' field (1.34 KB, patch)
2020-04-15 10:51 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-03-06 09:19:47 UTC
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.
Comment 1 Martin Renvoize 2020-03-06 11:39:59 UTC
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.
Comment 2 David Nind 2020-03-23 18:36:08 UTC
Test plan?
Comment 3 Martin Renvoize 2020-03-30 07:29:51 UTC
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.
Comment 4 David Nind 2020-03-30 08:56:27 UTC
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
Comment 5 Martin Renvoize 2020-03-30 11:56:17 UTC
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.
Comment 6 Martin Renvoize 2020-03-30 11:56:30 UTC
Rebased
Comment 7 David Nind 2020-03-31 11:08:15 UTC
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?
Comment 8 Martin Renvoize 2020-04-06 09:39:24 UTC
The bug with cashup on the branch details page has now been corrected in master. Well spotted, it was a rebase error.
Comment 9 David Nind 2020-04-06 17:18:32 UTC
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>
Comment 10 David Nind 2020-04-06 17:21:28 UTC
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
.....
Comment 11 Katrin Fischer 2020-04-13 21:10:57 UTC
(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
Comment 12 Katrin Fischer 2020-04-13 21:18:17 UTC
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 } }
Comment 13 Martin Renvoize 2020-04-14 08:05:20 UTC
(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
Comment 14 Martin Renvoize 2020-04-14 09:21:12 UTC
Patch now attached to bug 25139 for the aforementioned previous transactions issue.
Comment 15 Katrin Fischer 2020-04-14 20:37:08 UTC
(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...
Comment 16 Katrin Fischer 2020-04-14 21:30:43 UTC
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>
Comment 17 Martin Renvoize 2020-04-15 07:35:53 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Martin Renvoize 2020-04-15 10:51:58 UTC
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>
Comment 19 Martin Renvoize 2020-04-15 10:52:15 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 20 Joy Nelson 2020-05-05 22:29:54 UTC
missing dependencies - not backported to 19.11.x