Bug 17708 - Renewal log seems empty
Summary: Renewal log seems empty
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Transaction logs (show other bugs)
Version: master
Hardware: All All
: P1 - high normal (vote)
Assignee: Baptiste
QA Contact: Testopia
URL:
Keywords:
: 8752 (view as bug list)
Depends on:
Blocks: 19076
  Show dependency treegraph
 
Reported: 2016-12-01 16:31 UTC by vimal kumar
Modified: 2018-06-04 20:11 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Renewal log (34.46 KB, image/png)
2016-12-01 16:31 UTC, vimal kumar
Details
Added a logaction function in Addrenewal in order to send renew logs (2.58 KB, patch)
2017-02-08 09:06 UTC, Baptiste
Details | Diff | Splinter Review
Added a logaction function in Addrenewal in order to send renew logs (2.78 KB, patch)
2017-02-17 09:56 UTC, Marc Véron
Details | Diff | Splinter Review
BUG 8010: Added a logaction function in Addrenewal in order to send renew logs (4.81 KB, patch)
2017-02-20 09:42 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (4.81 KB, patch)
2017-02-20 09:47 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.41 KB, patch)
2017-02-20 10:20 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.41 KB, patch)
2017-02-20 10:25 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.41 KB, patch)
2017-02-20 10:53 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.58 KB, patch)
2017-02-22 09:22 UTC, Marc Véron
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.98 KB, patch)
2017-02-24 08:43 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Manual Merged (4.41 KB, patch)
2017-03-21 08:53 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs (5.98 KB, patch)
2017-03-21 11:28 UTC, Baptiste
Details | Diff | Splinter Review
BUG 17708: Manual Merged (4.41 KB, patch)
2017-03-21 11:29 UTC, Baptiste
Details | Diff | Splinter Review
Bug 17708: Added a logaction function in Addrenewal in order to send renew logs (6.05 KB, patch)
2017-03-21 19:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17708: Add the IGNORE clause (1.00 KB, patch)
2017-03-21 19:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17708: Fix use statements (1.13 KB, patch)
2017-03-21 19:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17708: Rename 'RENEW' with 'RENEWAL' (2.56 KB, patch)
2017-03-21 19:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description vimal kumar 2016-12-01 16:31:56 UTC
Created attachment 57861 [details]
Renewal log

Koha version 16.05.5

Tools > Log > Circulation > Renewal does not display result.

Please check the screenshots.
Comment 1 Baptiste 2017-02-08 09:06:21 UTC
Created attachment 60007 [details] [review]
Added a logaction function in Addrenewal in order to send renew logs

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 2 vimal kumar 2017-02-14 10:39:41 UTC
This patch available with latest versions?
Comment 3 Marc Véron 2017-02-17 09:56:04 UTC
Created attachment 60383 [details] [review]
Added a logaction function in Addrenewal in order to send renew logs

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.

https://bugs.koha-community.org/show_bug.cgi?id=17708

Followed test plan, works as expected. Circulation.t passes OK
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 4 Marc Véron 2017-02-17 09:57:39 UTC
Note: Tested on current master, switching version to master.
Comment 5 Jonathan Druart 2017-02-20 08:49:56 UTC
Baptiste,
An action log should be tied to a syspref. Take a look at the different logaction calls in C4::Circulation.
Moreover it seems that the call is wrong, third arg should be the borrowernumber, not itemnumber (which should be the 4th I guess).
Comment 6 Jonathan Druart 2017-02-20 08:51:30 UTC
(In reply to vimal kumar from comment #2)
> This patch available with latest versions?

Patches are always provided for master, unless they fix a bug in a specific version.
Comment 7 Baptiste 2017-02-20 09:42:27 UTC
Created attachment 60454 [details] [review]
BUG 8010: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 8 Baptiste 2017-02-20 09:47:32 UTC
Created attachment 60455 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 9 Marc Véron 2017-02-20 10:14:31 UTC
Hi Babtiste,

There is a tiny typo in the prefs:
"when item are renewed. " (items)

Do not forget to switch back to Needs Signoff.

Marc
Comment 10 Baptiste 2017-02-20 10:20:38 UTC
Created attachment 60458 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 11 Baptiste 2017-02-20 10:25:39 UTC
Created attachment 60459 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 12 Marc Véron 2017-02-20 10:43:43 UTC
There is somethng more in bug_17708_add-RenewalLog.sql and sysprefs.sql

log information about reports - I think you mean: log information about renewals
Comment 13 Baptiste 2017-02-20 10:53:40 UTC
Created attachment 60460 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 14 Marc Véron 2017-02-22 09:22:21 UTC
Created attachment 60541 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.

https://bugs.koha-community.org/show_bug.cgi?id=17708

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 15 Jonathan Druart 2017-02-23 16:08:03 UTC
The test does not pass for me, I guess it is because you forget to mock pref.
What you need to do is:

  mock_preference('RenewalLog', 0)
  AddRenewal
  test that there is no log added
  mock_preference('RenewalLog', 1)
  AddRenewal
  test that 1 log has been added

IMO the test should be
  is ($new_log_size, $old_log_size +1, '');
which make more sense than
  is ($new_log_size - 1, $old_log_size, '');
Comment 16 Baptiste 2017-02-24 08:43:43 UTC
Created attachment 60641 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog
24/20/17 : added a test for the syspref Renewal Log

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 17 jmbroust 2017-03-20 09:57:20 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 17708:
<h1>Something went wrong !</h1>Applying: BUG 17708: Added a logaction function in Addrenewal in order to send renew logs
Using index info to reconstruct a base tree...
M C4/Circulation.pm
M installer/data/mysql/sysprefs.sql
M t/db_dependent/Circulation.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Circulation.pm
Patch failed at 0001 BUG 17708: Added a logaction function in Addrenewal in order to send renew logs
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.
error: Failed to merge in the changes.
Bug 17708 - Renewal log seems empty

60641 - BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

Apply? [(y)es, (n)o, (i)nteractive] Patch left in tmpBUG-17708-Added-a-logaction-function-in-Addrenewal-dBVGdR.patch .
Comment 18 Baptiste 2017-03-21 08:53:12 UTC
Created attachment 61353 [details] [review]
BUG 17708: Manual Merged

Merged to be up to date with master
Comment 19 julien.comte 2017-03-21 10:39:30 UTC
Some problems occurred applying patches from bug 17708:
<h1>Something went wrong !</h1>Applying: BUG 17708: Added a logaction function in Addrenewal in order to send renew logs
Using index info to reconstruct a base tree...
M       C4/Circulation.pm
M       installer/data/mysql/sysprefs.sql
M       t/db_dependent/Circulation.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Circulation.pm
Patch failed at 0001 BUG 17708: Added a logaction function in Addrenewal in order to send renew logs
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.
error: Failed to merge in the changes.
Bug 17708 - Renewal log seems empty

60641 - BUG 17708: Added a logaction function in Addrenewal in order to send renew logs
61353 - BUG 17708: Manual Merged

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/BUG-17708-Added-a-logaction-function-in-Addrenewal-4Iph8r.patch .
Comment 20 Baptiste 2017-03-21 11:28:58 UTC
Created attachment 61368 [details] [review]
BUG 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog
24/20/17 : added a test for the syspref Renewal Log

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Comment 21 Baptiste 2017-03-21 11:29:06 UTC
Created attachment 61369 [details] [review]
BUG 17708: Manual Merged

Merged to be up to date with master
Comment 22 julien.comte 2017-03-21 12:55:46 UTC
The renew appears when I set RenewalLog to 1.
Comment 23 Jonathan Druart 2017-03-21 19:37:25 UTC
Created attachment 61417 [details] [review]
Bug 17708: Added a logaction function in Addrenewal in order to send renew logs

20/02/17 : added the syspref RenewalLog
24/20/17 : added a test for the syspref Renewal Log

test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew

I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.

Signed-off-by: Julien Comte <julien.comte@u-psud.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2017-03-21 19:37:46 UTC
Created attachment 61418 [details] [review]
Bug 17708: Add the IGNORE clause

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2017-03-21 19:37:51 UTC
Created attachment 61419 [details] [review]
Bug 17708: Fix use statements

For an unknown reason, the use_ok('Circulation') does not work as
intended (see 3660c451a36d).
With the new use of C4::Log, the trick does no longer work.
It does not make sense to add the use_ok('C4::Log') in Circulation.t,
removing it.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2017-03-21 19:37:56 UTC
Created attachment 61420 [details] [review]
Bug 17708: Rename 'RENEW' with 'RENEWAL'

Sounds more appropriate and consistent with existing action logs.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Kyle M Hall 2017-03-31 13:18:55 UTC
Pushed to master for 17.05, thanks Baptiste, Jonathan!
Comment 28 Katrin Fischer 2017-04-02 17:08:12 UTC
This adds a new system preference and appears more like an enh to me.
Not pushing to 16.11.x.
Comment 29 Nick Clemens 2017-08-10 14:55:52 UTC
*** Bug 8752 has been marked as a duplicate of this bug. ***