Description
Phan Tung Bui
2024-02-07 20:17:23 UTC
Created attachment 161852 [details] [review] Bug 36027: search_for_data_inconsistencies.pl - make each section optional Test plan : 1. Apply batch 2. Run the script 3. Enter the corresponding number to choose a method to run 4. Complete the Test 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. Created attachment 161953 [details] [review] Bug 36027: search_for_data_inconsistencies.pl - make each section optional 1. Code has been tidied up for better readability and maintainability. 2. The Data::Dumper import has been removed as it was unnecessary. 3. Unnecessary empty lines have been removed to improve code cleanliness. 4. The script has been updated to accept parameters instead of relying on interactive input. Now you can specify which checks to run using command-line parameters, while retaining the default behavior of running all checks if no parameters are provided. Thank you for your feedback! Created attachment 161954 [details] [review] Bug 36027: search_for_data_inconsistencies.pl - make each section optional Hi Phan, Patch isn't applying for me on current master, can you double-check please? Maybe current patches were written on top of the now obsolete patch? Thanks. Created attachment 162162 [details] [review] Bug 36027 : (follow up) Fix patch isn't applying error Hi Phan, The first patch of the 3 is the first patch that does not apply, it needs to either be rebased and resubmitted or made obsolete. If you don't want to rebase and want to submit a new one, you can make it obsolete by visiting https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161953&action=edit and clicking "edit details", tick "obsolete". Me and others are available to chat on IRC and provide help if needed. Created attachment 162349 [details] [review] Bug 36027: Tidy code for QA script Created attachment 162351 [details] [review] Bug 36027: change interactive to pass param and tidy the code Putting this as "Needs Signoff" to look at again later and there's no sign-off line by anyone just yet. 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) Created attachment 162540 [details] [review] Bug 36027 : (follow-up) Improvement: Simplified command line options and add --help option This commit addresses the issue of opaque command line arguments by implementing option flags for better clarity and user-friendliness. Instead of passing arbitrary numbers, users can now utilize descriptive flags such as --skip-items-branch and --skip-age-for-category to disable specific sections of checks. Additionally, the documentation in POD format has been updated to reflect these changes, including the addition of a --help flag to provide usage instructions. Updated test plan 1. Apply batch 2. Run the script and using options suggested in --help 3. Complete the test Created attachment 162546 [details] [review] Bug 36027 : Tidy the code Created attachment 163259 [details] [review] Bug 36027 : (follow-up) Improvement : Add check option i add the check option so that instead of skipping certain checks, you can also choose a specific test to run 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 => { }, }, ]; Created attachment 163956 [details] [review] Bug 36027: change interactive to pass param and add check options Created attachment 166117 [details] [review] Bug 36027: Change interactive to pass param and tidy the code Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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. 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) Created attachment 167766 [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. Created attachment 167787 [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. Created attachment 168953 [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> Created attachment 168954 [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> Thanks Alexandre for the patch. I've signed this off, as it does what it says it will do. I have made some comments. I will leave it up to you (or the QA Team) to decide if these need addressing. I think this is an enhancement, rather than fixing something that is wrong. So I have changed the status. I have also added a draft release note. Testing notes (using koha-testing-docker (KTD)): 1. Set up data so there are inconsistencies: 1.1 There is already one inconsistency in the sample data, for record 369 (encoding errors) 1.2 I created an inconsistency for patron age and patron category type: . koha-mysql kohadev . update borrowers set categorycode = "K" where borrowernumber=49; 2. Step 3, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --check-status --check-age 3. Step 4, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --skip-status 4. Step 5, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --check-age --skip-status Some comments: 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) 3. Warning when you use --check-* and --skip-* options in the same command (step 5). I've made a wording change suggestion. The current text says to me 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." 4. Description and help text. I think this needs some work with formatting and the text. I would be happy to add a follow-up patch with some suggested changes. 5. The QA script has warnings (to run the QA script when using koha-testing-docker): qa WARN misc/maintenance/search_for_data_inconsistencies.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 83, now: 206) Created attachment 169228 [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> Created attachment 169229 [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> 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) Created attachment 170847 [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> Created attachment 170848 [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> Created attachment 170849 [details] [review] Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl Created attachment 173269 [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> Created attachment 173270 [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> Created attachment 173271 [details] [review] Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl Fixed merge conflict for master Everything still seems to work as expected. However, I've Failed QA as any command run generates this line in the output, which I don't think it should: Subroutine CheckRelationshipsLoop redefined at misc/maintenance/search_for_data_inconsistencies.pl line 708. Updated testing notes (from comment #25) ===================== Testing notes (using KTD): 1. Set up data so that there are inconsistencies: 1.1 There is already one inconsistency in the sample data, for record 369 (encoding errors) 1.2 I created an inconsistency for patron age and patron category type: . koha-mysql kohadev . update borrowers set categorycode = "K" where borrowernumber=49; 2. Step 3, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --check-status --check-age 3. Step 4, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --skip-status 4. Step 5, ran check with: misc/maintenance/search_for_data_inconsistencies.pl --check-age --skip-status Updated comments (from me, comment #25) ================ I'll file separate bugs for these comments. 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) 3. Warning when you use --check-* and --skip-* options in the same command (step 5). I've made a wording change suggestion. The current text says to me 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." 4. Description and help text. I think this needs some work with formatting and the text. I will add a follow-up patch with some suggested changes. Additional comments (from Katrin, comment #28) =================== Just so that these don't get lost from Katrin's comments. 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. 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) 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! Created attachment 173556 [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. 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 *** Bug 31093 has been marked as a duplicate of this bug. *** 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. Created attachment 177270 [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. Created attachment 177283 [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. Created attachment 177438 [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> 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> Created attachment 177440 [details] [review] Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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> Created attachment 177444 [details] [review] Bug 36027: (follow-up) Fix QA script errors 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) 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> 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> Created attachment 177851 [details] [review] Bug 36027: (follow-up) Tidy search_for_data_inconsistencies.pl Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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> Patches rebased using the auto-rebase script. Please retest. (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. 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> 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> 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> 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> 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> 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> 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. 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 |