Bug 28717 - NewsLog doesn't work
Summary: NewsLog doesn't work
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords: rel_21_11_candidate
Depends on: 22544 28718
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-15 22:24 UTC by Lucas Gass
Modified: 2021-07-29 06:31 UTC (History)
1 user (show)

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


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
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
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

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
Created attachment 122864 [details] [review]
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
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
Created attachment 122921 [details] [review]
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
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
Created attachment 122927 [details] [review]
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
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
Created attachment 122941 [details] [review]
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
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
Created attachment 122943 [details] [review]
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
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]
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]
Bug 28717: (follow-up) add title to logged information