Bug 28717 - NewsLog doesn't work
Summary: NewsLog doesn't work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22544 26205 28718
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-15 22:24 UTC by Lucas Gass
Modified: 2022-06-06 20:24 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch fixes a regression in the NewsLog caused by Bug 28718.
Version(s) released in:
21.11.00


Attachments
Bug 28717: Add logging logic to koha-news.pl (1.97 KB, patch)
2021-07-16 00:12 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28717: Add logging logic to koha-news.pl (3.82 KB, patch)
2021-07-16 16:03 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28717: Add logging logic to koha-news.pl (3.91 KB, patch)
2021-07-16 20:37 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28717: Add logging logic to koha-news.pl (3.96 KB, patch)
2021-07-19 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28717: Add logging logic to koha-news.pl (3.90 KB, patch)
2021-07-19 13:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
[21.05] Bug 28717: Add logging logic to koha-news.pl (3.95 KB, patch)
2021-07-19 14:21 UTC, Owen Leonard
Details | Diff | Splinter Review
[21.05] Bug 28717: (follow-up) add title to logged information (1.61 KB, patch)
2021-07-19 15:13 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28717: Fix additional content logs (5.57 KB, patch)
2021-08-18 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28717: Fix additional content logs (5.57 KB, patch)
2021-09-30 06:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28717: Fix additional content logs (5.75 KB, patch)
2021-09-30 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28717: Fix additional content logs (5.80 KB, patch)
2021-10-01 16:57 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28717: Fix additional content logs (5.87 KB, patch)
2021-10-02 22:06 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2021-07-15 22:24:03 UTC
The logging for News items added in Bug 26205 is broken. 

This was caused by Bug 22544
Comment 1 Lucas Gass 2021-07-16 00:12:54 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2021-07-16 07:21:55 UTC
Sorry about that, Lucas!

I think there is something missing in your patch, the $lang won't be set for DELETE. Can you have another look please?
Comment 3 Lucas Gass 2021-07-16 16:03:12 UTC Comment hidden (obsolete)
Comment 4 Lucas Gass 2021-07-16 16:12:33 UTC
(In reply to Jonathan Druart from comment #2)
> Sorry about that, Lucas!
> 
> I think there is something missing in your patch, the $lang won't be set for
> DELETE. Can you have another look please?

Thanks Jonathan! I submitted a new patch. Upon deletion I am just sending the ID to the logs. Do we need to also log the content of the deleted news item?
Comment 5 Owen Leonard 2021-07-16 19:54:06 UTC
(In reply to Lucas Gass from comment #4)

> Upon deletion I am just sending
> the ID to the logs. Do we need to also log the content of the deleted news
> item?

I think one of the major reasons to log news changes, especially deletions, is to offer a means of recovering a deleted entry. Because of that I think the full text of the news item should be logged.
Comment 6 Lucas Gass 2021-07-16 19:56:24 UTC
> I think one of the major reasons to log news changes, especially deletions,
> is to offer a means of recovering a deleted entry. Because of that I think
> the full text of the news item should be logged.


Yes, very true. Marking this as FQA and coming back with another patch ASAP :)
Comment 7 Lucas Gass 2021-07-16 20:37:42 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2021-07-19 10:56:28 UTC
Does not apply cleanly on top of Bug 28718 or master
Comment 9 Jonathan Druart 2021-07-19 12:16:45 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2021-07-19 13:13:32 UTC
When I try to delete multiple news entries at one time I get an error:

DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column '29' in 'where clause' [for Statement "DELETE FROM `opac_news` WHERE ( ( `29` = ? AND `idnew` = ? ) )" with ParamValues: 0="27", 1="44"] at /kohadevbox/koha/Koha/Objects.pm line 208
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

The deletions fail but are logged as if they were successful.
Comment 11 Jonathan Druart 2021-07-19 13:44:04 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2021-07-19 13:45:34 UTC
Sorry, it was caused by my rebase. I didn't get the conflict when I applied it this morning, it auto-solved using a previous resolution (that was wrong apparently!)
Comment 13 Owen Leonard 2021-07-19 14:21:58 UTC
Created attachment 122944 [details] [review]
[21.05] Bug 28717: Add logging logic to koha-news.pl

To test:
1. have some news items
2. make sure NewsLog is turned on
3. Add, modify, and delete some news
4. Check the action logs, nothing is logged
5. Apply patch
6. do step 3 again
7. now changes should be logged
8. make sure you try deleting multiple news items at once and confirm that all deletions are logged

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2021-07-19 14:22:36 UTC
This works well, so I've signed off. I'd love to see a follow-up which adds title to the logged information.
Comment 15 Lucas Gass 2021-07-19 15:13:04 UTC
Created attachment 122955 [details] [review]
[21.05] Bug 28717: (follow-up) add title to logged information
Comment 16 Jonathan Druart 2021-08-18 16:01:59 UTC
Created attachment 123964 [details] [review]
Bug 28717: Fix additional content logs

The logging for additional contents added by bug 26205 has been broken
by but 22544.

This patch is a revisited version as bug 24387 has been pushed.
It does not log MODIFY if no modification has been made on a template
(useful when only 1 version/lang of a content has been modified)

Test plan:
Turn on NewsLog
Add/modify and delete additional contents/News and confirm that
modification are logged.
Comment 17 Jonathan Druart 2021-08-18 16:03:20 UTC
We should actually log Dumper of the additional content object, but we need bug 28692 to prevent logging DateTime object.
Comment 18 Jonathan Druart 2021-08-18 16:05:42 UTC
(In reply to Jonathan Druart from comment #17)
> We should actually log Dumper of the additional content object, but we need
> bug 28692 to prevent logging DateTime object.

I meant bug 28700.
Comment 19 Jonathan Druart 2021-08-19 07:47:07 UTC
To clarify, only the last patch is for master.
Comment 20 Lucas Gass 2021-09-29 21:23:49 UTC
this patch, the one for master, no longer cleanly applies.
Comment 21 Jonathan Druart 2021-09-30 06:26:40 UTC
Created attachment 125471 [details] [review]
Bug 28717: Fix additional content logs

The logging for additional contents added by bug 26205 has been broken
by but 22544.

This patch is a revisited version as bug 24387 has been pushed.
It does not log MODIFY if no modification has been made on a template
(useful when only 1 version/lang of a content has been modified)

Test plan:
Turn on NewsLog
Add/modify and delete additional contents/News and confirm that
modification are logged.
Comment 22 Lucas Gass 2021-09-30 13:42:52 UTC
When I apply this patch and go to create a new HTML customization on additional-contents.pl I now get this error:

Can't call method "code" on an undefined value at /kohadevbox/koha/tools/additional-contents.pl line 123
Comment 23 Jonathan Druart 2021-09-30 14:41:49 UTC
Created attachment 125543 [details] [review]
Bug 28717: Fix additional content logs

The logging for additional contents added by bug 26205 has been broken
by but 22544.

This patch is a revisited version as bug 24387 has been pushed.
It does not log MODIFY if no modification has been made on a template
(useful when only 1 version/lang of a content has been modified)

Test plan:
Turn on NewsLog
Add/modify and delete additional contents/News and confirm that
modification are logged.
Comment 24 Jonathan Druart 2021-09-30 14:41:58 UTC
Sorry, Lucas!
Comment 25 Lucas Gass 2021-10-01 16:57:30 UTC
Created attachment 125637 [details] [review]
Bug 28717: Fix additional content logs

The logging for additional contents added by bug 26205 has been broken
by but 22544.

This patch is a revisited version as bug 24387 has been pushed.
It does not log MODIFY if no modification has been made on a template
(useful when only 1 version/lang of a content has been modified)

Test plan:
Turn on NewsLog
Add/modify and delete additional contents/News and confirm that
modification are logged.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 26 Katrin Fischer 2021-10-02 22:06:46 UTC
Created attachment 125659 [details] [review]
Bug 28717: Fix additional content logs

The logging for additional contents added by bug 26205 has been broken
by but 22544.

This patch is a revisited version as bug 24387 has been pushed.
It does not log MODIFY if no modification has been made on a template
(useful when only 1 version/lang of a content has been modified)

Test plan:
Turn on NewsLog
Add/modify and delete additional contents/News and confirm that
modification are logged.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Jonathan Druart 2021-10-06 08:37:31 UTC
Pushed to master for 21.11, thanks to everybody involved!