Bug 23838 - Add ability to view item renew history
Summary: Add ability to view item renew history
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 30275
Blocks: 35310 17436
  Show dependency treegraph
 
Reported: 2019-10-18 08:49 UTC by Andrew Isherwood
Modified: 2023-11-10 11:08 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact: Aude Charillon
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/605
Text to go in the release notes:
This enhancement adds a modal to display checkout renewal history details where appropriate. This includes the circulation history and the items tab of the staff detail page.
Version(s) released in:
22.11.00


Attachments
Bug 23838: Add 'renewals' API route (17.04 KB, patch)
2019-10-18 08:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.94 KB, patch)
2019-10-18 08:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: Add 'renewals' API route (17.16 KB, patch)
2019-10-23 15:35 UTC, Myka Kennedy Stephens
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (8.07 KB, patch)
2019-10-23 15:35 UTC, Myka Kennedy Stephens
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix QA tool failures (2.11 KB, patch)
2019-10-24 13:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix JS strings (2.35 KB, patch)
2019-10-24 13:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: (follow-up) Rephrase log message (1.89 KB, patch)
2019-10-24 14:12 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: Add 'renewals' API route (17.17 KB, patch)
2019-10-24 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (8.07 KB, patch)
2019-10-24 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix QA tool failures (2.16 KB, patch)
2019-10-24 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix JS strings (2.41 KB, patch)
2019-10-24 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: (follow-up) Rephrase log message (1.95 KB, patch)
2019-10-24 14:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: Add 'renewals' API route (17.17 KB, patch)
2019-11-15 20:49 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (8.09 KB, patch)
2019-11-15 20:49 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix QA tool failures (2.21 KB, patch)
2019-11-15 20:49 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix JS strings (2.45 KB, patch)
2019-11-15 20:49 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23838: (follow-up) Rephrase log message (1.99 KB, patch)
2019-11-15 20:50 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 17374: (follow-up) Unit tests and remove warns (5.38 KB, patch)
2019-11-15 20:57 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown (21.58 KB, patch)
2019-11-15 20:57 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23838: Add 'renewals' API route (17.26 KB, patch)
2019-12-11 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (8.17 KB, patch)
2019-12-11 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix QA tool failures (2.27 KB, patch)
2019-12-11 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: (follow-up) Fix JS strings (2.51 KB, patch)
2019-12-11 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: (follow-up) Rephrase log message (2.05 KB, patch)
2019-12-11 10:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.00 KB, patch)
2022-03-18 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.00 KB, patch)
2022-03-18 14:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (1.83 KB, patch)
2022-03-18 14:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.00 KB, patch)
2022-04-26 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (1.83 KB, patch)
2022-04-26 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.00 KB, patch)
2022-04-26 16:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (1.83 KB, patch)
2022-04-26 16:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: (follow-up) Add missing template filter (1.24 KB, patch)
2022-04-26 16:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.16 KB, patch)
2022-05-11 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (1.94 KB, patch)
2022-05-11 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: (follow-up) Add missing template filter (1.30 KB, patch)
2022-05-11 11:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.16 KB, patch)
2022-07-05 13:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (1.94 KB, patch)
2022-07-05 13:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: (follow-up) Add missing template filter (1.30 KB, patch)
2022-07-05 13:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Use $patron_to_html formater (1.93 KB, patch)
2022-07-22 09:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add renewals modal (7.22 KB, patch)
2022-07-22 13:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23838: Use $datetime formater (2.00 KB, patch)
2022-07-22 13:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23838: (follow-up) Add missing template filter (1.36 KB, patch)
2022-07-22 13:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23838: Use $patron_to_html formater (1.99 KB, patch)
2022-07-22 13:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23838: (QA follow-up) Rename patron-renewal => checkout-renewal (6.22 KB, patch)
2022-07-22 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23838: Add 'view' link to item detail page (1.99 KB, patch)
2022-07-22 15:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23838: (QA follow-up) Rename patron-renewal => checkout-renewal (6.28 KB, patch)
2022-07-22 15:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: Add 'view' link to item detail page (2.05 KB, patch)
2022-07-22 15:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23838: (QA follow-up) Move include files (5.92 KB, patch)
2022-07-22 15:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-10-18 08:49:46 UTC
It should be possible, when viewing a patron's circulation history, to view details of an item's renewals. Each renewal should be displayed as:

Renewed on <renewal date> by <renewed by>
Comment 1 Andrew Isherwood 2019-10-18 08:59:35 UTC Comment hidden (obsolete)
Comment 2 Andrew Isherwood 2019-10-18 08:59:39 UTC Comment hidden (obsolete)
Comment 3 Andrew Isherwood 2019-10-18 08:59:51 UTC
Test plan
---------

- Apply the patch
- Navigate to Koha administration and locate the "RenewalLog" syspref
- Ensure the syspref is initially set to "Don't log"
- Navigate to Patrons
- Find a patron with at least one loan, or loan an item to a patron
- Renew an item that is on loan, feel free to do this a few times
- Click "Circulation history"
- TEST: Observe that the item renewal count is displayed but it is not possible to view details of the renewals
- Navigate to Koha administration and locate the "RenewalLog" syspref
- Set the syspref to "Log"
- Return to the patron's loans
- Renew the item a few more times
- Click "Circulation history"
- TEST: Note that the renewal count has increased appropriately, and there is now a "View" link
- Click the "View" link
- TEST: Observe that an "Item renewals" modal appears containing details of the renewals
- TEST: Observe that, since not all renewals were logged, information about what proportion of renewals were logged is displayed
Comment 4 Myka Kennedy Stephens 2019-10-20 00:24:50 UTC
Passed test. Everything worked as expected and described in the test plan.
Comment 5 Myka Kennedy Stephens 2019-10-23 15:35:03 UTC Comment hidden (obsolete)
Comment 6 Myka Kennedy Stephens 2019-10-23 15:35:07 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2019-10-23 16:02:58 UTC
I see a couple of issues with this:

 - QA tool failures: tab characters
 - I think the way the translatable strings is handled isn't correct. Separating each little piece of a sentence isn't going to work for a lot of languages, so placeholders should be used instead.

https://wiki.koha-community.org/wiki/Coding_Guidelines#JS4:_Use_the_format.28.29_method_to_join_variables_with_translatable_strings

So something like this in the strings include:

var msg = _("Note: %s out of %s renewals have been logged");


And in the JS:

 msg.format( data.length, renewals )
Comment 8 Andrew Isherwood 2019-10-24 13:11:03 UTC Comment hidden (obsolete)
Comment 9 Andrew Isherwood 2019-10-24 13:11:06 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2019-10-24 13:44:48 UTC
"by" all by itself is going to be too ambiguous to translate effectively. What about wording it like this:

 - Renewed by Owen Leonard 2019-10-14

Or

 - 2019-10-14 Renewed by Owen Leonard
Comment 11 Andrew Isherwood 2019-10-24 14:12:30 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2019-10-24 14:49:31 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2019-10-24 14:49:34 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2019-10-24 14:49:37 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2019-10-24 14:49:40 UTC Comment hidden (obsolete)
Comment 16 Owen Leonard 2019-10-24 14:49:43 UTC Comment hidden (obsolete)
Comment 17 Bouzid Fergani 2019-11-15 20:49:40 UTC Comment hidden (obsolete)
Comment 18 Bouzid Fergani 2019-11-15 20:49:47 UTC Comment hidden (obsolete)
Comment 19 Bouzid Fergani 2019-11-15 20:49:52 UTC Comment hidden (obsolete)
Comment 20 Bouzid Fergani 2019-11-15 20:49:58 UTC Comment hidden (obsolete)
Comment 21 Bouzid Fergani 2019-11-15 20:50:02 UTC Comment hidden (obsolete)
Comment 22 Bouzid Fergani 2019-11-15 20:57:03 UTC Comment hidden (obsolete)
Comment 23 Bouzid Fergani 2019-11-15 20:57:09 UTC Comment hidden (obsolete)
Comment 24 Andrew Isherwood 2019-11-18 11:09:25 UTC
Hi Bouzid

I think when you attached your sign off patches to this bug, you accidentally attached an unrelated patch to the end. If this is correct, could you please obsolete it?

Many thanks
Andrew
Comment 25 Andrew Isherwood 2019-11-18 11:10:07 UTC
Sorry, correction, you appended two unrelated patches. They're clearly for another bug, so I'm going to obsolete them.
Comment 26 Andrew Isherwood 2019-12-11 10:52:30 UTC Comment hidden (obsolete)
Comment 27 Andrew Isherwood 2019-12-11 10:52:34 UTC Comment hidden (obsolete)
Comment 28 Andrew Isherwood 2019-12-11 10:52:37 UTC Comment hidden (obsolete)
Comment 29 Andrew Isherwood 2019-12-11 10:52:40 UTC Comment hidden (obsolete)
Comment 30 Andrew Isherwood 2019-12-11 10:52:44 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2019-12-16 15:36:45 UTC
Andrew, I think we will need to adapt this one to the new idea of putting the mapping on the Koha::Object level. We can chat about it whenever you want/need to.
Comment 32 Andrew Isherwood 2019-12-17 10:49:03 UTC
Hi Tomás

I'm not sure what you mean when you say "putting the mapping on the Koha::Object level", could you please elaborate? Do you mean add a "to_api" method into the Koha::ActionLog object rather than having it in Koha/REST/V1/Patrons/Renewals.pm

Thanks, Andrew
Comment 33 Jonathan Druart 2019-12-26 13:25:21 UTC
+                $('#patronRenewals #incomplete').append(note + ': ' + data.length + ' ' + out_of + ' ' + renewals + ' ' + renewals_logged).show();

To make this correctly translatable you should use the .format() JS function.
Comment 34 Andrew Isherwood 2020-01-06 12:12:43 UTC
(In reply to Jonathan Druart from comment #33)
> +                $('#patronRenewals #incomplete').append(note + ': ' +
> data.length + ' ' + out_of + ' ' + renewals + ' ' + renewals_logged).show();
> 
> To make this correctly translatable you should use the .format() JS function.

Hi Jonathan

This was addressed in this follow up patch https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96189&action=diff Did you not have all patches applied when you looked at this?
Comment 35 Jonathan Druart 2020-01-06 21:46:15 UTC
(In reply to Andrew Isherwood from comment #34)
Indeed, sorry about that. Looks like I forgot to squash the patches!
Comment 36 Katrin Fischer 2020-04-21 06:55:51 UTC
I started to test these and to code review and there are some points I have been wondering:

1) Patron (membership) renewal vs. checkouts renewal

There is a strong pattern in the patches, file names, the API path of patron-renewals. Tbh, I wonder if this is not misleading. I'd expect something to do with membership renewals for patrons here. I wonder if we should add the checkout aspect here. 


2) REST API

This adds a new route to our REST API:
/patrons/{patron_id}/renewals

But it doesn't appear to have an RFC and has not been voted on yet. If you could write something up, we could maybe fast track at in the meeting this Wednesday:
https://wiki.koha-community.org/wiki/REST_api_RFCs

Because of one, maybe this would be better:

GET /patrons/{patron_id}/checkouts/{checkout_id}/renewals


3) Database structure / tables

So far we have pulled statistics related data form issues, old_issues and statistics, which also logs renewals. I think the action_logs was mostly used for the log viewer (hope I didn't miss something). 

So deleting the action_logs, didn't have much consequence. I am worried, that libraries haven't kept the data in the action_logs so this won't work as expected. Since a long time, we have an option in the cleanup_database.pl script to delete log entries older than x days. 

Is there a strong reason to favor action_logs over statistics here?

In 20.05 we will also have an option for old_issues and statistics. Tbh - I would not recommend a library using either of these as they delete and you lose valueable data about the use of your collection. It will kill your reading histories, I'd always prefer anonymizing, but we don't have that option yet in the cleanup scripts. I think some strong warning notes about consequences should be the least we do.

I know our database situation here is less than ideal. :( If I am correct we are missing the issue_id in both tables?

4) Wrong data showing

The scenario (very likely in a lot of our libraries with course books especially):

1) Patron A checks out the book
2) Renewals take place until no more renewals are allowed
3) Patron A returns the book and checks it out again immediately
4) More renewals take place

In my testing after 2) I had 

3 renewals showing in the table, when I click on Details 2 showed.
This was due to me turning on RenewalLog too late (so ok)

But after 4) I have the same information show on both entries.
3 renewals = 2 renewals logged on 2020-05-14
1 renewal = 2 renwals logged on 2020-05-15

This leads me to believe that this can't deal with multiple checkouts of the same item.


5) QA script

Only 2 very minor problems, probably caused by clean-up since these patches were written:

 FAIL	t/db_dependent/api/v1/patrons_renewals.t
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545). (line 16)

 FAIL	Koha/REST/V1/Patrons/Renewals.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545). (line 16)


Failing for 4) and 5) (would have fixed 5) in QA, but needs more work). 
1) should be resolved as well to our agreed workflow. 
2) and 3) I hope to discuss, maybe get more opinions on.
Comment 37 Katrin Fischer 2020-04-21 07:29:06 UTC
> But after 4) I have the same information show on both entries.
> 3 renewals = 2 renewals logged on 2020-05-14
> 1 renewal = 2 renwals logged on 2020-05-15

Sorry, this doesn't make sense: I typoed. Both show the SAME date and information.
Comment 38 Andrew Isherwood 2020-09-24 11:23:41 UTC
(In reply to Katrin Fischer from comment #36)

1 & 2)

> There is a strong pattern in the patches, file names, the API path of
> patron-renewals. Tbh, I wonder if this is not misleading. I'd expect
> something to do with membership renewals for patrons here. I wonder if we
> should add the checkout aspect here.

I agree it could be misleading. I think your suggestion of an alternative API route makes sense. Looking at the spec I created, I had the patron_id as an optional parameter, though I'm not sure that makes sense, I can't think of any case where you'd want to retrieve *all* a patron's renewals. I'm going to retain the "embed" optional parameter as I do think this is useful. So, to summarise, I'll change the route to:

GET /patrons/{patron_id}/checkouts/{checkout_id}/renewals(?embed=patron,renewed_by)

3) Is there a strong reason to favor action_logs over statistics here?

Statistics doesn't store the identity of who performed the renewal, which was a key requirement for our customer. There is no reliable way of obtaining that information, hence the use of action_logs which, I agree, are not an ideal solution for the reasons you mention. I'm not sure how reasonable it would be to add a, perhaps, "operator" column to statistics, which would enable us to record the renewing staff member. I'm not really familiar with the purpose of the statistics table, but it looks pretty multipurpose, so adding a column of this nature may not be appropriate.

4)

> But after 4) I have the same information show on both entries.
> 3 renewals = 2 renewals logged on 2020-05-14
> 1 renewal = 2 renwals logged on 2020-05-15

> This leads me to believe that this can't deal with multiple checkouts of the same item.

This seems to be a bug. Currently the API call just gets all renewals for a specific patron / item combination, hence the result you're seeing. We could pass the appropriate issue ID to the modal, which would enable it to get just the renewals for that specific issue. However, the issue ID is not recorded in entries made in the action_logs, so we'd have to add that. Or, assuming, we decide to stop using action_logs, it would need to be added whereever we record it.

5) OK, thanks, I'll fix those.
Comment 39 Tomás Cohen Arazi 2020-09-24 13:08:31 UTC
(In reply to Andrew Isherwood from comment #38)
> (In reply to Katrin Fischer from comment #36)
> 
> 1 & 2)
> 
> > There is a strong pattern in the patches, file names, the API path of
> > patron-renewals. Tbh, I wonder if this is not misleading. I'd expect
> > something to do with membership renewals for patrons here. I wonder if we
> > should add the checkout aspect here.
> 
> I agree it could be misleading. I think your suggestion of an alternative
> API route makes sense. Looking at the spec I created, I had the patron_id as
> an optional parameter, though I'm not sure that makes sense, I can't think
> of any case where you'd want to retrieve *all* a patron's renewals. I'm
> going to retain the "embed" optional parameter as I do think this is useful.
> So, to summarise, I'll change the route to:
> 
> GET
> /patrons/{patron_id}/checkouts/{checkout_id}/renewals(?embed=patron,
> renewed_by)

use the x-koha-embed header instead.

> 3) Is there a strong reason to favor action_logs over statistics here?

Let's make renewals be new checkout types hehe.
Comment 40 Katrin Fischer 2020-09-29 18:58:24 UTC
Hm, for the API it might be good to get opinions of people more familiar with it.

I can see the issue about statistics and action_logs not having all the information we need. :(

I would not be opposed to improve the tables in a way that allows us to link it reliably. The issue_id is not that old (given the age of Koha), so maybe that's the main reason we haven't been logging it? It seems the way that things are we will keep "guessing" to some extend as not even the timestamp could be slightly different on action_logs and lastrenewed.
Comment 41 Tomás Cohen Arazi 2020-10-07 12:54:53 UTC
(In reply to Katrin Fischer from comment #40)
> Hm, for the API it might be good to get opinions of people more familiar
> with it.
> 
> I can see the issue about statistics and action_logs not having all the
> information we need. :(
> 
> I would not be opposed to improve the tables in a way that allows us to link
> it reliably. The issue_id is not that old (given the age of Koha), so maybe
> that's the main reason we haven't been logging it? It seems the way that
> things are we will keep "guessing" to some extend as not even the timestamp
> could be slightly different on action_logs and lastrenewed.

We should add something like:

parent_issue_id
is_renewal

to the issues table.

And then make /checkouts/:checkout_id/renewals return, well, the renewals.
I wouldn't oppose to an intermediate solution 'in the meantime' but it is not clear the data is good enough to make the route reliable right now.
Comment 42 Andrew Isherwood 2020-10-08 08:06:36 UTC
(In reply to Tomás Cohen Arazi from comment #41)
> 
> We should add something like:
> 
> parent_issue_id
> is_renewal
> 
> to the issues table.

OK, this makes sense, I'll take this approach

> And then make /checkouts/:checkout_id/renewals return, well, the renewals.

OK

> I wouldn't oppose to an intermediate solution 'in the meantime' but it is
> not clear the data is good enough to make the route reliable right now.

I think it would be better to do this properly now. Clearly historical renewals won't be populated fully, but that's unavoidable and will improve over time.
Comment 43 Andrew Isherwood 2020-10-08 08:17:26 UTC
(In reply to Tomás Cohen Arazi from comment #41)
> We should add something like:
> 
> parent_issue_id
> is_renewal
> 
> to the issues table.

Actually, thinking about this, I'm not sure the is_renewal flag is necessary, surely the fact that it's a renewal can be inferred from a populated parent_issue_id column? I may be missing something in your reasoning though! :)
Comment 44 Katrin Fischer 2020-10-08 10:35:18 UTC
> We should add something like:
> 
> parent_issue_id
> is_renewal
> 
> to the issues table.
> 
> And then make /checkouts/:checkout_id/renewals return, well, the renewals.
> I wouldn't oppose to an intermediate solution 'in the meantime' but it is
> not clear the data is good enough to make the route reliable right now.

Which table to you intend to add this to? 

If you meant issues I am not sure if I like it. It would mean adding a new line for every renewal... once again throwing off all reports people may have written on these tables. I'd like it better if we could 'pimp' statistics to work in the indended way. I could imagine adding a 'issue_id' column there. then we have the entries for issue, return, renew that make it clear what was done.
Comment 45 Andrew Isherwood 2020-10-08 10:58:52 UTC
Hi Katrin

(Looks like I was typing my message below at the same time you were typing yours, I think we came to the same conclusion)

The more I look at this, the more it feels like it's a major undertaking which is way bigger than my original development.

I had not appreciated that a renewal isn't currently separately recorded, instead the original issue is just updated with a renewed date and renewal count. In order to achieve the sort of thing you described above Tomás, I'll need to change to recording a new row in the issues table for each renewal, which I'm sure will have implications in many places.

This seems to me to be a fundamental, and very significant, change in the way renewals are recorded and I must confess I am not at all confident of being able to achieve it without substantial regressions.

A less impactful approach may be to retain the current behaviour of modifying the original issue, but also store details of each renewal in a separate "renewals" table. I appreciate that this is basically the same approach as storing in action_logs, but in a less volatile location. It would also make all the renewal related columns in the issues table effectively redundant, but it would allow a incremental approach with minimal potential regressions. The redundant columns in issues could be migrated as a separate bug.

In fact, it feels like the change detailed above should be carried out as a bug separate to this one first, which this bug is then dependent on.

What I've said above is probably naive and full of holes, but it would be good to come up with an approach that's not going to introduce and tonne of regressions and will allow me to achieve what I'm aiming for.
Comment 46 Tomás Cohen Arazi 2020-10-08 11:05:16 UTC
(In reply to Andrew Isherwood from comment #43)
> (In reply to Tomás Cohen Arazi from comment #41)
> > We should add something like:
> > 
> > parent_issue_id
> > is_renewal
> > 
> > to the issues table.
> 
> Actually, thinking about this, I'm not sure the is_renewal flag is
> necessary, surely the fact that it's a renewal can be inferred from a
> populated parent_issue_id column? I may be missing something in your
> reasoning though! :)

TBH, my original thinking was we needed a 'checkout_type' or similar field, in which we put 'renewal' (there's already an 'onsite' flag that would be moved there). But thought it was simpler to write is_renewal to make a point.
Comment 47 Andrew Isherwood 2020-10-12 09:44:25 UTC
I can't really say what the best way forward for this is, I don't really have the history or context to be able to judge. Obviously this bug isn't the place for far reaching changes with regard to issue and renewal storage, but it may be that this bug becomes dependent on such changes. What are people's thoughts as to the best way forward here?
Comment 48 Katrin Fischer 2020-10-12 09:52:03 UTC
I'd really like to see this one move forward, but I haven't been able to come up with a good suggestion yet.

My impression is that if the same person checks out the same item multiple times, which might happen easily depending on library rules (in some you will have to bring in the item before you can then renew it again), things will get really messed up.

I think we should move the discussion on how to link issues and renewals reliably onto a different bug by now and link this one to it?

So far we've discussed:
1 Adding the issue_id to statistics and/or action logs
2 Adding a new table for renewals
3 Treating renewals as new entries to the issues table

I am more keen on 1 or 2 at the moment as 3 would be a huge change and I think easier and better solutions can be found.
Comment 49 Martin Renvoize 2020-10-19 13:38:30 UTC
Assuming I'm reading this right, I think an minimal version 2 option would be the best route to goal.. a simple 'renewals' table with just 'issue_id' and 'timestamp' required really.. all other details can still come from the issues table directly... or are there other details we would want to record like 'type' to denote autorenewals vs manual ones, and interface to denote a staff client renewal vs an online/opac renewal?... Either way, I think we should record the minimum in the 'renewals' table and rely on the issues table link to fill out the rest of the details we may need.
Comment 50 Katrin Fischer 2020-10-19 13:45:39 UTC
(In reply to Martin Renvoize from comment #49)
> Assuming I'm reading this right, I think an minimal version 2 option would
> be the best route to goal.. a simple 'renewals' table with just 'issue_id'
> and 'timestamp' required really.. all other details can still come from the
> issues table directly... or are there other details we would want to record
> like 'type' to denote autorenewals vs manual ones, and interface to denote a
> staff client renewal vs an online/opac renewal?... Either way, I think we
> should record the minimum in the 'renewals' table and rely on the issues
> table link to fill out the rest of the details we may need.

In statistics or action_logs (sorry, I didn't check that) we also record:
- where the renewal has taken place (branch)
- what type of renewal it was (opac, staff, etc.)
- for this bug: who did the renewal
- in the future: seen or unseen?

A lot of this information we have already, but the reliable link (issue_id) is missing. I am still wondering if not adding the issue_id to the statistics/action_logs tables would be a good way to handle this as otherwise we do end up with a lot of duplicate data that then has to be anonymized etc. again.

Also: do we need an old_renewals then as well? (what happens when the items are returned? we want to keep the history)
Comment 51 Martin Renvoize 2022-03-11 16:15:25 UTC
Right.. I've added bug 30275 to deal with creating the new renewals table.. having discussed it with Tomas deeply we decided this was the best option to keep the API code clean in the long run, and has the benefit that we don't need to worry about anonymisation as we're using links back to already anonymised tables instead of repeating data here.

Also, because of said links, we don't need old_* tables either.

Next step is to complete unit tests on that bug and then adapt this one to use the API's introduced there.
Comment 52 Martin Renvoize 2022-03-18 12:22:21 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2022-03-18 12:24:38 UTC
OK, I squashed the existing patches and reworked them slightly to use the new API's introduced in bug 30275 to try and get this one moving again.

Keeping the signoff as the feature continue to work as it did in my testing, but it worth a refreshed round of QA at this point.
Comment 54 Martin Renvoize 2022-03-18 14:54:46 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2022-03-18 14:54:50 UTC Comment hidden (obsolete)
Comment 56 Andrew Fuerste-Henry 2022-04-19 20:33:00 UTC
I applied this and 30275 and tried to run through the test plan here. I'm not getting a View button next to the renewal count in patron Circulation History even when the Renewal Log is enabled and there are renewals in the action_logs for the checkout. Is there something else I should be doing to get it to show?
Comment 57 Katrin Fischer 2022-04-21 20:08:30 UTC
(In reply to Andrew Fuerste-Henry from comment #56)
> I applied this and 30275 and tried to run through the test plan here. I'm
> not getting a View button next to the renewal count in patron Circulation
> History even when the Renewal Log is enabled and there are renewals in the
> action_logs for the checkout. Is there something else I should be doing to
> get it to show?

Setting to FQA to get attention on this.
Comment 58 Martin Renvoize 2022-04-26 15:11:35 UTC
Created attachment 133945 [details] [review]
Bug 23838: Add renewals modal

This patch adds the display of the renewals modal when appropriate. A
"View" link is displayed next to renewals count where appropriate.
Clicking the link opens the modal that displays the logged renewals.

Signed-off-by: Myka Kennedy Stephens <mkstephens@lancasterseminary.edu>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I rescued this patchset by squashing previous work and updating it to
utilise the new renewals API routes introduced in bug 30275.
Comment 59 Martin Renvoize 2022-04-26 15:11:40 UTC
Created attachment 133946 [details] [review]
Bug 23838: Use $datetime formater
Comment 60 Martin Renvoize 2022-04-26 15:19:58 UTC
(In reply to Andrew Fuerste-Henry from comment #56)
> I applied this and 30275 and tried to run through the test plan here. I'm
> not getting a View button next to the renewal count in patron Circulation
> History even when the Renewal Log is enabled and there are renewals in the
> action_logs for the checkout. Is there something else I should be doing to
> get it to show?

OK, We're back to ready for testing here.

Right, the test plan comment way above is now rather out of date.. we moved the code from action logs into it's own table and a more traditional API route.. soo.

1) Apply bug 30275 and run the database update
2) Renewal some on loan items
3) Check that the Circulation history displays `X [View]` in the 'Number of renewals' column where 'X' is the number of renewals that have taken place and `[View]` is a link that pops up a modal.
4) For renewals that have taken place prior to the patch being applied, you should see the correct number but the 'View' modal will give a message 'Note: 0 out of X renewals have been logged'
5) For renewals that have taken place since applying this patch you should see details in the modal that pops up on clicking 'View'
Comment 61 Owen Leonard 2022-04-26 16:59:24 UTC Comment hidden (obsolete)
Comment 62 Owen Leonard 2022-04-26 16:59:30 UTC Comment hidden (obsolete)
Comment 63 Owen Leonard 2022-04-26 16:59:35 UTC Comment hidden (obsolete)
Comment 64 Martin Renvoize 2022-04-28 10:09:37 UTC
Note to self, must add 'Sponsored-by: Loughborough University' line back into patches
Comment 65 Martin Renvoize 2022-05-11 11:51:53 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2022-05-11 11:51:58 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize 2022-05-11 11:52:03 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize 2022-05-11 11:52:52 UTC
Added sponsorship line to attribute original sponsor of the feature from long long ago.. and rebased ;)
Comment 69 Martin Renvoize 2022-07-05 13:08:50 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize 2022-07-05 13:08:55 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2022-07-05 13:09:00 UTC Comment hidden (obsolete)
Comment 72 Martin Renvoize 2022-07-05 13:12:33 UTC
Rebased now that the dependency bug has been pushed :)
Comment 73 Katrin Fischer 2022-07-22 09:27:39 UTC
Overall, this works really well, but I found a small display glitch:

If the user (in my case it was 42) has no first name, the display is:

22.07.2022 09:25 Renewed by null koha

Possibly the same issue arises if there is no surname.
Comment 74 Martin Renvoize 2022-07-22 09:57:39 UTC
Created attachment 138007 [details] [review]
Bug 23838: Use $patron_to_html formater

This patch updates the renewals modal to correctly format the staff
member who renewed the checkout.
Comment 75 Katrin Fischer 2022-07-22 13:25:15 UTC
Created attachment 138019 [details] [review]
Bug 23838: Add renewals modal

This patch adds the display of the renewals modal when appropriate. A
"View" link is displayed next to renewals count where appropriate.
Clicking the link opens the modal that displays the logged renewals.

Sponsored-by: Loughborough University
Signed-off-by: Myka Kennedy Stephens <mkstephens@lancasterseminary.edu>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I rescued this patchset by squashing previous work and updating it to
utilise the new renewals API routes introduced in bug 30275.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 76 Katrin Fischer 2022-07-22 13:25:21 UTC
Created attachment 138020 [details] [review]
Bug 23838: Use $datetime formater

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 77 Katrin Fischer 2022-07-22 13:25:26 UTC
Created attachment 138021 [details] [review]
Bug 23838: (follow-up) Add missing template filter

Quieting an issue highlighted by the QA tool.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 78 Katrin Fischer 2022-07-22 13:25:31 UTC
Created attachment 138022 [details] [review]
Bug 23838: Use $patron_to_html formater

This patch updates the renewals modal to correctly format the staff
member who renewed the checkout.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 79 Tomás Cohen Arazi 2022-07-22 15:16:58 UTC
Created attachment 138039 [details] [review]
Bug 23838: (QA follow-up) Rename patron-renewal => checkout-renewal

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 80 Tomás Cohen Arazi 2022-07-22 15:17:04 UTC
Created attachment 138040 [details] [review]
Bug 23838: Add 'view' link to item detail page

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 81 Martin Renvoize 2022-07-22 15:27:14 UTC
Created attachment 138041 [details] [review]
Bug 23838: (QA follow-up) Rename patron-renewal => checkout-renewal

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2022-07-22 15:27:20 UTC
Created attachment 138042 [details] [review]
Bug 23838: Add 'view' link to item detail page

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Tomás Cohen Arazi 2022-07-22 15:29:37 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 84 Tomás Cohen Arazi 2022-07-22 15:54:42 UTC
Created attachment 138043 [details] [review]
Bug 23838: (QA follow-up) Move include files

As talked with Martin, this patches were originally developed before we
added the modals/ and str/ dirs, but we need to align it with current
way of doing it. This patch does that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Lucas Gass 2022-09-02 15:08:23 UTC
Enhancement will not be backported to 22.05.x series
Comment 86 Aude Charillon 2023-01-04 15:02:40 UTC
Manual updated.