Bug 32334

Summary: Sync comments in database with schema
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: DatabaseAssignee: Marcel de Rooy <m.de.rooy>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, david, jacob.omara, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26947
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31028
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00
Bug Depends on:    
Bug Blocks: 30486, 34843, 33584    
Attachments: Bug 32334: Exceptions - Add parameter to WrongParameter
Bug 32334: Add general exception FileNotFound
Bug 32334: Introduce Koha::Database::Commenter
Bug 32334: Add sync_db_comments script
Bug 32334: Exceptions - Add parameter to WrongParameter
Bug 32334: Add general exception FileNotFound
Bug 32334: Introduce Koha::Database::Commenter
Bug 32334: Add sync_db_comments script
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter
Bug 32334: (QA follow-up) Remove unused modules
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages
Bug 32334: (QA follow-up) Improve finding schema file
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS
Bug 32334: (follow-up) Add the -schema parameter to script too
Bug 32334: (follow-up) Improvement for schema parameter
Bug 32334: Exceptions - Add parameter to WrongParameter
Bug 32334: Add general exception FileNotFound
Bug 32334: Introduce Koha::Database::Commenter
Bug 32334: Add sync_db_comments script
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter
Bug 32334: (QA follow-up) Remove unused modules
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages
Bug 32334: (QA follow-up) Improve finding schema file
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS
Bug 32334: (follow-up) Improvement for schema parameter
Bug 32334: Exceptions - Add parameter to WrongParameter
Bug 32334: Add general exception FileNotFound
Bug 32334: Introduce Koha::Database::Commenter
Bug 32334: Add sync_db_comments script
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter
Bug 32334: (QA follow-up) Remove unused modules
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages
Bug 32334: (QA follow-up) Improve finding schema file
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS
Bug 32334: (follow-up) Improvement for schema parameter
Bug 32334: Fix spelling

Description Marcel de Rooy 2022-11-23 12:22:58 UTC
More info follows.
Comment 1 Marcel de Rooy 2022-12-02 10:50:32 UTC
Created attachment 144391 [details] [review]
Bug 32334: Exceptions - Add parameter to WrongParameter

Test plan:
Run t/Koha/Exceptions.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-12-02 10:50:36 UTC
Created attachment 144392 [details] [review]
Bug 32334: Add general exception FileNotFound

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-12-02 10:50:40 UTC
Created attachment 144393 [details] [review]
Bug 32334: Introduce Koha::Database::Commenter

Test plan:
[1] Make a backup of your database. Just for precaution.
[2] Run t/db_dependent/Koha/Database/Commenter.t.
* Note: This test script adds and removes a test table.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2022-12-02 10:50:44 UTC
Created attachment 144394 [details] [review]
Bug 32334: Add sync_db_comments script

Test plan:
[1] Backup your database, if not done already.
[2] Check output of dry_run when clearing a table:
    misc/maintenance/sync_db_comments.pl -clear -table items -dry
[3] Save output of misc/devel/update_dbix_class_files before changing
    comments in order to compare later. (Commit your changes.)
    You may not have changes after running (at least on a fresh
    database). That's fine.
[4] Clear all comments:
    misc/maintenance/sync_db_comments.pl -clear
[5] Renumber all comments:
    misc/maintenance/sync_db_comments.pl -renum
[6] Reset all comments to schema. Make sure that script finds your
    structure in installer/data/mysql folder.
    misc/maintenance/sync_db_comments.pl -reset
[7] Run update_dbix_class_files again and inspect changes as compared
    to previous run.
    Can you explain them? You should only see changes related to
    column comments. If you did not have changes in step 3, you
    should not have them here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2022-12-02 10:56:52 UTC
A few excerpts from the POD of the new module and script:

===
SYNOPSIS
        perl sync_db_comments.pl [-h] [-v] [-database DB_NAME] [-table TABLE_NAME] [-dry_run] [-clear|-reset|-renumber]

        Synchronize column comments in database with Koha schema. Allows you
        to clear comments too. Operates additionally on specific tables only.
        And provides a dry run mode that prints sql statements.

        This script may prove helpful to track synchronization issues between
        Koha schema and actual database structure due to inconsistencies in
        database revisions. It reduces the noise from missing column comments
        when running script update_dbix_class_files.pl.

and

        The advantage of keeping in sync is that you can easily track differences
        between schema and database with the maintenance script
        update_dbix_class_files.pl.

===

Yes, it adds another module. But keeping track of schema issues is imo important enough. The noise of missing or changed comments can be handled in various ways. This approach was the most practical one for me in the end.
Comment 6 Jonathan Druart 2023-01-13 10:53:08 UTC
1. > The flags -clear, -reset and -renumber are mutually exclusive.

Then I think the script should catch this situation.

2. I think we discussed that already, but there is more --confirm flags than --dry-run in our scripts. It's confusing to have both.

3. You should remove references to 'verbose' in the module

4. Path to structure depend on cwd, we can do better.

% cd misc/maintenance
% perl sync_db_comments.pl --database koha_kohadev --reset
Exception 'Koha::Exceptions::FileNotFound' thrown 'File not found' with filename => installer/data/mysql/kohastructure.sql

5. use Data::Dumper (x2)
Comment 7 Jonathan Druart 2023-01-13 10:56:27 UTC
Do we really need to call clear in reset_to_schema?
Comment 8 Jonathan Druart 2023-01-31 10:00:08 UTC
Asking for feedback.
Comment 9 Marcel de Rooy 2023-02-16 08:16:58 UTC
(In reply to Jonathan Druart from comment #6)
> 1. > The flags -clear, -reset and -renumber are mutually exclusive.
> 
> Then I think the script should catch this situation.
> 
> 2. I think we discussed that already, but there is more --confirm flags than
> --dry-run in our scripts. It's confusing to have both.
> 
> 3. You should remove references to 'verbose' in the module
> 
> 4. Path to structure depend on cwd, we can do better.
> 
> % cd misc/maintenance
> % perl sync_db_comments.pl --database koha_kohadev --reset
> Exception 'Koha::Exceptions::FileNotFound' thrown 'File not found' with
> filename => installer/data/mysql/kohastructure.sql
> 
> 5. use Data::Dumper (x2)

Thx for your feedback.
I definitely like --dry-run but will invert the cmdline flag to --confirm.
Removing verbose from the module, although I do not think it is necessary.
Other observations addressed too. Will resubmit.
Comment 10 Marcel de Rooy 2023-02-16 08:17:58 UTC
(In reply to Jonathan Druart from comment #7)
> Do we really need to call clear in reset_to_schema?

I think so, yes. A reset to schema implies removing comments that are not in the schema. We could improve later when needed by adding an option or so.
Comment 11 Marcel de Rooy 2023-02-16 08:51:20 UTC
Created attachment 146712 [details] [review]
Bug 32334: Exceptions - Add parameter to WrongParameter

Test plan:
Run t/Koha/Exceptions.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2023-02-16 08:51:22 UTC
Created attachment 146713 [details] [review]
Bug 32334: Add general exception FileNotFound

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-02-16 08:51:25 UTC
Created attachment 146714 [details] [review]
Bug 32334: Introduce Koha::Database::Commenter

Test plan:
[1] Make a backup of your database. Just for precaution.
[2] Run t/db_dependent/Koha/Database/Commenter.t.
* Note: This test script adds and removes a test table.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-02-16 08:51:27 UTC
Created attachment 146715 [details] [review]
Bug 32334: Add sync_db_comments script

Test plan:
[1] Backup your database, if not done already.
[2] Check output of dry_run when clearing a table:
    misc/maintenance/sync_db_comments.pl -clear -table items -dry
[3] Save output of misc/devel/update_dbix_class_files before changing
    comments in order to compare later. (Commit your changes.)
    You may not have changes after running (at least on a fresh
    database). That's fine.
[4] Clear all comments:
    misc/maintenance/sync_db_comments.pl -clear
[5] Renumber all comments:
    misc/maintenance/sync_db_comments.pl -renum
[6] Reset all comments to schema. Make sure that script finds your
    structure in installer/data/mysql folder.
    misc/maintenance/sync_db_comments.pl -reset
[7] Run update_dbix_class_files again and inspect changes as compared
    to previous run.
    Can you explain them? You should only see changes related to
    column comments. If you did not have changes in step 3, you
    should not have them here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2023-02-16 08:51:30 UTC
Created attachment 146716 [details] [review]
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags

Test plan:
Run sync_db_comments.pl --clear --renumber
Comment 16 Marcel de Rooy 2023-02-16 08:51:32 UTC
Created attachment 146717 [details] [review]
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter

Note: This is only done (and 'needed') for the command line, not
for the module subroutines.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2023-02-16 08:51:35 UTC
Created attachment 146718 [details] [review]
Bug 32334: (QA follow-up) Remove unused modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2023-02-16 08:51:37 UTC
Created attachment 146719 [details] [review]
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages

The module returns messages. The script can print them in verbose
mode. Test script adjusted accordingly.

Test plan:
Run t/db_dependent/Koha/Database/Commenter.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2023-02-16 08:51:40 UTC
Created attachment 146720 [details] [review]
Bug 32334: (QA follow-up) Improve finding schema file

Adds a schema parameter to the cmdline script now too.

Test plan:
Run sync_db_comments.pl with -schema file where file does not exist.
(On dev install) rename kohastructure.sql, try with[out] referring
to it using -schema. You could also use the standard path
intranet/cgi-bin/installer/data/mysql.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 David Nind 2023-03-16 11:04:54 UTC
I've had a go at testing, but I am stuck on step 6.

Running 'misc/maintenance/sync_db_comments.pl -reset' gives me this output and error message:

-- DRY RUN
Koha::Database::Commenter::_columns_info(): DBI Exception: DBD::mysql::db selectall_hashref failed: Table 'koha_kohadev.IF' doesn't exist at /kohadevbox/koha/Koha/Database/Commenter.pm line 221
Comment 21 Marcel de Rooy 2023-03-16 12:37:43 UTC
(In reply to David Nind from comment #20)
> I've had a go at testing, but I am stuck on step 6.
> 
> Running 'misc/maintenance/sync_db_comments.pl -reset' gives me this output
> and error message:
> 
> -- DRY RUN
> Koha::Database::Commenter::_columns_info(): DBI Exception: DBD::mysql::db
> selectall_hashref failed: Table 'koha_kohadev.IF' doesn't exist at
> /kohadevbox/koha/Koha/Database/Commenter.pm line 221

Ahh.
This is a new recent construct in the kohastructure file:

CREATE TABLE IF NOT EXISTS `tickets` (

I will add a small regexp change to include IF NOT EXISTS..
Comment 22 Marcel de Rooy 2023-03-16 12:38:59 UTC
Altough it actually is a bit odd:

DROP TABLE IF EXISTS `tickets`;
/*!40101 SET @saved_cs_client     = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE IF NOT EXISTS `tickets` (

First drop table and then check if it not exists.. Ok.
Comment 23 Marcel de Rooy 2023-03-16 12:42:05 UTC
(In reply to Marcel de Rooy from comment #22)
> Altough it actually is a bit odd:
> 
> DROP TABLE IF EXISTS `tickets`;
> /*!40101 SET @saved_cs_client     = @@character_set_client */;
> /*!40101 SET character_set_client = utf8 */;
> CREATE TABLE IF NOT EXISTS `tickets` (
> 
> First drop table and then check if it not exists.. Ok.

Bug 31028
Comment 24 Marcel de Rooy 2023-03-16 12:58:55 UTC
Created attachment 148266 [details] [review]
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS

We need to expect CREATE TABLE IF NOT EXISTS now in kohastructure.

Test plan:
Run misc/maintenance/sync_db_comments.pl -reset

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 David Nind 2023-03-16 23:42:18 UTC
Thanks Marcel!

I managed to get through the test plan - there were no changes in step 3, so n changes in step 7 (if I did it correctly).

I think I tested it correctly, if not I hope QA and those more familiar with what is going on review it.

For the (QA follow-up) Improve finding schema file (see comment #19), I'm not sure I did it correctly/what to expect. In KTD the file is in installer/data/mysql rather than intranet/cgi-bin/installer/data/mysql. I:
1. Renamed the file (mv installer/data/mysql/kohastructure.sql installer/data/mysql/renamedkohastructure.sql)
2. Ran the command misc/maintenance/sync_db_comments.pl -schema kohastructure.sql
   ==> output is:
   Usage:
        perl sync_db_comments.pl [-h] [-v] [-schema FILE ] [-database DB_NAME] [-table TABLE_NAME] [-commit] [-clear|-reset|-renumber]
3. If I run with the renamed file, the output is the same.
Comment 26 Marcel de Rooy 2023-03-17 07:52:22 UTC
(In reply to David Nind from comment #25)
> I managed to get through the test plan - there were no changes in step 3, so
> n changes in step 7 (if I did it correctly).

That should be fine.

> I think I tested it correctly, if not I hope QA and those more familiar with
> what is going on review it.

Great!

> For the (QA follow-up) Improve finding schema file (see comment #19), I'm
> not sure I did it correctly/what to expect. In KTD the file is in
> installer/data/mysql rather than intranet/cgi-bin/installer/data/mysql. I:
> 1. Renamed the file (mv installer/data/mysql/kohastructure.sql
> installer/data/mysql/renamedkohastructure.sql)
> 2. Ran the command misc/maintenance/sync_db_comments.pl -schema
> kohastructure.sql
>    ==> output is:
>    Usage:
>         perl sync_db_comments.pl [-h] [-v] [-schema FILE ] [-database
> DB_NAME] [-table TABLE_NAME] [-commit] [-clear|-reset|-renumber]
> 3. If I run with the renamed file, the output is the same.

Yeah, the schema parameter was not yet in the cmdline script. And that was a bit confusing. So I added a follow-up for it here after all.
If you specify a wrong file now, it warns you:
$misc/maintenance/sync_db_comments.pl -schema notexist
File notexist not found!
Usage:
        perl sync_db_comments.pl [-h] [-v] [-database DB_NAME] [-table TABLE_NAME] [-schema SCHEMA_FILE] [-commit] [-clear|-reset|-renumber]

And when I add the reset option, you will see:
$misc/maintenance/sync_db_comments.pl -schema notexist -reset
File notexist not found!
-- DRY RUN
Exception 'Koha::Exceptions::FileNotFound' thrown 'File not found' with filename => notexist

When I run with an existing file without reset|clear|renumber, you will not see the warn, only the usage.
Comment 27 Marcel de Rooy 2023-03-17 07:52:43 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2023-03-17 12:27:58 UTC
Created attachment 148335 [details] [review]
Bug 32334: (follow-up) Improvement for schema parameter

Essentially only adds a warn, some cosmetic changes too.

Test plan:
Copy your kohastructure to xx.sql.
Run sync_db_comments.pl -schema xx.sql. You will see usage.
Run sync_db_comments.pl -schema xyz.sql. You will see a warn and
the usage statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2023-03-17 12:28:59 UTC
(In reply to Marcel de Rooy from comment #26)
> Yeah, the schema parameter was not yet in the cmdline script. And that was a
> bit confusing. So I added a follow-up for it here after all.

Not in my local branch, but on Bugzilla it was already. Adding to the confusion. Revised the last follow-up.
Comment 30 David Nind 2023-03-17 13:32:12 UTC
Thanks Marcel for clarifying and updating, retested and sign off on the way.
Comment 31 David Nind 2023-03-17 13:33:53 UTC
Created attachment 148338 [details] [review]
Bug 32334: Exceptions - Add parameter to WrongParameter

Test plan:
Run t/Koha/Exceptions.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2023-03-17 13:33:56 UTC
Created attachment 148339 [details] [review]
Bug 32334: Add general exception FileNotFound

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 33 David Nind 2023-03-17 13:34:00 UTC
Created attachment 148340 [details] [review]
Bug 32334: Introduce Koha::Database::Commenter

Test plan:
[1] Make a backup of your database. Just for precaution.
[2] Run t/db_dependent/Koha/Database/Commenter.t.
* Note: This test script adds and removes a test table.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2023-03-17 13:34:03 UTC
Created attachment 148341 [details] [review]
Bug 32334: Add sync_db_comments script

Test plan:
[1] Backup your database, if not done already.
[2] Check output of dry_run when clearing a table:
    misc/maintenance/sync_db_comments.pl -clear -table items -dry
[3] Save output of misc/devel/update_dbix_class_files before changing
    comments in order to compare later. (Commit your changes.)
    You may not have changes after running (at least on a fresh
    database). That's fine.
[4] Clear all comments:
    misc/maintenance/sync_db_comments.pl -clear
[5] Renumber all comments:
    misc/maintenance/sync_db_comments.pl -renum
[6] Reset all comments to schema. Make sure that script finds your
    structure in installer/data/mysql folder.
    misc/maintenance/sync_db_comments.pl -reset
[7] Run update_dbix_class_files again and inspect changes as compared
    to previous run.
    Can you explain them? You should only see changes related to
    column comments. If you did not have changes in step 3, you
    should not have them here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2023-03-17 13:34:06 UTC
Created attachment 148342 [details] [review]
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags

Test plan:
Run sync_db_comments.pl --clear --renumber

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 David Nind 2023-03-17 13:34:09 UTC
Created attachment 148343 [details] [review]
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter

Note: This is only done (and 'needed') for the command line, not
for the module subroutines.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 37 David Nind 2023-03-17 13:34:13 UTC
Created attachment 148344 [details] [review]
Bug 32334: (QA follow-up) Remove unused modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2023-03-17 13:34:16 UTC
Created attachment 148345 [details] [review]
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages

The module returns messages. The script can print them in verbose
mode. Test script adjusted accordingly.

Test plan:
Run t/db_dependent/Koha/Database/Commenter.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2023-03-17 13:34:19 UTC
Created attachment 148346 [details] [review]
Bug 32334: (QA follow-up) Improve finding schema file

Adds a schema parameter to the cmdline script now too.

Test plan:
Run sync_db_comments.pl with -schema file where file does not exist.
(On dev install) rename kohastructure.sql, try with[out] referring
to it using -schema. You could also use the standard path
intranet/cgi-bin/installer/data/mysql.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2023-03-17 13:34:22 UTC
Created attachment 148347 [details] [review]
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS

We need to expect CREATE TABLE IF NOT EXISTS now in kohastructure.

Test plan:
Run misc/maintenance/sync_db_comments.pl -reset

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 41 David Nind 2023-03-17 13:34:25 UTC
Created attachment 148348 [details] [review]
Bug 32334: (follow-up) Improvement for schema parameter

Essentially only adds a warn, some cosmetic changes too.

Test plan:
Copy your kohastructure to xx.sql.
Run sync_db_comments.pl -schema xx.sql. You will see usage.
Run sync_db_comments.pl -schema xyz.sql. You will see a warn and
the usage statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 42 Marcel de Rooy 2023-03-20 07:07:37 UTC
(In reply to David Nind from comment #30)
> Thanks Marcel for clarifying and updating, retested and sign off on the way.

Great!
Comment 43 Jonathan Druart 2023-04-07 11:48:58 UTC
Created attachment 149259 [details] [review]
Bug 32334: Exceptions - Add parameter to WrongParameter

Test plan:
Run t/Koha/Exceptions.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2023-04-07 11:49:01 UTC
Created attachment 149260 [details] [review]
Bug 32334: Add general exception FileNotFound

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Jonathan Druart 2023-04-07 11:49:04 UTC
Created attachment 149261 [details] [review]
Bug 32334: Introduce Koha::Database::Commenter

Test plan:
[1] Make a backup of your database. Just for precaution.
[2] Run t/db_dependent/Koha/Database/Commenter.t.
* Note: This test script adds and removes a test table.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 46 Jonathan Druart 2023-04-07 11:49:08 UTC
Created attachment 149262 [details] [review]
Bug 32334: Add sync_db_comments script

Test plan:
[1] Backup your database, if not done already.
[2] Check output of dry_run when clearing a table:
    misc/maintenance/sync_db_comments.pl -clear -table items -dry
[3] Save output of misc/devel/update_dbix_class_files before changing
    comments in order to compare later. (Commit your changes.)
    You may not have changes after running (at least on a fresh
    database). That's fine.
[4] Clear all comments:
    misc/maintenance/sync_db_comments.pl -clear
[5] Renumber all comments:
    misc/maintenance/sync_db_comments.pl -renum
[6] Reset all comments to schema. Make sure that script finds your
    structure in installer/data/mysql folder.
    misc/maintenance/sync_db_comments.pl -reset
[7] Run update_dbix_class_files again and inspect changes as compared
    to previous run.
    Can you explain them? You should only see changes related to
    column comments. If you did not have changes in step 3, you
    should not have them here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 47 Jonathan Druart 2023-04-07 11:49:10 UTC
Created attachment 149263 [details] [review]
Bug 32334: (QA follow-up) Force mutual exclusiveness of cmdline flags

Test plan:
Run sync_db_comments.pl --clear --renumber

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 48 Jonathan Druart 2023-04-07 11:49:13 UTC
Created attachment 149264 [details] [review]
Bug 32334: (QA follow-up) Invert dry run mode to commit parameter

Note: This is only done (and 'needed') for the command line, not
for the module subroutines.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 49 Jonathan Druart 2023-04-07 11:49:16 UTC
Created attachment 149265 [details] [review]
Bug 32334: (QA follow-up) Remove unused modules

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Jonathan Druart 2023-04-07 11:49:19 UTC
Created attachment 149266 [details] [review]
Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages

The module returns messages. The script can print them in verbose
mode. Test script adjusted accordingly.

Test plan:
Run t/db_dependent/Koha/Database/Commenter.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 51 Jonathan Druart 2023-04-07 11:49:22 UTC
Created attachment 149267 [details] [review]
Bug 32334: (QA follow-up) Improve finding schema file

Adds a schema parameter to the cmdline script now too.

Test plan:
Run sync_db_comments.pl with -schema file where file does not exist.
(On dev install) rename kohastructure.sql, try with[out] referring
to it using -schema. You could also use the standard path
intranet/cgi-bin/installer/data/mysql.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 52 Jonathan Druart 2023-04-07 11:49:26 UTC
Created attachment 149268 [details] [review]
Bug 32334: (follow-up) Fix for bug 31028, introducing IF NOT EXISTS

We need to expect CREATE TABLE IF NOT EXISTS now in kohastructure.

Test plan:
Run misc/maintenance/sync_db_comments.pl -reset

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Jonathan Druart 2023-04-07 11:49:29 UTC
Created attachment 149269 [details] [review]
Bug 32334: (follow-up) Improvement for schema parameter

Essentially only adds a warn, some cosmetic changes too.

Test plan:
Copy your kohastructure to xx.sql.
Run sync_db_comments.pl -schema xx.sql. You will see usage.
Run sync_db_comments.pl -schema xyz.sql. You will see a warn and
the usage statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Jonathan Druart 2023-04-07 11:49:31 UTC
Created attachment 149270 [details] [review]
Bug 32334: Fix spelling

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 55 Tomás Cohen Arazi 2023-04-12 14:22:34 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 56 Marcel de Rooy 2023-04-12 14:25:08 UTC
(In reply to Tomás Cohen Arazi from comment #55)
> Pushed to master for 23.05.
> 
> Nice work everyone, thanks!

Thanks Jonathan and Tomas.
Comment 57 Jacob O'Mara 2023-04-17 07:26:28 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!
Comment 58 Caroline Cyr La Rose 2023-05-24 19:49:46 UTC
Back end stuff, doesn't affect the end user so there's nothing to add/amend in the manual.

I believe the schema (https://schema.koha-community.org/) is updated automatically, so there's no need to put DB bugs as Needs documenting.