Created attachment 158822 [details] [review]
Bug 35310: Only show 'Current renewals' <li> entry if there are current renewals

'Current renewals' is always showing, even if empty.

Test plan:
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 all items have the 'Current renewals' label entry, but only the checked out item in step 1) has the 'Current renewals' label entry
5) Apply patch. Notice only the item with current renewals is showing the 'Current renewals' label entry with the respective value.

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 158823 [details] [review]
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>

Created attachment 158824 [details] [review]
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

Signed-off-by: David Nind <david@davidnind.com>

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

(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-ff6b8814c72c6c6b8a9e53514a409d2dba8ce865849b8c1957a24e42cef90aeaR111
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

> Note that when the work was originally done, readingrec.tt was tracking issues
> (which I believe is all the checkouts of an item)

An issue is the historic name of a checkout. https://wiki.koha-community.org/wiki/Terminology#I
That can create confusion when looking at history or places where it's not practical to change ^^"

So it's likely the same in that case.


> 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

Looks good :)


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

Looking more at it: the data-renewals attribute changed by these patches is what is used by the `if (data.length < renewals)` in checkout_renewals_modal.js so I'm pretty sure it's rendered obsolete.

As for the cleanup it seems to me that it's the following that needs to be removed:

checkout_renewals_modal.js:
       if (data.length < renewals) {...}

checkout_renewals.inc:
                <div id="incomplete" class="alert" style="display:none"></div>

What does it looks like to you? And to which degree of certainty? ^^

Thank Victor, great insight!

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

(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.

(In reply to Katrin Fischer from comment #12)
> (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.

That can certainly be the case, yes.

We are not even on a version with the table yet, I'd like to keep it then :)

(In reply to Katrin Fischer from comment #14)
> We are not even on a version with the table yet, I'd like to keep it then :)

Please let me know what's needed from me to move this bug, Katrin. Thanks!

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 reply to Katrin Fischer from comment #12)
> 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.

(In reply to Katrin Fischer from comment #14)
> We are not even on a version with the table yet, I'd like to keep it then :)

So is it about backportability?

---

(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).

How should it be possible to get no entries in the table but renewals count on the checkout > 0 ?

The table created like that, right?:

            var items = data.map(function(item) {
                return createLi(item);
            });

It's the same `data` as the previous `if (data.length < renewals)`. Or maybe there is still a way to have the two compared values different?

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.

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.

(In reply to Katrin Fischer from comment #18)
> 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.

Ah ok the DB table, not the HTML table!


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

Indeed, thanks for catching that! :)

(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.

(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.

(In reply to Pedro Amorim from comment #22)
> 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!

Maybe «Note: 1 out of 2 renewals have been logged (old versions of Koha didn't log the date)»
Or «Old versions of Koha didn't keep renewals dates, so only 1 out of 2 renewals have them.»


> (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.

I recall from the patron checkout history that the old checkouts still have the number of renewals. (View => 1 out of 2 renewals [...])

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

(In reply to Katrin Fischer from comment #24)
> Maybe we should move discussion about improving the GUI to a separate bug?

Yes, I would say so. The patches here fix the reported bug and I believe are an overall improvement. We've gone back enough in time to understand past decisions made re this.
@Victor please if you find any additional issues with current patches, provide step-by-step instructions on how to reproduce it.
@Katrin, same, when you have the chance to test, please.

Otherwise I think this is good to move along.

(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.

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.

Can we push it into the QA queue at this point?

(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 :)

Hi Victor, thanks for your continued analysis on this issue.
It did reveal itself to be a bit deeper than the simple bug it appeared to be initially.
I believe I did address your last concern when I replied:

(In reply to Pedro Amorim from comment #22)
> 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.

I may be incorrect in my way of thinking, but I addressed it, unless I misunderstood the issue you're mentioning.

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

> Patron circulation history shows everything.

Ah yes, indeed it w

as about that! (accidental tab strike!)

Thanks all for the contributions here :)

Please rebase! I had a look at the conflict, but was not completely sure, so will better leave you to it.

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

(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?

Created attachment 159723 [details] [review]
Bug 35310: Only show 'Current renewals' <li> entry if there are current renewals

'Current renewals' is always showing, even if empty.

Test plan:
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 all items have the 'Current renewals' label entry, but only the checked out item in step 1) has the 'Current renewals' label entry
5) Apply patch. Notice only the item with current renewals is showing the 'Current renewals' label entry with the respective value.

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 159724 [details] [review]
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>

Created attachment 159725 [details] [review]
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

Signed-off-by: David Nind <david@davidnind.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.

Pushed for 24.05!

Well done everyone, thank you!

Pushed to 23.11.x for 23.11.02

Backported to 23.05.x for upcoming 23.07.08