Bug 40370 - about.pl should NOT say "Run the following SQL to fix the database"
Summary: about.pl should NOT say "Run the following SQL to fix the database"
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: About (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker
Assignee: David Cook
QA Contact: Emily Lamancusa (emlam)
URL:
Keywords: release-notes-needed
Depends on: 36039
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-15 00:55 UTC by David Cook
Modified: 2025-07-15 23:30 UTC (History)
2 users (show)

See Also:
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:


Attachments
Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting (1.53 KB, patch)
2025-07-15 01:17 UTC, David Cook
Details | Diff | Splinter Review
Bug 40370: Remove incorrect DB audit instruction and fix HTML formatting (1.60 KB, patch)
2025-07-15 13:29 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 40370: (QA follow-up) Change raw filter to html (1.13 KB, patch)
2025-07-15 13:29 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.)