Tagger errors: Improve them, change API to access them.

And display errors when editing tags on a paper list.
ubc · Mar 6, 2021 · 2d25c64 · 2d25c64
1 parent 8120bce
commit 2d25c64
Show file tree

Hide file tree

Showing 15 changed files with 115 additions and 50 deletions.
diff --git a/autoassign.php b/autoassign.php
@@ -259,7 +259,7 @@ function __construct(Contact $user, Qrequest $qreq, SearchSelection $ssel) {
             if (($tag = $tagger->check($tag, Tagger::NOVALUE))) {
                 $this->discordertag = $tag;
             } else {
-                $this->errors["discordertag"] = $tagger->error_html;
+                $this->errors["discordertag"] = $tagger->error_html();
             }
         }
 

diff --git a/lib/tagger.php b/lib/tagger.php
@@ -580,6 +580,7 @@ function is_autosearch($tag) {
     }
 
 
+    /** @return string */
     private function sitewide_regex_part() {
         if ($this->sitewide_re_part === null) {
             $x = [];
@@ -591,6 +592,7 @@ private function sitewide_regex_part() {
         return $this->sitewide_re_part;
     }
 
+    /** @return string */
     private function hidden_regex_part() {
         if ($this->hidden_re === null) {
             $x = [];
@@ -602,6 +604,7 @@ private function hidden_regex_part() {
         return $this->hidden_re;
     }
 
+    /** @return string */
     private function public_peruser_regex_part() {
         if ($this->public_peruser_re_part === null) {
             $x = [];
@@ -1060,15 +1063,21 @@ class Tagger {
     const ALLOWSTAR = 16;
     const ALLOWCONTACTID = 32;
     const NOTAGKEYWORD = 64;
-    const CHECKVERBOSE = 128;
+    const EEMPTY = -1;
+    const EINVAL = -2;
+    const EMULTIPLE = -3;
+    const E2BIG = -4;
 
-    public $error_html = false;
     /** @var Conf */
     private $conf;
     /** @var Contact */
     private $contact;
     /** @var int */
     private $_contactId = 0;
+    /** @var int */
+    private $errcode = 0;
+    /** @var ?string */
+    private $errtag;
 
     /** @readonly */
     private static $value_increment_map = [1, 1, 1, 1, 1, 2, 2, 2, 3, 4];
@@ -1134,12 +1143,49 @@ static function value_increment($sequential) {
     }
 
 
-    /** @return false */
-    private function set_error_html($tag, $flags, $e) {
-        if (($flags & self::CHECKVERBOSE) && str_ends_with($e, ".")) {
-            $e = substr($e, 0, -1) . " “" . htmlspecialchars($tag) . "”.";
+    /** @param bool $verbose
+     * @return ?string */
+    function error_html($verbose = false) {
+        $t = $verbose ? " “" . htmlspecialchars($this->errtag ?? "") . "”" : "";
+        switch ($this->errcode) {
+        case 0:
+            return null;
+        case self::EEMPTY:
+            return "Tag missing.";
+        case self::EMULTIPLE:
+            return "Expected a single tag.";
+        case self::E2BIG:
+            return "Tag too long.";
+        case self::ALLOWSTAR:
+            return "Invalid tag{$t} (stars aren’t allowed here).";
+        case self::NOCHAIR:
+            if ($this->contact->privChair) {
+                return "Invalid tag{$t} (chair tags aren’t allowed here).";
+            } else {
+                return "Invalid tag{$t} (tag reserved for chair).";
+            }
+        case self::NOPRIVATE:
+            return "Private tags aren’t allowed here.";
+        case self::ALLOWCONTACTID:
+            if ($verbose && ($twiddle = strpos($this->errtag ?? "", "~"))) {
+                return "Invalid tag{$t} (did you mean “#" . substr($this->errtag, $twiddle) . "”?).";
+            } else {
+                return "Invalid private tag.";
+            }
+        case self::NOVALUE:
+            return "Tag values aren’t allowed here.";
+        case self::ALLOWRESERVED:
+            return $verbose ? "Tag{$t} is reserved." : "Tag reserved.";
+        case self::EINVAL:
+        default:
+            return "Invalid tag{$t}.";
         }
-        $this->error_html = $e;
+    }
+
+    /** @return false */
+    private function set_error_code($tag, $errcode) {
+        $this->errcode = $errcode;
+        $this->errtag = $tag;
         return false;
     }
 
@@ -1148,7 +1194,7 @@ private function set_error_html($tag, $flags, $e) {
      * @return string|false */
     function check($tag, $flags = 0) {
         if ($tag === null || $tag === "" || $tag === "#") {
-            return $this->set_error_html($tag, 0, "Tag missing.");
+            return $this->set_error_code($tag, self::EEMPTY);
         }
         if (!$this->contact->privChair) {
             $flags |= self::NOCHAIR;
@@ -1159,47 +1205,48 @@ function check($tag, $flags = 0) {
         if (!preg_match('/\A(|~|~~|[1-9][0-9]*~)(' . TAG_REGEX_NOTWIDDLE . ')(|[#=](?:-?\d+(?:\.\d*)?|-?\.\d+|))\z/', $tag, $m)) {
             if (preg_match('/\A([-a-zA-Z0-9!@*_:.\/#=]+)[\s,]+\S+/', $tag, $m)
                 && $this->check($m[1], $flags)) {
-                return $this->set_error_html($tag, $flags, "Expected a single tag.");
+                return $this->set_error_code($tag, self::EMULTIPLE);
             } else {
-                return $this->set_error_html($tag, $flags, "Invalid tag.");
+                return $this->set_error_code($tag, self::EINVAL);
             }
         }
         if (!($flags & self::ALLOWSTAR) && strpos($tag, "*") !== false) {
-            return $this->set_error_html($tag, $flags, "Wildcards aren’t allowed here.");
+            return $this->set_error_code($tag, self::ALLOWSTAR);
         }
         // After this point we know `$tag` contains no HTML specials
         if ($m[1] === "") {
             // OK
         } else if ($m[1] === "~~") {
             if ($flags & self::NOCHAIR) {
-                return $this->set_error_html($tag, $flags, "Tag exclusively for chairs.");
+                return $this->set_error_code($tag, self::NOCHAIR);
             }
         } else {
             if ($flags & self::NOPRIVATE) {
-                return $this->set_error_html($tag, $flags, "Twiddle tags aren’t allowed here.");
+                return $this->set_error_code($tag, self::NOPRIVATE);
             } else if ($m[1] === "~") {
                 if ($this->_contactId) {
                     $m[1] = $this->_contactId . "~";
                 }
             } else if ($m[1] !== $this->_contactId . "~"
                        && !($flags & self::ALLOWCONTACTID)) {
-                return $this->set_error_html($tag, $flags, "Reference to another user’s twiddle tag.");
+                return $this->set_error_code($tag, self::ALLOWCONTACTID);
             }
         }
         if ($m[3] !== "" && ($flags & self::NOVALUE)) {
-            return $this->set_error_html($tag, $flags, "Tag values aren’t allowed here.");
+            return $this->set_error_code($tag, self::NOVALUE);
         }
         if (!($flags & self::ALLOWRESERVED)
             && (!strcasecmp("none", $m[2]) || !strcasecmp("any", $m[2]))) {
-            return $this->set_error_html($tag, $flags, "Reserved tag.");
+            return $this->set_error_code($tag, self::ALLOWRESERVED);
         }
         $t = $m[1] . $m[2];
         if (strlen($t) > TAG_MAXLEN) {
-            return $this->set_error_html($tag, $flags, "Tag too long.");
+            return $this->set_error_code($tag, self::E2BIG);
         }
         if ($m[3] !== "") {
             $t .= "#" . substr($m[3], 1);
         }
+        $this->errcode = 0;
         return $t;
     }
 

diff --git a/scripts/script.js b/scripts/script.js
@@ -3773,18 +3773,33 @@ function setajaxcheck(elt, rv) {
             }
     }
     make_outline_flasher(elt);
-    if (!rv || (!rv.ok && !rv.error))
-        rv = {error: "Error"};
-    if (rv.ok && !rv.warning)
+    var t = "", i, status = 0;
+    if (rv && rv.message_list) {
+        for (var i = 0; i !== rv.message_list.length; ++i) {
+            t += render_feedback(rv.message_list[i].message, rv.message_list[i].status);
+            status = Math.max(status, rv.message_list[i].status);
+        }
+    }
+    if (t === "") {
+        if (rv && rv.error) {
+            t = rv.error;
+            status = 2;
+        } else if (rv && rv.warning) {
+            t = rv.warning;
+            status = rv.ok ? 1 : 2;
+        } else if (!rv || !rv.ok) {
+            t = "Error";
+            status = 2;
+        }
+    }
+    if (status === 0)
         make_outline_flasher(elt, "0, 200, 0");
-    else if (rv.ok)
+    else if (status === 1)
         make_outline_flasher(elt, "133, 92, 4");
     else
         elt.style.outline = "5px solid red";
-    if (rv.error)
-        make_bubble(rv.error, "errorbubble").near(elt).removeOn(elt, "input change click hide");
-    else if (rv.warning)
-        make_bubble(rv.warning, "warningbubble").near(elt).removeOn(elt, "input change click hide focus blur");
+    if (t)
+        make_bubble(t, status === 2 ? "errorbubble" : "warningbubble").near(elt).removeOn(elt, "input change click hide" + (status === 2 ? "" : " focus blur"));
 }
 
 function link_urls(t) {
@@ -6646,8 +6661,9 @@ function make_tag_save_callback(elt) {
 
 function set_tags_callback(evt, data) {
     var si = analyze_rows(data.pid);
-    if (highlight_entries && rowanal[si].entry)
+    if (highlight_entries && rowanal[si].entry) {
         setajaxcheck(rowanal[si].entry, {ok: true});
+    }
     row_move(si);
 }
 
@@ -8275,8 +8291,9 @@ function save_pstags(evt) {
         success: function (data) {
             $f.find("input").prop("disabled", false);
             if (data.ok) {
-                if (!data.message_list || !data.message_list.length)
+                if (!data.message_list || !data.message_list.length) {
                     foldup.call($f[0], null, {f: true});
+                }
                 $(window).trigger("hotcrptags", [data]);
                 removeClass(f.elements.tags, "has-error");
                 removeClass(f.elements.save, "btn-highlight");

diff --git a/src/api/api_taganno.php b/src/api/api_taganno.php
@@ -6,7 +6,7 @@ class TagAnno_API {
     static function get(Contact $user, Qrequest $qreq) {
         $tagger = new Tagger($user);
         if (!($tag = $tagger->check($qreq->tag, Tagger::NOVALUE))) {
-            return ["ok" => false, "error" => $tagger->error_html];
+            return ["ok" => false, "error" => $tagger->error_html()];
         }
         $j = ["ok" => true, "tag" => $tag, "editable" => $user->can_change_tag_anno($tag),
               "anno" => []];
@@ -21,7 +21,7 @@ static function get(Contact $user, Qrequest $qreq) {
     static function set(Contact $user, Qrequest $qreq) {
         $tagger = new Tagger($user);
         if (!($tag = $tagger->check($qreq->tag, Tagger::NOVALUE))) {
-            return ["ok" => false, "error" => $tagger->error_html];
+            return ["ok" => false, "error" => $tagger->error_html()];
         }
         if (!$user->can_change_tag_anno($tag)) {
             return ["ok" => false, "error" => "Permission error."];

diff --git a/src/api/api_tags.php b/src/api/api_tags.php
@@ -164,7 +164,7 @@ static function settags_api(Contact $user, $qreq, $prow) {
     static function votereport_api(Contact $user, Qrequest $qreq, PaperInfo $prow) {
         $tagger = new Tagger($user);
         if (!($tag = $tagger->check($qreq->tag, Tagger::NOVALUE))) {
-            return ["ok" => false, "error" => $tagger->error_html];
+            return ["ok" => false, "error" => $tagger->error_html()];
         }
         if (!$user->can_view_peruser_tag($prow, $tag)) {
             return ["ok" => false, "error" => "Permission error."];

diff --git a/src/assigners/a_tag.php b/src/assigners/a_tag.php
@@ -155,7 +155,7 @@ private function apply1($tag, PaperInfo $prow, Contact $contact, $req,
         $xvalue = trim((string) $req["tag_value"]);
         if (!preg_match('/\A([-+]?#?)(|~~|[^-~+#]*~)([a-zA-Z@*_:.][-+a-zA-Z0-9!@*_:.\/]*)(\z|#|#?[=!<>]=?|#?≠|#?≤|#?≥)(.*)\z/', $tag, $m)
             || ($m[4] !== "" && $m[4] !== "#")) {
-            $state->error("“" . htmlspecialchars($tag) . "”: Invalid tag.");
+            $state->error("Invalid tag “" . htmlspecialchars($tag) . "”.");
             return false;
         } else if ($xvalue !== "" && $m[5] !== "") {
             $state->error("“" . htmlspecialchars($tag) . "”: You have a <code>tag value</code> column, so the tag value specified here is ignored.");
@@ -164,7 +164,7 @@ private function apply1($tag, PaperInfo $prow, Contact $contact, $req,
             $state->warning("“" . htmlspecialchars($tag) . "”: Tag values ignored when removing a tag.");
         } else if (($this->remove && str_starts_with($m[1], "+"))
                    || ($this->remove === false && str_starts_with($m[1], "-"))) {
-            $state->error("“" . htmlspecialchars($tag) . "” is incompatible with this action.");
+            $state->error("Tag “" . htmlspecialchars($tag) . "” is incompatible with this action.");
             return false;
         }
 
@@ -238,7 +238,7 @@ private function apply1($tag, PaperInfo $prow, Contact $contact, $req,
 
         // otherwise handle adds
         if (strpos($xtag, "*") !== false) {
-            $state->error("“" . htmlspecialchars($tag) . "”: Wildcards aren’t allowed here.");
+            $state->error("Invalid tag “" . htmlspecialchars($tag) . "” (stars aren’t allowed here).");
             return false;
         }
         if ($xuser !== ""
@@ -256,8 +256,8 @@ private function apply1($tag, PaperInfo $prow, Contact $contact, $req,
             $xuser = $twiddlecids[0] . "~";
         }
         $tagger = new Tagger($state->user);
-        if (!$tagger->check($xtag, Tagger::CHECKVERBOSE)) {
-            $state->error($tagger->error_html);
+        if (!$tagger->check($xtag)) {
+            $state->error($tagger->error_html(true));
             return false;
         }
 

diff --git a/src/listactions/la_getrank.php b/src/listactions/la_getrank.php
@@ -70,7 +70,7 @@ function run(Contact $user, Qrequest $qreq, SearchSelection $ssel) {
                     . "# " . $user->conf->hoturl_absolute("offline", null, Conf::HOTURL_RAW) . "\n\n"
                     . $real . ($real === "" ? "" : "\n") . $null);
         } else {
-            Conf::msg_error($tagger->error_html);
+            Conf::msg_error($tagger->error_html());
         }
     }
 }
diff --git a/src/listactions/la_tag.php b/src/listactions/la_tag.php
@@ -100,7 +100,7 @@ function run(Contact $user, Qrequest $qreq, SearchSelection $ssel) {
                     $qreq->q = "order:$tagreq";
                 }
             } else {
-                $assignset->error($tagger->error_html);
+                $assignset->error($tagger->error_html());
             }
         }
         if (($errors = $assignset->messages_div_html())) {

diff --git a/src/settings/s_tags.php b/src/settings/s_tags.php
@@ -105,14 +105,14 @@ static function parse_list(Tagger $tagger, SettingValues $sv, Si $si,
                                $checkf, $min_idx) {
         $ts = array();
         foreach (preg_split('/\s+/', $sv->reqv($si->name)) as $t) {
-            if ($t !== "" && ($tx = $tagger->check($t, $checkf | Tagger::CHECKVERBOSE))) {
+            if ($t !== "" && ($tx = $tagger->check($t, $checkf))) {
                 list($tag, $idx) = Tagger::unpack($tx);
                 if ($min_idx) {
                     $tx = $tag . "#" . max($min_idx, (float) $idx);
                 }
                 $ts[$tag] = $tx;
             } else if ($t !== "") {
-                $sv->error_at($si, $tagger->error_html);
+                $sv->error_at($si, $tagger->error_html(true));
             }
         }
         return array_values($ts);

diff --git a/src/settings/s_tracks.php b/src/settings/s_tracks.php
@@ -255,10 +255,11 @@ function parse(SettingValues $sv, Si $si) {
             }
             $trackname = $tagger->check($trackname, Tagger::NOPRIVATE | Tagger::NOCHAIR | Tagger::NOVALUE);
             if (!$trackname || ($trackname === "_" && $i !== 1)) {
-                if ($trackname !== "_")
-                    $sv->error_at("name_track$i", $tagger->error_html);
-                else
+                if ($trackname !== "_") {
+                    $sv->error_at("name_track$i", $tagger->error_html());
+                } else {
                     $sv->error_at("name_track$i", "Track name “_” is reserved.");
+                }
                 $sv->error_at("tracks");
                 $ok = false;
             }
@@ -277,7 +278,7 @@ function parse(SettingValues $sv, Si $si) {
                     } else if (($ttag = $tagger->check($ttag, Tagger::NOPRIVATE | Tagger::NOCHAIR | Tagger::NOVALUE))) {
                         $t->$type = $ttype . $ttag;
                     } else {
-                        $sv->error_at("{$type}_track$i", "Track permission tag: " . $tagger->error_html);
+                        $sv->error_at("{$type}_track$i", "Track permission tag: " . $tagger->error_html());
                         $sv->error_at("{$type}_tag_track$i");
                         $sv->error_at("tracks");
                     }

diff --git a/src/settingvalues.php b/src/settingvalues.php
@@ -1447,7 +1447,7 @@ function parse_value(Si $si) {
             if ($v) {
                 return $v;
             }
-            $err = $tagger->error_html;
+            $err = $tagger->error_html();
         } else if ($si->type === "emailheader") {
             $mt = new MimeText;
             $v = $mt->encode_email_header("", $v);

diff --git a/src/tagrankparser.php b/src/tagrankparser.php
@@ -62,14 +62,14 @@ function parse($text, $filename = null) {
                     $tag = $t;
                     $curIndex = 0;
                 } else {
-                    $settings[] = [null, null, $landmark, "Bad tag: $tagger->error_html", null];
+                    $settings[] = [null, null, $landmark, "Bad tag: " . $tagger->error_html(), null];
                 }
             } else if ($pid === "tag") {
                 if (($t = $tagger->check($idxs, Tagger::NOVALUE))) {
                     $tag = $t;
                     $curIndex = 0;
                 } else {
-                    $settings[] = [null, null, $landmark, "Bad tag: $tagger->error_html", null];
+                    $settings[] = [null, null, $landmark, "Bad tag: " . $tagger->error_html(), null];
                 }
             } else {
                 if ($idxs === "X" || $idxs === "x" || $idxs === "clear") {