Bug 36027 - search_for_data_inconsistencies.pl - add options so you can choose what checks to run
Summary: search_for_data_inconsistencies.pl - add options so you can choose what check...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Hammat wele
QA Contact: Kyle M Hall (khall)
URL:
Keywords: no-sandbox
: 31093 (view as bug list)
Depends on:
Blocks: 38755
  Show dependency treegraph
 
Reported: 2024-02-07 20:17 UTC by Phan Tung Bui
Modified: 2025-04-02 13:14 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement updates the data inconsistencies script (misc/maintenance/search_for_data_inconsistencies.pl) so that you can now choose what checks to run, instead of running all the checks at once (which can take a lot of time for larger libraries). Running the script without any options now displays the help text, instead of running all the checks (for consistency with other scripts). To run all the checks at the same time, use the --check-all option. Examples: - Run all checks (--check-all): misc/maintenance/search_for_data_inconsistencies.pl --check-all - Run checks for item type issues and patrons with an invalid age for their patron category: misc/maintenance/search_for_data_inconsistencies.pl --check-status --check-age - Skip specific checks, but run all the others: misc/maintenance/search_for_data_inconsistencies.pl --skip-status --skip-title Use misc/maintenance/search_for_data_inconsistencies.pl --help for all the check and skip options.
Version(s) released in:
Circulation function:


Attachments
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (3.77 KB, patch)
2024-02-07 21:25 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (30.15 KB, patch)
2024-02-08 20:26 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (950 bytes, patch)
2024-02-08 20:26 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027 : (follow up) Fix patch isn't applying error (6.41 KB, patch)
2024-02-14 17:42 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: Tidy code for QA script (2.56 KB, patch)
2024-02-22 21:56 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: change interactive to pass param and tidy the code (23.38 KB, patch)
2024-02-22 22:10 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027 : (follow-up) Improvement: Simplified command line options and add --help option (6.07 KB, patch)
2024-02-28 16:15 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027 : Tidy the code (1.81 KB, patch)
2024-02-28 17:25 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027 : (follow-up) Improvement : Add check option (6.30 KB, patch)
2024-03-15 17:48 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: change interactive to pass param and add check options (26.89 KB, patch)
2024-03-26 18:56 UTC, Phan Tung Bui
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (26.96 KB, patch)
2024-05-03 11:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (33.45 KB, patch)
2024-06-14 20:24 UTC, Alexandre Noel
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (13.31 KB, patch)
2024-06-17 14:47 UTC, Alexandre Noel
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (27.00 KB, patch)
2024-07-15 03:00 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (13.35 KB, patch)
2024-07-15 03:00 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (27.05 KB, patch)
2024-07-19 13:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (13.41 KB, patch)
2024-07-19 13:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (27.05 KB, patch)
2024-08-28 21:41 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (13.41 KB, patch)
2024-08-28 21:41 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl (27.44 KB, patch)
2024-08-28 21:41 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (27.12 KB, patch)
2024-10-24 13:46 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (18.57 KB, patch)
2024-10-24 13:46 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl (27.44 KB, patch)
2024-10-24 13:46 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 36027: (follow-up) Fixed code duplication and add mandatory options (7.63 KB, patch)
2024-10-28 15:21 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 36027: (follow-up) Adding check-all option (2.63 KB, patch)
2025-01-28 12:46 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 36027: (follow-up) Adding check-all option (3.50 KB, patch)
2025-01-28 15:43 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (27.17 KB, patch)
2025-02-01 02:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (18.61 KB, patch)
2025-02-01 02:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl (27.49 KB, patch)
2025-02-01 02:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: (follow-up) Fixed code duplication and add mandatory options (7.68 KB, patch)
2025-02-01 02:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: (follow-up) Adding check-all option (3.55 KB, patch)
2025-02-01 02:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: (follow-up) Update help text and error message (5.42 KB, patch)
2025-02-01 02:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: (follow-up) Fix QA script errors (3.54 KB, patch)
2025-02-01 02:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (11.78 KB, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (18.64 KB, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl (838 bytes, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: (follow-up) Fixed code duplication and add mandatory options (6.93 KB, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: (follow-up) Adding check-all option (3.55 KB, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: (follow-up) Update help text and error message (5.43 KB, patch)
2025-02-12 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36027: Change interactive to pass param and tidy the code (11.92 KB, patch)
2025-04-02 08:39 UTC, Eric Bégin
Details | Diff | Splinter Review
Bug 36027: search_for_data_inconsistencies.pl - make each section optional (18.78 KB, patch)
2025-04-02 08:39 UTC, Eric Bégin
Details | Diff | Splinter Review
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl (978 bytes, patch)
2025-04-02 08:39 UTC, Eric Bégin
Details | Diff | Splinter Review
Bug 36027: (follow-up) Fixed code duplication and add mandatory options (7.07 KB, patch)
2025-04-02 08:39 UTC, Eric Bégin
Details | Diff | Splinter Review
Bug 36027: (follow-up) Adding check-all option (3.68 KB, patch)
2025-04-02 08:39 UTC, Eric Bégin
Details | Diff | Splinter Review
Bug 36027: (follow-up) Update help text and error message (5.56 KB, patch)
2025-04-02 08:40 UTC, Eric Bégin
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phan Tung Bui 2024-02-07 20:17:23 UTC
Modification of the script allows us to choose which test to run instead of running all tests.
Comment 1 Phan Tung Bui 2024-02-07 21:25:16 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2024-02-08 13:51:42 UTC
Failing QA here:
1) Not tidy
2) Leftover Data::Dumper import
3) Many empty new lines
4) This enhancement turns the script into an interactive script, requiring interactive input. This change in behavior will potentially cause existing systems running this script (as a cron or otherwise) to fail. My suggestion here is to update the code to instead allow for params instead of turning it interactive, e.g.:

perl misc/maintenance/search_for_data_inconsistencies.pl --items-branch --items-status

And keep functionality of running all checks by default as is currently, this ensures existing default functionality plus the new enhancement, instead of changing current default functionality.
Comment 3 Phan Tung Bui 2024-02-08 20:26:20 UTC Comment hidden (obsolete)
Comment 4 Phan Tung Bui 2024-02-08 20:26:22 UTC Comment hidden (obsolete)
Comment 5 Pedro Amorim 2024-02-09 16:34:35 UTC Comment hidden (obsolete)
Comment 6 Phan Tung Bui 2024-02-14 17:42:02 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2024-02-15 09:59:32 UTC Comment hidden (obsolete)
Comment 8 Phan Tung Bui 2024-02-22 21:56:23 UTC Comment hidden (obsolete)
Comment 9 Phan Tung Bui 2024-02-22 22:10:56 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2024-02-23 11:52:12 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2024-02-23 12:07:46 UTC
Passing arbitrary numbers as command line arguments is not a good UI as it's opaque (no way to guess that "3" means "check item status") and it's very hard to remember what each number does.
Also it can make future changes more complicated: for instance if section 3 should be removed, what should we do with this number ? Leave it unassigned and have "gaps" in the sequence ? Re-assign it to another check and break backward compatibility ?

IMO it's far better to have option flags. And as the default behavior is to run all checks, those option flags should disable a specific section when given.
For instance: --skip-items-branch --skip-age-for-category.
Getopt::Long can be used for that

Also documentation in POD should be updated and a --help flag should be added to print usage help (see Pod::Usage)
Comment 12 Phan Tung Bui 2024-02-28 16:15:26 UTC Comment hidden (obsolete)
Comment 13 Phan Tung Bui 2024-02-28 16:17:35 UTC Comment hidden (obsolete)
Comment 14 Phan Tung Bui 2024-02-28 17:25:20 UTC Comment hidden (obsolete)
Comment 15 Phan Tung Bui 2024-03-15 17:48:07 UTC Comment hidden (obsolete)
Comment 16 Frédéric Demians 2024-03-25 09:40:27 UTC
Would it be possible to have just one patch? In my opinion, there are already too many parameters. May I suggest an implementation based on an array of inconsistencies tests? Something like that:

my $inconsistencies = [
    {
        name => 'item-branch',
        description => 'Items without home or holding library',
        sub => {

        },
    },
    {
        name => 'item-type',
        description => 'Biblios/items without item type or with invalid item type',
        sub => {

        },
    },
];
Comment 17 Phan Tung Bui 2024-03-26 18:56:32 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize (ashimema) 2024-05-03 11:55:13 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize (ashimema) 2024-05-03 11:58:25 UTC
This is a big improvement, signing off.

For QA, I'd encourage a better commit title, a summary of what it does and a test plan to be included in the commit message.

Generally, though, I like where this is headed and wouldn't block it.

I'd love to see the functions moved into the Database::Audit module Pedro introduces in bug 36039 eventually and exposure of this data in there.
Comment 20 Julian Maurice 2024-06-07 13:54:42 UTC
If I use at least one --check-* option, all --skip-* options are ignored, which make sense given that when a --check-* option is used all other checks are disabled. But this is not documented.
And it would be nice to have a warning when using --skip-* and --check-* options together.

The code can be improved too. For instance:

  CheckItemsBranch() if $options{'check-branch'} || $options{'skip-branch'};

is not easy to understand ($options{skip-branch} == 1 means "do not skip" which is confusing)

Failing QA for lack of documentation and also for what Martin said in comment 19 (more detailed commit message, including a test plan)
Comment 21 Alexandre Noel 2024-06-14 20:24:09 UTC Comment hidden (obsolete)
Comment 22 Alexandre Noel 2024-06-17 14:47:19 UTC Comment hidden (obsolete)
Comment 23 David Nind 2024-07-15 03:00:13 UTC Comment hidden (obsolete)
Comment 24 David Nind 2024-07-15 03:00:16 UTC Comment hidden (obsolete)
Comment 25 David Nind 2024-07-15 03:21:27 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall (khall) 2024-07-19 13:49:13 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall (khall) 2024-07-19 13:49:22 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2024-08-02 16:56:44 UTC
I know that bug 36039 is blocked by this, but I'd like to ask some questions at least before pushing.

1) Quite a lot of untidyness added, please fix.

 WARN	misc/maintenance/search_for_data_inconsistencies.pl
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 83, now: 206)

Can we fix this here or on the other bug?

2) New parameters (not blocker)

I wonder if the chosen pattern is good as it requires that we add 2 new parameters for every check that we add to the script. 

We do have something similar in cleanup_database.pl for action logs and it seems more logical to me. 

   --log-module       Specify which action log modules to trim. Repeatable.
   --preserve-log     Specify which action logs to exclude. Repeatable.
   --logs DAYS        purge entries from action_logs older than DAYS days.
                      Defaults to 180 days if no days specified.

So instead of a unique parameter, you can use preserve-log with different codes.

I am not sure what is better, just putting the question out here.

3) Script should not start without -c or any parameters... (not blocker)

This is something for a separate bug. But we once said that scripts should output help when run without parameter and require a confirmation flag. 
As this can run quite a time, this still seems a good idea... 
(leaving this here for filing later)
Comment 29 Hammat wele 2024-08-28 21:41:11 UTC Comment hidden (obsolete)
Comment 30 Hammat wele 2024-08-28 21:41:14 UTC Comment hidden (obsolete)
Comment 31 Hammat wele 2024-08-28 21:41:18 UTC Comment hidden (obsolete)
Comment 32 Olivier Vezina 2024-10-24 13:46:31 UTC Comment hidden (obsolete)
Comment 33 Olivier Vezina 2024-10-24 13:46:34 UTC Comment hidden (obsolete)
Comment 34 Olivier Vezina 2024-10-24 13:46:37 UTC Comment hidden (obsolete)
Comment 35 Olivier Vezina 2024-10-24 13:46:55 UTC
Fixed merge conflict for master
Comment 36 David Nind 2024-10-28 02:46:33 UTC Comment hidden (obsolete)
Comment 37 Olivier Vezina 2024-10-28 13:26:56 UTC
Thank you for testing, I'll check what i can do for the marcxml i am not sure myself. I also agree that the terminology should be change to library!
Comment 38 Olivier Vezina 2024-10-28 15:21:22 UTC Comment hidden (obsolete)
Comment 39 Olivier Vezina 2024-10-28 15:24:25 UTC
It seems possible to add the MarcXML report as its own option. If this feature could be helpful, it should be possible to implement.

I was not exactly sure if what i did with the script without options was the right thing to do. It now blocks you and shows you the help sheet. I'll fix it if its wrong
Comment 40 Magnus Enger 2024-12-23 12:20:32 UTC
*** Bug 31093 has been marked as a duplicate of this bug. ***
Comment 41 Magnus Enger 2024-12-23 12:28:22 UTC
Would it make sense to have a --check-all option, to run all the checks without specifying each one? This would also make it possible to run all checks added in the future.
Comment 42 Hammat wele 2025-01-28 12:46:57 UTC Comment hidden (obsolete)
Comment 43 Hammat wele 2025-01-28 15:43:27 UTC Comment hidden (obsolete)
Comment 44 David Nind 2025-02-01 02:20:48 UTC Comment hidden (obsolete)
Comment 45 David Nind 2025-02-01 02:20:52 UTC
Created attachment 177439 [details] [review]
Bug 36027: search_for_data_inconsistencies.pl - make each section optional

We can specify which sections to search for data inconsistencies using check options, or which sections to skip using skip options.

TEST PLAN:
Make sure there are data inconsistencies present in at least two categories to validate the results of the command.

1. Apply the patch
2. Run the command without any argument and confirm that every inconsistences are reported.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 46 David Nind 2025-02-01 02:20:56 UTC
Created attachment 177440 [details] [review]
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 David Nind 2025-02-01 02:21:00 UTC
Created attachment 177441 [details] [review]
Bug 36027: (follow-up) Fixed code duplication and add mandatory options

An option is now mandatory for the script to lunch.

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 David Nind 2025-02-01 02:21:04 UTC
Created attachment 177442 [details] [review]
Bug 36027: (follow-up) Adding check-all option

This patch add --check-all option, to run all the checks without specifying each one

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with 'check-all' arguments (ex: ./search_for_data_inconsistencies.pl --check-all), confirm that all the check is done.

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 David Nind 2025-02-01 02:21:08 UTC
Created attachment 177443 [details] [review]
Bug 36027: (follow-up) Update help text and error message

This updates the help text and the error message when --check*
and --skip* options are used in the same command.

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 David Nind 2025-02-01 02:35:26 UTC
Created attachment 177444 [details] [review]
Bug 36027: (follow-up) Fix QA script errors
Comment 51 David Nind 2025-02-01 02:45:45 UTC
Updated testing notes (from comment #25 and #36)
=====================

Testing notes (using KTD):

1. Apply the patch.
2. Set up data so that there are inconsistencies:
   2.1 There is already one inconsistency in the sample data, for record 369 (encoding errors)
   2.2 I created an inconsistency for patron age and patron category type:
       . koha-mysql kohadev
       . update borrowers set categorycode = "K" where borrowernumber=49;
3. Run the script without any options: misc/maintenance/search_for_data_inconsistencies.pl
   ==> It should output the help for the script (a change in behavour - previously it ran all checks).
3. Run the script with one or more check options: misc/maintenance/search_for_data_inconsistencies.pl --check-status --check-age
   ==> It should output the inconsistencies (an invalid MARCXML record and an invalid age category).
4. Run the script with one or more --skip* options: misc/maintenance/search_for_data_inconsistencies.pl --skip-status
   ==> It should skip the checks specified.
6. Run the script with a check option and a skip option: misc/maintenance/search_for_data_inconsistencies.pl --check-age --skip-status
   ==> The skip option should be ignored (not run) and only the check option should be run.
7. Run the script with all options: misc/maintenance/search_for_data_inconsistencies.pl --check-all
   ==> It should output the results from running all the checks (in this case with KTD, the invalid MARCXML document and invalid age category).
8. Check the help to make sure it is improved, makes sense, and incorporates all the changes made by this bug: misc/maintenance/search_for_data_inconsistencies.pl --help

Updated comments (from me, comment #25 and #36)
===============================================

I'll file separate bugs for these comments when the bug passes QA:

1. The check-status option includes checking for item type issues (as described in the help description), but it also checks for invalid MARCXML. I think that the MARCXML check should have its own option (--check-marcxml Check for invalid MARCXML in bibliographic records).
   1.1 I'm not sure how hard this to do (I am not a developer).
   1.2 I'm not sure whether there are other checks in the script that are not included in the description and have their own options.
   1.2 Instead of --check-status, --check-itemtype

2. Instead of --check-branch, use --check-library. While branch is used in the underlying code, the terminology guidelines suggest we should use library. (https://wiki.koha-community.org/wiki/Terminology#L)

Follow-up - help and error message text
=======================================

I've added a follow-up for:

1. Warning when you use --check-* and --skip-* options in the same command (step 6). The current text says (to me, anyway) that if you use skip and check options in the same command, the skip options are ignored. That is, it will ignore/not run the skip options:
   . Current warning: "Warning : skip options are ignored when check options are provided"
   . Suggested warning: "Warning: The --skip-* option(s) were ignored. Only the --check-* options in the command were run."

2. Description and help text - some formatting and text changes.

Additional comments (from Katrin, comment #28)
==============================================

Just so that these don't get lost.

1. New parameters (not blocker)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I wonder if the chosen pattern is good as it requires that we add 2 new parameters for every check that we add to the script. 

We do have something similar in cleanup_database.pl for action logs and it seems more logical to me. 

   --log-module       Specify which action log modules to trim. Repeatable.
   --preserve-log     Specify which action logs to exclude. Repeatable.
   --logs DAYS        purge entries from action_logs older than DAYS days.
                      Defaults to 180 days if no days specified.

So instead of a unique parameter, you can use preserve-log with different codes.

I am not sure what is better, just putting the question out here.

(I think that would be better, for example, two options --check and --skip with a list of codes for each check available. David)
  

2. Script should not start without -c or any parameters... (not blocker)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is something for a separate bug. But we once said that scripts should output help when run without parameter and require a confirmation flag. 

As this can run quite a time, this still seems a good idea... (leaving this here for filing later)
Comment 52 Jonathan Druart 2025-02-12 13:06:39 UTC
Created attachment 177849 [details] [review]
Bug 36027: Change interactive to pass param and tidy the code

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 53 Jonathan Druart 2025-02-12 13:06:44 UTC
Created attachment 177850 [details] [review]
Bug 36027: search_for_data_inconsistencies.pl - make each section optional

We can specify which sections to search for data inconsistencies using check options, or which sections to skip using skip options.

TEST PLAN:
Make sure there are data inconsistencies present in at least two categories to validate the results of the command.

1. Apply the patch
2. Run the command without any argument and confirm that every inconsistences are reported.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 54 Jonathan Druart 2025-02-12 13:06:48 UTC
Created attachment 177851 [details] [review]
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 Jonathan Druart 2025-02-12 13:06:52 UTC
Created attachment 177852 [details] [review]
Bug 36027: (follow-up) Fixed code duplication and add mandatory options

An option is now mandatory for the script to lunch.

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 Jonathan Druart 2025-02-12 13:06:56 UTC
Created attachment 177853 [details] [review]
Bug 36027: (follow-up) Adding check-all option

This patch add --check-all option, to run all the checks without specifying each one

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with 'check-all' arguments (ex: ./search_for_data_inconsistencies.pl --check-all), confirm that all the check is done.

Signed-off-by: David Nind <david@davidnind.com>
Comment 57 Jonathan Druart 2025-02-12 13:06:59 UTC
Created attachment 177854 [details] [review]
Bug 36027: (follow-up) Update help text and error message

This updates the help text and the error message when --check*
and --skip* options are used in the same command.

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 Jonathan Druart 2025-02-12 13:07:17 UTC
Patches rebased using the auto-rebase script. Please retest.
Comment 59 David Nind 2025-04-01 11:00:57 UTC
(In reply to Jonathan Druart from comment #58)
> Patches rebased using the auto-rebase script. Please retest.

Have retested. Step 4 of my testing notes in comment #51 no longer seems to work:

4. Run the script with one or more --skip* options: misc/maintenance/search_for_data_inconsistencies.pl --skip-status
   ==> It should skip the checks specified.

It just outputs the help.

It was a while ago since I last tested, so not sure whether it is supposed to do that or not.

Otherwise, everything else seems to work OK.
Comment 60 Eric Bégin 2025-04-02 08:39:40 UTC
Created attachment 180257 [details] [review]
Bug 36027: Change interactive to pass param and tidy the code

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 61 Eric Bégin 2025-04-02 08:39:44 UTC
Created attachment 180258 [details] [review]
Bug 36027: search_for_data_inconsistencies.pl - make each section optional

We can specify which sections to search for data inconsistencies using check options, or which sections to skip using skip options.

TEST PLAN:
Make sure there are data inconsistencies present in at least two categories to validate the results of the command.

1. Apply the patch
2. Run the command without any argument and confirm that every inconsistences are reported.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 62 Eric Bégin 2025-04-02 08:39:48 UTC
Created attachment 180259 [details] [review]
Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 63 Eric Bégin 2025-04-02 08:39:53 UTC
Created attachment 180260 [details] [review]
Bug 36027: (follow-up) Fixed code duplication and add mandatory options

An option is now mandatory for the script to lunch.

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with one or more check arguments (ex: ./search_for_data_inconsistencies.pl --check-title --check-loop), confirm that it displayed the specified inconsistences.
4. Run the command with one or more skip arguments (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title), confirm that it displayed the inconsistences unsless the skip ones.
5. Run the command with check and skip options (ex: ./search_for_data_inconsistencies.pl --skip-auth --skip-title --check-loop), confirm that it displayed the specified check options and that a warning message indicate that the skip options have been ignored.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 64 Eric Bégin 2025-04-02 08:39:56 UTC
Created attachment 180261 [details] [review]
Bug 36027: (follow-up) Adding check-all option

This patch add --check-all option, to run all the checks without specifying each one

1. Apply the patch
2. Run the command without any argument and confirm it blocks you and shows the help message.
3. Run the command with 'check-all' arguments (ex: ./search_for_data_inconsistencies.pl --check-all), confirm that all the check is done.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 65 Eric Bégin 2025-04-02 08:40:00 UTC
Created attachment 180262 [details] [review]
Bug 36027: (follow-up) Update help text and error message

This updates the help text and the error message when --check*
and --skip* options are used in the same command.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Eric Bégin <eric.begin@inlibro.com>
Comment 66 Eric Bégin 2025-04-02 08:43:14 UTC
I test David's case and you get the help info since having --skip* does nothing if you don't use the --check-all option.

When you wrote the test plan, that script was doing the equivalent of --check-all when no option was set.  That is not the case anymore.
Comment 67 David Nind 2025-04-02 13:14:51 UTC
It no longer seems possible to use any of the --skip-* options, as far as I can see.

Set up the data in KTD so that there is an inconsistency for patron age and patron category type:
1. koha-mysql kohadev
2. update borrowers set categorycode = "K" where borrowernumber=49;

There is already a record in KTD with invalid MARCXML.

Example:

1. Skip the age checks, but run all the others: misc/maintenance/search_for_data_inconsistencies.pl --skip-age
==> Result: You just get the help.
==> Expected result: All checks are run, except the status checks

2. If you include the --check-all in the command, the --skip-* is ignored and all the checks are run: misc/maintenance/search_for_data_inconsistencies.pl --skip-status --check-all
==> Result:
    . You get a warning "Warning: The --skip-* option(s) were ignored. Only the --check-* options in the command were run."
    . All the checks are run