fix: various fixes

* generate_superset_context expects a course_id
* superset_guest_token_url should be absolute to LMS
* CSRF cookie fix from #28
* XBlock uses handler instead of view
* view handles Superset server errors

platform_plugin_aspects/extensions/filters.py

@@ -40,6 +40,7 @@ def run_filter(
         if formatted_language not in settings.SUPERSET_DASHBOARD_LOCALES:
             formatted_language = "en_US"
 
+        context["course_id"] = course.id
         context = generate_superset_context(
             context,
             dashboards=dashboards,
@@ -55,7 +56,8 @@ def run_filter(
             "fragment": frag,
             "section_key": BLOCK_CATEGORY,
             "section_display_name": _("Analytics"),
-            "course_id": str(course.id),
+            "course_id": str(context.get("course_id")),
+            "superset_guest_token_url": str(context.get("superset_guest_token_url")),
             "superset_url": str(context.get("superset_url")),
             "template_path_prefix": TEMPLATE_ABSOLUTE_PATH,
         }

platform_plugin_aspects/extensions/tests/test_filters.py

@@ -20,66 +20,62 @@ def setUp(self) -> None:
         """
         self.filter = AddSupersetTab(filter_type=Mock(), running_pipeline=Mock())
         self.template_name = "test-template-name"
-        self.context = {"course": Mock()}
+        self.course_id = "course-v1:org+course+run"
+        self.context = {"course": Mock(id=self.course_id), "sections": []}
 
-    @patch("platform_plugin_aspects.extensions.filters.generate_superset_context")
     @patch("platform_plugin_aspects.extensions.filters.get_model")
     def test_run_filter_with_language(
-        self, mock_get_model, mock_generate_superset_context
+        self,
+        mock_get_model,
     ):
         """
         Check the filter is not executed when there are no LimeSurvey blocks in the course.
 
         Expected result:
             - The context is returned without modifications.
         """
-        mock_generate_superset_context.return_value = {
-            "sections": [],
-            "superset_url": "http://superset.testing",
-        }
-
         mock_get_model.return_value.get_value.return_value = "not-a-language"
 
         context = self.filter.run_filter(self.context, self.template_name)
 
         self.assertDictContainsSubset(
             {
-                "course_id": str(self.context["course"].id),
+                "course_id": self.course_id,
                 "section_key": BLOCK_CATEGORY,
                 "section_display_name": "Analytics",
-                "superset_url": "http://superset.testing",
+                "superset_url": "http://superset-dummy-url/",
+                "superset_guest_token_url": f"https://lms.url/superset_guest_token/{self.course_id}",
                 "template_path_prefix": "/instructor_dashboard/",
             },
             context["context"]["sections"][0],
         )
+        mock_get_model.assert_called_once()
 
-    @patch("platform_plugin_aspects.extensions.filters.generate_superset_context")
     @patch("platform_plugin_aspects.extensions.filters.get_model")
     def test_run_filter_without_language(
-        self, mock_get_model, mock_generate_superset_context
+        self,
+        mock_get_model,
     ):
         """
         Check the filter is not executed when there are no LimeSurvey blocks in the course.
 
         Expected result:
             - The context is returned without modifications.
         """
-        mock_generate_superset_context.return_value = {
-            "sections": [],
-            "superset_url": "http://superset.testing",
-        }
-
         mock_get_model.return_value.get_value.return_value = None
 
         context = self.filter.run_filter(self.context, self.template_name)
 
         self.assertDictContainsSubset(
             {
-                "course_id": str(self.context["course"].id),
+                "course_id": self.course_id,
                 "section_key": BLOCK_CATEGORY,
                 "section_display_name": "Analytics",
-                "superset_url": "http://superset.testing",
+                "superset_url": "http://superset-dummy-url/",
+                "superset_guest_token_url": f"https://lms.url/superset_guest_token/{self.course_id}",
                 "template_path_prefix": "/instructor_dashboard/",
             },
             context["context"]["sections"][0],
         )
+
+        mock_get_model.assert_called_once()

platform_plugin_aspects/signals.py

@@ -56,7 +56,7 @@ def on_user_profile_updated(  # pylint: disable=unused-argument  # pragma: no co
 # (prevents celery errors during tests)
 _user_profile = get_model("user_profile")
 if _user_profile:
-    post_save.connect(on_user_profile_updated, sender=_user_profile)
+    post_save.connect(on_user_profile_updated, sender=_user_profile)  # pragma: no cover
 
 
 def on_externalid_saved(  # pylint: disable=unused-argument  # pragma: no cover
@@ -82,7 +82,7 @@ def on_externalid_saved(  # pylint: disable=unused-argument  # pragma: no cover
 # (prevents celery errors during tests)
 _external_id = get_model("external_id")
 if _external_id:
-    post_save.connect(on_externalid_saved, sender=_external_id)
+    post_save.connect(on_externalid_saved, sender=_external_id)  # pragma: no cover
 
 
 @receiver(USER_RETIRE_LMS_MISC)

platform_plugin_aspects/static/js/embed_dashboard.js

@@ -1,16 +1,36 @@
 function embedDashboard(dashboard_uuid, superset_url, guest_token_url, xblock_id) {
   xblock_id = xblock_id || "";
 
-  async fetchGuestToken() {
-    // Fetch the guest token from your backend
-    const response = await fetch(guest_token_url, {
+  function getCookie(name) {
+    let cookieValue = null;
+    if (document.cookie && document.cookie !== "") {
+      const cookies = document.cookie.split(";");
+      for (let i = 0; i < cookies.length; i++) {
+        const cookie = cookies[i].trim();
+        // Does this cookie string begin with the name we want?
+        if (cookie.substring(0, name.length + 1) === name + "=") {
+          cookieValue = decodeURIComponent(cookie.substring(name.length + 1));
+          break;
+        }
+      }
+    }
+    return cookieValue;
+  }
+
+  async function fetchGuestToken() {
+    return fetch(guest_token_url, {
       method: 'POST',
-      body: JSON.stringify({
-        // TODO csrf_token: csrf_token,
-      })
+      headers: {
+        "X-CSRFToken": getCookie('csrftoken'),
+      },
+    }).then(response => {
+        if (response.ok) {
+          const data = response.json();
+          return data.guestToken;
+        } else {
+          console.error(response);
+        }
     });
-    const data = await response.json();
-    return data.guestToken;
   }
 
   window.supersetEmbeddedSdk
@@ -41,6 +61,6 @@ function embedDashboard(dashboard_uuid, superset_url, guest_token_url, xblock_id
 
 if (window.superset_dashboards !== undefined) {
   window.superset_dashboards.forEach(function(dashboard) {
-    embedDashboard(dashboard.uuid, window.superset_url, window.superset_guest_token_url);
+    embedDashboard(dashboard.uuid, window.superset_url, window.superset_guest_token_url, dashboard.uuid);
   });
 }

platform_plugin_aspects/static/js/superset.js

@@ -2,7 +2,7 @@
 function SupersetXBlock(runtime, element, context) {
   const dashboard_uuid = context.dashboard_uuid;
   const superset_url = context.superset_url;
-  const superset_guest_token_url = context.superset_guest_token_url;
+  const superset_guest_token_url = runtime.handlerUrl(element, 'get_superset_guest_token');
   const xblock_id = context.xblock_id
 
   function initSuperset(supersetEmbeddedSdk) {

platform_plugin_aspects/tests/test_utils.py

@@ -7,6 +7,7 @@
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured
 from django.test import TestCase
+from requests.exceptions import HTTPError
 
 from platform_plugin_aspects.utils import (
     generate_guest_token,
@@ -95,21 +96,11 @@ def test_get_ccx_courses_feature_disabled(self):
 
         self.assertEqual(list(courses), [])
 
-    @patch.object(
-        settings,
-        "SUPERSET_CONFIG",
-        {
-            "internal_service_url": "http://superset:8088",
-            "service_url": "http://superset-dummy-url/",
-            "username": "superset",
-            "password": "superset",
-        },
-    )
     def test_generate_superset_context(self):
         """
         Test generate_superset_context
         """
-        context = {"course": COURSE_ID}
+        context = {"course_id": COURSE_ID}
         dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS
 
         dashboards.append(
@@ -127,19 +118,44 @@ def test_generate_superset_context(self):
         )
 
         self.assertEqual(
-            context["superset_guest_token_url"], f"/superset_guest_token/{COURSE_ID}"
+            context["superset_guest_token_url"],
+            f"https://lms.url/superset_guest_token/{COURSE_ID}",
         )
         self.assertEqual(context["superset_dashboards"], dashboards)
         self.assertEqual(context["superset_url"], "http://superset-dummy-url/")
         self.assertNotIn("superset_token", context)
         self.assertNotIn("exception", context)
 
+    @patch("platform_plugin_aspects.utils.SupersetClient")
+    def test_generate_guest_token_with_superset_client_http_error(
+        self, mock_superset_client
+    ):
+        """
+        Test generate_guest_token when Superset throws an HTTP error.
+        """
+        filter_mock = Mock()
+        user_mock = Mock()
+        response_mock = Mock()
+        mock_superset_client.side_effect = HTTPError(
+            "server error", response=response_mock
+        )
+
+        with self.assertRaises(ImproperlyConfigured):
+            generate_guest_token(
+                user=user_mock,
+                course=COURSE_ID,
+                dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}],
+                filters=[filter_mock],
+            )
+
+        mock_superset_client.assert_called_once()
+
     @patch("platform_plugin_aspects.utils.SupersetClient")
     def test_generate_guest_token_with_superset_client_exception(
         self, mock_superset_client
     ):
         """
-        Test generate_guest_token
+        Test generate_guest_token when there's a general Exception.
         """
         filter_mock = Mock()
         user_mock = Mock()
@@ -155,20 +171,10 @@ def test_generate_guest_token_with_superset_client_exception(
 
         mock_superset_client.assert_called_once()
 
-    @patch.object(
-        settings,
-        "SUPERSET_CONFIG",
-        {
-            "internal_service_url": "http://superset:8088",
-            "service_url": "http://dummy-superset-url",
-            "username": "superset",
-            "password": "superset",
-        },
-    )
     @patch("platform_plugin_aspects.utils.SupersetClient")
     def test_generate_guest_token_succesful(self, mock_superset_client):
         """
-        Test generate_guest_token
+        Test generate_guest_token works.
         """
         response_mock = Mock(status_code=200)
         mock_superset_client.return_value.session.post.return_value = response_mock

platform_plugin_aspects/utils.py

@@ -8,10 +8,12 @@
 import os
 import uuid
 from importlib import import_module
+from urllib.parse import urljoin
 
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured
 from django.urls import reverse
+from requests.exceptions import HTTPError
 from supersetapiclient.client import SupersetClient
 from xblock.reference.user_service import XBlockUser
 
@@ -37,14 +39,14 @@ def generate_superset_context(
     Update context with superset token and dashboard id.
 
     Args:
-        context (dict): the context for the instructor dashboard. It must include a course ID
+        context (dict): the context for the instructor dashboard. It must include a course_id.
         user (XBlockUser or User): the current user.
         superset_config (dict): superset config.
         dashboards (list): list of superset dashboard uuid.
         filters (list): list of filters to apply to the dashboard.
         language (str): the language code of the end user.
     """
-    course = context["course"]
+    course_id = context["course_id"]
     superset_config = settings.SUPERSET_CONFIG
 
     if language:
@@ -56,9 +58,13 @@ def generate_superset_context(
 
     superset_url = _fix_service_url(superset_config.get("service_url"))
 
-    guest_token_url = reverse(
-        "platform_plugin_aspects:superset_guest_token",
-        kwargs={"course_id": course},
+    # Use an absolute LMS URL here, just in case we're being rendered in an MFE.
+    guest_token_url = urljoin(
+        settings.LMS_ROOT_URL,
+        reverse(
+            "platform_plugin_aspects:superset_guest_token",
+            kwargs={"course_id": course_id},
+        ),
     )
 
     context.update(
@@ -112,18 +118,27 @@ def generate_guest_token(user, course, dashboards, filters) -> str:
             username=superset_username,
             password=superset_password,
         )
-
-        logger.info(f"Requesting guest token from Superset, {data}")
         response = client.session.post(
             url=f"{superset_internal_host}api/v1/security/guest_token/",
             json=data,
             headers={"Content-Type": "application/json"},
         )
         response.raise_for_status()
-        token = response.json()["token"]
-
+        token = response.json().get("token")
         return token
 
+    except HTTPError as err:
+        # Superset server errors sometimes come with messages, so log the response.
+        logger.error(
+            f"{err.response.status_code} {err.response.json()} for url: {err.response.url}, data: {data}"
+        )
+        raise ImproperlyConfigured(
+            _(
+                "Unable to fetch Superset guest token, "
+                "Superset server error {server_response}"
+            ).format(server_response=err.response.json())
+        ) from err
+
     except Exception as exc:
         logger.error(exc)
         raise ImproperlyConfigured(

platform_plugin_aspects/xblock.py

@@ -101,7 +101,7 @@ def student_view(self, context=None):
         context = context or {}
         context.update(
             {
-                "course": self.runtime.course_id,
+                "course_id": self.runtime.course_id,
                 "display_name": self.display_name,
             }
         )
@@ -120,10 +120,9 @@ def student_view(self, context=None):
         )
         context["xblock_id"] = str(self.scope_ids.usage_id.block_id)
 
-        # Use our xblock handler instead of the platform plugin's view.
-        context["superset_guest_token_url"] = self.runtime.handlerUrl(
-            self, "get_superset_guest_token"
-        )
+        # Remove this URL from the context to avoid confusion.
+        # Our XBlock handler URL will be used instead, provided in superset.js
+        del context["superset_guest_token_url"]
 
         frag = Fragment()
         frag.add_content(self.render_template("static/html/superset.html", context))