Bug 23838 - Add ability to view item renew history
Summary: Add ability to view item renew history
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Tomás Cohen Arazi
URL:
Keywords: rel_20_05_target
Depends on:
Blocks: 17436
  Show dependency treegraph
 
Reported: 2019-10-18 08:49 UTC by Andrew Isherwood
Modified: 2020-01-06 21:46 UTC (History)
6 users (show)

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


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

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
Created attachment 94681 [details] [review]
Bug 23838: Add 'renewals' API route

This patch adds a /patrons/x/renewals API GET route which returns a patron's
renewals, optionally filtered by item_id. Tests are also included

Sponsored-by: Loughborough University

Signed-off-by: Myka Kennedy Stephens <mkstephens@lancasterseminary.edu>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Owen Leonard 2019-10-24 14:49:34 UTC
Created attachment 94682 [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 that have logs associated with
them. 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>
Comment 14 Owen Leonard 2019-10-24 14:49:37 UTC
Created attachment 94683 [details] [review]
Bug 23838: (follow-up) Fix QA tool failures

Remove those pesky tabs as requested by Owen in comment #7

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Owen Leonard 2019-10-24 14:49:40 UTC
Created attachment 94684 [details] [review]
Bug 23838: (follow-up) Fix JS strings

As requested by Owen in comment #7

Owen, I have modified the "x out y renewals have been logged" formation
as you suggested. The "Renewed on x by y" string is harder because it
contains markup and I suspected the strings file should contain such
things. I have however very slightly reworded it which will hopefully
mean it's structure is suitable for other languages too.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Owen Leonard 2019-10-24 14:49:43 UTC
Created attachment 94685 [details] [review]
Bug 23838: (follow-up) Rephrase log message

As suggested by Owen in comment #10

Thanks Owen, I went for the second option

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 17 Bouzid Fergani 2019-11-15 20:49:40 UTC
Created attachment 95466 [details] [review]
Bug 23838: Add 'renewals' API route

This patch adds a /patrons/x/renewals API GET route which returns a patron's
renewals, optionally filtered by item_id. Tests are also included

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>
Comment 18 Bouzid Fergani 2019-11-15 20:49:47 UTC
Created attachment 95467 [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 that have logs associated with
them. 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>
Comment 19 Bouzid Fergani 2019-11-15 20:49:52 UTC
Created attachment 95468 [details] [review]
Bug 23838: (follow-up) Fix QA tool failures

Remove those pesky tabs as requested by Owen in comment #7

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 20 Bouzid Fergani 2019-11-15 20:49:58 UTC
Created attachment 95469 [details] [review]
Bug 23838: (follow-up) Fix JS strings

As requested by Owen in comment #7

Owen, I have modified the "x out y renewals have been logged" formation
as you suggested. The "Renewed on x by y" string is harder because it
contains markup and I suspected the strings file should contain such
things. I have however very slightly reworded it which will hopefully
mean it's structure is suitable for other languages too.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 21 Bouzid Fergani 2019-11-15 20:50:02 UTC
Created attachment 95470 [details] [review]
Bug 23838: (follow-up) Rephrase log message

As suggested by Owen in comment #10

Thanks Owen, I went for the second option

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 22 Bouzid Fergani 2019-11-15 20:57:03 UTC
Created attachment 95471 [details] [review]
Bug 17374: (follow-up) Unit tests and remove warns

Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 23 Bouzid Fergani 2019-11-15 20:57:09 UTC
Created attachment 95472 [details] [review]
Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown

This patch preserves the current dropdown choices for patron search and adds fields from
the DefaultPatronSearchFields system preference

To test:
1 - View the regular patron search and note fields in dropdown
2 - Apply patch, ensure dropdown has not changed
3 - Add fields to DefaultPatronSearchFields, note they are available in
dropdown
4 - Ensure existing and new fields search properly

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
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
Created attachment 96186 [details] [review]
Bug 23838: Add 'renewals' API route

This patch adds a /patrons/x/renewals API GET route which returns a patron's
renewals, optionally filtered by item_id. Tests are also included

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>
Comment 27 Andrew Isherwood 2019-12-11 10:52:34 UTC
Created attachment 96187 [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 that have logs associated with
them. 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>
Comment 28 Andrew Isherwood 2019-12-11 10:52:37 UTC
Created attachment 96188 [details] [review]
Bug 23838: (follow-up) Fix QA tool failures

Remove those pesky tabs as requested by Owen in comment #7

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>
Comment 29 Andrew Isherwood 2019-12-11 10:52:40 UTC
Created attachment 96189 [details] [review]
Bug 23838: (follow-up) Fix JS strings

As requested by Owen in comment #7

Owen, I have modified the "x out y renewals have been logged" formation
as you suggested. The "Renewed on x by y" string is harder because it
contains markup and I suspected the strings file should contain such
things. I have however very slightly reworded it which will hopefully
mean it's structure is suitable for other languages too.

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>
Comment 30 Andrew Isherwood 2019-12-11 10:52:44 UTC
Created attachment 96190 [details] [review]
Bug 23838: (follow-up) Rephrase log message

As suggested by Owen in comment #10

Thanks Owen, I went for the second option

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>
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!