Merge pull request #161 from sparkiegeek/active-topic-fix

Don't mutate shared navigation model between requests

canonicalwebteam/discourse/parsers/docs.py

@@ -1,4 +1,5 @@
 # Standard library
+import copy
 import os
 import re
 from urllib.parse import urlparse
@@ -29,9 +30,9 @@ def __init__(
         tutorials_index_topic_id=None,
         tutorials_url_prefix=None,
     ):
-        self.active_topic = None
+        self.active_topic_id = None
         self.versions = []
-        self.navigations = []
+        self.navigations = {}
         self.url_map_versions = {}
 
         # Tutorials
@@ -99,7 +100,7 @@ def parse_topic(self, topic, docs_version=""):
                     (e.g. "3 days ago")
         - forum_link: The link to the original forum post
         """
-        self.active_topic = topic
+        self.active_topic_id = topic["id"]
 
         updated_datetime = dateutil.parser.parse(
             topic["post_stream"]["posts"][0]["updated_at"]
@@ -272,7 +273,9 @@ def _generate_url_map(self, navigations):
                     pretty_path = url_prefix + pretty_path
 
                 if not topic_match or not pretty_path.startswith(url_prefix):
-                    self.warnings.append("Could not parse URL map item {item}")
+                    self.warnings.append(
+                        f"Could not parse URL map item {item}"
+                    )
                     continue
 
                 topic_id = int(topic_match.groupdict()["topic_id"])
@@ -553,7 +556,10 @@ def _has_active_nav_child(self, item):
         return False
 
     def _generate_navigation(self, navigations, version_path):
-        navigation = navigations[version_path]
+        # we mutate the navigations[version_path] dictionary and so to
+        # avoid retaining state between document views, we need to
+        # take a (deep)copy.
+        navigation = copy.deepcopy(navigations[version_path])
 
         # Replace links with url_map
         for item in navigation["nav_items"]:
@@ -570,7 +576,7 @@ def _generate_navigation(self, navigations, version_path):
                         item["navlink_href"] = href
 
                 # Check if given item should be marked as active
-                if topic_id == self.active_topic["id"]:
+                if topic_id == self.active_topic_id:
                     item["is_active"] = True
 
         # Generate tree structure with levels

setup.py

@@ -4,7 +4,7 @@
 
 setup(
     name="canonicalwebteam.discourse",
-    version="5.4.0",
+    version="5.4.1",
     author="Canonical webteam",
     author_email="webteam@canonical.com",
     url="https://github.com/canonical/canonicalwebteam.discourse",

tests/test_parser.py

@@ -292,6 +292,60 @@ def test_nav(self):
         self.assertEqual(page_z["path"], "/page-z")
         self.assertEqual(page_z["navlink_text"], "Page Z")
 
+    def test_active_topic(self):
+        for page_id in [10, 26]:
+            httpretty.register_uri(
+                httpretty.GET,
+                f"https://discourse.example.com/t/{page_id}.json",
+                body=json.dumps(
+                    {
+                        "id": page_id,
+                        "category_id": 2,
+                        "title": "A topic page",
+                        "slug": "a-page",
+                        "post_stream": {
+                            "posts": [
+                                {
+                                    "id": 3434,
+                                    "cooked": "<h1>Content</h1>",
+                                    "updated_at": "2023-04-01T12:34:56.789Z",
+                                }
+                            ]
+                        },
+                    }
+                ),
+                content_type="application/json",
+            )
+        self.assertEqual(self.parser.active_topic_id, 34)
+        root_navigation = self.parser.navigation
+
+        root_page_a = root_navigation["nav_items"][0]
+        self.assertFalse(root_page_a["is_active"])
+        self.assertFalse(root_page_a["has_active_child"])
+
+        # Simulate clicking on child page
+        self.parser.parse_topic(self.parser.api.get_topic(10))
+        self.assertEqual(self.parser.active_topic_id, 10)
+        child = self.parser.navigation["nav_items"][0]
+        self.assertTrue(child["is_active"])
+        self.assertFalse(child["has_active_child"])
+
+        # Simulate clicking on grand-child page
+        self.parser.parse_topic(self.parser.api.get_topic(26))
+        self.assertEqual(self.parser.active_topic_id, 26)
+        child = self.parser.navigation["nav_items"][0]
+        self.assertFalse(child["is_active"])
+        self.assertTrue(child["has_active_child"])
+        grandchild = child["children"][0]
+        self.assertTrue(grandchild["is_active"])
+        self.assertFalse(grandchild["has_active_child"])
+
+        # Simulate clicking on root
+        self.parser.parse_topic(self.parser.index_topic)
+        child = self.parser.navigation["nav_items"][0]
+        self.assertFalse(child["is_active"])
+        self.assertFalse(child["has_active_child"])
+
     def test_versions(self):
         versions = self.parser.versions
         self.assertEqual(len(versions), 1)