Bug 16486

I like this change but shouldn't the date formatting be done in the template using '$KohaDates with_hours => 1' ?

(In reply to Owen Leonard from comment #2)
> I like this change but shouldn't the date formatting be done in the template
> using '$KohaDates with_hours => 1' ?

+1

Thank you all for your work on this request.
I can verify that the provided patch works as expected!
[FYI, for Koha 3.20 the patch needs one line less, because 'Hold waiting too long' is not available]

Having said that, unless you print slips with the text from the description, I think it would be more generic to simply change the date column to show hours/minutes in ALL fine lines.
Following Owen's suggestion, it would be something like this:

-  <td><span title="[% account.date %]">[% account.date |$KohaDates %]</span></td>
+  <td><span title="[% account.timestamp %]">[% account.timestamp |$KohaDates with_hours => 1 %]</span></td>

What do you think?

Would not it make sense to add it everywhere?

% git grep ', thanks' **/*.tt|cut -d':' -f1|uniq
koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
koha-tmpl/intranet-tmpl/prog/en/modules/members/printinvoice.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt

It seems that occurrences in pay.tt are not relevant, we do not display payments in this table.

(In reply to Jonathan Druart from comment #7)
> Would not it make sense to add it everywhere?
> 
> % git grep ', thanks' **/*.tt|cut -d':' -f1|uniq
> koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/members/printinvoice.tt
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt
> 
> It seems that occurrences in pay.tt are not relevant, we do not display
> payments in this table.

No response on this question.
Changing status to reflect need for feedback.

I had this error trying to access the account tab in fines:

Global symbol "$input" requires explicit package name (did you forget to declare "my $input"?) at /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl line 41.
syntax error at /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl line 44, near "1cc607141dd67a2373996a1cf5116b1334354420

"
Global symbol "$input" requires explicit package name (did you forget to declare "my $input"?) at /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl line 56.
Execution of /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl aborted due to compilation errors.

(In reply to Dominic Pichette from comment #11)
> I had this error trying to access the account tab in fines:

Hi Dominic,
When you find an issue, you can switch the status to Failed QA.
Thanks for testing!

Found a conflict in the first patch so fixed it.

(In reply to Dominic Pichette from comment #11)
> I had this error trying to access the account tab in fines:
> 
> Global symbol "$input" requires explicit package name (did you forget to
> declare "my $input"?) at
> /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl line 41.
> syntax error at /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl
> line 44, near "1cc607141dd67a2373996a1cf5116b1334354420
> 
> "
> Global symbol "$input" requires explicit package name (did you forget to
> declare "my $input"?) at
> /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl line 56.
> Execution of /inlibro/git/koha-master-dev-inlibro/members/boraccount.pl
> aborted due to compilation errors.

Thank you for testing but I was unable to reproduce these errors, everything still works as expected for me. Can you please test again and make sure your master is up to date?

Hey,
I tried testing, and got this error message upon trying to apply the patch :

fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 16486: [FOLLOW-UP] Putting time in date column

Unsure what it means, whether it's my setup or the patches not working. (my master is up to date)

Are the patches still in the right order ?

(In reply to Marcel de Rooy from comment #17)
> Are the patches still in the right order ?

oops no they are not - i will fix this

(In reply to Aleisha Amohia from comment #20)
> Created attachment 68303 [details] [review] [review]
> Bug 16486: [FOLLOW-UP] Putting time in date column
> 
> In reply to Comment 6, I have moved the time into a column separate from
> the description. Unfortunately the timestamp has the date and the time,
> so I figured it was easier to just have them in the same column and
> replace the original date column.

I think the date column has different info than the timestamp and shouldn't be replaced for all instances - for things like lost fees or account fees date is when it was created - if a partial payment is applied that updates timestamp but not date

We should probably have both columns available, or show both info:

Original date:
2017-07-07
Updated:
2017-10-21 12:12:38

or

2017-10-21 12:12:38 (2017-07-07)

(In reply to Aleisha Amohia from comment #28)
> Created attachment 70583 [details] [review] [review]
> Bug 16486: [FOLLOW-UP] Show original date and timestamp for fines
> 
> This patch has two columns in fines tables, one showing the original
> date and one showing the timestamp (when the patch was last updated)
> 
> Fixed merge conflicts

Hi Aleisha,

I tested on a sandbox and it works very well.
But I was wondering if the column Updated should not appear also in the report Cash register statistics (reports/cash_register_stats.pl)?


Regards,
Marjorie

Patch tested with a sandbox, by Marjorie Barry-Vila <marjorie.barry-vila@collecto.ca>

I am sorry, Aleisha, I know this has been sitting for a bit. 
Can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16486: Display the timestamp for account payments/writeoffs
Applying: Bug 16486: [FOLLOW-UP] Putting time in date column
Applying: Bug 16486: [FOLLOW-UP] Adding timestamp to date in other places
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/printinvoice.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt
M	members/printfeercpt.pl
M	members/printinvoice.pl
Falling back to patching base and 3-way merge...
Auto-merging members/printinvoice.pl
CONFLICT (content): Merge conflict in members/printinvoice.pl
Auto-merging members/printfeercpt.pl
CONFLICT (content): Merge conflict in members/printfeercpt.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-account.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/printinvoice.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/printinvoice.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16486: [FOLLOW-UP] Adding timestamp to date in other places
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16486-FOLLOW-UP-Adding-timestamp-to-date-in-ot-9RSFdA.patch

If the rebase was not very severe, it's ok if you put this back to 'signed off'

Hi Aleisha,

QA script passes with the exception of the commit message descriptions. Could you please fix with the next iteraton? (https://wiki.koha-community.org/wiki/Commit_messages)


1) borraccount.tt
+          <th class="title-string">Original date</th>
+          <th class="title-string">Updated</th>

Maybe make this "Created" or "Creation date" and "Updated". Not sure original date will be understood, thinking of translation too.

Why the change from Writeoff to Written off?

The last table row (total due) is not correctly formatted with the patch (empty last cell).

2) printfeercpt.tt

Table layout is broken by the patch here too. Same for terminology as above.

3) cash_register_stats.tt

Table layout is broken here too (last total row).

Stopping here. Please consider if changing the description would make sense and recheck the other changed templates for layout problems too.

*** Bug 2601 has been marked as a duplicate of this bug. ***

Comment on attachment 103938 [details] [review]
Bug 16486: Display the timestamp when an accountline is updated

Review of attachment 103938 [details] [review]:
-----------------------------------------------------------------

Some first notes here:

1) Database updates

$dbh->do(q{DELETE FROM letter WHERE code = 'ACCOUNT_DEBIT'});
This means we'd be destroying any customized templates the libraries have created themselves and also will force them back to English if they have translated them. I think we should remove these and leave the templates for existing installations as they are. We could add a sample template including the new information to the notices sample page on the wiki maybe

2) Needs a little rebase - es-ES is now using the .yml files to create translations.

3) Cash register changes work well, but the column should also be added to the CSV export to make them match.

Created attachment 117233 [details] [review]
Bug 16486: Display the timestamp when an accountline is updated

This patch updates the Date field to be 'Created' and adds a new column
'Updated' which shows the timestamp of the accountline.

To test:
1) Apply patch, restart services
2) Test the following staff client pages:
- Reports -> Cash register
- Patron Accounting tab -> Transactions
- Create a manual invoice. Click 'Print' on the transactions tab to
generate Invoice slip
- Make a payment. Click 'Print' on the transactions tab to generate
Receipt slip
3) Test the OPAC:
- your charges

Sponsored-by: Catalyst IT

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Created attachment 117348 [details] [review]
Bug 16486: Display the timestamp when an accountline is updated

This patch updates the Date field to be 'Created' and adds a new column
'Updated' which shows the timestamp of the accountline.

To test:
1) Apply patch, restart services
2) Test the following staff client pages:
- Reports -> Cash register
- Patron Accounting tab -> Transactions
- Create a manual invoice. Click 'Print' on the transactions tab to
generate Invoice slip
- Make a payment. Click 'Print' on the transactions tab to generate
Receipt slip
3) Test the OPAC:
- your charges

Sponsored-by: Catalyst IT

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Sorry Aliesha, I didn't spot this one in the tree... else I'd have tried to fold it into the now pushed bug 26734.. it was only upon going to update the wiki pages at Katrins request I saw a link to this bug.

Would you like me to have a go at rebasing for you or are you OK to take a look.. it appears that it stopped applying prior to my patch but I feel a bit guilty having made it even harder.

(In reply to Martin Renvoize from comment #51)
> Sorry Aliesha, I didn't spot this one in the tree... else I'd have tried to
> fold it into the now pushed bug 26734.. it was only upon going to update the
> wiki pages at Katrins request I saw a link to this bug.
> 
> Would you like me to have a go at rebasing for you or are you OK to take a
> look.. it appears that it stopped applying prior to my patch but I feel a
> bit guilty having made it even harder.

No worries. Yes please if you can try rebasing that would be awesome, I'm not sure when I'll have time to revisit this.

Created attachment 120479 [details] [review]
Bug 16486: Display the timestamp when an accountline is updated

This patch updates the Date field to be 'Created' and adds a new column
'Updated' which shows the timestamp of the accountline.

To test:
1) Apply patch, restart services
2) Test the following staff client pages:
- Reports -> Cash register
- Patron Accounting tab -> Transactions
- Create a manual invoice. Click 'Print' on the transactions tab to
generate Invoice slip
- Make a payment. Click 'Print' on the transactions tab to generate
Receipt slip
3) Test the OPAC:
- your charges

Sponsored-by: Catalyst IT

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 120485 [details] [review]
Bug 16486: Display the timestamp when an accountline is updated

This patch updates the Date field to be 'Created' and adds a new column
'Updated' which shows the timestamp of the accountline.

To test:
1) Apply patch, restart services
2) Test the following staff client pages:
- Reports -> Cash register
- Patron Accounting tab -> Transactions
- Create a manual invoice. Click 'Print' on the transactions tab to
generate Invoice slip
- Make a payment. Click 'Print' on the transactions tab to generate
Receipt slip
3) Test the OPAC:
- your charges

Sponsored-by: Catalyst IT
Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

This works as described and causes no regressions I could see.. as such I think it warrants a PQA.

I do however slightly question the logic in some of the notices... I find it a little strange adding the updated time to the ACCOUNT_CREDIT and ACCOUNT_DEBIT slips.. both of those, as they stand, only show the one credit or debit line.. and the created and updated times should match in all cases I believe.  That said.. there's ongoing work to refine those notices and make them more useful by iterating the offsets to get the linked accountlines so I think this isn't a bad first step overall.

Pushed to master for 21.05, thanks to everybody involved!

Enhancement not pushed to 20.11.x