Description
Andrew Isherwood
2019-10-18 08:49:46 UTC
Created attachment 94394 [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 Created attachment 94395 [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. 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 Passed test. Everything worked as expected and described in the test plan. Created attachment 94635 [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> https://bugs.koha-community.org/show_bug.cgi?id=23392 Created attachment 94636 [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> https://bugs.koha-community.org/show_bug.cgi?id=23392 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 ) Created attachment 94663 [details] [review] Bug 23838: (follow-up) Fix QA tool failures Remove those pesky tabs as requested by Owen in comment #7 Created attachment 94664 [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. "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 Created attachment 94676 [details] [review] Bug 23838: (follow-up) Rephrase log message As suggested by Owen in comment #10 Thanks Owen, I went for the second option 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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 Sorry, correction, you appended two unrelated patches. They're clearly for another bug, so I'm going to obsolete them. 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> 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> 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> 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> 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> 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. 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 + $('#patronRenewals #incomplete').append(note + ': ' + data.length + ' ' + out_of + ' ' + renewals + ' ' + renewals_logged).show(); To make this correctly translatable you should use the .format() JS function. (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? (In reply to Andrew Isherwood from comment #34) Indeed, sorry about that. Looks like I forgot to squash the patches! 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.
> 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.
(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. (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. 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. (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. (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. (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! :)
> 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.
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. (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. 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? 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. 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 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) 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. Created attachment 131892 [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. 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. Created attachment 131914 [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. Created attachment 131915 [details] [review] Bug 23838: Use $datetime formater 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? (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. 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. Created attachment 133946 [details] [review] Bug 23838: Use $datetime formater (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' Created attachment 133966 [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. Created attachment 133967 [details] [review] Bug 23838: Use $datetime formater Created attachment 133968 [details] [review] Bug 23838: (follow-up) Add missing template filter Quieting an issue highlighted by the QA tool. Note to self, must add 'Sponsored-by: Loughborough University' line back into patches Created attachment 134890 [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> Created attachment 134891 [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> Created attachment 134892 [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> Added sponsorship line to attribute original sponsor of the feature from long long ago.. and rebased ;) Created attachment 137167 [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> Created attachment 137168 [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> Created attachment 137169 [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> Rebased now that the dependency bug has been pushed :) 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. 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. 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> 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> 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> 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> Created attachment 138039 [details] [review] Bug 23838: (QA follow-up) Rename patron-renewal => checkout-renewal Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 138040 [details] [review] Bug 23838: Add 'view' link to item detail page Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! 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> Enhancement will not be backported to 22.05.x series Manual updated. |