From e52b1fe5b8c1b84be62f9d78ae8fff96ab148cec Mon Sep 17 00:00:00 2001
From: jachamp <28732543+jimchamp@users.noreply.github.com>
Date: Tue, 26 Nov 2024 17:26:15 -0800
Subject: [PATCH] Address `TODO` items

- Sanitize tag description in tag view
- Update subject key derivation in tag view
- Reduce number of DB calls required to decorate a subject page with tags
- Tag edit `GET` handler returns edit form specific to the existing tag's type
- Use correct sub-type validations on tag edit
- Remove `utils.unflatten` call when creating tag
  - `utils.unflatten` is made specifically for processing the book edit form
---
 openlibrary/core/models.py                 |  2 +-
 openlibrary/plugins/upstream/addtag.py     | 18 +++++++++++++-----
 openlibrary/plugins/worksearch/subjects.py | 19 +++++++------------
 openlibrary/templates/type/tag/view.html   |  9 ++++-----
 4 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/openlibrary/core/models.py b/openlibrary/core/models.py
index 0180efc3b20d..c75860d814c4 100644
--- a/openlibrary/core/models.py
+++ b/openlibrary/core/models.py
@@ -1146,7 +1146,7 @@ def get_url_suffix(self):
 
     @classmethod
     def find(cls, tag_name, tag_type=None):
-        """Returns a Tag key for a given tag name and tag type."""
+        """Returns a Tag key for a given tag name."""
         q = {'type': '/type/tag', 'name': tag_name}
         if tag_type:
             q['tag_type'] = tag_type
diff --git a/openlibrary/plugins/upstream/addtag.py b/openlibrary/plugins/upstream/addtag.py
index e308e9ba651a..2d80890e37d3 100644
--- a/openlibrary/plugins/upstream/addtag.py
+++ b/openlibrary/plugins/upstream/addtag.py
@@ -10,7 +10,7 @@
 from infogami.utils import delegate
 
 from openlibrary.accounts import get_current_user
-from openlibrary.plugins.upstream import spamcheck, utils
+from openlibrary.plugins.upstream import spamcheck
 from openlibrary.plugins.upstream.models import Tag
 from openlibrary.plugins.upstream.addbook import safe_seeother, trim_doc
 from openlibrary.plugins.upstream.utils import render_template
@@ -159,6 +159,11 @@ def GET(self, key):
         if tag is None:
             raise web.notfound()
 
+        if tag.tag_type in SUBJECT_SUB_TYPES:
+            return render_template("type/tag/subject/edit", tag)
+        elif tag.tag_type == "collection":
+            return render_template("type/tag/collection/edit", tag)
+
         return render_template('type/tag/edit', tag)
 
     def POST(self, key):
@@ -169,8 +174,7 @@ def POST(self, key):
         i = web.input(_comment=None)
         formdata = self.process_input(i)
         try:
-            # TODO : use type-based validation here
-            if not formdata or not validate_subject_tag(formdata):
+            if not formdata or not self.validate(formdata, tag.tag_type):
                 raise web.badrequest()
             elif "_delete" in i:
                 tag = web.ctx.site.new(
@@ -187,11 +191,15 @@ def POST(self, key):
             return render_template("type/tag/edit", tag)
 
     def process_input(self, i):
-        # TODO : `unflatten` call not needed for tag form data
-        i = utils.unflatten(i)
         tag = trim_doc(i)
         return tag
 
+    def validate(self, data, tag_type):
+        if tag_type in SUBJECT_SUB_TYPES:
+            return validate_subject_tag(data)
+        else:
+            return validate_tag(data)
+
 
 class add_typed_tag(delegate.page):
     path = "/tag/([^/]+)/add"
diff --git a/openlibrary/plugins/worksearch/subjects.py b/openlibrary/plugins/worksearch/subjects.py
index 00e774b63f1c..d21a5f66f5f2 100644
--- a/openlibrary/plugins/worksearch/subjects.py
+++ b/openlibrary/plugins/worksearch/subjects.py
@@ -45,9 +45,7 @@ def GET(self, key):
             web.ctx.status = "404 Not Found"
             page = render_template('subjects/notfound.tmpl', key)
         else:
-            self.decorate_with_disambiguations(subj)
-            # TODO : use disambiguations to find linked tag
-            self.decorate_with_tag(subj)
+            self.decorate_with_tags(subj)
             page = render_template("subjects", page=subj)
 
         return page
@@ -62,17 +60,14 @@ def normalize_key(self, key):
             key = key.replace("/times/", "/time:")
         return key
 
-    def decorate_with_disambiguations(self, subject):
-        matches = Tag.find(subject.name)
-        if matches:
-            tags = web.ctx.site.get_many(matches)
+    def decorate_with_tags(self, subject) -> None:
+        tag_keys = Tag.find(subject.name)
+        if tag_keys:
+            tags = web.ctx.site.get_many(tag_keys)
             subject.disambiguations = tags
 
-    def decorate_with_tag(self, subject):
-        matches = Tag.find(subject.name, tag_type=subject.subject_type)
-        if matches:
-            tag = web.ctx.site.get(matches[0])
-            subject.tag = tag
+            if filtered_tags := [tag for tag in tags if tag.tag_type == subject.subject_type]:
+                subject.tag = filtered_tags[0]
 
 
 class subjects_json(delegate.page):
diff --git a/openlibrary/templates/type/tag/view.html b/openlibrary/templates/type/tag/view.html
index a44ef3b0bc50..e6a48f0ed479 100644
--- a/openlibrary/templates/type/tag/view.html
+++ b/openlibrary/templates/type/tag/view.html
@@ -8,8 +8,8 @@
         key = "/subjects/"
         if tag.tag_type != "subject":
             key += "%s:" % tag.tag_type
-        # TODO : normalize the same as subject label URLs
         key += tag.name.replace(" ", "_")
+        key = key.lower()
         return key
 
 <div id="contentHead">
@@ -17,10 +17,9 @@
     <h1 itemprop="name">
         $title
     </h1>
-    <h3 itemprop="description">
-        $# TODO : sanitize and format
-        $tag.tag_description
-    </h3>
+    <div itemprop="description">
+        $:sanitize(format(tag.tag_description))
+    </div>
     $if tag.tag_type in get_subject_tag_types():
         <div>
             $:sanitize(format(tag.body))