Bug 36039 - Output of database_audit.pl should be accessible through UI
Summary: Output of database_audit.pl should be accessible through UI
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: About (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 32693 36027 34064
Blocks:
  Show dependency treegraph
 
Reported: 2024-02-08 12:06 UTC by Pedro Amorim
Modified: 2024-05-07 09:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 36039: PoC continuing from discussion in bug 34063. (2.39 KB, patch)
2024-02-08 12:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36039: Move audit_database script logic to a dedicated module (8.27 KB, patch)
2024-02-26 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36039: New 'datatabase audit' tab in about page (2.84 KB, patch)
2024-02-26 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36039: Move audit_database script logic to a dedicated module (8.33 KB, patch)
2024-05-03 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36039: New 'datatabase audit' tab in about page (2.80 KB, patch)
2024-05-03 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36039: (follow-up) Move view logic to script/template (7.73 KB, patch)
2024-05-03 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36039: Move audit_database script logic to a dedicated module (8.38 KB, patch)
2024-05-03 17:34 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 36039: New 'datatabase audit' tab in about page (2.85 KB, patch)
2024-05-03 17:34 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 36039: (follow-up) Move view logic to script/template (7.77 KB, patch)
2024-05-03 17:34 UTC, Jesse Maseto
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-02-08 12:06:21 UTC

    
Comment 1 Pedro Amorim 2024-02-08 12:07:21 UTC
Created attachment 161892 [details] [review]
Bug 36039: PoC continuing from discussion in bug 34063.

Test plan:
1) Apply patch
2) Install new dep:
  sudo cpanm HTML::TextToHTML
3) Restart plack and access /cgi-bin/koha/about.pl?tab=database
Comment 2 David Cook 2024-02-08 23:39:51 UTC
In hindsight, I should've put the code for "audit_database.pl" into a module. One of those things where a person is like "oh I'll just write a quick script for this..." and it becomes more useful than that heh.

Also, I don't think this PoC would work in a non-git environment, since "audit_database.pl" needs to be told where to find kohastructure.sql. 

So I reckon we refactor this to put it into a module (Koha::Database::Auditor perhaps), and then in about.pl we could find kohastructure.sql (using "intranetdir" from koha-conf.xml probably) and pass it as a parameter to Koha::Database::Auditor.
Comment 3 David Cook 2024-02-08 23:42:31 UTC
An additional note, I'm not a fan of shelling out. It's sometimes unavoidable when integrating with a third-party app, but since we're using our own code here I think we can do better. 

Koha also has a lot of dependencies already. I don't know if we want to add another one just for this. HTML::TextToHTML is unmaintained, and I think we could manage without it.
Comment 4 David Cook 2024-02-08 23:43:36 UTC
I am stoked for bug 32693 and this one though. I'm keeping it on my TODO list.

I think having it in the About will also help motivate people to fix things. Sometimes when this information stays hidden on the sysadmin side of things, it's really easy to ignore. But if you have a web admin go to that tab and say "oh hey my database is our of sync..." it's more likely to get fixed sooner.
Comment 5 Pedro Amorim 2024-02-09 16:50:49 UTC
Hi David, that's awesome, I broadly agree with everything you said. Yes the PoC would not work in a non-git environment as it was designed only to be a platform for the discussion we're now having.

I also strongly agree that having these scripts/reports accessible through the UI will make this info more readily available to every Staff member, not just sys admins, thus likely resulting in a faster call to action to fixing/preventing bugs in individual systems due to database issues.

I'm a big fan of the Koha::Database::Auditor idea, and would like to hear what others also think about this.
Comment 6 David Nind 2024-02-09 19:25:29 UTC
While I'm not familiar with the script this is based on, I think it is a good idea.

I'm assuming it is a good tool to use before an upgrade, to make sure that there are no potential issues (as far as this is possible to do before an upgrade).

After an upgrade, it might not be accessible if the upgrade doesn't complete.

Happy to help with the messages displayed so that they are more staff friendly.

I think the tab might be better placed after the system information tab.
Comment 7 Martin Renvoize 2024-02-15 10:34:02 UTC
I'm always happy to see things moved into modules from scripts.. it makes them more easily accessible for re-use and testable.

I also agree, it's a good move to lower the requirements for further dependancies.
Comment 8 David Cook 2024-02-15 22:27:41 UTC
I can look at refactoring audit_database.pl later. Got some other priorities at the moment.

I was just thinking in terms of showing the output in the web UI... I wonder if there are security implications there. In theory, it could allow someone with staff access to see if an attack has managed to change the database schema. But I suppose our concern should be more with stopping attacks. Just some food for thought.
Comment 9 Pedro Amorim 2024-02-26 16:44:47 UTC
Created attachment 162462 [details] [review]
Bug 36039: Move audit_database script logic to a dedicated module

Test plan #1 ensure same script functionality (same as bug 34064):
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Test plan #2:
1. After doing test plan #1, access the new 'database audit' tab, visit:
<staff_url>/cgi-bin/koha/about.pl?tab=database
2. Ensure the output is the same as test plan #1
Comment 10 Pedro Amorim 2024-02-26 16:44:49 UTC
Created attachment 162463 [details] [review]
Bug 36039: New 'datatabase audit' tab in about page
Comment 11 David Cook 2024-02-26 23:14:06 UTC
Thanks for taking this on, Pedro. I've been super busy, so I really appreciate it.

(I probably would've done it a bit differently. Instead of passing a "is_cli" parameter to Koha::Database::Auditor, I would just return a data structure, and then based on whether it's about.pl or audit_database.pl handle the output accordingly. As for the printing of strings within functions, that could be done using a logger object. But I don't think it's the end of the world keeping it as is.)

Once again thanks for taking this on. I've still got a lot on, but I'm keeping my eye on this one, and will come back once I have some time if folk haven't already gone through it by then.
Comment 12 Pedro Amorim 2024-02-27 09:20:34 UTC
Hi David, fair points. I'd love to see a follow-up further improving this! Thank you!
Comment 13 Pedro Amorim 2024-02-29 09:27:42 UTC
search_for_data_inconsistencies.pl could also be merged into to this Koha::Database::Auditor.
That cron should also be invoking methods from the module instead of having logic built-in.

This new "Database" tab in about page could be extended to include these data inconsistencies on top of database diff, perhaps with async actions, or else that tab would take very long to render.

Thoughts?
Comment 14 Martin Renvoize 2024-05-03 09:25:27 UTC
Created attachment 166097 [details] [review]
Bug 36039: Move audit_database script logic to a dedicated module

Test plan #1 ensure same script functionality (same as bug 34064):
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Test plan #2:
1. After doing test plan #1, access the new 'database audit' tab, visit:
<staff_url>/cgi-bin/koha/about.pl?tab=database
2. Ensure the output is the same as test plan #1

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2024-05-03 09:25:30 UTC
Created attachment 166098 [details] [review]
Bug 36039: New 'datatabase audit' tab in about page

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2024-05-03 09:25:33 UTC
Created attachment 166099 [details] [review]
Bug 36039: (follow-up) Move view logic to script/template

This patch updates the module as suggested by David to handle just the
diff code and leaves the display logic up to the templates and script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Marcel de Rooy 2024-05-03 09:39:50 UTC
Looking here
Comment 18 Marcel de Rooy 2024-05-03 09:40:42 UTC
Note that Commenter also contains similar code (while only doing from the db comment perspective). Also reading structure etc.
Comment 19 Marcel de Rooy 2024-05-03 09:58:04 UTC
Looks very nice! Maybe creating another diff display and showing the alter commands less prominently would be an improvement to think about still

Hard to require tests here. Structure changes imply commits etc.
Comment 20 Jesse Maseto 2024-05-03 17:34:49 UTC
Created attachment 166155 [details] [review]
Bug 36039: Move audit_database script logic to a dedicated module

Test plan #1 ensure same script functionality (same as bug 34064):
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Test plan #2:
1. After doing test plan #1, access the new 'database audit' tab, visit:
<staff_url>/cgi-bin/koha/about.pl?tab=database
2. Ensure the output is the same as test plan #1

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: JesseM <jesse@bywatersolutions.com>
Comment 21 Jesse Maseto 2024-05-03 17:34:52 UTC
Created attachment 166156 [details] [review]
Bug 36039: New 'datatabase audit' tab in about page

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: JesseM <jesse@bywatersolutions.com>
Comment 22 Jesse Maseto 2024-05-03 17:34:55 UTC
Created attachment 166157 [details] [review]
Bug 36039: (follow-up) Move view logic to script/template

This patch updates the module as suggested by David to handle just the
diff code and leaves the display logic up to the templates and script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: JesseM <jesse@bywatersolutions.com>