Bug 14803

Summary: Rental and fine fees are not updating on circulation screen
Product: Koha Reporter: Debbie <dpriceewen>
Component: CirculationAssignee: Srdjan Jankovic <srdjan>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: cbrannon, fiona.borthwick, gmcharlt, gwilliams, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, liz, marjorie.barry-vila, paul.poulain, srdjan, veron
Version: masterKeywords: Academy
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Bug Depends on: 11703    
Bug Blocks:    
Attachments: Fix test data in issue.t and defaults for stats
Test rental charges properly
Bug 14803: Reload checkouts table on renewal
Bug 14803: Reload checkouts table on renewal
bug 14803: KOHA.Checkouts js object
bug 14803: KOHA.Checkouts js object
bug 14803: KOHA.Checkouts js object
Bug 14803: Reload checkouts table on renewal
Bugfix: added missing use Koha::Account
bug 14803: KOHA.Checkouts js object
Screenshot showing a problem with the checkout box
bug 14803: KOHA.Checkouts js object
Checking Out
bug 14803: KOHA.Checkouts js object
bug 14803: KOHA.Checkouts js object
Bug 14803: Reload checkouts table on renewal
Bugfix: added missing use Koha::Account
bug 14803: KOHA.Checkouts js object
bug 14803: KOHA.Checkouts js object
bug 14803: KOHA.Checkouts js object

Description Debbie 2015-09-10 23:19:10 UTC
To replicate:
 - From circulation screen attempt to renew an item with a rental fee
 - Note that fees and charges aren't updating: e.g. 
  the circulation screen should read under 
Attention:
Fees & Charges: Patron has Outstanding fees & charges of "x" amount. Make payment.
- Note "x" amount has not increase
- All fees/fines appear not to be updating
Comment 1 Jonathan Druart 2015-09-15 10:09:50 UTC
Kyle,
Have you any idea how we could fix that? Looks tricky.
Comment 2 Liz Rea 2015-10-02 02:34:26 UTC
Similar to Bug 14798
Comment 3 Srdjan Jankovic 2016-06-10 00:58:09 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2016-06-10 00:58:25 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2016-06-10 00:59:29 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2016-06-10 01:03:49 UTC
To test - Librarian interface - Circulation:

Have an item with rental charges. Checkout. Renew. Checkouts table should reload with rental charges added.
Comment 7 Srdjan Jankovic 2016-06-17 02:13:18 UTC Comment hidden (obsolete)
Comment 8 Srdjan Jankovic 2016-06-17 02:13:31 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2016-06-17 02:18:35 UTC
This is a bit bigger than expected. Requires testing issue blocking with fines over limit.

Also "Patron details" top " Checkouts are BLOCKED" message needs checking because it uses same component.
Comment 10 Owen Leonard 2016-07-12 18:16:55 UTC
(In reply to Srdjan Jankovic from comment #6)
> To test - Librarian interface - Circulation:
> 
> Have an item with rental charges. Checkout. Renew. Checkouts table should
> reload with rental charges added.

Is this the whole test plan for all patches?
Comment 11 Srdjan Jankovic 2016-07-13 00:03:44 UTC
Yes. And run t/db_dependent/Circulation/issue.t, first two patches is the test fix.
Comment 12 Owen Leonard 2016-07-13 13:17:50 UTC
This breaks the list of current checkouts. On the circulation page it says "0 checkouts" even though I have many. On the patron detail page it shows the correct count in the checkouts tab but lists no checkouts.

I strongly object to the delay this patch introduces to the appearance of the checkout form. In my test system there is a 5-second delay between the time the rest of the page renders and the time the checkout form appears.

The more AJAX we introduce on the checkout page the slower it gets. I don't think that's what we intend, but that's the real consequence.
Comment 13 Srdjan Jankovic 2016-07-15 01:54:45 UTC Comment hidden (obsolete)
Comment 14 Srdjan Jankovic 2016-07-15 02:07:50 UTC
Owen, I'm not sure why you are getting the delay. In my case once page has loaded form comes up instantaneously. Admittedly, my database is not a large one. Nevertheless, the only overhead introduced here is a http request/response, so if the connection is decent (and it should be for the librarian interface) everything should be fine.
The change moved one database query from the main handler to a service. As a matter of fact it replaced two calls to the same function with only one, which should only be faster. So when comparing, a fair comparison would be full time from the circulation.pl request inception to the point where the checkout form is shown.
Can you please give it another go with timing pre and post patch from the point patron card number is entered to the appearance of the item checkout form. In my env I can detect no difference.
Comment 15 Srdjan Jankovic 2016-07-19 00:33:05 UTC Comment hidden (obsolete)
Comment 16 Christopher Brannon 2016-08-07 07:20:13 UTC
From what I can tell, when the patches are applied, it breaks the form #mainform, and is not displayed.  Cannot follow test plan.
Comment 17 Srdjan Jankovic 2017-02-24 04:56:27 UTC
Created attachment 60629 [details] [review]
Bug 14803: Reload checkouts table on renewal

svc/checkouts: get real rental charges for patron, rather than itemtype
indication
Comment 18 Srdjan Jankovic 2017-02-24 04:56:40 UTC
Created attachment 60630 [details] [review]
Bugfix: added missing use Koha::Account
Comment 19 Srdjan Jankovic 2017-02-24 04:57:38 UTC
Created attachment 60631 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 20 Srdjan Jankovic 2017-02-24 05:00:38 UTC
Things have moved on meanwhile, and it seems to me that the page is being force reloaded now, so I'm not sure how useful this patch is. Maybe just a start of a full ajax solution.
Comment 21 Marc VĂ©ron 2017-04-06 08:04:12 UTC
(In reply to Srdjan Jankovic from comment #20)
> Things have moved on meanwhile, and it seems to me that the page is being
> force reloaded now, so I'm not sure how useful this patch is. Maybe just a
> start of a full ajax solution.

Srdjan, do I get it right: No sign-off needed at the moment? Can status be switched e.g. to "In Discussion"?
Comment 22 Srdjan Jankovic 2017-04-06 23:51:06 UTC
Ideally someone should try to replicate the original report in the top note. I could not. But in discussion is fine with me.
Comment 23 Owen Leonard 2017-04-18 13:59:13 UTC
Created attachment 62294 [details]
Screenshot showing a problem with the checkout box

(In reply to Srdjan Jankovic from comment #20)
> Things have moved on meanwhile, and it seems to me that the page is being
> force reloaded now, so I'm not sure how useful this patch is.

I don't find that this page is being reloaded when renewing from the checkboxes. This looks like it's working correctly to update the fee numbers, but something weird happens when the patron's fines are past the noissuescharge limit. The checkout form is hidden from all users, whether they have permission to override or not.
Comment 24 Srdjan Jankovic 2017-04-20 03:06:48 UTC
Created attachment 62432 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 25 Srdjan Jankovic 2017-04-20 03:10:53 UTC
(In reply to Owen Leonard from comment #23)
> Created attachment 62294 [details]
> Screenshot showing a problem with the checkout box
> 
> something weird happens when the patron's fines are past the
> noissuescharge limit. The checkout form is hidden from all users, whether
> they have permission to override or not.

That may be an existing condition. I've made some changes to take in account AllowFineOverride pref.
Comment 26 Josef Moravec 2017-05-04 07:02:06 UTC
Created attachment 63047 [details]
Checking Out

(In reply to Srdjan Jankovic from comment #25)
> (In reply to Owen Leonard from comment #23)
> > Created attachment 62294 [details]
> > Screenshot showing a problem with the checkout box
> > 
> > something weird happens when the patron's fines are past the
> > noissuescharge limit. The checkout form is hidden from all users, whether
> > they have permission to override or not.
> 
> That may be an existing condition. I've made some changes to take in account
> AllowFineOverride pref.

But now, when I have no fines owed, I can't see the checkout form and even any current checkouts...

See attached screenshot
Comment 27 Srdjan Jankovic 2017-05-05 01:25:53 UTC
Created attachment 63153 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 28 Srdjan Jankovic 2017-05-05 01:27:44 UTC
> But now, when I have no fines owed, I can't see the checkout form and even
> any current checkouts...

Oops. Corrected
Comment 29 Josef Moravec 2017-05-05 09:41:43 UTC
I have to more concerns about it:
1) The barcode field doesn't have focus after page is loaded - probably easy to fix, but important
2) Looks like it waits than issues are loaded and then show the barcode input field, maybe I am wrong and it just looks like, but in any case it is much slower then current master...
3) Message "loading, you can continue scanning" is badly placed (in the left top corner)
Comment 30 Srdjan Jankovic 2017-05-08 00:47:01 UTC
Created attachment 63230 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 31 Srdjan Jankovic 2017-05-08 00:55:50 UTC
(In reply to Josef Moravec from comment #29)

Hi Josef, thanks for testing this.

> I have to more concerns about it:
> 1) The barcode field doesn't have focus after page is loaded - probably easy
> to fix, but important
> 2) Looks like it waits than issues are loaded and then show the barcode
> input field, maybe I am wrong and it just looks like, but in any case it is
> much slower then current master...

Ok, now it should be behaving like master. I tried to add some accuracy, but drawbacks are obviously too big...

> 3) Message "loading, you can continue scanning" is badly placed (in the left
> top corner)

For me it is placed center of the table. At any rate I believe my patch did not change the table, it just makes a issuesTable.api().ajax.reload() call. Could you please a) check it in another browser b) compare to master. Thanks a lot.
Comment 32 Josef Moravec 2017-05-09 14:52:12 UTC
(In reply to Srdjan Jankovic from comment #31)
> (In reply to Josef Moravec from comment #29)
> 
> Hi Josef, thanks for testing this.
> 
> > I have to more concerns about it:
> > 1) The barcode field doesn't have focus after page is loaded - probably easy
> > to fix, but important
> > 2) Looks like it waits than issues are loaded and then show the barcode
> > input field, maybe I am wrong and it just looks like, but in any case it is
> > much slower then current master...
> 
> Ok, now it should be behaving like master. I tried to add some accuracy, but
> drawbacks are obviously too big...

It is Ok now, thanks

> > 3) Message "loading, you can continue scanning" is badly placed (in the left
> > top corner)
> 
> For me it is placed center of the table. At any rate I believe my patch did
> not change the table, it just makes a issuesTable.api().ajax.reload() call.
> Could you please a) check it in another browser b) compare to master. Thanks
> a lot.

Hm, strange... On master it's ok for me, on this patchset still bad... but I can't find out where's the problem...
Comment 33 Katrin Fischer 2017-08-08 12:40:49 UTC
Sorry, can't test, the third patch doesn't apply:

Applying: Bugfix: added missing use Koha::Account
Applying: bug 14803: KOHA.Checkouts js object
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/checkouts.js).
error: could not build fake ancestor
Patch failed at 0001 bug 14803: KOHA.Checkouts js object
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/bug-14803-KOHACheckouts-js-object-hfCTB4.patch
Comment 34 Srdjan Jankovic 2017-08-09 03:51:03 UTC
Created attachment 65696 [details] [review]
Bug 14803: Reload checkouts table on renewal

svc/checkouts: get real rental charges for patron, rather than itemtype
indication
Comment 35 Srdjan Jankovic 2017-08-09 03:51:21 UTC
Created attachment 65697 [details] [review]
Bugfix: added missing use Koha::Account
Comment 36 Srdjan Jankovic 2017-08-09 03:52:15 UTC
Created attachment 65698 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 37 Katrin Fischer 2017-08-09 08:13:54 UTC
1) Check out an item 
2) Verfiy it shows as checked out
3) Renew item from the table 
4) Verify the amount has been updated (OK!)
5) Check in the item from the table
6) Verify the amount has been updated (OK!)
BUT: Instead of seeing the success message the checkout vanishes instantly.
Without the patch, the checked-in items remain visible, highlighted in yellow showing the success message. With the patch they vanish immediately. I think we need to keep the former behaviour.
Comment 38 Katrin Fischer 2017-08-09 08:15:15 UTC
Sorry, last test report was partially nonsense:

1) Check out an item 
2) Verfiy it shows as checked out
3) Verify the amount has been updated (OK!)
4) Renew item from the table 
5) Verify the amount has been updated (OK!)
6) Check in the item from the table
BUT: Instead of seeing the success message the checkout vanishes instantly.
Without the patch, the checked-in items remain visible, highlighted in yellow showing the success message. With the patch they vanish immediately. I think we need to keep the former behavior.
Comment 39 Srdjan Jankovic 2017-08-10 02:33:33 UTC
Created attachment 65783 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 40 Srdjan Jankovic 2017-08-10 02:35:32 UTC
(In reply to Katrin Fischer from comment #38)
>
> 6) Check in the item from the table
> BUT: Instead of seeing the success message the checkout vanishes instantly.
> Without the patch, the checked-in items remain visible, highlighted in
> yellow showing the success message. With the patch they vanish immediately.
> I think we need to keep the former behavior.

You are right, better if it stays there. I just could not live with "Renew" still showing. So I came up with something else.
Comment 41 Katrin Fischer 2017-08-10 09:43:05 UTC
I am sorry Srdjan, but something else weird popped up now. Applied the new patches, restarted plack, reloaded to get new Javascript...

The amount outstanding is not showing at all.

I found the following JavaScript error in the logs, not sure if it's related:
ReferenceError: checkouts_only is not defined
Comment 42 Srdjan Jankovic 2017-08-10 23:28:50 UTC
Created attachment 65868 [details] [review]
bug 14803: KOHA.Checkouts js object

* Use javascript to:
- update fines/checkouts info
- show/hide checkouts form and messages
* Created svc/patron/fines
Comment 43 Srdjan Jankovic 2017-08-10 23:31:26 UTC
(In reply to Katrin Fischer from comment #41)
> I found the following JavaScript error in the logs, not sure if it's related:
> ReferenceError: checkouts_only is not defined

Sorry Katrin, that was a leftover. Out of curiosity, which browser do you use please? I had the error, but it did not seem to stop the execution.
Comment 44 Katrin Fischer 2017-08-10 23:32:38 UTC
Firefox 54.0 (64-bit)
Comment 45 Christopher Brannon 2017-08-12 18:09:09 UTC
Testing at Koha US on master.  The process seems to work fine without the patch.  Is this still needed???
Comment 46 Katrin Fischer 2017-08-13 11:00:35 UTC
Hi Christopher, is the amount outstanding on top updated immediately with the checkout?
Comment 47 Katrin Fischer 2017-08-15 14:46:34 UTC
@Christopher - did you check only check-out and check-in or renewals as well? This bug is about fixing the renewals, checkouts update already.

@Srdjan

I am sorry, but this just doesn't work well or reliably. I am seeing lots of oddness in testing.

1) Checkouts: 
With the patch the specified due date (today, 23:59) is set to a date that can't be cleared. Switching back to master and restarting everything + reloading it's ok. It appears to be connected to the patches and is repeatable.
Tested in 2 different browsers (Firefox, Chrome) and with different patrons.

2) Renewals:
I think it's quite irritating that the items resort immediately after successful renewal with the last items going to the bottom and there is no success message. It really confused me as I was testing with multiple items of the same record. The rental amount display works fine and updates for every renewal. I feel a success message or highlighting the renewed lines in color (like for checkout) could help a lot with this. This kind of behaviour is something that ideally should be tested by a few librarians.

3) Checkins:
Works well!

Failing for 1) - sorry!
Comment 48 Katrin Fischer 2017-08-15 14:47:22 UTC
> I am sorry, but this just doesn't work well or reliably. I am seeing lots of
> oddness in testing.

Scratch that - I had forgotten to reload properly first. Apart from the 2 noted things it works well and not too much oddness!
Comment 49 Christopher Brannon 2017-08-17 17:16:31 UTC
(In reply to Katrin Fischer from comment #46)
> Hi Christopher, is the amount outstanding on top updated immediately with
> the checkout?

Yes.  No patch applied.  The attention message is updated to reflect the new charge after renewal.  I see no need for the patch.
Comment 50 Katrin Fischer 2017-08-17 17:35:36 UTC
It updates in the checkout table - the rental fine column. I think that didn't happen for me without the patch. It also allows multiple renewals of the same item without reloading the page.