|
Description
Lucas Gass (lukeg)
2021-07-15 22:24:03 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 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? 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 (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? (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.
> 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 :)
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 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 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. 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 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!) 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> This works well, so I've signed off. I'd love to see a follow-up which adds title to the logged information. Created attachment 122955 [details] [review] [21.05] Bug 28717: (follow-up) add title to logged information 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. We should actually log Dumper of the additional content object, but we need bug 28692 to prevent logging DateTime object. (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. To clarify, only the last patch is for master. this patch, the one for master, no longer cleanly applies. 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. 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 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. Sorry, Lucas! 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> 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> Pushed to master for 21.11, thanks to everybody involved! |