From 17b0f8f1bf9e9478b0f34752e995a8c6412a993c Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 6 Nov 2025 16:30:12 -0500 Subject: [PATCH 1/2] ETT-825 Operational bug: CRMS order-by - Minimal correct modification to `CRMS::GetNextItemForReview` I could come up with - Split `ORDER BY` parameters into "critical" and "noncritical" - Any project-supplied `ORDER BY`s get prepended to the noncritical array. - Remove obsolete comment. - Add `params` arrays for both of the new arrays to address an existing (and commented-on) parameter land mine. This helps ensure clauses and their parameters get assembled in the right order. --- cgi/CRMS.pm | 24 +++++++++++++----------- t/CRMS.t | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/cgi/CRMS.pm b/cgi/CRMS.pm index ee15408a..33e08045 100755 --- a/cgi/CRMS.pm +++ b/cgi/CRMS.pm @@ -4784,8 +4784,6 @@ sub ProjectDispatch } } -# Code commented out with #### are race condition mitigations -# to be considered for a later release. # Test param prints debug info and iterates 5 times to test mitigation. # If the query returns no results CRMS errors will contain one entry. # If there is a Bib API issue, CRMS errors will hold an error for each failed item. @@ -4801,7 +4799,10 @@ sub GetNextItemForReview my $proj = $self->GetUserCurrentProject($user); my $project_ref = $self->GetProjectRef($proj); my @params = ($user, $proj); - my @orders = ('q.priority DESC', 'cnt DESC', 'hash', 'q.time ASC'); + my @critical_order = ('q.priority DESC', 'cnt DESC'); + my @critical_order_params = (); + my @noncritical_order = ('hash', 'q.time ASC'); + my @noncritical_order_params = (); my $sysid; if ($project_ref->group_volumes && $self->IsUserAdvanced($user)) { $sql = 'SELECT b.sysid FROM reviews r INNER JOIN bibdata b ON r.id=b.id'. @@ -4818,14 +4819,15 @@ sub GetNextItemForReview $excludeh = ' AND NOT EXISTS (SELECT * FROM historicalreviews r3 WHERE r3.id=q.id AND r3.user IN '. $wc. ')'; push @params, @{$inc}; } + # The `PresentationOrder` interface is not rich enough to support wildcard parameters + # but if it were then we would want to unshift them on `@noncritical_order_params` + # the way we do below with sysid. if (defined $porder) { - unshift @orders, $porder; + unshift @noncritical_order, $porder; } if (defined $sysid) { - # First order, last param (assumes any order param will be last). - # Adding any additional parameterized ordering will be trickier. - unshift @orders, 'IF(b.sysid=?,1,0) DESC,q.id ASC'; - push @params, $sysid; + unshift @noncritical_order, 'IF(b.sysid=?,1,0) DESC,q.id ASC'; + unshift @noncritical_order_params, $sysid; } $sql = 'SELECT q.id,(SELECT COUNT(*) FROM reviews r WHERE r.id=q.id) AS cnt,'. 'SHA2(CONCAT(?,q.id),0) AS hash,q.priority,q.project,b.sysid'. @@ -4833,12 +4835,12 @@ sub GetNextItemForReview ' WHERE q.project=? AND q.locked IS NULL AND q.status<2'. ' AND q.unavailable=0'. $excludei. $excludeh. - ' HAVING cnt<2 ORDER BY '. join ',', @orders; + ' HAVING cnt<2 ORDER BY '. join ',', (@critical_order, @noncritical_order); if (defined $test) { $sql .= ' LIMIT 5'; - printf "$user: %s\n", Utilities::StringifySql($sql, @params); + printf "$user: %s\n", Utilities::StringifySql($sql, @params, @critical_order_params, @noncritical_order_params); } - $ref = $self->SelectAll($sql, @params); + $ref = $self->SelectAll($sql, @params, @critical_order_params, @noncritical_order_params); }; # eval # Deal with error with database query setup or execution. if ($@) { diff --git a/t/CRMS.t b/t/CRMS.t index ec53d81e..a759dd68 100755 --- a/t/CRMS.t +++ b/t/CRMS.t @@ -144,6 +144,24 @@ subtest '#UpdateMetadata' => sub { delete $ENV{CRMS_METADATA_FIXTURES_PATH}; }; +subtest 'GetNextItemForReview' => sub { + $ENV{CRMS_METADATA_FIXTURES_PATH} = $ENV{'SDRROOT'} . '/crms/t/fixtures/metadata'; + my $htid = 'coo.31924000029250'; + my $record = Metadata->new('id' => $htid); + $crms->UpdateMetadata($htid, 1, $record); + $crms->PrepareSubmitSql('INSERT INTO queue (id,project) VALUES (?,?)', $htid, 1); + # Assign autocrms to this project + $crms->PrepareSubmitSql('INSERT INTO projectusers (project,user) VALUES (?,?)',1, 'autocrms'); + $crms->PrepareSubmitSql('UPDATE users SET project=1 WHERE id=?', 'autocrms'); + $crms->ClearErrors; + my $next_volume = $crms->GetNextItemForReview('autocrms'); + is($next_volume, $htid, 'expected volume is returned'); + $crms->PrepareSubmitSql('DELETE FROM projectusers'); + $crms->PrepareSubmitSql('DELETE FROM queue'); + $crms->PrepareSubmitSql('DELETE FROM bibdata'); + delete $ENV{CRMS_METADATA_FIXTURES_PATH}; +}; + subtest '#LinkToJira' => sub { is($crms->LinkToJira('DEV-000'), 'DEV-000'); From bc36f80eeba183fd305d12736adfa476e3411ae0 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 7 Nov 2025 10:01:46 -0500 Subject: [PATCH 2/2] Formatting fix --- t/CRMS.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/CRMS.t b/t/CRMS.t index a759dd68..4fa3f68d 100755 --- a/t/CRMS.t +++ b/t/CRMS.t @@ -151,7 +151,7 @@ subtest 'GetNextItemForReview' => sub { $crms->UpdateMetadata($htid, 1, $record); $crms->PrepareSubmitSql('INSERT INTO queue (id,project) VALUES (?,?)', $htid, 1); # Assign autocrms to this project - $crms->PrepareSubmitSql('INSERT INTO projectusers (project,user) VALUES (?,?)',1, 'autocrms'); + $crms->PrepareSubmitSql('INSERT INTO projectusers (project,user) VALUES (?,?)', 1, 'autocrms'); $crms->PrepareSubmitSql('UPDATE users SET project=1 WHERE id=?', 'autocrms'); $crms->ClearErrors; my $next_volume = $crms->GetNextItemForReview('autocrms');