Bug 19076

Summary: Renewal via Checkout screen is logged as both a renewal and a checkout
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: CirculationAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: normal    
Priority: P5 - low CC: cbrannon, cslone, fridolin.somers, gmcharlt, jonathan.druart, julian.maurice, katrin.fischer, kyle.m.hall
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17708    
Bug Blocks:    
Attachments: Bug 19076 - unit tests
Bug 19076 - Move issue logging in AddIssue into condtional clause
Bug 19076 - unit tests
Bug 19076 - Move issue logging in AddIssue into condtional clause
Bug 19076 - followup - delete fines from additional renewal
[SIGNED OFF ] Bug 19076 - followup - delete fines from additional renewal
Bug 19076 - unit tests
Bug 19076 - Move issue logging in AddIssue into condtional clause
Bug 19076 - followup - delete fines from additional renewal

Description Nick Clemens (kidclamp) 2017-08-10 14:58:04 UTC
To recreate:
1 - Enable both issue and renewal logs
2 - Checkout an item to a patron
3 - View the logs - the issue is captured
4 - Checkout the item to the patron again and confirm renewal
5 - Both an issue and a renewal are logged
Comment 1 Nick Clemens (kidclamp) 2017-08-10 15:24:21 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens (kidclamp) 2017-08-10 15:24:24 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens (kidclamp) 2017-08-11 15:36:53 UTC
Created attachment 65907 [details] [review]
Bug 19076 - unit tests
Comment 4 Nick Clemens (kidclamp) 2017-08-11 15:36:56 UTC
Created attachment 65908 [details] [review]
Bug 19076 - Move issue logging in AddIssue into condtional clause

Currently AddIssue tests if renewal, but logs an issue even if so. This
patch moves the logging into the conditional so a log entry is only
added if we aren't renewing (as renewals are logged separately)

To test:
1 - prove t/db_dependent/Circulation.t - one test should fail
2 - Enable both issue and renewal logs
3 - Checkout an item to a patron
4 - View the logs - the issue is captured
5 - Checkout the item to the patron again and confirm renewal
6 - Both an issue and a renewal are logged
7 - Apply patch
8 Repeat 1-6, tests should pass and only renewal should be logged
Comment 5 Nick Clemens (kidclamp) 2017-08-11 15:40:44 UTC
Created attachment 65916 [details] [review]
Bug 19076 - followup - delete fines from additional renewal
Comment 6 Christopher Brannon 2017-08-12 18:36:38 UTC
Please rebase.
Comment 7 Nick Clemens (kidclamp) 2017-08-12 19:36:04 UTC
Applies cleanly for me on master, can you pull and retest?
Comment 8 Christopher Brannon 2017-08-12 22:12:44 UTC
Created attachment 65939 [details] [review]
[SIGNED OFF ] Bug 19076 - followup - delete fines from additional renewal

Works as described.

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 9 Julian Maurice 2017-09-01 10:44:13 UTC
Created attachment 66701 [details] [review]
Bug 19076 - unit tests

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Julian Maurice 2017-09-01 10:44:17 UTC
Created attachment 66702 [details] [review]
Bug 19076 - Move issue logging in AddIssue into condtional clause

Currently AddIssue tests if renewal, but logs an issue even if so. This
patch moves the logging into the conditional so a log entry is only
added if we aren't renewing (as renewals are logged separately)

To test:
1 - prove t/db_dependent/Circulation.t - one test should fail
2 - Enable both issue and renewal logs
3 - Checkout an item to a patron
4 - View the logs - the issue is captured
5 - Checkout the item to the patron again and confirm renewal
6 - Both an issue and a renewal are logged
7 - Apply patch
8 Repeat 1-6, tests should pass and only renewal should be logged

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 11 Julian Maurice 2017-09-01 10:44:20 UTC
Created attachment 66703 [details] [review]
Bug 19076 - followup - delete fines from additional renewal

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 12 Jonathan Druart 2017-09-01 15:02:59 UTC
(In reply to Christopher Brannon from comment #8)

You only signed off on the last patch, please read the wiki page https://wiki.koha-community.org/wiki/Git_bz_configuration#Bugs_with_more_than_one_patch
and/or follow the tutorial: https://wiki.koha-community.org/wiki/Koha_howto
Comment 13 Jonathan Druart 2017-09-01 15:06:20 UTC
(In reply to Nick Clemens from comment #5)
> Created attachment 65916 [details] [review] [review]
> Bug 19076 - followup - delete fines from additional renewal

Why this patch here? Sounds like it should be moved somewhere else.
Comment 14 Nick Clemens (kidclamp) 2017-09-01 15:17:14 UTC
It should be squashed into unit tests patch, the extra renewal I add for tests added a fine, had to remove that to not break other tests
Comment 15 Jonathan Druart 2017-09-01 16:08:23 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 16 Fridolin Somers 2017-10-10 09:07:05 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 17 Katrin Fischer 2017-10-14 11:58:15 UTC
Dependency on bug 17708 which is not in 16.11.x.