ReviewValues API update.

kohler · Jun 24, 2024 · 5d50b3b · 5d50b3b
1 parent 1634d24
commit 5d50b3b
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -886,23 +886,37 @@ 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];
         }
         $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
@@ -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
@@ -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,20 +155,20 @@ 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");
         xassert($tf->has_problem_at("s01"));
 
         // 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 <mgbaker193r8219@butt.com>', preg_replace('/^4/m', "5", $this->review1A)), "review1A-5.txt");
+        $tf = (new ReviewValues($this->conf))->set_text(preg_replace('/Reviewer: .*/m', 'Reviewer: Mary Baaaker <mgbaker193r8219@butt.com>', 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 <mgbaker193r8219@butt.com>', 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 <mgbaker193r8219@butt.com>', 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,15 +768,15 @@ 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));
 
         $rrow = fresh_review($paper18, $user_diot);
         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");