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

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.

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.

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.

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.

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.

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.

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.

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

Created attachment 162463 [details] [review]
Bug 36039: New 'datatabase audit' tab in about page

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.

Hi David, fair points. I'd love to see a follow-up further improving this! Thank you!

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?

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>

Created attachment 166098 [details] [review]
Bug 36039: New 'datatabase audit' tab in about page

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

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>

Looking here

Note that Commenter also contains similar code (while only doing from the db comment perspective). Also reading structure etc.

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.

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>

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>

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>

Created attachment 169225 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 169226 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 169227 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Blocked by bug 36027 ... but that should be quick to resolve.

Created attachment 174403 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 174404 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 174405 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

I'm removing the dependency on bug 36027 here. And putting this back to PQA.
Test plan still works and patches apply after a minor rebase on a conflict caused by bug 37260.

This bug and bug 36027 don't relate to each other (audit_database.pl != search_for_data_inconsistencies.pl), and I believe the dependency was possibly added with plans to move bug 36027 logic into Koha::Database::Auditor. But can be it's own follow-up bug, of that bug. It should not block this.

(In reply to Pedro Amorim from comment #30)
> I'm removing the dependency on bug 36027 here. And putting this back to PQA.
> Test plan still works and patches apply after a minor rebase on a conflict
> caused by bug 37260.
> 
> This bug and bug 36027 don't relate to each other (audit_database.pl !=
> search_for_data_inconsistencies.pl), and I believe the dependency was
> possibly added with plans to move bug 36027 logic into
> Koha::Database::Auditor. But can be it's own follow-up bug, of that bug. It
> should not block this.

Sounds reasonable

I am hesitant to push this as is, as this will expose some useful, but also dangerous information to a lot more eyes. 

I'd like to work out some better display for the errors. This is unlikely to conflict, so I think we can take a little extra time to fix it up before push.


1) This includes some untranslatable strings that should be handled in the template:

+    print "Parsing schema for database '$database_name'\n" if $self->{is_cli};
+    print "Parsing schema for file $self->{filename} \n" if $self->{is_cli};

We can move this to a separate bug if that helps.


2) Make the message easier to understand for people without code knowledge.

+            <h3>WARNING!!!</h3>
+            <p>
+               These commands are only suggestions! They are not a replacement for updatedatabase.pl!<br />
+               Review the database, updatedatabase.pl, and kohastructure.sql before making any changes!<br />
+            </p>

We are referring to updatedatabase.pl here, but in package installations you will never encounter this script even as an update. Should we make the note more general maybe to make it also easier to understand?

Suggestion:

<p>
These commands are only suggestions. There are not a replacement for the database update scripts run during installations and updates
</p>
<p>
Review the database, the atomic update files and the table definitions in kohastructure.sql before making any changes.<p>

I think better to create a visual warn with formatting than with many !. I'd also reduce to Warning! but maybe make it bold instead.

<p>Following SQL statements might help in fixing your database. Review before use.</p>

3) Success message

            -- Convert schema '' to '':;

-- No differences found;

This doesn't look nicely formatted and also might be a bit confusing to some. Could we show some nice "All is well" type of note instead or at least remove the first line?

Created attachment 175933 [details] [review]
Bug 36039: (QA follow-up): Address i18n for print messages

Created attachment 175934 [details] [review]
Bug 36039: (QA follow-up): Improve audit messages

Improve messages clarity.
Show success message if no schema diff was found from audit.
Code refactor to ensure the title+message shown in UI is the same as the one shown in CLI and are all translatable.

Thanks for the follow-ups!

Fixed before push:
 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
   FAIL	  filters
		missing_filter at line 806 (            <h3><strong>[% db_audit.title %]</strong></h3>)
		missing_filter at line 807 (            <p><pre>[% db_audit.message %]</pre><p>)


I see you took me literally on the "All is well" :)

Pushed for 25.05!

Well done everyone, thank you!