More info follows.
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>
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>
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>
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>
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.
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)
Do we really need to call clear in reset_to_schema?
Asking for feedback.
(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.
(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.
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>
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>
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>
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>
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
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>
Created attachment 146718 [details] [review] Bug 32334: (QA follow-up) Remove unused modules Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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
(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..
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.
(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
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>
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.
(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.
Created attachment 148299 [details] [review] Bug 32334: (follow-up) Add the -schema parameter to script too Finally adding it here 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>
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>
(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.
Thanks Marcel for clarifying and updating, retested and sign off on the way.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
(In reply to David Nind from comment #30) > Thanks Marcel for clarifying and updating, retested and sign off on the way. Great!
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 149270 [details] [review] Bug 32334: Fix spelling Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 23.05. Nice work everyone, thanks!
(In reply to Tomás Cohen Arazi from comment #55) > Pushed to master for 23.05. > > Nice work everyone, thanks! Thanks Jonathan and Tomas.
Enhancement - not backporting to 22.11.x Nice work everyone!
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.