|
Description
Indranil Das Gupta
2015-05-20 07:07:29 UTC
This would also be a great use case for the compare prefs patch that Owen has worked on: Bug 26207 - Compare values of system preference log entries With COVID-19 and going fines free we had to make multiple changes to notices text as well as stopping notices for awhile so it would be very helpful to have logging for notices. Also agree that the compare prefs patch would be helpful with notices. *** Bug 27602 has been marked as a duplicate of this bug. *** At a bare minimum, we could add it to the action_logs, no? (In reply to David Cook from comment #4) > At a bare minimum, we could add it to the action_logs, no? Indeed, that's what I though to do. However, it will need a little imaginative thinking was the table structure doesn't lend itself to the code + category approach in the letter table. I actually do use action logs in the database update for bug 26734. Created attachment 116649 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Created attachment 116650 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Created attachment 116651 [details] [review] Bug 14233: Add logging to notice creation/update actions This is a proof of concept right now... just looking for some feedback. Created attachment 116660 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Created attachment 116661 [details] [review] Bug 14233: Add logging to notice creation/update actions This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Created attachment 116662 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Created attachment 116668 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Created attachment 116671 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Created attachment 116672 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Created attachment 116673 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Created attachment 116674 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Note, to test this you will need to run a dbic schema update (In reply to Martin Renvoize from comment #15) > Created attachment 116672 [details] [review] [review] > Bug 14233: Add logging to notice create/update/delete > > This patch adds the call to logaction to record when a notice is added > or changed. > > Test plan > 1/ Add a new notice and check a corresponding line appears in the > action_logs table. At this stage I get an error: C4::Log::logaction(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: 'reserves' for column 'object' at row 1 [for Statement "Insert into action_logs (timestamp,user,module,action,object,info,interface) values (now(),?,?,?,?,?,?)" with ParamValues: 0="20375", 1='NOTICES', 2='CREATE', 3="reserves", 4="Lorem ipsum", 5='Intranet'] at /kohadevbox/koha/tools/letter.pl line 346 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 The notice gets added but I don't see anything in the log. Created attachment 116713 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update Hmm, I can't seem to replicate that here... I've uploaded a DO NOT PUSH schema patch to make testing easier.. just in case it was a missing dbic schema dump that caused the problem.? Created attachment 116760 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Created attachment 116761 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update I had a go at testing this and a few things I noted: 1. The link in the log viewer 'Object' column doesn't go to the actual notice - it just lists the notices. This is because there are two ?? after letter.pl (for example: /cgi-bin/koha/tools/letter.pl??op=add_form&branchcode=&module=members&code=A_NOTICE_2&lang=default) 2. I'm not sure whether changes to the notice name should be logged or not - if I change the notice name it doesn't show in the log. I'm therefore assuming that only actual changes to the notice itself are logged. 3. The changes logged - I'm not sure whether this is the correct behaviour, but assume it is: . If I make a change to a notice, in the log viewer 'Info' column it shows what it was before the change was made. . If that is what is expected, then it is working - you can only compare previous changes, but not to the current version of the notice. 4. I could add new notices, the change was logged, and I didn't get the error the Owen did in Comment 19, presumably because of the follow up patch. 5. Is it possible to add the notice code somewhere? Maybe in front of the notice name in the log viewer's 'Object' column. Maybe a future enhancement? 6. I was confused that there is already a 'Letter' module listed (before and after the patch is applied). Is the 'Letter' module something different from the 'Notices' module, or the same - I prefer notices as it matches up with the name in the tools section. Sorry about all the questions! Testing notes (using koha-testing-docker): - Apply patches and then updatedatabase, dbic, flush_memcached, restart_all - MySQL commands for database table checks in test plans: . koha-mysql kohadev . show columns from letter; . show index from letter; Created attachment 117251 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Created attachment 117252 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Created attachment 117253 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Created attachment 117254 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Created attachment 117255 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Created attachment 117256 [details] [review] Bug 14233: (QA follow-up) Fix typo in viewlog.tt We had a double ?? in the object link.. Thanks for spotting that David :) Created attachment 117257 [details] [review] Bug 14233: (QA follow-up) Record updated content We should record the updated template, not the content prior to the update.. That way we can properly compare between versions to see the changes made. Created attachment 117258 [details] [review] Bug 14233: (follow-up) Add notice code to log viewer Created attachment 117259 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update (In reply to David Nind from comment #24) Great feedback David, thanks for testing :) > 1. The link in the log viewer 'Object' column doesn't go to the actual > notice - it just lists the notices. This is because there are two ?? after > letter.pl (for example: > /cgi-bin/koha/tools/letter. > pl??op=add_form&branchcode=&module=members&code=A_NOTICE_2&lang=default) Good catch, fixed in the first QA follow-up. > 2. I'm not sure whether changes to the notice name should be logged or not - > if I change the notice name it doesn't show in the log. I'm therefore > assuming that only actual changes to the notice itself are logged. I didn't add logging for title changes at this time, but perhaps that could be added too.. I wasn't sure of the best format to take though and felt at the moment it was more important to expose the template changes diff view than record the title changes and lose the diff view. Perhaps a later followup bug :) > 3. The changes logged - I'm not sure whether this is the correct behaviour, > but assume it is: > . If I make a change to a notice, in the log viewer 'Info' column it > shows what it was before the change was made. > . If that is what is expected, then it is working - you can only compare > previous changes, but not to the current version of the notice. Oops, this is a mistake on my part, it should be the 'after' so we can do the comparison with current and prior. I've corrected this in the second QA follow-up. > 4. I could add new notices, the change was logged, and I didn't get the > error the Owen did in Comment 19, presumably because of the follow-up patch. Correct, that was the earlier follow-up :) > 5. Is it possible to add the notice code somewhere? Maybe in front of the > notice name in the log viewer's 'Object' column. Maybe a future enhancement? Done, added in the 'follow-up'. > 6. I was confused that there is already a 'Letter' module listed (before and > after the patch is applied). Is the 'Letter' module something different from > the 'Notices' module, or the same - I prefer notices as it matches up with > the name in the tools section. I hadn't even noticed the existing 'Letter' filtering.. when I dug into it, it seems that it's linked to the acquisitions claims emails. I wonder if we could rename it to 'Claims' in the UI to clarify it? This is working as expected however, I think a System Preference for Logging of Notices is important. From what I can see, these logs do not have an option of being turned off since they are not in the Logging System preferences. Can this be added? (In reply to Kelly McElligott from comment #35) > This is working as expected however, I think a System Preference for Logging > of Notices is important. From what I can see, these logs do not have an > option of being turned off since they are not in the Logging System > preferences. > > Can this be added? I considered this at the time but then took president from the preferences log.. changing preferences is always logged as far as I can tell without any ability to turn that off... I felt notices should fall into the same category.. they are unlikely to be changed often, but when they are changed logging is rather important.. Do you believe they should be preference driven then.. and if so what should we default to.. logging or not logging? Hi, Thanks for the quick response. Talking to the other educators, we would like to keep it consistent with a system preference to allow the ability to turn it on/off. I don't know what logging is set to ON by default? Created attachment 117294 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Created attachment 117295 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Created attachment 117296 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Created attachment 117297 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Created attachment 117298 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Created attachment 117299 [details] [review] Bug 14233: (QA follow-up) Fix typo in viewlog.tt We had a double ?? in the object link.. Thanks for spotting that David :) Created attachment 117300 [details] [review] Bug 14233: (QA follow-up) Record updated content We should record the updated template, not the content prior to the update.. That way we can properly compare between versions to see the changes made. Created attachment 117301 [details] [review] Bug 14233: (follow-up) Add notice code to log viewer Created attachment 117302 [details] [review] Bug 14233: (follow-up) Clarify 'LetterLog' The LetterLog logging wasn't clear as to what it was logging, it actually logs aquisitions related notices being sent.. mostly 'Claims'. This patch updates LetterLog to ClaimsLog to more clearly reflect what it means. (I also discovered that there was a bug in the MODULE these were being logged as.. they were in fact being logged as 'ACQUISITION' and not 'LETTER' in the first place.. and as such not filtering correctly on the viewlog page). Created attachment 117303 [details] [review] Bug 14233: (follow-up) Add NoticesLog preference This patch adds a preference to enable/disable the notice template logging. Created attachment 117304 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update Created attachment 117311 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117312 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117313 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117314 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117315 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117316 [details] [review] Bug 14233: (QA follow-up) Fix typo in viewlog.tt We had a double ?? in the object link.. Thanks for spotting that David :) Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117317 [details] [review] Bug 14233: (QA follow-up) Record updated content We should record the updated template, not the content prior to the update.. That way we can properly compare between versions to see the changes made. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117318 [details] [review] Bug 14233: (follow-up) Add notice code to log viewer Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117319 [details] [review] Bug 14233: (follow-up) Clarify 'LetterLog' The LetterLog logging wasn't clear as to what it was logging, it actually logs aquisitions related notices being sent.. mostly 'Claims'. This patch updates LetterLog to ClaimsLog to more clearly reflect what it means. (I also discovered that there was a bug in the MODULE these were being logged as.. they were in fact being logged as 'ACQUISITION' and not 'LETTER' in the first place.. and as such not filtering correctly on the viewlog page). Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117320 [details] [review] Bug 14233: (follow-up) Add NoticesLog preference This patch adds a preference to enable/disable the notice template logging. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 117321 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Thanks Martin for all the follow ups and the responses to questions! I've retested (and Kelly has just signed off!), there is one more thing I noticed: - I added a notice, made some changes, then deleted. - These all show up in the log. - After deleting a notice, the Object column 'name' is changed to the ID (which makes sense, as it is deleted and doesn't know the name). - If I click on compare for a deleted item it doesn't filter down to all the items with the same ID. The 'Message Log' in the browser shows 'Showing results for' and after that it is blank, with a non-deleted item it shows as 'Showing results for <object name>' with the name of the object showing up in the search box. - I can still compare by put the ID in the data table search box, then the compare works as expected. David Created attachment 117345 [details] [review] Bug 14233: (QA follow-up) Fix filtering issue As highlighted by David, the filtering was broken for the case where a notice had been deleted. This patch resolves that issue and also adds in the 'Log not enabled' display when the logs are not enabled. Thanks for testing again guys, and nice work spotting that David... I've added another followup that should hopefully resolve that final issue :) Thanks Martin! Can confirm that that fixes things. Like the little icon saying 'Log not enabled'. Created attachment 118132 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118133 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118134 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118135 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118136 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118137 [details] [review] Bug 14233: (QA follow-up) Fix typo in viewlog.tt We had a double ?? in the object link.. Thanks for spotting that David :) Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118138 [details] [review] Bug 14233: (QA follow-up) Record updated content We should record the updated template, not the content prior to the update.. That way we can properly compare between versions to see the changes made. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118139 [details] [review] Bug 14233: (follow-up) Add notice code to log viewer Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118140 [details] [review] Bug 14233: (follow-up) Clarify 'LetterLog' The LetterLog logging wasn't clear as to what it was logging, it actually logs aquisitions related notices being sent.. mostly 'Claims'. This patch updates LetterLog to ClaimsLog to more clearly reflect what it means. (I also discovered that there was a bug in the MODULE these were being logged as.. they were in fact being logged as 'ACQUISITION' and not 'LETTER' in the first place.. and as such not filtering correctly on the viewlog page). Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118141 [details] [review] Bug 14233: (follow-up) Add NoticesLog preference This patch adds a preference to enable/disable the notice template logging. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118142 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118143 [details] [review] Bug 14233: (QA follow-up) Fix filtering issue As highlighted by David, the filtering was broken for the case where a notice had been deleted. This patch resolves that issue and also adds in the 'Log not enabled' display when the logs are not enabled. Signed-off-by: David Nind <david@davidnind.com> Created attachment 118155 [details] [review] Bug 14233: Add 'id' field to `letter` table to allow linking To allow for 'object' linking from the action_logs table we need an int(11) identifier. This patch updates the letter table to add such a field as a replacement for the multi-field primary key and moves that key to a unique constraint. Test plan 1/ Run the database update 2/ Check that the 'letter' table now includes an 'id' primary key and 'letter_uniq_1' multi-field unique key 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118156 [details] [review] Bug 14233: Add logging to notice create/update/delete This patch adds the call to logaction to record when a notice is added or changed. Test plan 1/ Add a new notice and check a corresponding line appears in the action_logs table. 2/ Update a notice and check that a corresponding line appears in the action_logs table. 3/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118157 [details] [review] Bug 14233: Add filtering of NOTICES type action logs to the logviewer This patch adds handling for the new 'NOTICES' type action_logs to the logviewer page. Test plan 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 5/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118158 [details] [review] Bug 14233: Allow comparing notice changes This patch adds the 'compare' option to the Notices logs. Test plan 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. 7/ Signoff Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118159 [details] [review] Bug 14233: (follow-up) Fix for delete entire notice This fixes the overall notice delete option. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118160 [details] [review] Bug 14233: (QA follow-up) Fix typo in viewlog.tt We had a double ?? in the object link.. Thanks for spotting that David :) Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118161 [details] [review] Bug 14233: (QA follow-up) Record updated content We should record the updated template, not the content prior to the update.. That way we can properly compare between versions to see the changes made. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118162 [details] [review] Bug 14233: (follow-up) Add notice code to log viewer Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118163 [details] [review] Bug 14233: (follow-up) Clarify 'LetterLog' The LetterLog logging wasn't clear as to what it was logging, it actually logs aquisitions related notices being sent.. mostly 'Claims'. This patch updates LetterLog to ClaimsLog to more clearly reflect what it means. (I also discovered that there was a bug in the MODULE these were being logged as.. they were in fact being logged as 'ACQUISITION' and not 'LETTER' in the first place.. and as such not filtering correctly on the viewlog page). Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118164 [details] [review] Bug 14233: (follow-up) Add NoticesLog preference This patch adds a preference to enable/disable the notice template logging. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118165 [details] [review] Bug 14233: [DO NOT PUSH] DBIC Schema File Update Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118166 [details] [review] Bug 14233: (QA follow-up) Fix filtering issue As highlighted by David, the filtering was broken for the case where a notice had been deleted. This patch resolves that issue and also adds in the 'Log not enabled' display when the logs are not enabled. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 118167 [details] [review] Bug 14233: (QA follow-up) Fix template filters Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I think we should move the "clarify 'LetterLog'" patch to a separate bug report. I don't think "claims" should be a "module" and it will delay these patches if we have the discussion now. (In reply to Jonathan Druart from comment #89) > I don't think "claims" should be a "module" and it will delay these patches > if we have the discussion now. Katrin, Kelly, what's your opinion on this? I agree Jonathan. I can provide a followup to change from 'Claims' to 'Acquisitions' which is the module claims sits in.. I think that would be the cleanest approach. Currently, without this patch here the functionality is broken. Created attachment 118323 [details] [review] Bug 14233: (QA follow-up) Claims -> Acquisitions Change the CLAIMS and ClaimsLog strings to ACQUSITIONS and AcquisitionsLog to clarify it's module level. The code is not nice to read, but I believe AcquisitionsLog is wrong here, because we also send serial claims in this part of the code (claimissues). So I think we should really leave the clarification to a separate patch as suggested. NoticesLog makes sense for all changes done in Tools > Notices and slips. We should only deal with that here, the other thing is a separate issue and looks like it requires careful untangling. It's also something we should not "hide" here so it's easy to spot in release notes. One issue at a time! (In reply to Martin Renvoize from comment #92) > I can provide a followup to change from 'Claims' to 'Acquisitions' which is > the module claims sits in.. I think that would be the cleanest approach. > Currently, without this patch here the functionality is broken. Why is the funtionality broken without the patch? (probably missing something from previous discussion as I am not sure how we ended up with wanting to rename it here in the first place) (In reply to Katrin Fischer from comment #95) > (In reply to Martin Renvoize from comment #92) > > I can provide a followup to change from 'Claims' to 'Acquisitions' which is > > the module claims sits in.. I think that would be the cleanest approach. > > Currently, without this patch here the functionality is broken. > > Why is the funtionality broken without the patch? (probably missing > something from previous discussion as I am not sure how we ended up with > wanting to rename it here in the first place) See comment #34 - you can blame me! (And Martin's follow up patch) 8-) What needs doing to get this moving again.. :( ? (In reply to Martin Renvoize from comment #97) > What needs doing to get this moving again.. :( ? Please answer comment 94. Can you also clarify why we cannot stick to the NoticesLog changes only on this bug report? Created attachment 119291 [details] [review] Bug 14233: DBIC schema changes Created attachment 119292 [details] [review] Bug 14233: DB changes - NoticesLog pref + letter.id PK Created attachment 119293 [details] [review] Bug 14233: Add logging support to notices and slips management This enhancements adds support for logging changes to notices and slips. Features include: - a system preference (NoticesLog) which allows you to enable logging changes for notices and slips - logging new notices, changes to notices, and deletion of notices - filtering in the log viewer so you can show all changes for a specific notice - a comparison feature, so you can see what changes were made to a notice. Test plan: 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. Created attachment 119294 [details] [review] Bug 14233: Select the correct tab Trying to unblock the situation. I squashed the patches and remove the changes related to LetterLog. So basically we will now have the same behaviour we have in master for claims/LetterLog and a new NoticesLog pref will drive the CRUD for notice templates. Back to Needs Signoff. Created attachment 119312 [details] [review] Bug 14233: DBIC schema changes Signed-off-by: David Nind <david@davidnind.com> Created attachment 119313 [details] [review] Bug 14233: DB changes - NoticesLog pref + letter.id PK Signed-off-by: David Nind <david@davidnind.com> Created attachment 119314 [details] [review] Bug 14233: Add logging support to notices and slips management This enhancements adds support for logging changes to notices and slips. Features include: - a system preference (NoticesLog) which allows you to enable logging changes for notices and slips - logging new notices, changes to notices, and deletion of notices - filtering in the log viewer so you can show all changes for a specific notice - a comparison feature, so you can see what changes were made to a notice. Test plan: 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. Signed-off-by: David Nind <david@davidnind.com> Created attachment 119315 [details] [review] Bug 14233: Select the correct tab Signed-off-by: David Nind <david@davidnind.com> Comment on attachment 119315 [details] [review] Bug 14233: Select the correct tab Review of attachment 119315 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt @@ +340,4 @@ > <a href="/cgi-bin/koha/authorities/detail.pl?authid=[% loopro.object | uri %]" title="Display detail for this authority">Authority [% loopro.object | html %]</a> > [% ELSIF ( loopro.module == 'NOTICES' ) %] > [% IF ( loopro.notice ) %] > + <a href="/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=[% loopro.notice.branchcode | uri %]&module=[% loopro.notice.module | uri %]&code=[% loopro.notice.code | uri %]#lang_[% loopro.notice.lang | uri %]">Notice [% loopro.notice.title | html %] ([% loopro.notice.code | html %])</a> I don't understand this change. & => # (In reply to Martin Renvoize from comment #108) > Comment on attachment 119315 [details] [review] [review] > Bug 14233: Select the correct tab > > Review of attachment 119315 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt > @@ +340,4 @@ > > <a href="/cgi-bin/koha/authorities/detail.pl?authid=[% loopro.object | uri %]" title="Display detail for this authority">Authority [% loopro.object | html %]</a> > > [% ELSIF ( loopro.module == 'NOTICES' ) %] > > [% IF ( loopro.notice ) %] > > + <a href="/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=[% loopro.notice.branchcode | uri %]&module=[% loopro.notice.module | uri %]&code=[% loopro.notice.code | uri %]#lang_[% loopro.notice.lang | uri %]">Notice [% loopro.notice.title | html %] ([% loopro.notice.code | html %])</a> > > I don't understand this change. & => # if you are editing es-ES, on the log viewer the link was not redirecting you the the letter.pl es-ES tab, but the default one instead Created attachment 119359 [details] [review] Bug 14233: DBIC schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119360 [details] [review] Bug 14233: DB changes - NoticesLog pref + letter.id PK Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119361 [details] [review] Bug 14233: Add logging support to notices and slips management This enhancements adds support for logging changes to notices and slips. Features include: - a system preference (NoticesLog) which allows you to enable logging changes for notices and slips - logging new notices, changes to notices, and deletion of notices - filtering in the log viewer so you can show all changes for a specific notice - a comparison feature, so you can see what changes were made to a notice. Test plan: 1/ Add some 'Notice' action logs by adding a new notice or updateing an existing notice 2/ Go to the 'Log viewer' under 'Tools' 3/ Note you can filter by 'Notices' 4/ Confirm the changes you made appear in the logs and that the 'Object' field links back to the updated notice ready for editing. 1/ Make some changes to at least two different notices to trigger logs 2/ Go to the 'Log viewer' from 'Tools' and filter on 'Notices' 3/ Note, you can now see the 'Compare' checkbox option for these logs 4/ Select the checkbox on one of the notices and note that the results are now filtered by that specific notice. 5/ Select a second checkbox to expose the option to 'View comparison' 6/ Click 'View comparison' to see the 'Diff view' of the two notices. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119362 [details] [review] Bug 14233: Select the correct tab Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Pushed to master for 21.05, thanks to everybody involved! Created attachment 119510 [details] [review] Bug 14233: Set default for NoticesLog to 0 To prevent a warning on the about page: System preference 'NoticesLog' must be '0' or '1', but is ''. Follow-up pushed to master. Enhancement not pushed to 20.11.x |