Bug 24865

The information for deleted items is still in Koha - maybe we could solve this without changing the description. It could also avoid display/linking isses.

The main issue I have with the description field is translatability. In the past we often stored strings there that would always remain in English and could not be changed. Nowadays we usually pull the information about the items using the itemnumbers and other identifiers, which is much nicer. But I am still finding things like bug 25596. I am just a little worried this could bring back the old behavior. 

Maybe we could add additional columns for the information instead?

I can make a solution where the item id that is removed is saved in a new column in accountlines. 

Addition of functionality in the staff fines view to see owning library etc also for removed items.

Created attachment 130749 [details] [review]
Bug 24865: Customize the Accountlines Description

It would be great if we could customize what information was added to the "Description of charges" field when a fine was made so data could be stored even when the item is deleted.

Test Plan:
1) Create an overdue checkout that will get a fine
2) Run fines.pl
3) Note the description for the fine
4) Delete the fine from the database
5) Apply this patch
6) Run updatedatabase.pl
7) Restart all the things!
8) Run fines.pl
9) Note the description of the fine is unchanged
10) Delete the fine again
11) Browse to Slips & Notices
12) Edit the new notice OVERDUE_FINE_DESC
    You will have access to the objects checkout, item, and borrower
13) Run fines.pl
14) Note your new description was used

A very minor quibble, the current behavior is to give the due date including the time. The default notice in this patch calls the due date without the time. It should be changed to maintain current behavior.

Created attachment 131286 [details] [review]
Bug 24865: Customize the Accountlines Description

It would be great if we could customize what information was added to the "Description of charges" field when a fine was made so data could be stored even when the item is deleted.

Test Plan:
1) Create an overdue checkout that will get a fine
2) Run fines.pl
3) Note the description for the fine
4) Delete the fine from the database
5) Apply this patch
6) Run updatedatabase.pl
7) Restart all the things!
8) Run fines.pl
9) Note the description of the fine is unchanged
10) Delete the fine again
11) Browse to Slips & Notices
12) Edit the new notice OVERDUE_FINE_DESC
    You will have access to the objects checkout, item, and borrower
13) Run fines.pl
14) Note your new description was used

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

I have my doubts about whats going on here. Maybe I dont fully understand.

[1] Isnt it a very expensive way to go thru notices to concatenate two fields:
[% item.biblio.title %] [% checkout.date_due | $KohaDates %]

[2] In the description a reference is made to deleted items. If an item is deleted, there is no issue anymore too. I dont see how you create a better description here? Can you explain in which exact situation you reach the else block where you call the letter? (No accountline found, amount>0, item deleted?)

Note: commit message refers to item deletion, but test plan does not include it.

Changing status: need for feedback.

Created attachment 133344 [details] [review]
Bug 24865: Customize the Accountlines Description

It would be great if we could customize what information was added to the "Description of charges" field when a fine was made so data could be stored even when the item is deleted.

Test Plan:
1) Create an overdue checkout that will get a fine
2) Run fines.pl
3) Note the description for the fine
4) Delete the fine from the database
5) Apply this patch
6) Run updatedatabase.pl
7) Restart all the things!
8) Run fines.pl
9) Note the description of the fine is unchanged
10) Delete the fine again
11) Browse to Slips & Notices
12) Edit the new notice OVERDUE_FINE_DESC
    You will have access to the objects checkout, item, and borrower
13) Run fines.pl
14) Note your new description was used

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

1 - The idea though is adding the ability to customize which fields are being concatenated there, to allow librarians to add their own values in an understandable format. Additionally, we are now adding the ability to add translation here by getting patron language. I think it is worth the cost for new functionality

2 - The code addresses the creation of new overdue accountlines - the code is hit whenever a new overdue amount is generated. The note about deleted refers to the fact the info like 'Barcode' is calculated on display from information in the accountlines - when an item is deleted that link is broken and that info is not displayed. If [% item.barcode %] is added to the description it will be retained after deletion

Hmm, this is clever and I generally like it.

It feels like a bit of a hack being in letters/notices though, which is a bit of a shame, but I can appreciate why you added it there.

I wonder if this might helpful elsewhere too and a future work might split it out of notices and into a simplified interface a little more like the News and HTML Content stuff.. i.e. without the superfluous transport types handling.

We almost need a base class for the template handling and then more specific classes for the use cases like letters/notices vs blocks/pages.

(In reply to Martin Renvoize from comment #10)
> Hmm, this is clever and I generally like it.
> 
> It feels like a bit of a hack being in letters/notices though, which is a
> bit of a shame, but I can appreciate why you added it there.
> 
> I wonder if this might helpful elsewhere too and a future work might split
> it out of notices and into a simplified interface a little more like the
> News and HTML Content stuff.. i.e. without the superfluous transport types
> handling.
> 
> We almost need a base class for the template handling and then more specific
> classes for the use cases like letters/notices vs blocks/pages.

FWIW Bug 27334 adds template toolkit support to news. If that makes it in we could move this template from slips to there.

(In reply to Nick Clemens from comment #9)
> 1 - The idea though is adding the ability to customize which fields are
> being concatenated there, to allow librarians to add their own values in an
> understandable format. Additionally, we are now adding the ability to add
> translation here by getting patron language. I think it is worth the cost
> for new functionality
> 
> 2 - The code addresses the creation of new overdue accountlines - the code
> is hit whenever a new overdue amount is generated. The note about deleted
> refers to the fact the info like 'Barcode' is calculated on display from
> information in the accountlines - when an item is deleted that link is
> broken and that info is not displayed. If [% item.barcode %] is added to the
> description it will be retained after deletion

Thx. Will finish it this one now.

(In reply to Martin Renvoize from comment #10)
> It feels like a bit of a hack being in letters/notices though, which is a
> bit of a shame, but I can appreciate why you added it there.

True. We might resolve that with a small follow-up.

Created attachment 133402 [details] [review]
Bug 24865: Customize the Accountlines Description

It would be great if we could customize what information was added to the "Description of charges" field when a fine was made so data could be stored even when the item is deleted.

Test Plan:
1) Create an overdue checkout that will get a fine
2) Run fines.pl
3) Note the description for the fine
4) Delete the fine from the database
5) Apply this patch
6) Run updatedatabase.pl
7) Restart all the things!
8) Run fines.pl
9) Note the description of the fine is unchanged
10) Delete the fine again
11) Browse to Slips & Notices
12) Edit the new notice OVERDUE_FINE_DESC
    You will have access to the objects checkout, item, and borrower
13) Run fines.pl
14) Note your new description was used

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 133403 [details] [review]
Bug 24865: (QA follow-up) Remove hardcoded notice name from protected_letters

If the letter has been removed, fall back to itemnumber/due date. (Title is
no longer fetched.) We may assume that the notice is present.

Note: The option to 'protect' a notice may need some more thought. Perhaps
it needs to be an attribute on itself.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by deleting notice, running fines again.

Good call on QA.. I was about to say, lets be pragmatic and handle follow-ups and follow-ups :)

This new notice is missing from fr-FR/1-Obligatoire/sample_notices.sql

Note there is a work in progress Bug 27619

At this point in the cycle now I think I'd hold off and push this early next cycle..

I'm going to take another look at bug 27334 to see what's holding it up and as we've got collisions with bug 27619 too I think it's sensible to postpone this one a couple of weeks ;)

Pushed to master for 22.11.

Nice work everyone, thanks!

Enhancement will not be backported to 22.05.x series