Repository: FHIR/sushi (ts)

Issue:
Double application of external icon in menus
If using `config.yaml` to define your menu, and you mark an item as `external`, the menu will show _two_ external icons.  I think that the templates recently starting adding the external icons by default.  We should investigate, and if that is the case, deprecate the `external` flag in the menu configuration.

Pull request:
Deprecate "external" Keyword in Menu Configuration
This PR fixes #588.

This adds a warning if the "external" keyword is used in a menu item configuration. ~Right now, if someone uses the "external" keyword, the warning will be logged but there will still be two external link icons in the menu item. If it makes more sense to completely remove support for adding the icon ourselves and just log the warning, I can remove the support for adding the icon altogether.~

Edit: I updated this PR to remove all support for the `external` keyword. If someone uses it in a menu configuration, a warning will be logged, but the keyword will be ignored and no external icon image will be generated.

Patch:
diff --git a/src/fshtypes/Configuration.ts b/src/fshtypes/Configuration.ts
index cc014e7..84f0962 100644
--- a/src/fshtypes/Configuration.ts
+++ b/src/fshtypes/Configuration.ts
@@ -149,7 +149,6 @@ export type ConfigurationResource = ImplementationGuideDefinitionResource & { om
 export type ConfigurationMenuItem = {
   name: string;
   url?: string;
-  isExternal?: boolean;
   openInNewTab?: boolean;
   subMenu?: ConfigurationMenuItem[];
 };
diff --git a/src/ig/IGExporter.ts b/src/ig/IGExporter.ts
index a5be599..25912ef 100644
--- a/src/ig/IGExporter.ts
+++ b/src/ig/IGExporter.ts
@@ -769,7 +769,6 @@ export class IGExporter {
       }
       menuItem += item.name;
       if (item.url) {
-        if (item.isExternal) menuItem += ' <img src="external.png" style="text-align: baseline"/>';
         menuItem += '</a>';
       }
       menuItem += `${EOL}${prefixSpaces}</li>${EOL}`;
diff --git a/src/import/ensureConfigurationFile.ts b/src/import/ensureConfigurationFile.ts
index 27e3889..6d3247c 100644
--- a/src/import/ensureConfigurationFile.ts
+++ b/src/import/ensureConfigurationFile.ts
@@ -420,8 +420,6 @@ function getBoxComment(title: string, comment: string): string {
 
 // Helper functions for converting menu XML file
 const itemInNewTab = (item: any) => (item.a._attributes?.target === '_blank' ? 'new-tab ' : '');
-const itemIsExternal = (item: any) =>
-  item.a.img?._attributes?.src === 'external.png' ? 'external ' : '';
 
 /**
  * Convert a menu XML file into the required menu object format for the YAML configuration.
@@ -474,7 +472,7 @@ function getMenuObjectFromMenuXML(menuXML: string): YAMLConfigurationMenuTree {
         if (li.a && !Array.isArray(li.a)) {
           const name = li.a._text;
           const link = li.a._attributes?.href;
-          const menuLinkWithKeywords = `${itemInNewTab(li)}${itemIsExternal(li)}${link}`;
+          const menuLinkWithKeywords = `${itemInNewTab(li)}${link}`;
           if (li.ul && li.ul.li) {
             const subMenu: YAMLConfigurationMenuTree = {};
             const subItems = Array.isArray(li.ul.li) ? li.ul.li : [li.ul.li];
@@ -483,7 +481,7 @@ function getMenuObjectFromMenuXML(menuXML: string): YAMLConfigurationMenuTree {
                 const subName = subLi.a._text;
                 const subLink = subLi.a._attributes?.href;
                 if (subLink && subLink !== '#') {
-                  subMenu[subName] = `${itemInNewTab(subLi)}${itemIsExternal(subLi)}${subLink}`;
+                  subMenu[subName] = `${itemInNewTab(subLi)}${subLink}`;
                 }
                 // NOTE: if there is another sub-sub-menu, we drop it since publisher doesn't support it
               }
@@ -625,7 +623,7 @@ const DEFAULT_MENU: YAMLConfigurationMenuTree = {
   'Table of Contents': 'toc.html',
   'Artifact Index': 'artifacts.html',
   Support: {
-    'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html'
+    'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html'
   }
 };
 
diff --git a/src/import/importConfiguration.ts b/src/import/importConfiguration.ts
index 9713dbb..498d58d 100644
--- a/src/import/importConfiguration.ts
+++ b/src/import/importConfiguration.ts
@@ -731,7 +731,11 @@ function parseMenu(yamlMenu: YAMLConfigurationMenuTree): ConfigurationMenuItem[]
       }
 
       if (value.includes('external ')) {
-        item.isExternal = true;
+        logger.warn(
+          'The "external" keyword in menu configuration has been deprecated. The IG Publisher' +
+            ' will detect external links and add the external link icon automatically. To resolve' +
+            ` this warning, remove the "external" keyword from the "${name}" menu item.`
+        );
         value = value.replace('external', '');
       }
 


Test patch:
diff --git a/test/ig/IGExporter.menu-xml.test.ts b/test/ig/IGExporter.menu-xml.test.ts
index 2798fa8..0bc24ec 100644
--- a/test/ig/IGExporter.menu-xml.test.ts
+++ b/test/ig/IGExporter.menu-xml.test.ts
@@ -9,7 +9,6 @@ import { minimalConfig } from '../utils/minimalConfig';
 import {
   simpleMenuXMLContent,
   simpleMenuXMLWithOpenInNewTabContent,
-  simpleMenuXMLWithExternalContent,
   subMenuXMLContent,
   subMenuWithWarningXMLContent
 } from './fixtures/menuXMLContent';
@@ -176,23 +175,6 @@ describe('IGExporter', () => {
       expect(loggerSpy.getAllMessages()).toHaveLength(0);
     });
 
-    it('should build menu with external icon when provided in config.menu', () => {
-      const config = { ...minimalConfig };
-      config.menu = [
-        { name: 'Animals', url: 'animals.html', isExternal: true },
-        { name: 'Plants', url: 'plants.html' },
-        { name: 'Other' }
-      ];
-      const pkg = new Package(config);
-      const exporter = new IGExporter(pkg, null, '', true);
-      exporter.addMenuXML(tempOut);
-      const menuPath = path.join(tempOut, 'fsh-generated', 'includes', 'menu.xml');
-      expect(fs.existsSync(menuPath)).toBeTruthy();
-      const content = fs.readFileSync(menuPath, 'utf8');
-      expect(content).toContain(simpleMenuXMLWithExternalContent.replace(/\n/g, EOL));
-      expect(loggerSpy.getAllMessages()).toHaveLength(0);
-    });
-
     it('should build menu with a sub-menu when provided in config.menu', () => {
       const config = { ...minimalConfig };
       config.menu = [
@@ -202,7 +184,7 @@ describe('IGExporter', () => {
           subMenu: [
             { name: 'Trees', url: 'plants.html#trees' },
             { name: 'Flowers', url: 'buds.html' },
-            { name: 'Cacti', url: 'prickly.com', isExternal: true, openInNewTab: true }
+            { name: 'Cacti', url: 'prickly.com', openInNewTab: true }
           ]
         }
       ];
diff --git a/test/ig/fixtures/menuXMLContent.ts b/test/ig/fixtures/menuXMLContent.ts
index 3755f43..72ab82a 100644
--- a/test/ig/fixtures/menuXMLContent.ts
+++ b/test/ig/fixtures/menuXMLContent.ts
@@ -24,19 +24,6 @@ export const simpleMenuXMLWithOpenInNewTabContent = `
   </li>
 </ul>`;
 
-export const simpleMenuXMLWithExternalContent = `
-<ul xmlns="http://www.w3.org/1999/xhtml" class="nav navbar-nav">
-  <li>
-    <a href="animals.html">Animals <img src="external.png" style="text-align: baseline"/></a>
-  </li>
-  <li>
-    <a href="plants.html">Plants</a>
-  </li>
-  <li>
-    Other
-  </li>
-</ul>`;
-
 export const subMenuXMLContent = `
 <ul xmlns="http://www.w3.org/1999/xhtml" class="nav navbar-nav">
   <li>
@@ -54,7 +41,7 @@ export const subMenuXMLContent = `
         <a href="buds.html">Flowers</a>
       </li>
       <li>
-        <a target="_blank" href="prickly.com">Cacti <img src="external.png" style="text-align: baseline"/></a>
+        <a target="_blank" href="prickly.com">Cacti</a>
       </li>
     </ul>
   </li>
diff --git a/test/import/YAMLConfiguration.test.ts b/test/import/YAMLConfiguration.test.ts
index 869083d..ed5efa7 100644
--- a/test/import/YAMLConfiguration.test.ts
+++ b/test/import/YAMLConfiguration.test.ts
@@ -100,7 +100,7 @@ describe('YAMLConfiguration', () => {
         },
         Downloads: 'downloads.html',
         History: 'http://hl7.org/fhir/us/example/history.html',
-        'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html'
+        'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html'
       });
       expect(config.parameters).toEqual({
         excludettl: true,
diff --git a/test/import/ensureConfigurationFile.test.ts b/test/import/ensureConfigurationFile.test.ts
index c3ccf2a..fcb3785 100644
--- a/test/import/ensureConfigurationFile.test.ts
+++ b/test/import/ensureConfigurationFile.test.ts
@@ -94,7 +94,7 @@ describe('ensureConfigurationFile', () => {
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
         Support: {
-          'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html'
+          'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html'
         }
       },
       history: {
@@ -164,7 +164,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
@@ -240,7 +240,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
@@ -339,7 +339,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
@@ -398,7 +398,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
@@ -471,7 +471,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
@@ -529,7 +529,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       // history should no longer exist since it will just use the provided package-list.json
       indexPageContent: 'Provides a simple example of how FSH can be used to create an IG'
@@ -631,7 +631,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       // history should still not exist since it will just use the provided package-list.json
       indexPageContent: 'Provides a simple example of how FSH can be used to create an IG'
@@ -744,7 +744,7 @@ describe('ensureConfigurationFile', () => {
       },
       'Artifact Index': 'artifacts.html',
       Support: {
-        'FHIR Spec': 'new-tab external {{site.data.fhir.path}}index.html',
+        'FHIR Spec': 'new-tab {{site.data.fhir.path}}index.html',
         Downloads: 'downloads.html'
       }
     });
@@ -794,7 +794,7 @@ describe('ensureConfigurationFile', () => {
         'IG Home': 'index.html',
         'Table of Contents': 'toc.html',
         'Artifact Index': 'artifacts.html',
-        Support: { 'FHIR Spec': 'new-tab external http://hl7.org/fhir/R4/index.html' }
+        Support: { 'FHIR Spec': 'new-tab http://hl7.org/fhir/R4/index.html' }
       },
       history: {
         current: 'http://build.fhir.org/ig/example/example-ig',
diff --git a/test/import/fixtures/example-config.yaml b/test/import/fixtures/example-config.yaml
index c5e00a7..6a66246 100644
--- a/test/import/fixtures/example-config.yaml
+++ b/test/import/fixtures/example-config.yaml
@@ -172,7 +172,7 @@ menu:
     Value Sets: artifacts.html#4
   Downloads: downloads.html
   History: http://hl7.org/fhir/us/example/history.html
-  FHIR Spec: new-tab external http://hl7.org/fhir/R4/index.html
+  FHIR Spec: new-tab http://hl7.org/fhir/R4/index.html
 
 # The parameters property represents IG.definition.parameter. Rather
 # than a list of code/value pairs (as in the ImplementationGuide
diff --git a/test/import/importConfiguration.test.ts b/test/import/importConfiguration.test.ts
index 52da2f7..8c00275 100644
--- a/test/import/importConfiguration.test.ts
+++ b/test/import/importConfiguration.test.ts
@@ -152,7 +152,6 @@ describe('importConfiguration', () => {
         {
           name: 'FHIR Spec',
           url: 'http://hl7.org/fhir/R4/index.html',
-          isExternal: true,
           openInNewTab: true
         }
       ],
@@ -1879,10 +1878,13 @@ describe('importConfiguration', () => {
         {
           name: 'FHIR Spec',
           url: 'http://hl7.org/fhir/R4/index.html',
-          isExternal: true,
           openInNewTab: true
         }
       ]);
+      expect(loggerSpy.getAllMessages('warn')).toHaveLength(1);
+      expect(loggerSpy.getLastMessage('warn')).toMatch(
+        /The "external" keyword in menu configuration has been deprecated/
+      );
     });
   });