Bug 19076 - Renewal via Checkout screen is logged as both a renewal and a checkout
Summary: Renewal via Checkout screen is logged as both a renewal and a checkout
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Julian Maurice
URL:
Whiteboard:
Keywords:
Depends on: 17708
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-10 14:58 UTC by Nick Clemens
Modified: 2018-02-09 17:28 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19076 - unit tests (2.23 KB, patch)
2017-08-10 15:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19076 - Move issue logging in AddIssue into condtional clause (1.54 KB, patch)
2017-08-10 15:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19076 - unit tests (2.55 KB, patch)
2017-08-11 15:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19076 - Move issue logging in AddIssue into condtional clause (1.54 KB, patch)
2017-08-11 15:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19076 - followup - delete fines from additional renewal (1.05 KB, patch)
2017-08-11 15:40 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF ] Bug 19076 - followup - delete fines from additional renewal (1.13 KB, patch)
2017-08-12 22:12 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 19076 - unit tests (2.62 KB, patch)
2017-09-01 10:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19076 - Move issue logging in AddIssue into condtional clause (1.61 KB, patch)
2017-09-01 10:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19076 - followup - delete fines from additional renewal (1.18 KB, patch)
2017-09-01 10:44 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 2017-08-10 15:24:21 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2017-08-10 15:24:24 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2017-08-11 15:36:53 UTC
Created attachment 65907 [details] [review]
Bug 19076 - unit tests
Comment 4 Nick Clemens 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 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 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 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.