Merge pull request #1308 from nextstrain/node-attrs-urls

Allow node traits to define URLs

src/components/tree/infoPanels/click.js

@@ -42,15 +42,6 @@ const item = (key, value, href) => (
   </tr>
 );
 
-const formatURL = (url) => {
-  if (url !== undefined && url.startsWith("https_")) {
-    return url.replace("https_", "https:");
-  } else if (url !== undefined && url.startsWith("http_")) {
-    return url.replace("http_", "http:");
-  }
-  return url;
-};
-
 const Link = ({url, title, value}) => (
   <tr>
     <th style={infoPanelStyles.item}>{title}</th>
@@ -96,13 +87,10 @@ const MutationTable = ({mutations}) => {
 
 
 const AccessionAndUrl = ({node}) => {
-  const accession = getAccessionFromNode(node);
-  const url = getUrlFromNode(node);
-  const genbank_accession = getTraitFromNode(node, "genbank_accession");
-
-  /* `gisaid_epi_isl` is a special value attached to nodes introduced during the 2019 nCoV outbreak.
-  If set, the display is different from the normal behavior */
+  /* If `gisaid_epi_isl` or `genbank_accession` exist as node attrs, these preempt normal use of `accession` and `url`.
+  These special values were introduced during the 2019 nCoV outbreak. */
   const gisaid_epi_isl = getTraitFromNode(node, "gisaid_epi_isl");
+  const genbank_accession = getTraitFromNode(node, "genbank_accession");
   if (isValueValid(gisaid_epi_isl)) {
     return (
       <>
@@ -114,22 +102,24 @@ const AccessionAndUrl = ({node}) => {
       </>
     );
   }
-
   if (isValueValid(genbank_accession)) {
     return (
       <Link title={"Genbank accession"} value={genbank_accession} url={"https://www.ncbi.nlm.nih.gov/nuccore/" + genbank_accession}/>
     );
-  } else if (isValueValid(accession) && isValueValid(url)) {
+  }
+
+  const {accession, url} = getAccessionFromNode(node);
+  if (accession && url) {
     return (
-      <Link url={formatURL(url)} value={accession} title={"Accession"}/>
+      <Link url={url} value={accession} title={"Accession"}/>
     );
-  } else if (isValueValid(accession)) {
+  } else if (accession) {
     return (
       item("Accession", accession)
     );
-  } else if (isValueValid(url)) {
+  } else if (url) {
     return (
-      <Link title={"Strain URL"} url={formatURL(url)} value={"click here"}/>
+      <Link title={"Strain URL"} url={url} value={"click here"}/>
     );
   }
   return null;
@@ -218,7 +208,7 @@ const SampleDate = ({node, t}) => {
 const getTraitsToDisplay = (node) => {
   // TODO -- this should be centralised somewhere
   if (!node.node_attrs) return [];
-  const ignore = ["author", "div", "num_date", "gisaid_epi_isl", "genbank_accession"];
+  const ignore = ["author", "div", "num_date", "gisaid_epi_isl", "genbank_accession", "accession", "url"];
   return Object.keys(node.node_attrs).filter((k) => !ignore.includes(k));
 };
 
@@ -230,10 +220,16 @@ const Trait = ({node, trait, colorings}) => {
       value = Number.parseFloat(value_tmp).toPrecision(3);
     }
   }
+  if (!isValueValid(value)) return null;
+
   const name = (colorings && colorings[trait] && colorings[trait].title) ?
     colorings[trait].title :
     trait;
-  return isValueValid(value) ? item(name, value) : null;
+  const url = getUrlFromNode(node, trait);
+  if (url) {
+    return <Link title={name} url={url} value={value}/>;
+  }
+  return item(name, value);
 };
 
 /**

src/util/treeMiscHelpers.js

@@ -69,15 +69,42 @@ export const getFullAuthorInfoFromNode = (node) =>
 
 export const getAccessionFromNode = (node) => {
   /* see comment at top of this file */
-  if (node.node_attrs && node.node_attrs.accession) {
-    return node.node_attrs.accession;
+  let accession, url;
+  if (node.node_attrs) {
+    if (isValueValid(node.node_attrs.accession)) {
+      accession = node.node_attrs.accession;
+    }
+    url = validateUrl(node.node_attrs.url);
   }
-  return undefined;
+  return {accession, url};
 };
 
 /* see comment at top of this file */
-export const getUrlFromNode = (node) =>
-  (node.node_attrs && node.node_attrs.url) ? node.node_attrs.url : undefined;
+export const getUrlFromNode = (node, trait) => {
+  if (!node.node_attrs || !node.node_attrs[trait]) return undefined;
+  return validateUrl(node.node_attrs[trait].url);
+};
+
+/**
+ * Check if a URL seems valid & return it.
+ * For historical reasons, we allow URLs to be defined as `http[s]_` and coerce these into `http[s]:`
+ * URls are interpreted by `new URL()` and thus may be returned with a trailing slash
+ * @param {String} url URL string to validate
+ * @returns {String|undefined} potentially modified URL string or `undefined` (if it doesn't seem valid)
+ */
+function validateUrl(url) {
+  if (url===undefined) return undefined; // urls are optional, so return early to avoid the console warning
+  try {
+    if (typeof url !== "string") throw new Error();
+    if (url.startsWith("http_")) url = url.replace("http_", "http:"); // eslint-disable-line no-param-reassign
+    if (url.startsWith("https_")) url = url.replace("https_", "https:"); // eslint-disable-line no-param-reassign
+    const urlObj = new URL(url);
+    return urlObj.href;
+  } catch (err) {
+    console.warn(`Dataset provided the invalid URL ${url}`);
+    return undefined;
+  }
+}
 
 /**
  * Traverses the tree and returns a set of genotype states such as

test/treeHelpers.test.js

@@ -1,4 +1,4 @@
-import { collectMutations } from "../src/util/treeMiscHelpers";
+import { collectMutations, getUrlFromNode, getAccessionFromNode } from "../src/util/treeMiscHelpers";
 import { treeJsonToState } from "../src/util/treeJsonProcessing";
 
 /**
@@ -53,3 +53,37 @@ function getNodeByName(tree, name) {
   recurse(tree[0]);
   return namedNode;
 }
+
+describe('Extract various values from node_attrs', () => {
+  beforeEach(() => {
+    jest.spyOn(console, 'warn').mockImplementation(() => {});
+  });
+
+  // the following test also covers `validateUrl`
+  test("getUrlFromNode correctly handles various URLs", () => {
+    expect(getUrlFromNode({}, "trait")).toEqual(undefined); // no node_attrs on node
+    expect(getUrlFromNode({node_attrs: {}}, "trait")).toEqual(undefined); // no "trait" defined in node_attrs
+    expect(getUrlFromNode({node_attrs: {trait: "str_value"}}, "trait")).toEqual(undefined); // incorrectly formatted "trait"
+    expect(getUrlFromNode({node_attrs: {trait: {}}}, "trait")).toEqual(undefined); // correctly formatted "trait", no URL
+    expect(getUrlFromNode({node_attrs: {trait: {url: 1234}}}, "trait")).toEqual(undefined); // invalid URL
+    expect(getUrlFromNode({node_attrs: {trait: {url: "bad url"}}}, "trait")).toEqual(undefined); // invalid URL
+    expect(getUrlFromNode({node_attrs: {trait: {url: "https://nextstrain.org"}}}, "trait")).toEqual("https://nextstrain.org/");
+    expect(getUrlFromNode({node_attrs: {trait: {url: "http://nextstrain.org"}}}, "trait")).toEqual("http://nextstrain.org/");
+    expect(getUrlFromNode({node_attrs: {trait: {url: "https_//nextstrain.org"}}}, "trait")).toEqual("https://nextstrain.org/"); // see code for details
+    expect(getUrlFromNode({node_attrs: {trait: {url: "http_//nextstrain.org"}}}, "trait")).toEqual("http://nextstrain.org/"); // see code for details
+  });
+
+  test("extract accession & corresponding URL from a node", () => {
+    expect(getAccessionFromNode({}))
+      .toStrictEqual({accession: undefined, url: undefined});  // no node_attrs on node
+    expect(getAccessionFromNode({node_attrs: {}}))
+      .toStrictEqual({accession: undefined, url: undefined}); // no "accession" on node_attrs
+    expect(getAccessionFromNode({node_attrs: {accession: "MK049251", url: "https://www.ncbi.nlm.nih.gov/nuccore/MK049251"}}))
+      .toStrictEqual({accession: "MK049251", url: "https://www.ncbi.nlm.nih.gov/nuccore/MK049251"});
+    expect(getAccessionFromNode({node_attrs: {url: "https://www.ncbi.nlm.nih.gov/nuccore/MK049251"}}))
+      .toStrictEqual({accession: undefined, url: "https://www.ncbi.nlm.nih.gov/nuccore/MK049251"}); // url can be defined without accession
+    expect(getAccessionFromNode({node_attrs: {accession: "MK049251", url: "nuccore/MK049251"}}))
+      .toStrictEqual({accession: "MK049251", url: undefined}); // invalid URL
+  });
+
+});