From 5d50b3b7b5ab3184881cf46f38211e03e12809e3 Mon Sep 17 00:00:00 2001 From: Eddie Kohler Date: Mon, 24 Jun 2024 14:43:02 -0400 Subject: [PATCH] ReviewValues API update. --- batch/savepapers.php | 2 +- src/pages/p_offline.php | 6 +++--- src/pages/p_review.php | 22 ++++++++------------- src/papertable.php | 2 +- src/reviewform.php | 34 +++++++++++++++++++++++---------- test/t_comments.php | 2 +- test/t_reviews.php | 42 ++++++++++++++++++++--------------------- 7 files changed, 59 insertions(+), 51 deletions(-) diff --git a/batch/savepapers.php b/batch/savepapers.php index 3791a40f48..6618884650 100644 --- a/batch/savepapers.php +++ b/batch/savepapers.php @@ -69,7 +69,7 @@ function __construct(Conf $conf) { $this->conf = $conf; $this->user = $conf->root_user(); $this->user->set_overrides(Contact::OVERRIDE_CONFLICT); - $this->tf = new ReviewValues($conf->review_form(), ["no_notify" => true]); + $this->tf = new ReviewValues($conf, ["no_notify" => true]); } /** @return $this */ diff --git a/src/pages/p_offline.php b/src/pages/p_offline.php index 8e4b028168..57165140c6 100644 --- a/src/pages/p_offline.php +++ b/src/pages/p_offline.php @@ -35,9 +35,9 @@ function handle_upload() { $this->ms->error_at("file"); return false; } - $rf = $this->conf->review_form(); - $tf = ReviewValues::make_text($rf, $this->qreq->file_contents("file"), - $this->qreq->file_filename("file")); + $tf = (new ReviewValues($this->conf)) + ->set_text($this->qreq->file_contents("file"), + $this->qreq->file_filename("file")); while ($tf->parse_text($this->qreq->override)) { $tf->check_and_save($this->user, null, null); } diff --git a/src/pages/p_review.php b/src/pages/p_review.php index 44adabc83e..a7d4a4e0bb 100644 --- a/src/pages/p_review.php +++ b/src/pages/p_review.php @@ -34,11 +34,6 @@ function pt() { return $this->pt; } - /** @return ReviewForm */ - function rf() { - return $this->conf->review_form(); - } - /** @param bool $is_error */ function print_header($is_error) { PaperTable::print_header($this->pt, $this->qreq, $is_error); @@ -121,7 +116,7 @@ function handle_update() { $this->qreq->ready = 1; } - $rv = new ReviewValues($this->rf()); + $rv = new ReviewValues($this->conf); $rv->paperId = $this->prow->paperId; if (($whynot = ($this->rrow ? $this->user->perm_edit_review($this->prow, $this->rrow, true) @@ -150,9 +145,8 @@ function handle_upload_form() { $this->conf->error_msg("<0>File upload required"); return; } - $rv = ReviewValues::make_text($this->rf(), - $this->qreq->file_contents("file"), - $this->qreq->file_filename("file")); + $rv = (new ReviewValues($this->conf)) + ->set_text($this->qreq->file_contents("file"), $this->qreq->file_filename("file")); if ($rv->parse_text($this->qreq->override) && $rv->check_and_save($this->user, $this->prow, $this->rrow)) { $this->qreq->r = $this->qreq->reviewId = $rv->review_ordinal_id; @@ -169,7 +163,7 @@ function handle_upload_form() { function handle_download_form() { $filename = "review-" . ($this->rrow ? $this->rrow->unparse_ordinal_id() : $this->prow->paperId); - $rf = $this->rf(); + $rf = $this->conf->review_form(); $this->conf->make_csvg($filename, CsvGenerator::TYPE_STRING) ->set_inline(false) ->add_string($rf->text_form_header(false) @@ -179,7 +173,7 @@ function handle_download_form() { } function handle_download_text() { - $rf = $this->rf(); + $rf = $this->conf->review_form(); if ($this->rrow && $this->rrow_explicit) { $this->conf->make_csvg("review-" . $this->rrow->unparse_ordinal_id(), CsvGenerator::TYPE_STRING) ->add_string($rf->unparse_text($this->prow, $this->rrow, $this->user)) @@ -219,7 +213,7 @@ function handle_adopt() { return; } - $rv = new ReviewValues($this->rf()); + $rv = new ReviewValues($this->conf); $rv->paperId = $this->prow->paperId; $my_rrow = $this->prow->review_by_user($this->user); $my_rid = ($my_rrow ?? $this->rrow)->unparse_ordinal_id(); @@ -233,7 +227,7 @@ function handle_adopt() { $my_rid = $rv->review_ordinal_id; if (!$rv->has_problem_at("ready")) { // mark the source review as approved - $rvx = new ReviewValues($this->rf()); + $rvx = new ReviewValues($this->conf); $rvx->set_approved(); $rvx->check_and_save($this->user, $this->prow, $this->rrow); } @@ -343,7 +337,7 @@ function print() { if ($this->rv) { $pt->set_review_values($this->rv); } else if ($this->qreq->has_annex("after_login")) { - $rv = new ReviewValues($this->rf()); + $rv = new ReviewValues($this->conf); $rv->parse_qreq($this->qreq, !!$this->qreq->override); $pt->set_review_values($rv); } diff --git a/src/papertable.php b/src/papertable.php index c3e714f742..e26734cbff 100644 --- a/src/papertable.php +++ b/src/papertable.php @@ -3101,7 +3101,7 @@ function print_review_form() { if (!$this->user->can_clickthrough("review", $this->prow)) { self::print_review_clickthrough(); } - $rvalues = $this->review_values ?? new ReviewValues($this->conf->review_form()); + $rvalues = $this->review_values ?? new ReviewValues($this->conf); $this->conf->review_form()->print_form($this->prow, $this->editrrow, $this->user, $rvalues); } else { $this->print_rc([$this->editrrow], false); diff --git a/src/reviewform.php b/src/reviewform.php index 32b02bf997..33371e3a4d 100644 --- a/src/reviewform.php +++ b/src/reviewform.php @@ -886,9 +886,15 @@ class ReviewValues extends MessageSet { /** @var bool */ private $no_notify = false; - function __construct(ReviewForm $rf, $options = []) { - $this->conf = $rf->conf; - $this->rf = $rf; + /** @param ReviewForm|Conf $rf */ + function __construct($rf, $options = []) { + if ($rf instanceof ReviewForm) { + $this->conf = $rf->conf; + $this->rf = $rf; + } else { + $this->conf = $rf; + $this->rf = $this->conf->review_form(); + } foreach (["no_notify"] as $k) { if (array_key_exists($k, $options)) $this->$k = $options[$k]; @@ -896,13 +902,21 @@ function __construct(ReviewForm $rf, $options = []) { $this->set_want_ftext(true); } - /** @return ReviewValues */ - static function make_text(ReviewForm $rf, $text, $filename = null) { - $rv = new ReviewValues($rf); - $rv->text = $text; - $rv->lineno = 0; - $rv->filename = $filename; - return $rv; + /** @param string $text + * @param ?string $filename + * @return $this */ + function set_text($text, $filename = null) { + $this->text = $text; + $this->filename = $filename; + $this->lineno = 0; + return $this; + } + + /** @param ReviewForm|Conf $rf + * @return ReviewValues + * @deprecated */ + static function make_text($rf, $text, $filename = null) { + return (new ReviewValues($rf))->set_text($text, $filename); } /** @param int|string $field diff --git a/test/t_comments.php b/test/t_comments.php index 0ede6cfa0f..9519e9b690 100644 --- a/test/t_comments.php +++ b/test/t_comments.php @@ -123,7 +123,7 @@ function test_response_combination() { // ensure a PC member has a submitted review $paper = $this->conf->checked_paper_by_id(4); $reviewer = $this->conf->user_by_email("estrin@usc.edu"); - $tf = new ReviewValues($this->conf->review_form()); + $tf = new ReviewValues($this->conf); xassert($tf->parse_json(["ovemer" => 2, "revexp" => 1, "papsum" => "No summary", "comaut" => "No comments"])); xassert($tf->check_and_save($reviewer, $paper)); diff --git a/test/t_reviews.php b/test/t_reviews.php index e0ae50ef79..c73b08dc76 100644 --- a/test/t_reviews.php +++ b/test/t_reviews.php @@ -146,7 +146,7 @@ function test_offline_review_update() { $this->review1A = file_get_contents(SiteLoader::find("test/review1A.txt")); // correct update - $tf = ReviewValues::make_text($this->conf->review_form(), $this->review1A, "review1A.txt"); + $tf = (new ReviewValues($this->conf))->set_text($this->review1A, "review1A.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker)); @@ -155,12 +155,12 @@ function test_offline_review_update() { xassert_eqq($rrow->fidval("t03"), " This is a test of leading whitespace\n\n It should be preserved\nAnd defended\n"); // different-conference form fails - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/Testconf I/', 'Testconf IIII', $this->review1A), "review1A-1.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/Testconf I/', 'Testconf IIII', $this->review1A), "review1A-1.txt"); xassert(!$tf->parse_text(false)); xassert($tf->has_error_at("confid")); // invalid value fails - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/^4/m', 'Mumps', $this->review1A), "review1A-2.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/^4/m', 'Mumps', $this->review1A), "review1A-2.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker)); xassert_eqq(join(" ", $tf->unchanged), "#1A"); @@ -168,7 +168,7 @@ function test_offline_review_update() { // invalid “No entry” fails //$this->print_review_history($rrow); - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/^4/m', 'No entry', $this->review1A), "review1A-3.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/^4/m', 'No entry', $this->review1A), "review1A-3.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker)); xassert_eqq(join(" ", $tf->unchanged ?? []), "#1A"); @@ -179,12 +179,12 @@ function test_offline_review_update() { } function test_offline_review_different_reviewer() { - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/Reviewer: .*/m', 'Reviewer: butt@butt.com', $this->review1A), "review1A-4.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/Reviewer: .*/m', 'Reviewer: butt@butt.com', $this->review1A), "review1A-4.txt"); xassert($tf->parse_text(false)); xassert(!$tf->check_and_save($this->u_mgbaker)); xassert($tf->has_problem_at("reviewerEmail")); - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/Reviewer: .*/m', 'Reviewer: Mary Baaaker ', preg_replace('/^4/m', "5", $this->review1A)), "review1A-5.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/Reviewer: .*/m', 'Reviewer: Mary Baaaker ', preg_replace('/^4/m', "5", $this->review1A)), "review1A-5.txt"); xassert($tf->parse_text(false)); $paper1 = $this->conf->checked_paper_by_id(1); xassert(!$tf->check_and_save($this->u_mgbaker, $paper1, fresh_review($paper1, $this->u_mgbaker))); @@ -194,7 +194,7 @@ function test_offline_review_different_reviewer() { // it IS ok to save a form that's meant for a different EMAIL but same name // Also add a description of the field - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/Reviewer: .*/m', 'Reviewer: Mary Baker ', preg_replace('/^4/m', "5. Strong accept", $this->review1A)), "review1A-5.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/Reviewer: .*/m', 'Reviewer: Mary Baker ', preg_replace('/^4/m', "5. Strong accept", $this->review1A)), "review1A-5.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker, $paper1, fresh_review($paper1, $this->u_mgbaker))); xassert(!$tf->has_problem_at("reviewerEmail")); @@ -221,7 +221,7 @@ function test_change_review_choices() { xassert(!$rfield->required); // now it's OK to save “no entry” - $tf = ReviewValues::make_text($this->conf->review_form(), preg_replace('/^4/m', 'No entry', $this->review1A), "review1A-6.txt"); + $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/^4/m', 'No entry', $this->review1A), "review1A-6.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker)); xassert_eqq(join(" ", $tf->updated ?? []), "#1A"); @@ -230,7 +230,7 @@ function test_change_review_choices() { xassert_search($this->u_chair, "has:ovemer", ""); // Restore review - $tf = ReviewValues::make_text($this->conf->review_form(), $this->review1A, "review1A-7.txt"); + $tf = (new ReviewValues($this->conf))->set_text($this->review1A, "review1A-7.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($this->u_mgbaker)); xassert_eqq(join(" ", $tf->updated), "#1A"); @@ -643,7 +643,7 @@ function test_body() { $rrow17m = fresh_review($paper17, $user_mgbaker); xassert(!$rrow17m->reviewModified); - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 2, "revexp" => 1, "papsum" => "No summary", "comaut" => "No comments"])); xassert($tf->check_and_save($user_mgbaker, $paper17)); @@ -673,7 +673,7 @@ function test_body() { // Check review diffs $paper18 = $user_diot->checked_paper_by_id(18); - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 2, "revexp" => 1, "papsum" => "No summary", "comaut" => "No comments"])); xassert($tf->check_and_save($user_diot, $paper18)); @@ -699,7 +699,7 @@ function test_body() { xassert_eq($rrow18d2->fidval("s02"), 1); xassert_eqq($rrow18d2->fidval("t01"), "No summary\n"); - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["papsum" => "Four score and seven years ago our fathers brought forth on this continent, a new nation, conceived in Liberty, and dedicated to the proposition that all men are created equal.\n\ \n\ @@ -734,13 +734,13 @@ function test_body() { xassert($sv->execute()); $review18A = file_get_contents(SiteLoader::find("test/review18A.txt")); - $tf = ReviewValues::make_text($conf->review_form(), $review18A, "review18A.txt"); + $tf = (new ReviewValues($conf))->set_text($review18A, "review18A.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($user_diot)); xassert_eqq($tf->summary_status(), MessageSet::SUCCESS); xassert_eqq($tf->full_feedback_text(), "Updated review #18A\n"); - $tf = ReviewValues::make_text($conf->review_form(), $review18A, "review18A.txt"); + $tf = (new ReviewValues($conf))->set_text($review18A, "review18A.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($user_diot)); xassert_eqq($tf->summary_status(), MessageSet::WARNING); @@ -751,7 +751,7 @@ function test_body() { $review18A2 = str_replace("This is the stuff", "That was the stuff", str_replace("authors’ response\n", "authors' response\n", $review18A)); - $tf = ReviewValues::make_text($conf->review_form(), $review18A2, "review18A2.txt"); + $tf = (new ReviewValues($conf))->set_text($review18A2, "review18A2.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($user_diot)); @@ -768,7 +768,7 @@ function test_body() { $review18A3 = str_replace("That was the stuff", "Whence the stuff", str_replace("authors' response\n", "authors' response (hidden from authors)\n", $review18A2)); - $tf = ReviewValues::make_text($conf->review_form(), $review18A3, "review18A3.txt"); + $tf = (new ReviewValues($conf))->set_text($review18A3, "review18A3.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($user_diot)); @@ -776,7 +776,7 @@ function test_body() { xassert_eqq($rrow->fidval("t04"), "Whence the stuff I want to add for the authors’ response.\n"); $review18A4 = file_get_contents(SiteLoader::find("test/review18A-4.txt")); - $tf = ReviewValues::make_text($conf->review_form(), $review18A4, "review18A-4.txt"); + $tf = (new ReviewValues($conf))->set_text($review18A4, "review18A-4.txt"); xassert($tf->parse_text(false)); xassert($tf->check_and_save($user_diot)); @@ -794,7 +794,7 @@ function test_review_history() { xassert_eqq($rrow17a->fidval("t01"), "No summary\n"); xassert_eqq($rrow17a->fidval("t02"), "No comments\n"); - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 3, "revexp" => 2, "papsum" => "This institution, perhaps one should say enterprise out of respect for which one says one need not change one's mind about a thing one has believed in, requiring public promises of one's intention to fulfill a private obligation;\n", "comaut" => "Now there are comments\n"])); xassert($tf->check_and_save($this->u_mgbaker, $paper17)); $rrow17b = fresh_review($paper17, $this->u_mgbaker); @@ -806,7 +806,7 @@ function test_review_history() { xassert($rrow17b->reviewModified > $rrow17a->reviewModified); xassert($rrow17b->reviewTime > $rrow17a->reviewTime); - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 4, "revexp" => 3, "papsum" => "This institution, perhaps one should say Starship Enterprise out of respect for which one says one need not change one's mind about a thing one has believed in, requiring public promises of one's intention to fulfill a private obligation;\n", "comaut" => "Now there are comments\n"])); xassert($tf->check_and_save($this->u_mgbaker, $paper17)); $rrow17c = fresh_review($paper17, $this->u_mgbaker); @@ -835,7 +835,7 @@ function test_review_history() { xassert_eqq($rrow17a->fidval("t02"), $rrow17a2->fidval("t02")); // restore original scores - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 2, "revexp" => 1])); xassert($tf->check_and_save($this->u_mgbaker, $paper17)); } @@ -869,7 +869,7 @@ function test_review_visibility() { xassert($user_external->can_view_review_identity($paper17, $rrow17m)); // per-round review visibility - $tf = new ReviewValues($conf->review_form()); + $tf = new ReviewValues($conf); xassert($tf->parse_json(["ovemer" => 2, "revexp" => 1, "papsum" => "Radical", "comaut" => "Nonradical"])); xassert($tf->check_and_save($this->u_lixia, $paper17)); MailChecker::check_db("test06-17lixia");