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
Kyle, Have you any idea how we could fix that? Looks tricky.
Similar to Bug 14798
Created attachment 52208 [details] [review] Fix test data in issue.t and defaults for stats 'Null' iand '' are not really NULL values
Created attachment 52209 [details] [review] Test rental charges properly * AddIssuingCharge() test was always returning pass - removed and replaced with: * Test rental charges properly for both checkout and renewal
Created attachment 52210 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
To test - Librarian interface - Circulation: Have an item with rental charges. Checkout. Renew. Checkouts table should reload with rental charges added.
Created attachment 52473 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
Created attachment 52474 [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
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.
(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?
Yes. And run t/db_dependent/Circulation/issue.t, first two patches is the test fix.
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.
Created attachment 53406 [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
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.
Created attachment 53482 [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
From what I can tell, when the patches are applied, it breaks the form #mainform, and is not displayed. Cannot follow test plan.
Created attachment 60629 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
Created attachment 60630 [details] [review] Bugfix: added missing use Koha::Account
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
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.
(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"?
Ideally someone should try to replicate the original report in the top note. I could not. But in discussion is fine with me.
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.
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
(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.
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
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
> But now, when I have no fines owed, I can't see the checkout form and even > any current checkouts... Oops. Corrected
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)
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
(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.
(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...
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
Created attachment 65696 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
Created attachment 65697 [details] [review] Bugfix: added missing use Koha::Account
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
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.
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.
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
(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.
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
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
(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.
Firefox 54.0 (64-bit)
Testing at Koha US on master. The process seems to work fine without the patch. Is this still needed???
Hi Christopher, is the amount outstanding on top updated immediately with the checkout?
@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!
> 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!
(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.
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.
Created attachment 66261 [details] [review] Bug 14803 follow-up: outstanding-fees-amount is a class, not id Multiple outstanding-fees-amount displays on the page
Katrin is correct. And we can make an even stronger statement: It does not show correct charges per item in general - does not take renewals in account. In that spirit, I've added a small follow-up to update Fines & Charges tab as well.
Patch doesn't apply any longer - wanted to give this another try. Could you rebase?
Created attachment 73541 [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
Created attachment 73542 [details] [review] Bug 14803 follow-up: outstanding-fees-amount is a class, not id Multiple outstanding-fees-amount displays on the page
Rebased, and fines look fine, however I think checkouts list is somewhat broken on the circ issuing screen. And I believe that's not to do with this patch, but cannot be 100% sure. Can you please have a look. Thanks
Hi Srdjan, QA tests pass, but there is definitely a problem with the checkouts - it claimes I have none now, when I have indeed 4 items checked out.
Created attachment 98891 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
Created attachment 98892 [details] [review] bug 14803: KOHA.Checkouts js object * Use javascript to: - update fines/checkouts info - show/hide checkouts form and messages * Created svc/fines
Created attachment 115620 [details] [review] Bug 14803: Reload checkouts table on renewal svc/checkouts: get real rental charges for patron, rather than itemtype indication
Created attachment 115621 [details] [review] bug 14803: KOHA.Checkouts js object * Use javascript to: - update fines/checkouts info - show/hide checkouts form and messages * Created svc/fines
Patch no longer applies 8-( Applying: Bug 14803: Reload checkouts table on renewal Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/js/checkouts.js Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/js/checkouts.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/checkouts.js error: Failed to merge in the changes. Patch failed at 0001 Bug 14803: Reload checkouts table on renewal The copy of the patch that failed is found in: .git/rebase-apply/patch Testing notes: - Was going to use the steps in the description of the issue to a) replicate the issue b) apply the patch c) see if that fixes the issue