Fix a rare problem with auto-add topics

There was a problem (unlikely to ever happen, but a problem) with secondary topics options, or with multiple PaperStatus objects. Store auto-add topics *in the topic set*.
kohler · Nov 30, 2024 · 948817b · 948817b
1 parent ced8eaa
commit 948817b
Show file tree

Hide file tree

Showing 6 changed files with 166 additions and 102 deletions.
diff --git a/batch/savepapers.php b/batch/savepapers.php
@@ -313,10 +313,8 @@ private function _run_main(&$jl) {
         $this->prefetch_authors($jl);
 
         if ($this->add_topics) {
-            foreach ($this->conf->options()->form_fields() as $opt) {
-                if ($opt instanceof Topics_PaperOption)
-                    $opt->allow_new_topics(true);
-            }
+            $this->conf->topic_set()->set_auto_add(true);
+            $this->conf->invalidate_topics();
         }
         if ($this->silent) {
             foreach ($this->conf->options()->form_fields() as $opt) {

diff --git a/src/options/o_checkboxesbase.php b/src/options/o_checkboxesbase.php
@@ -86,10 +86,6 @@ function value_export_json(PaperValue $ov, PaperExport $pex) {
     }
 
     function value_store(PaperValue $ov, PaperStatus $ps) {
-        if ($ov->has_anno("new_values") && count($ov->anno("new_values")) > 0) {
-            $this->value_store_new_values($ov, $ps);
-        }
-
         $vs = $ov->value_list();
         $this->topic_set()->sort($vs); // to reduce unnecessary diffs
         $ov->set_value_data($vs, array_fill(0, count($vs), null));
@@ -100,9 +96,6 @@ function value_store(PaperValue $ov, PaperStatus $ps) {
         }
     }
 
-    function value_store_new_values(PaperValue $ov, PaperStatus $ps) {
-    }
-
     function parse_qreq(PaperInfo $prow, Qrequest $qreq) {
         $vs = [];
         foreach ($this->topic_set() as $tid => $tname) {
@@ -155,18 +148,22 @@ function parse_json(PaperInfo $prow, $j) {
                 $tid = $topicset->find_exact($tk);
                 if ($tid !== null) {
                     $vs[] = $tid;
+                } else if (empty($tids) && $topicset->auto_add()) {
+                    $newvs[] = $tk;
+                    $topicset->mark_auto_add($tk);
                 } else {
                     $badvs[] = $tk;
-                    if (empty($tids)) {
-                        $newvs[] = $tk;
-                    }
                 }
             }
         }
 
         $ov = PaperValue::make_multi($prow, $this, $vs, array_fill(0, count($vs), null));
-        $ov->set_anno("bad_values", $badvs);
-        $ov->set_anno("new_values", $newvs);
+        if (!empty($badvs)) {
+            $ov->set_anno("bad_values", $badvs);
+        }
+        if (!empty($newvs)) {
+            $ov->set_anno("new_values", $newvs);
+        }
         return $ov;
     }
 

diff --git a/src/options/o_topics.php b/src/options/o_topics.php
@@ -3,12 +3,10 @@
 // Copyright (c) 2006-2023 Eddie Kohler; see LICENSE.
 
 class Topics_PaperOption extends CheckboxesBase_PaperOption {
-    /** @var bool */
-    private $_add_topics = false;
-
     function __construct(Conf $conf, $args) {
         parent::__construct($conf, $args);
-        if (!$this->conf->has_topics()) {
+        $topic_set = $this->conf->topic_set();
+        if ($topic_set->count() === 0 && !$topic_set->auto_add()) {
             $this->override_exists_condition(false);
         }
     }
@@ -21,54 +19,45 @@ function interests($user) {
         return $user ? $user->topic_interest_map() : [];
     }
 
-    /** @param bool $allow */
-    function allow_new_topics($allow) {
-        $this->_add_topics = $allow;
-        $x = $allow || $this->conf->has_topics() ? null : false;
-        $this->override_exists_condition($x);
+    function value_force(PaperValue $ov) {
+        if ($this->id === PaperOption::TOPICSID) {
+            $vs = $ov->prow->topic_list();
+            $ov->set_value_data($vs, array_fill(0, count($vs), null));
+        }
     }
 
-    function value_store_new_values(PaperValue $ov, PaperStatus $ps) {
-        if (!$this->_add_topics) {
-            return;
-        }
+    private function _store_new_values(PaperValue $ov, PaperStatus $ps) {
+        $this->topic_set()->commit_auto_add();
         $vs = $ov->value_list();
         $newvs = $ov->anno("new_values");
         '@phan-var list<string> $newvs';
-        $lctopics = $newids = [];
         foreach ($newvs as $tk) {
-            if (in_array(strtolower($tk), $lctopics)) {
-                continue;
+            if (($tid = $this->topic_set()->find_exact($tk)) !== null) {
+                $vs[] = $tid;
             }
-            $lctopics[] = strtolower($tk);
-            $result = $ps->conf->qe("insert into TopicArea set topicName=?", $tk);
-            $vs[] = $result->insert_id;
-        }
-        if (!$this->conf->has_topics()) {
-            $this->conf->save_refresh_setting("has_topics", 1);
         }
-        $this->conf->invalidate_topics();
+        $this->topic_set()->sort($vs); // to reduce unnecessary diffs
         $ov->set_value_data($vs, array_fill(0, count($vs), null));
-        $ov->set_anno("bad_values", array_values(array_diff($ov->anno("bad_values"), $newvs)));
-    }
-
-
-    function value_force(PaperValue $ov) {
-        if ($this->id === PaperOption::TOPICSID) {
-            $vs = $ov->prow->topic_list();
-            $ov->set_value_data($vs, array_fill(0, count($vs), null));
-        }
+        $ov->set_anno("new_values", null);
     }
 
     function value_save(PaperValue $ov, PaperStatus $ps) {
-        if ($ov->equals($ov->prow->base_option($this->id))) {
+        if (!$ov->anno("new_values")
+            && $ov->equals($ov->prow->base_option($this->id))) {
             return true;
         }
-        if ($this->id !== PaperOption::TOPICSID) {
-            return false;
+        if ($ov->anno("new_values")) {
+            if ($ps->save_status() < PaperStatus::SAVE_STATUS_PREPARED) {
+                $ps->request_resave($this);
+                $ps->change_at($this);
+            } else {
+                $this->_store_new_values($ov, $ps);
+            }
         }
         $ps->change_at($this);
-        $ov->prow->set_prop("topicIds", join(",", $ov->value_list()));
+        if ($this->id === PaperOption::TOPICSID) {
+            $ov->prow->set_prop("topicIds", join(",", $ov->value_list()));
+        }
         return true;
     }
 }
diff --git a/src/paperstatus.php b/src/paperstatus.php
@@ -34,6 +34,8 @@ class PaperStatus extends MessageSet {
     private $_fdiffs;
     /** @var list<string> */
     private $_xdiffs;
+    /** @var ?list<PaperOption> */
+    private $_resave_fields;
     /** @var associative-array<int,array{int,int,int}> */
     private $_conflict_values;
     /** @var ?list<array{int,int,int}> */
@@ -644,6 +646,11 @@ private function status_change_at($field) {
         }
     }
 
+    /** @param PaperOption $field */
+    function request_resave($field) {
+        $this->_resave_fields[] = $field;
+    }
+
     /** @return bool */
     function has_change() {
         return !empty($this->_fdiffs) || !empty($this->_xdiffs);
@@ -1012,7 +1019,7 @@ private function _reset(PaperInfo $prow, $pid) {
         $this->prow = $prow;
         $this->paperId = $this->title = null;
         $this->_fdiffs = $this->_xdiffs = [];
-        $this->_unknown_fields = null;
+        $this->_unknown_fields = $this->_resave_fields = null;
         $this->_conflict_values = [];
         $this->_conflict_ins = $this->_created_contacts = null;
         $this->_author_change_cids = null;
@@ -1461,6 +1468,12 @@ function execute_save() {
         assert($this->paperId === null);
         $this->_save_status = self::SAVE_STATUS_SAVED;
 
+        // call back to fields that need a second store pass
+        // (this stage must not error)
+        foreach ($this->_resave_fields ?? [] as $opt) {
+            $opt->value_save($this->prow->option($opt), $this);
+        }
+
         if ($this->prow->prop_changed()) {
             $modified_at = max(Conf::$now, $this->prow->timeModified + 1);
             $this->prow->set_prop("timeModified", $modified_at);