From cb819ab49a5eb989a8e389f3e4090a2a8b3733ed Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Thu, 28 Nov 2024 14:37:59 +1100
Subject: [PATCH] FIX: Rerender Chart component if config changes (#29955)

The chart component was not rerendering if the chart
config passed to it was changed, this commit fixes the issue
by getting the config from `this.args` before trying to
access it inside an async call, so if the args change Ember
correctly rerenders. Also adds tests for this and general
chart rendering.

---------

Co-authored-by: Martin Brennan <martin@discourse.org>
---
 .../admin/addon/components/chart.gjs          |   7 +-
 .../integration/components/chart-test.gjs     | 106 ++++++++++++++++++
 2 files changed, 109 insertions(+), 4 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/tests/integration/components/chart-test.gjs

diff --git a/app/assets/javascripts/admin/addon/components/chart.gjs b/app/assets/javascripts/admin/addon/components/chart.gjs
index 5e4c4f28f5c..c2e91a4ce0f 100644
--- a/app/assets/javascripts/admin/addon/components/chart.gjs
+++ b/app/assets/javascripts/admin/addon/components/chart.gjs
@@ -6,11 +6,10 @@ import loadScript from "discourse/lib/load-script";
 // chartConfig - object
 export default class Chart extends Component {
   renderChart = modifier((element) => {
+    const { chartConfig } = this.args;
+
     loadScript("/javascripts/Chart.min.js").then(() => {
-      this.chart = new window.Chart(
-        element.getContext("2d"),
-        this.args.chartConfig
-      );
+      this.chart = new window.Chart(element.getContext("2d"), chartConfig);
     });
 
     return () => this.chart?.destroy();
diff --git a/app/assets/javascripts/discourse/tests/integration/components/chart-test.gjs b/app/assets/javascripts/discourse/tests/integration/components/chart-test.gjs
new file mode 100644
index 00000000000..24166f26c4d
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/integration/components/chart-test.gjs
@@ -0,0 +1,106 @@
+import { tracked } from "@glimmer/tracking";
+import { render, settled } from "@ember/test-helpers";
+import { module, test } from "qunit";
+import { setupRenderingTest } from "discourse/tests/helpers/component-test";
+import Chart from "admin/components/chart";
+
+module("Integration | Component | Chart", function (hooks) {
+  setupRenderingTest(hooks);
+
+  function generateData() {
+    let data = [];
+    for (let i = 0; i < 10; i++) {
+      data.push({ x: i, y: Math.random() * 10 });
+    }
+    return data;
+  }
+
+  // We do this because comparing hashes at a pixel level is not reliable,
+  // even when calling a hash on a canvas that was not changing at all
+  // I was getting different results. PNG is more solid ground and deterministic.
+  // No need to hash anything, the data URLs are unique, and we are rendering
+  // quite small canvases so no need to worry about perf as much.
+  function renderAsPNGURL(canvas) {
+    return canvas.toDataURL("image/png");
+  }
+
+  function generateChartConfig(model, options = {}) {
+    return {
+      type: "bar",
+      data: {
+        labels: model.data.map((r) => r.x),
+        datasets: [
+          {
+            data: model.data.map((r) => r.y),
+            label: model.title,
+            backgroundColor: options.backgroundColor || "rgba(200,220,240,1)",
+          },
+        ],
+      },
+      // We don't want anything messing with the canvas rendering by
+      // moving things around as we are trying to capture it.
+      options: {
+        animation: false,
+        responsive: false,
+        maintainAspectRatio: false,
+      },
+    };
+  }
+
+  test("renders a chart", async function (assert) {
+    this.model = {
+      title: "Test Chart",
+      data: generateData(),
+    };
+
+    const chartConfig = generateChartConfig(this.model);
+    await render(<template><canvas class="empty-canvas"></canvas></template>);
+    const emptyCanvasDataURL = await renderAsPNGURL(
+      document.querySelector("canvas.empty-canvas")
+    );
+    await render(<template><Chart @chartConfig={{chartConfig}} /></template>);
+    const dataURL = await renderAsPNGURL(
+      document.querySelector("canvas.chart-canvas")
+    );
+    assert.notStrictEqual(
+      emptyCanvasDataURL,
+      dataURL,
+      "The canvas was rendered successfully"
+    );
+  });
+
+  test("rerenders the chart if the config changes", async function (assert) {
+    this.model = {
+      title: "Test Chart",
+      data: generateData(),
+    };
+
+    const testState = new (class {
+      @tracked chartConfig;
+    })();
+
+    testState.chartConfig = generateChartConfig(this.model);
+
+    await render(<template>
+      <Chart @chartConfig={{testState.chartConfig}} />
+    </template>);
+    const firstCanvasDataURL = await renderAsPNGURL(
+      document.querySelector("canvas.chart-canvas")
+    );
+
+    testState.chartConfig = generateChartConfig(this.model, {
+      backgroundColor: "red",
+    });
+
+    await settled();
+    const secondCanvasDataURL = await renderAsPNGURL(
+      document.querySelector("canvas.chart-canvas")
+    );
+
+    assert.notStrictEqual(
+      firstCanvasDataURL,
+      secondCanvasDataURL,
+      "The canvases are not identical, so the chart rerendered successfully"
+    );
+  });
+});