Merge pull request #2332 from concord-consortium/187848781-fix-image-…

…url-display-makes-graphs-unusable

fix: images as first category makes graph unusable (PT-187848781)

cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js

@@ -305,6 +305,35 @@ context('XYPlot Tool Tile', function () {
       xyTile.getGraphDot().should('have.length', 8);
     });
 
+    it("On initialization from data card containing image URLs, does not graph image URLs", () => {
+      beforeTest(`${Cypress.config("qaMothPlotUnitStudent5")}&mouseSensor`);
+      clueCanvas.addTile("datacard");
+      dataCard.getTile().should("exist");
+      dataCard.getAddAttributeButton().click();
+      dataCard.getAttrName().eq(0).dblclick().type("Image{enter}");
+      dataCard.getAddAttributeButton().click();
+      dataCard.getAttrName().eq(1).dblclick().type("Name{enter}");
+      dataCard.getAddAttributeButton().click();
+      dataCard.getAttrName().eq(2).dblclick().type("Type{enter}");
+      dataCard.getAttrValue().eq(0).click().type("https://concord.org/images/energy3d.png{enter}");
+      dataCard.getAttrValue().eq(1).click().type("Energy3D{enter}");
+      dataCard.getAttrValue().eq(2).click().type("download{enter}");
+      dataCard.getAddCardButton().click();
+      dataCard.getAttrValue().eq(0).click().type("https://concord.org/images/codap.png{enter}");
+      dataCard.getAttrValue().eq(1).click().type("CODAP{enter}");
+      dataCard.getAttrValue().eq(2).click().type("web app{enter}");
+      dataCard.getGraphItButton().click();
+      cy.wait(1000);
+      // Image URLs should not be graphed.
+      xyTile.getXAxisLabel().should("contain", "Name");
+      xyTile.getYAxisLabel().should("contain", "Type");
+      // If the user assigns the image URLs to an axis, "<image>" should be used in place of the full URL values
+      // for the tick labels.
+      xyTile.clickPortalButton("Name");
+      xyTile.getPortalButton().contains("Image").should("exist").click();
+      cy.get("[data-testid=axis-bottom]").find("text").should("contain", "<image>");
+    });
+
     it("Test undo redo actions", () => {
       beforeTest(queryParamsMultiDataset);
       cy.log("Undo redo  XY Plot Tile creation");

src/models/data/data-types.test.ts

@@ -163,7 +163,7 @@ describe("data-types", () => {
       return [value, isImageUrl(value)];
     });
     expect({ testCases }).toMatchInlineSnapshot(`
-"https://something.concord.org/hello.png" => false
+"https://something.concord.org/hello.png" => true
 "ccimg://fbrtdb.concord.org/hello.png" => true
 "ccimg://fbrtdb.concord.org/anything.txt" => true
 "ccimg://not-a-cc.domain.com/hello.png" => false

src/models/data/data-types.ts

@@ -126,12 +126,13 @@ export function isImageUrl(val: IValueType) {
     return false;
   }
 
-  // We might need to support more advanced image detection in the future
-  // For now we just look for strings matching the URL for an user uploaded
-  // image. Like:
+  // Look for strings matching the URL for a user uploaded image, like:
   // ccimg://fbrtdb.concord.org/devclass/-NcP-LmubeWUdANUM_vO
-  // The ImageMap has a isImageUrl function but it will pickup pretty
-  // much any http URL. And I'd guess we don't want to add a dependency
-  // on the imageMap to this shared code.
-  return val.startsWith("ccimg://fbrtdb.concord.org");
+  // Or that match more generic image URL strings, like:
+  // https://concord.org/image.png
+  // The ImageMap has an isImageUrl function but it will pickup pretty much any http URL.
+  // And we don't want to add a dependency on the imageMap to this shared code.
+  const ccImagePattern = /^ccimg:\/\/fbrtdb\.concord\.org/;
+  const imageUrlPattern = /^(https?:\/\/.*\.(gif|jpg|jpeg|png)|data:image\/(gif|jpg|jpeg|png);base64,)/i;
+  return ccImagePattern.test(val) || imageUrlPattern.test(val);
 }

src/plugins/graph/imports/components/axis/hooks/use-axis.ts

@@ -12,6 +12,7 @@ import {maxWidthOfStringsD3} from "../../../../utilities/graph-utils";
 import {useDataConfigurationContext} from "../../../../hooks/use-data-configuration-context";
 import {collisionExists, getStringBounds} from "../axis-utils";
 import graphVars from "../../../../components/graph.scss";
+import { isImageUrl } from "../../../../../../models/data/data-types";
 
 export interface IUseAxis {
   axisModel?: IAxisModel
@@ -66,7 +67,10 @@ export const useAxis = ({
       repetitions = multiScale?.repetitions ?? 1,
       bandWidth = ((ordinalScale?.bandwidth?.()) ?? 0) / repetitions,
       collision = collisionExists({bandWidth, categories, centerCategoryLabels}),
-      maxLabelExtent = maxWidthOfStringsD3(dataConfiguration?.categoryArrayForAttrRole(attrRole) ?? []),
+      categoryArrayValues = dataConfiguration?.categoryArrayForAttrRole(attrRole),
+      // Image URLs will be replaced with the placeholder "<image>", so do not measure the URL values
+      validCategoryArrayValues = categoryArrayValues?.map(cat => isImageUrl(cat) ? "<image>" : cat),
+      maxLabelExtent = maxWidthOfStringsD3(validCategoryArrayValues ?? []),
       d3Scale = multiScale?.scale ?? (type === 'numeric' ? scaleLinear() : scaleOrdinal());
     let desiredExtent =
       axisTitleHeight + 2 * (axisGap + kAxisLabelVerticalPadding) + kAxisTickLength + kAxisTickPadding;

src/plugins/graph/imports/components/axis/hooks/use-sub-axis.ts

@@ -12,6 +12,7 @@ import {
 import {DragInfo, collisionExists, computeBestNumberOfTicks,
         getCategoricalLabelPlacement,getCoordFunctions, IGetCoordFunctionsProps} from "../axis-utils";
 import { useGraphModelContext } from "../../../../hooks/use-graph-model-context";
+import { isImageUrl } from "../../../../../../models/data/data-types";
 
 // This function is used to style to style the axes of multiple types of plots below.
 // It would be better if we had functions like this to style other features consistently (tick marks, grid lines).
@@ -152,7 +153,9 @@ export const useSubAxis = ({subAxisIndex, axisModel, subAxisElt, showScatterPlot
       const dividerLength = layout.getAxisLength(otherPlace(place)) ?? 0;
       const isRightCat = place === 'rightCat';
       const isTop = place === 'top';
-      const categories = Array.from(categorySet?.values ?? []);
+      const _categories = Array.from(categorySet?.values ?? []);
+      // Replace image URLs with placeholder string
+      const categories = _categories.map(cat => isImageUrl(cat) ? "<image>" : cat);
       const numCategories = categories.length;
       const bandWidth = subAxisLength / numCategories;
       const collision = collisionExists({bandWidth, categories, centerCategoryLabels});
@@ -302,8 +305,11 @@ export const useSubAxis = ({subAxisIndex, axisModel, subAxisElt, showScatterPlot
       multiScale = layout.getAxisMultiScale(place),
       categorySet = multiScale?.categorySet,
       categories = Array.from(categorySet?.values ?? []),
-      categoryData: CatObject[] = categories.map((cat, index) =>
-        ({cat, index: isVertical(place) ? categories.length - index - 1 : index}));
+      categoryData: CatObject[] = categories.map((cat, index) => {
+        // Replace image URLs with a placeholder string
+        const validCatValue = isImageUrl(cat) ? "<image>" : cat;
+        return ({cat: validCatValue, index: isVertical(place) ? categories.length - index - 1 : index});
+      });
 
     subAxisSelectionRef.current = select(subAxisElt);
     const sAS = subAxisSelectionRef.current;

src/plugins/graph/models/graph-layer-model.ts

@@ -11,6 +11,7 @@ import { IDataSet, addCanonicalCasesToDataSet } from "../../../models/data/data-
 import { ISharedCaseMetadata } from "../../../models/shared/shared-case-metadata";
 import { DotsElt } from "../d3-types";
 import { ICaseCreation } from "../../../models/data/data-set-types";
+import { isImageUrl } from "../../../models/data/data-types";
 
 export const GraphLayerModel = types
   .model('GraphLayerModel')
@@ -101,10 +102,19 @@ export const GraphLayerModel = types
         const isValidYAttr = yAttrId && !!data?.attrFromID(yAttrId);
 
         if (!isValidXAttr && !isValidYAttr) {
-          this.autoAssignAttributeID("bottom", "x", data?.id ?? "", data?.attributes[0].id || '');
-          if (attributeCount > 1) {
-            this.autoAssignAttributeID("left", "y", data?.id ?? "", data?.attributes[1].id || '');
-          }
+          const validAttributes = data?.attributes.filter(attr => {
+            return data?.cases.every((c) => {
+              const value = data?.getValue(c.__id__, attr.id);
+              // Do not auto assign attributes containing image URLs.
+              return !(typeof value === "string" && isImageUrl(value));
+            });
+          });
+
+          const bottomAttrId = validAttributes && validAttributes.length > 0 ? validAttributes[0].id : "";
+          const leftAttrId = validAttributes && validAttributes.length > 1 ? validAttributes[1].id : "";
+
+          this.autoAssignAttributeID("bottom", "x", data?.id ?? "", bottomAttrId);
+          this.autoAssignAttributeID("left", "y", data?.id ?? "", leftAttrId);
         }
       } else {
         console.log('autoAssign is off');