Skip to content

Commit

Permalink
Fix DB errors caused by review deny -> accept
Browse files Browse the repository at this point in the history
Ugh^N. When we accept a review after refusing it, that inserts the
review back into the database--but that breaks the PaperReviewHistory
chain if we reset reviewTime. So this commit reconstructs the review
from history and uses the latest reviewTime.

Really we should unify PaperReviewRefused with PaperReview.

And drop unused `data` fields.
  • Loading branch information
kohler committed Feb 26, 2025
1 parent 88a3607 commit 8b2453a
Show file tree
Hide file tree
Showing 7 changed files with 223 additions and 98 deletions.
34 changes: 18 additions & 16 deletions src/api/api_requestreview.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
// api_requestreview.php -- HotCRP review-request API calls
// Copyright (c) 2008-2024 Eddie Kohler; see LICENSE.
// Copyright (c) 2008-2025 Eddie Kohler; see LICENSE.

class RequestReview_API {
/** @param Contact $user
Expand Down Expand Up @@ -337,21 +337,18 @@ static function acceptreview($user, $qreq, $prow) {
}

if (!$rrow) {
$reviewBlind = $prow->conf->is_review_blind(null) ? 1 : 0;
$prow->conf->qe("insert into PaperReview
set paperId=?, reviewId=?, contactId=?,
requestedBy=?, timeRequested=?,
reviewType=?, reviewRound=?, data=?,
reviewBlind=?,
rflags=?",
$prow->paperId, $refrow->refusedReviewId, $refrow->contactId,
$refrow->requestedBy, $refrow->timeRequested,
$refrow->refusedReviewType, $refrow->reviewRound, $refrow->data,
$reviewBlind,
ReviewInfo::RF_LIVE | (1 << $refrow->refusedReviewType) | ($reviewBlind ? ReviewInfo::RF_BLIND : 0));
$rxrow = ReviewInfo::make_reconstruct_refusal($refrow);
$rxrow->insert_full();
$prow->conf->qe("delete from PaperReviewRefused where refusedReviewId=?",
$refrow->refusedReviewId);
$rrow = $prow->fresh_review_by_id($refrow->refusedReviewId);
$rrow = $prow->fresh_review_by_id($r);
if ($rrow->reviewType < REVIEW_SECONDARY && $rrow->requestedBy > 0) {
$prow->conf->update_review_delegation($prow->paperId, $rrow->requestedBy, 1);
}
if ($rrow->reviewToken) {
$prow->conf->update_rev_tokens_setting(1);
}
$prow->conf->update_automatic_tags($prow, "review");
}

if ($rrow->reviewStatus < ReviewInfo::RS_ACKNOWLEDGED) {
Expand Down Expand Up @@ -416,13 +413,18 @@ static function declinereview($user, $qreq, $prow) {
set paperId=?, email=?, contactId=?,
requestedBy=?, timeRequested=?, refusedBy=?,
timeRefused=?, reason=?, refusedReviewType=?,
refusedReviewId=?, reviewRound=?, data=?
refusedReviewId=?, reviewRound=?
on duplicate key update reason=coalesce(?,reason)",
$prow->paperId, $rrow->reviewer()->email, $rrow->contactId,
$rrow->requestedBy, $rrow->timeRequested, $user->contactId,
Conf::$now, $reason, $rrow->reviewType,
$rrid, $rrow->reviewRound, $rrow->data_string(),
$rrid, $rrow->reviewRound,
$reason);

// record snapshot of review, then delete review
$rrow->set_prop("reviewType", REVIEW_REFUSAL);
$rrow->snapshot_fval_prop();
$rrow->save_prop();
$prow->conf->qe("delete from PaperReview where paperId=? and reviewId=?",
$prow->paperId, $rrid);

Expand Down
2 changes: 1 addition & 1 deletion src/conference.php
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ function __load_settings() {

function load_settings() {
$this->__load_settings();
if ($this->sversion < 307) {
if ($this->sversion < 308) {
$old_nerrors = Dbl::$nerrors;
while ((new UpdateSchema($this))->run()) {
usleep(50000);
Expand Down
31 changes: 29 additions & 2 deletions src/reviewdiffinfo.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
// reviewdiffinfo.php -- HotCRP class representing review diffs
// Copyright (c) 2006-2023 Eddie Kohler; see LICENSE.
// Copyright (c) 2006-2025 Eddie Kohler; see LICENSE.

class ReviewDiffInfo {
/** @var ReviewInfo
Expand All @@ -15,6 +15,8 @@ class ReviewDiffInfo {
/** @var int */
private $_x_view_score = VIEWSCORE_EMPTY;
/** @var bool */
private $_disable_patch = false;
/** @var bool */
public $notify = false;
/** @var bool */
public $notify_author = false;
Expand All @@ -29,6 +31,13 @@ function __construct(ReviewInfo $rrow) {
$this->rrow = $rrow;
}

/** @param bool $x
* @return $this */
function set_disable_patch($x) {
$this->_disable_patch = $x;
return $this;
}

/** @return bool */
function is_empty() {
return empty($this->_old_prop) && empty($this->_fields);
Expand Down Expand Up @@ -118,7 +127,8 @@ function make_patch($dir) {
$v = [$oldv, $this->rrow->finfoval($f)];
if (is_string($v[0])
&& is_string($v[1])
&& $f instanceof Text_ReviewField) {
&& $f instanceof Text_ReviewField
&& !$this->_disable_patch) {
$hcdelta = $this->dmp_hcdelta($v[1 - $dir], $v[$dir], true);
if ($hcdelta !== null
&& strlen($hcdelta) < strlen($v[$dir]) - 32) {
Expand Down Expand Up @@ -271,4 +281,21 @@ static function apply_patch(ReviewInfo $rrow, $patch) {
$rrow->_assign_fields();
return $ok;
}

/** @param array $patch
* @param array<string,true> &$known
* @return bool */
static function apply_patch_reconstruct(ReviewInfo $rrow, $patch, &$known) {
foreach ($patch as $n => $v) {
$nl = strlen($n);
if (($nl <= 2 || $n[$nl - 2] !== ":")
&& !isset($known[$n])
&& ($fi = ReviewFieldInfo::find($rrow->conf, $n))) {
$rrow->_set_finfoval($fi, $v);
$known[$n] = true;
}
}
$rrow->_seal_fstorage();
$rrow->_assign_fields();
}
}
Loading

0 comments on commit 8b2453a

Please sign in to comment.