Bug 40370

Summary: about.pl should NOT say "Run the following SQL to fix the database"
Product: Koha Reporter: David Cook <dcook>
Component: AboutAssignee: David Cook <dcook>
Status: Pushed to main --- QA Contact: Emily Lamancusa (emlam) <emily.lamancusa>
Severity: blocker    
Priority: P5 - low CC: emily.lamancusa, lucas
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: String patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 36039    
Bug Blocks:    
Attachments: Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting
Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting
Bug 40370: (QA follow-up) Change raw filter to html

Description David Cook 2025-07-15 00:55:13 UTC
The database audit tool output is meant to be a guideline. It should not be treated as a database repair tool. That's why I included the original warning "WARNING!!! These commands are only suggestions".

For instance, if you had a plugin that added a table, the database audit tool would tell you to drop it. And then your Koha plugin is broken. Not a great outcome.
Comment 1 David Cook 2025-07-15 01:02:28 UTC
When I originally wrote the database audit, I thought about outputting the differences differently, but I thought SQL would be the easiest way for people to understand, and it was a simple method provided by SQL::Translator::Diff. Perhaps I should've known that people might not heed the warning.
Comment 2 David Cook 2025-07-15 01:12:22 UTC
Note that the impetus for raising this bug was someone on the koha-devel listserv saying they saw the "Warning! These commands are only suggestions" but then they saw "Run the following SQL to fix the database", so they ran the DB audit SQL as is.
Comment 3 David Cook 2025-07-15 01:17:44 UTC
Created attachment 184065 [details] [review]
Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting

This change removes the incorrect instruction from about.pl:
"Run the following SQL to fix the database" and fixes the HTML
formatting so that the line "-- Convert schema '' to '':;" is
displayed correctly at the start of the line.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/about.pl?tab=database
2. Note that "Run the following SQL to fix the database" no
longer appears and "--Convert schema '' to '':;" starts
at the beginning of the line instead of with leading whitespaces.
Comment 4 David Cook 2025-07-15 01:39:40 UTC
I just ran into a fun issue.

When you add a FOREIGN KEY in MySQL/MariaDB, it will silently add an index. So if you don't include an index explicitly in kohastructure.sql, the DB audit will mistakenly tell you to drop an index that should not be dropped.
Comment 5 Emily Lamancusa (emlam) 2025-07-15 13:29:39 UTC
Created attachment 184094 [details] [review]
Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting

This change removes the incorrect instruction from about.pl:
"Run the following SQL to fix the database" and fixes the HTML
formatting so that the line "-- Convert schema '' to '':;" is
displayed correctly at the start of the line.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/about.pl?tab=database
2. Note that "Run the following SQL to fix the database" no
longer appears and "--Convert schema '' to '':;" starts
at the beginning of the line instead of with leading whitespaces.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 6 Emily Lamancusa (emlam) 2025-07-15 13:29:41 UTC
Created attachment 184095 [details] [review]
Bug 40370: (QA follow-up) Change raw filter to html

The diff doesn't contain intentional HTML formatting, and pre tags do
not prevent un-escaped HTML from being interpreted, so we should not
use the raw filter here

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 7 Emily Lamancusa (emlam) 2025-07-15 13:30:36 UTC
This is a clearly reasonable string patch; moving straight to PQA.

...with a follow-up to change the $raw filter to html, as the diff doesn't contain intentional HTML formatting and it's recommended to escape HTML characters inside of <pre> tags (https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/pre#escaping_ambiguous_characters). I hope this isn't an overstep, and I'm happy to move the patch to a separate bug if it is! But it didn't feel like a good use of anyone's time to have two separate bugs to make two string edits to the same line. :)
Comment 8 Lucas Gass (lukeg) 2025-07-15 22:35:21 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 9 David Cook 2025-07-15 23:30:11 UTC
(In reply to Emily Lamancusa (emlam) from comment #7)
> ...with a follow-up to change the $raw filter to html, as the diff doesn't
> contain intentional HTML formatting and it's recommended to escape HTML
> characters inside of <pre> tags
> (https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/
> pre#escaping_ambiguous_characters). I hope this isn't an overstep, and I'm
> happy to move the patch to a separate bug if it is! But it didn't feel like
> a good use of anyone's time to have two separate bugs to make two string
> edits to the same line. :)

Thanks, Emily. Not an overstep at all. It was a great choice. The thought crossed my mind yesterday, but then I got side-tracked with other things. (Funny enough working on this bug showed me some DB issues with a different bug heh.)