Feature/consistency checks (ledgersmb#7265)

* Introduce infrastructure for consistency checking * Move consistency check from temp file to new infrastructure * Add check for transactions being balanced * s/front matter/frontmatter/ * Check that all cleared lines are in a reconciliation report * Use precision from configuration table instead of hardcoding 2-digit precision * Document consistency check module * Add descriptions to checks and more checks
ylavoie · Mar 19, 2023 · 719153c · 719153c
1 parent 4e88dff
commit 719153c
Show file tree

Hide file tree

Showing 14 changed files with 339 additions and 38 deletions.
diff --git a/UI/setup/confirm_operation.html b/UI/setup/confirm_operation.html
@@ -106,6 +106,13 @@ <h2>[% text('Database Management Console') %]</h2>
    class = 'submit'
     text = text('System Info') #'
 } %]
+[% INCLUDE button element_data = {
+    name = 'action'
+   value = 'consistency'
+    type = 'submit'
+   class = 'submit'
+    text = text('Check consistency') #'
+} %]
 </div>
 [% END %]
 </form>

diff --git a/UI/setup/consistency_results.html b/UI/setup/consistency_results.html
@@ -0,0 +1,32 @@
+[% INCLUDE "ui-header.html"
+          stylesheet="ledgersmb.css"
+          include_stylesheet=["setup.css"]
+       %]
+[% PROCESS elements.html %]
+<body id="setup-system-info" class="lsmb [% dojo_theme %]">
+<div><div class="setupconsole">
+<h2>[% text('Consistency check results') %]</h2>
+[% INCLUDE 'setup/ui-db-credentials.html' %]
+<p>[% text('Note: '); %]</p>
+
+<table>
+  <caption>[% text('LedgerSMB consistency check results') %]</caption>
+  <tbody>
+    [% FOR item IN results %]
+    <tr>
+      <td style="text-align: right">
+        [% IF item.result == 'consistent' %]
+        <span style="font-weight:bold;color:green">✓</span>
+        [% ELSE %]
+        <span style="color:red" title="[% item.count %] failed row(s)">✗</span>[% END %]
+      </td>
+      <th>[% item.frontmatter.title %]</th>
+    </tr>
+    [% END %]
+  </tbody>
+</table>
+
+
+</div></div>
+</body>
+[% end_html %]
diff --git a/lib/LedgerSMB/Database/ConsistencyChecks.pm b/lib/LedgerSMB/Database/ConsistencyChecks.pm
@@ -0,0 +1,133 @@
+package LedgerSMB::Database::ConsistencyChecks;
+
+use v5.28.0;
+use warnings;
+
+use Exporter 'import';
+use File::Find::Rule;
+use File::Spec;
+use YAML::PP;
+
+my $yaml = YAML::PP->new();
+
+our @EXPORT = ## no critic
+    qw( find_checks load_checks run_checks );
+
+=head1 NAME
+
+LedgerSMB::Database::ConsistencyChecks - Asserting data validity
+
+=head1 DESCRIPTION
+
+A module to run a set of validation checks against a company database
+schema.
+
+=head1 SYNOPSIS
+
+  use LedgerSMB::Database::ConsistencyChecks;
+
+  my $check_paths = find_checks( $path );
+  my $checks = load_checks( $check_paths );
+  my $results = $run_checks( $checks );
+
+=head1 METHODS
+
+=head1 FUNCTIONS
+
+=head2 find_checks( $path )
+
+=cut
+
+sub find_checks {
+    my ($path) = @_;
+    my @checks = sort File::Find::Rule->new()
+        ->name( '*.sql' )
+        ->in( File::Spec->catdir( $path, 'consistency' ) );
+
+    return \@checks;
+}
+
+=head2 load_checks( $paths )
+
+=cut
+
+sub load_checks {
+    my ($paths) = @_;
+
+    return [
+        map {
+            my $path = $_;
+            my $content = do {
+                open my $fh, '<', $path
+                    or die "Unable to open file $path: $!";
+                local $/ = undef;
+                <$fh>;
+            };
+            $content =~ m/^---.*?\n(?<frontmatter>.*?)\n---.*?\n(?<query>.*)$/s;
+            {
+                path => $path,
+                query => $+{query},
+                frontmatter => $yaml->load_string( $+{frontmatter} )
+            };
+        } $paths->@* ];
+}
+
+=head2 run_checks( $checks )
+
+Returns a reference to an array of hash references. Each hash holds
+the following keys:
+
+=over
+
+=item path
+
+The location at which the source file of the check is stored.
+
+=item query
+
+The query to be executed returning rows failing the correctness criterion.
+
+=item frontmatter
+
+A hash containing the metadata read from the input source file.
+
+=item result
+
+Indicates whether the check failed (C<failed>) or succeeded (C<consistent>).
+
+=item count
+
+Number of failures in case the check failed.
+
+=back
+
+=cut
+
+sub run_checks {
+    my ($dbh, $checks) = @_;
+
+    for my $check ($checks->@*) {
+        my $query = qq|select count(*) from ($check->{query}) x|;
+        my ($count) = $dbh->selectrow_array($query);
+        die $dbh->errstr if $dbh->err != 0;
+
+        $check->{result} = $count ? 'failed' : 'consistent';
+        $check->{count} = $count;
+    }
+
+    return $checks;
+}
+
+
+=head1 LICENSE AND COPYRIGHT
+
+Copyright (C) 2023 The LedgerSMB Core Team
+
+This file is licensed under the GNU General Public License version 2, or at your
+option any later version.  A copy of the license should have been included with
+your software.
+
+=cut
+
+1;
+
diff --git a/lib/LedgerSMB/Scripts/setup.pm b/lib/LedgerSMB/Scripts/setup.pm
@@ -40,6 +40,7 @@ use LedgerSMB::App_State;
 use LedgerSMB::Company;
 use LedgerSMB::Database;
 use LedgerSMB::Database::Config;
+use LedgerSMB::Database::ConsistencyChecks;
 use LedgerSMB::Entity::User;
 use LedgerSMB::Entity::Person::Employee;
 use LedgerSMB::I18N;
@@ -503,6 +504,29 @@ sub run_backup {
     }
 }
 
+=item consistency
+
+=cut
+
+sub consistency {
+    my ($request) = @_;
+    my ($reauth, $database) = _get_database($request);
+    return $reauth if $reauth;
+
+    my $dbh = $database->connect({PrintError => 0, AutoCommit => 0});
+    my $paths = find_checks($request->{_wire}->get( 'paths/sql' ) );
+    my $checks = load_checks( $paths );
+    my $results = run_checks( $dbh, $checks );
+
+    return $request->{_wire}->get('ui')->render(
+        $request,
+        'setup/consistency_results',
+        {
+            results => $results
+        });
+}
+
+
 =item revert_migration
 
 =cut

diff --git a/sql/consistency/00000-transactions-linked-to-arapgl.sql b/sql/consistency/00000-transactions-linked-to-arapgl.sql
@@ -0,0 +1,15 @@
+--- yaml frontmatter
+title: Records in 'transactions' have an associated ar/ap/gl record
+description: |
+  The 'transactions' table exists to prevent records with the same ID from being
+  inserted into the 'ar', 'ap' and 'gl' tables by maintaining which table the record
+  belongs to. This leaves open the possibility that a record exists in the 'transactions'
+  table where no associated record exists in any of the three tables. This situation
+  should not exist.
+---
+
+select id
+  from transactions t
+ where not exists (select 1 from ar where ar.id = t.id)
+   and not exists (select 1 from ap where ap.id = t.id)
+   and not exists (select 1 from gl where gl.id = t.id)
diff --git a/sql/consistency/00001-balanced-transactions.sql b/sql/consistency/00001-balanced-transactions.sql
@@ -0,0 +1,15 @@
+--- yaml frontmatter
+title: All transactions are balanced
+description: |
+  Transactions must be balanced; a balance sheet must be balanced
+  at all times and transactions being the building blocks of a
+  balance sheet, they need to be balanced too.
+---
+
+select trans_id
+  from acc_trans
+ group by trans_id
+  having abs(sum(amount_bc)) >= power(10, -1*coalesce((select value::numeric
+                                                        from defaults
+                                                       where setting_key = 'decimal_places'),
+                                                       2))/2
diff --git a/sql/consistency/00002-transactions-linkedt-to-ar.sql b/sql/consistency/00002-transactions-linkedt-to-ar.sql
@@ -0,0 +1,15 @@
+--- yaml frontmatter
+title: 'ar' Records in 'transactions' are linked to the 'ar' table
+description: |
+  The 'transactions' table exists to prevent records with the same ID from being
+  inserted into the 'ar', 'ap' and 'gl' tables by maintaining which table the record
+  belongs to. This leaves open the possibility that a record exists in the 'transactions'
+  table which ends up being linked to one of the other tables (either additionally or
+  exclusively).
+---
+
+select *
+  from transactions
+         join ar
+             using (id)
+ where transactions.table_name is distinct from 'ar'
diff --git a/sql/consistency/00003-transactions-linked-to-ap.sql b/sql/consistency/00003-transactions-linked-to-ap.sql
@@ -0,0 +1,16 @@
+--- yaml frontmatter
+title: 'ap' Records in 'transactions' are linked to the 'ap' table
+description: |
+  The 'transactions' table exists to prevent records with the same ID from being
+  inserted into the 'ar', 'ap' and 'gl' tables by maintaining which table the record
+  belongs to. This leaves open the possibility that a record exists in the 'transactions'
+  table which ends up being linked to one of the other tables (either additionally or
+  exclusively).
+---
+
+
+select *
+  from transactions
+         join ap
+             using (id)
+ where transactions.table_name is distinct from 'ap'
diff --git a/sql/consistency/00004-transactions-linked-to-gl.sql b/sql/consistency/00004-transactions-linked-to-gl.sql
@@ -0,0 +1,15 @@
+--- yaml frontmatter
+title: 'gl' Records in 'transactions' are linked to the 'gl' table
+description: |
+  The 'transactions' table exists to prevent records with the same ID from being
+  inserted into the 'ar', 'ap' and 'gl' tables by maintaining which table the record
+  belongs to. This leaves open the possibility that a record exists in the 'transactions'
+  table which ends up being linked to one of the other tables (either additionally or
+  exclusively).
+---
+
+select *
+  from transactions
+         join gl
+             using (id)
+ where transactions.table_name is distinct from 'gl'
diff --git a/sql/consistency/00100-cleared-acc_trans-has-recon-account.sql b/sql/consistency/00100-cleared-acc_trans-has-recon-account.sql
@@ -0,0 +1,14 @@
+--- yaml frontmatter
+title: Cleared journal lines have an associated reconciliation account
+description: |
+  The 'cleared' indicator on journal lines indicates they have been successfully
+  reconciled. In order for lines to have been reconciled, they must be associated
+  with an account for which reconciliation is applicable.
+---
+
+select distinct chart_id
+  from acc_trans a
+ where not exists (select 1
+                     from cr_coa_to_account ca
+                    where ca.chart_id = a.chart_id)
+   and a.cleared
diff --git a/sql/consistency/00101-cleared-acc_trans-contained-in-recon.sql b/sql/consistency/00101-cleared-acc_trans-contained-in-recon.sql
@@ -0,0 +1,15 @@
+--- yaml frontmatter
+title: Journal lines marked 'cleared' are part of a reconciliation
+description: |
+  Journal lines can only be cleared by way of reconciliation. This means that
+  all lines which are cleared, must be part of a reconciliation which is itself
+  submitted and approved.
+---
+
+select *
+  from acc_trans
+ where cleared
+   and not exists (select 1
+                     from cr_report_line_links
+                    where acc_trans.entry_id = cr_report_line_links.entry_id
+                      and cleared)
diff --git a/sql/consistency/00102-submitted-recon-cleared-links.sql b/sql/consistency/00102-submitted-recon-cleared-links.sql
@@ -0,0 +1,14 @@
+--- yaml frontmatter
+title: Submitted reconciliation report journal line link status
+description: |
+  Each journal line link of a submitted report must have a
+  'cleared' status, if the report line has a 'cleared' status.
+---
+
+select *
+  from cr_report_line_links crll
+         join cr_report_line crl
+             on crll.report_line_id = crl.id
+         join cr_report cr
+             on crl.report_id = cr.id
+ where crll.cleared is distinct from (crl.cleared and cr.submitted)
diff --git a/sql/consistency/00103-recon-our-balance-vs-journals-total.sql b/sql/consistency/00103-recon-our-balance-vs-journals-total.sql
@@ -0,0 +1,24 @@
+--- yaml frontmatter
+title: Sum of journal balances equals report line balance
+description: |
+  Reconciliation lines refer to one or more journal lines. The total of the
+  journal lines underlying a reconciliation line must equal the amount on
+  the reconciliation line for the reconciliation to valid.
+---
+
+with ledger_balances as (
+  select crll.report_line_id,
+         case when cr.recon_fx then sum(amount_tc)
+         else sum(amount_bc) end as ledger_amount
+    from cr_report_line_links crll
+           join acc_trans using (entry_id)
+           join cr_report_line crl on crll.report_line_id = crl.id
+           join cr_report cr on crl.report_id = cr.id
+   group by cr.recon_fx, crll.report_line_id
+)
+select *
+  from cr_report_line crl
+         join ledger_balances lb
+             on crl.id = lb.report_line_id
+ where lb.ledger_amount <> crl.our_balance
+   and crl.cleared