https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #2 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 158800 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158800&action=edit Bug 35310: Only show 'Current renewals' link if item is currently checked out 1) Checkout an item to Koha user, barcode 39999000003192 2) Renew that barcode 3) Visit the items page of the biblio record, click on "View" in front of "Current renewals" /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=76 4) Notice it shows the renewals as expected 5) Return the same item 39999000003192 6) Repeat 3). Notice it never loads renewals and network console shows HTTP error 400 7) Apply this patch. Repeat the test plan. Notice the 'Current renewals' no longer shows for the item because it is no longer checked out, so whatever past renewals are no longer 'current' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #3 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 158801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158801&action=edit Bug 35310: Show current renewal value of the currently active check out Test plan, with the previous 2 patches applied: 1) Checkout an item to Koha user, barcode 39999000003192 2) Renew that barcode 3) Visit the items page of the biblio record, click on 'View' in front of 'Current renewals' /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=76 4) Notice it shows the renewals as expected 5) Return the same item: 39999000003192 6) Repeat 1) to 3). Notice it shows 2 current renewals even tho only 1 corresponds to the currently active check out 7) Renew again. Notice it shows 3 current renewals even tho only 2 correspond to the currently active check out 7) Apply this patch. Notice it now only shows 2 current renewals -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #158800|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 158823 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158823&action=edit Bug 35310: Only show 'Current renewals' link if item is currently checked out 1) Checkout an item to Koha user, barcode 39999000003192 2) Renew that barcode 3) Visit the items page of the biblio record, click on "View" in front of "Current renewals" /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=76 4) Notice it shows the renewals as expected 5) Return the same item 39999000003192 6) Repeat 3). Notice it never loads renewals and network console shows HTTP error 400 7) Apply this patch. Repeat the test plan. Notice the 'Current renewals' no longer shows for the item because it is no longer checked out, so whatever past renewals are no longer 'current' Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Text to go in the| |This fixes the current release notes| |renewals information (shown | |under the statuses section) | |on the item page for | |records in the staff | |interface so that: | |1. The | |current renewals row is | |only now shown if there are | |current renewals for the | |item (previously it was | |shown for all items, even | |if they had no renewals). | |2. It only shows the number | |of current renewals for the | |current check out | |(previously the number | |shown would include all | |renewals, including for | |previous check-outs). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #8 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- (In reply to Victor Grousset/tuxayo from comment #7)

...

6) Repeat 1) to 3). Notice it shows 2 current renewals even tho only 1 corresponds to the currently active check out

When clicking on "View" I see «Note: 1 out of 2 renewals have been logged». What is the function of this? Is that code in checkout_renewals_modal.js still relevant? I'm not sure after the patch we can still get that message.

Great observation. I'm not sure. Original work written 4 years ago (not the pushed patch) where the string was introduced is: https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=23838&attachment=137167 Note that when the work was originally done, readingrec.tt was tracking issues (which I believe is all the checkouts of an item), but after this: https://github.com/PTFS-Europe/koha/commit/7fd26e0#diff-ff6b8814c72c6c6b8a9e... It's now tracking checkouts instead . I know this is a different file from the one in my patch but I think it helps explain the history behind this as the renewal modal functionality is similar in both moredetail.tt and readingrec.tt I think the original string "Note: X out of Y renewals have been logged" was a way to let the user now that only X amount of renewals corresponded to the current checkout, out of a Y total, if that is the case then it's not needed anymore. But I'm not 100%, this is confusing. Additional testing to the previous test plans could also be done by visiting the readingrec page to make sure everything is ok: /cgi-bin/koha/members/readingrec.pl?cardnumber=42 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #10 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 158976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158976&action=edit Bug 35310: (QA follow-up): Remove obsolete code Repeat the previous test plans, notice everything works as expected. No console errors. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Pedro Amorim from comment #11)

Thank Victor, great insight!

I'm fairly confident the code can be safely removed.

I was wondering: could it be this was the fallback? In old installations there was no table for the renewals, I believe the text displayed in this case when there are no entries int he renewals table, but renewals happened before the table was added. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- We are not even on a version with the table yet, I'd like to keep it then :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I think in testing we need to make sure that there is no regression when there are no entries in the table, but the renewals count on the checkout is > 0 (maybe also needs a last renewed date if you are faking the data). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- It's not about porting back, but about data. The checkouts_renewals table was added in 22.11, so in Koha terms not that long ago. Before that we had no data bout individual renewals, just the last renewal date and the number of renewals, both stored in issues. So when updating, the checkouts_renewals table was not filled retroactively, because it was not possible. Instead the GUI reacts to it - if there are entries in checkouts_renewals it can show them. If not it shows just summary information. We need to keep that working, as libraries still have tons of old data, but dev environments usually don't. I already suggested how to fake it: (In reply to Katrin Fischer from comment #16)

I think in testing we need to make sure that there is no regression when there are no entries in the table, but the renewals count on the checkout is

...

0 (maybe also needs a last renewed date if you are faking the data).

In issues:Ö increment the number in renewals_count and add a lastreneweddate. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #19 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Talked to Katrin about this. She is right. I made the 4th patch obsolete. The message "X out of Y renewals have been logged" refers to the fact that a checkout may have a renewals_count > 0 in the issues tables but not have a corresponding entry in checkout_renewals. This will happen for renewals that happened prior to bug 30275, and the reason behind this is because the data to be moved on to the new checkout_renewals table didn't exist and inserting fake data wouldn't make sense either. The above was the background I was missing. To reproduce the issue she mentions (with all patches applied - including the (QA follow-up): Remove obsolete code patch), clean k-t-d: 1) Checkout barcode 39999000003154 for patron of cardnumber=42 2) Renew it through the database (emulating how the database worked prior to bug 30275), run query: update issues set renewals_count = 3 where issue_id = 1; 3) Visit /cgi-bin/koha/members/readingrec.pl?cardnumber=42 4) Notice "number of renewals" is 3. Click on "View", notice the modal is empty. 5) Remove the last patch, however you want, suggestions: git reset --hard <hash_of_previous_commit> git revert <hash_of_commit_to_remove> 6) Restart plack, repeat 3) Notice it now says "Note: 0 out of 3 renewals have been logged" as expected. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Katrin Fischer from comment #18)

I already suggested how to fake it:

(In reply to Katrin Fischer from comment #16)

...

I think in testing we need to make sure that there is no regression when there are no entries in the table, but the renewals count on the checkout is

...

0 (maybe also needs a last renewed date if you are faking the data).

In issues:Ö increment the number in renewals_count and add a lastreneweddate.

I see, because the old data isn't migrated due to missing date, it's left here and all places might need to account for two sources of renewals. So how should it be accounted here? After more testing of the current patches: - Item details page of a record doesn't show "Current renewals:" if there are only legacy renewals. - Item details page of a record and the modal only count stuff from checkout_renewals. But patron circulation history counts them all. Back to the root, if I understand correctly: that transient state that happens only when an instance just migrated causes to **either** 1. have (**temporary**) inconsistency in various places (if it affects circ history, inconsistency it lasts a long as personal data retention delay of the library) 2. add complexity in various places to account for two sources of renewal numbers 3. have this complexity put in a single place in the model layer 4. actually migrate the old data in checkouts_renewals even if with no renewal date. Does the way old data was let in issues.renewals_count means that the current strategy is to not bother with temporary inconsistency after migration? Then, current patches here are in line with that. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #22 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- (In reply to Victor Grousset/tuxayo from comment #21)

Does the way old data was let in issues.renewals_count means that the current strategy is to not bother with temporary inconsistency after migration? Then, current patches here are in line with that.

I would say so, I don't think tackling the temporary inconsistency is cost-effective, because the inconsistency only happens in the items page and the items page only considers current checkouts. After an upgrade it's only a matter of time until all current renewals exist in the checkout_renewals table. Patron circulation history shows everything. There may be some other things worth improving here, personally I don't think the string "«Note: 1 out of 2 renewals have been logged»" is a clear enough way of conveying to the staff user what is happening or has happened, i.e. a person who does not know the history behind these datatabase tables will likely not fully understand what that string is referring to (in my opinion anyway, I didn't and was thrown off by it, but that's me!). But I also don't have a better suggestion right now! (In reply to Victor Grousset/tuxayo from comment #21)

I see, because the old data isn't migrated due to missing date, it's left here and all places might need to account for two sources of renewals.

This is not my understanding of the situation, but I may be wrong. My understanding is that old data was not migrated due to missing datA, not date. Before the checkout_renewals table, renewals information just wasn't logged individually. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Maybe we should move discussion about improving the GUI to a separate bug? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #26 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Katrin Fischer from comment #24)

Maybe we should move discussion about improving the GUI to a separate bug?

Right, it was about dead code suspicious at the beginning but in the end it's not dead. And clarifying the phrasing is out of scope here. I lost focus on the following: (In reply to Victor Grousset/tuxayo from comment #21)

After more testing of the current patches: - Item details page of a record doesn't show "Current renewals:" if there are only legacy renewals.

The current patches introduce an inconsistency with the same feature elsewhere in Koha which has still the old behavior. By default I think that's in scope but there are a lot of case by case exceptions. What do you think? I don't have a strong opinion on it.

- Item details page of a record and the modal only count stuff from checkout_renewals. But patron circulation history counts them all.

From what I understand it was already inconsistent. It's not a case of "we are fixing it here but not somewhere else that has the same code". So out of the scope.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #28 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Martin Renvoize from comment #27)

Is there actually any more discussion to take place here.. it looks like we reached a conclusion that the code all made "sense" given the history of the feature.

The only remaining thing is the following and your message might implicitly mean "out of scope" for that point. At least that didn't strike you as an immediate worthwhile issue ^^

...

After more testing of the current patches: - Item details page of a record doesn't show "Current renewals:" if there are only legacy renewals.

The current patches introduce an inconsistency with the same feature elsewhere in Koha which has still the old behavior. By default I think that's in scope but there are a lot of case by case exceptions. What do you think? I don't have a strong opinion on it.

And it didn't seem to have caught the eye of Katrin and Pedro either since the 18th. So it safely enough feels like this can move on. Works, makes sense, QA script happy, code looks good, passing QA :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #30 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

I believe I did address your last concern when I replied:

Patron circulation history shows everything.

Ah yes, indeed it w -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Patch doesn't apply --- Comment #32 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Please rebase! I had a look at the conflict, but was not completely sure, so will better leave you to it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Pedro Amorim from comment #33)

This applies for me on current master, just pulled. Can you please double-check Katrin?

Maybe because you have more information in your git. Can you please re-upload the patches? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #158823|0 |1 is obsolete| | --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 159724 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159724&action=edit Bug 35310: Only show 'Current renewals' link if item is currently checked out 1) Checkout an item to Koha user, barcode 39999000003192 2) Renew that barcode 3) Visit the items page of the biblio record, click on "View" in front of "Current renewals" /cgi-bin/koha/catalogue/moredetail.pl?biblionumber=76 4) Notice it shows the renewals as expected 5) Return the same item 39999000003192 6) Repeat 3). Notice it never loads renewals and network console shows HTTP error 400 7) Apply this patch. Repeat the test plan. Notice the 'Current renewals' no longer shows for the item because it is no longer checked out, so whatever past renewals are no longer 'current' Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Patches all apply for me and I don't have a copy of the branch.. I just pull master and applied and they worked.. re-uploaded just in case. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 --- Comment #39 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|24.05.00,23.11.02 |24.05.00,23.11.02,23.05.08 released in| | CC| |lucas@bywatersolutions.com --- Comment #41 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 23.05.x for upcoming 23.07.08 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35310 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37591 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37591 [Bug 37591] Moredetail.tt page is opening very slowly -- You are receiving this mail because: You are watching all bug changes.