Merge pull request #141 from gtt-project/fix/js-robustness

JS robustness: fetch races, JSON-mode round trip, notification helper

Author: dkastl

app/views/subscription_templates/copy.js.erb

@@ -44,15 +44,8 @@ function copyToClipboard(command) {
   }
 }
 
-// Function to display notification
-function showNotification(message) {
-  var notification = document.getElementById('temporaryNotification');
-  notification.textContent = message;
-  notification.classList.add('visible');
-  setTimeout(function() {
-    notification.classList.remove('visible');
-  }, 3000);
-}
+// showNotification comes from the shared gtt_fiware.js asset, loaded on
+// every page by the layout hook.
 
 // Copy the cURL command to the clipboard
 copyToClipboard(command);

assets/javascripts/gtt_fiware.js

@@ -1,15 +1,21 @@
-function showNotification(message) {
-  // Get the notification box
-  var notification = document.getElementById('temporaryNotification');
+/* Shared page helpers, loaded on every page via the layout hook.
+ *
+ * showNotification is a deliberate global: the publish/unpublish/copy/sync
+ * js.erb responses and the subscription list call it by name. It writes to
+ * the #temporaryNotification box the list view renders; on pages without
+ * that box the call is a no-op rather than a TypeError.
+ */
+(function() {
+  'use strict';
 
-  // Change the text of the notification box
-  notification.textContent = message;
+  window.showNotification = function(message) {
+    var notification = document.getElementById('temporaryNotification');
+    if (!notification) { return; }
 
-  // Show the notification box
-  notification.classList.add('visible');
-
-  // Hide the notification box after 3 seconds
-  setTimeout(function() {
-    notification.classList.remove('visible');
-  }, 3000);
-}
+    notification.textContent = message;
+    notification.classList.add('visible');
+    setTimeout(function() {
+      notification.classList.remove('visible');
+    }, 3000);
+  };
+})();

assets/javascripts/gtt_fiware_form.js

@@ -32,11 +32,96 @@
       var json = document.getElementById('gtt-fiware-' + target + '-json');
       if (!rows || !json) { return; }
       var showJson = json.classList.contains('hidden');
-      if (showJson) { serialize(); }
+      if (showJson) {
+        serialize();
+      } else if (!restoreRows(target)) {
+        // The JSON cannot be shown by the picker; leaving JSON mode would
+        // overwrite the hand-edited JSON with stale rows on submit, so the
+        // form stays in JSON mode.
+        return;
+      }
       json.classList.toggle('hidden', !showJson);
       rows.style.display = showJson ? 'none' : '';
     }
 
+    // Rebuilds the picker rows from the JSON field when leaving JSON mode.
+    // Returns false when the JSON is not representable by the picker
+    // (unparsable, non-array, extra keys, non-string values); the caller
+    // then keeps JSON mode, so hand-edited JSON is never lost.
+    function restoreRows(target) {
+      var config = target === 'entities' ? {
+        fieldId: 'subscription_template_entities_string',
+        containerId: 'gtt-fiware-entity-rows',
+        prototypeId: 'gtt-fiware-entity-prototype',
+        rowClass: 'gtt-fiware-entity-row',
+        addId: 'gtt-fiware-entity-add',
+        rowValues: entityRowValues
+      } : {
+        fieldId: 'subscription_template_attachments_string',
+        containerId: 'gtt-fiware-attachment-rows',
+        prototypeId: 'gtt-fiware-attachment-prototype',
+        rowClass: 'gtt-fiware-attachment-row',
+        addId: 'gtt-fiware-attachment-add',
+        rowValues: attachmentRowValues
+      };
+      var field = document.getElementById(config.fieldId);
+      var container = document.getElementById(config.containerId);
+      var prototype = document.getElementById(config.prototypeId);
+      if (!field || !container || !prototype) { return false; }
+
+      var text = field.value.trim();
+      var parsed = [];
+      if (text !== '' && text !== 'null') {
+        try { parsed = JSON.parse(text); } catch (err) { return false; }
+        if (!Array.isArray(parsed)) { return false; }
+      }
+      var rows = parsed.map(config.rowValues);
+      if (rows.some(function(values) { return values === null; })) { return false; }
+
+      container.querySelectorAll('.' + config.rowClass).forEach(function(row) { row.remove(); });
+      rows.forEach(function(values) {
+        var clone = prototype.content.firstElementChild.cloneNode(true);
+        Object.keys(values).forEach(function(selector) {
+          var el = clone.querySelector(selector);
+          if (el) { el.value = values[selector]; }
+        });
+        container.insertBefore(clone, document.getElementById(config.addId));
+      });
+      return true;
+    }
+
+    // One picker row per entity selector: a type plus at most one
+    // id/idPattern member.
+    function entityRowValues(obj) {
+      if (!obj || typeof obj !== 'object' || typeof obj.type !== 'string') { return null; }
+      var rest = Object.keys(obj).filter(function(key) { return key !== 'type'; });
+      // Always set all three inputs: the cloned prototype row carries a
+      // default match value that must not leak into a restored row.
+      if (rest.length === 0) {
+        return { '.js-entity-type': obj.type, '.js-entity-match-kind': 'idPattern', '.js-entity-match-value': '' };
+      }
+      if (rest.length > 1 || (rest[0] !== 'id' && rest[0] !== 'idPattern')) { return null; }
+      if (typeof obj[rest[0]] !== 'string') { return null; }
+      return {
+        '.js-entity-type': obj.type,
+        '.js-entity-match-kind': rest[0],
+        '.js-entity-match-value': obj[rest[0]]
+      };
+    }
+
+    function attachmentRowValues(obj) {
+      if (!obj || typeof obj !== 'object' || typeof obj.url !== 'string') { return null; }
+      var allowed = ['url', 'filename', 'description'];
+      var keys = Object.keys(obj);
+      if (keys.some(function(key) { return allowed.indexOf(key) === -1; })) { return null; }
+      if (keys.some(function(key) { return typeof obj[key] !== 'string'; })) { return null; }
+      return {
+        '.js-attachment-url': obj.url,
+        '.js-attachment-filename': obj.filename || '',
+        '.js-attachment-description': obj.description || ''
+      };
+    }
+
     function rowValues(row, selectors) {
       return selectors.map(function(sel) {
         var el = row.querySelector(sel);
@@ -98,10 +183,24 @@
       var georel = document.getElementById('subscription_template_expression_georel');
       var geometry = document.getElementById('subscription_template_expression_geometry');
       var coords = document.getElementById('subscription_template_expression_coords');
+      if (!georel || !geometry || !coords) { return; }
       if (mode.value === 'anywhere') {
         georel.value = ''; geometry.value = ''; coords.value = '';
       } else if (mode.value === 'boundary') {
-        var geom = JSON.parse(mode.dataset.geom).geometry.coordinates[0]
+        var ring;
+        try {
+          ring = JSON.parse(mode.dataset.geom).geometry.coordinates[0];
+        } catch (err) {
+          ring = null;
+        }
+        // data-geom is server-rendered; if it is missing, malformed or not
+        // a ring of number pairs, leave the stored triple untouched rather
+        // than throwing from inside the submit listener.
+        var isPair = function(c) {
+          return Array.isArray(c) && typeof c[0] === 'number' && typeof c[1] === 'number';
+        };
+        if (!Array.isArray(ring) || ring.length === 0 || !ring.every(isPair)) { return; }
+        var geom = ring
           .map(function(c) { return [Number(c[1].toFixed(5)), Number(c[0].toFixed(5))]; })
           .join(';');
         georel.value = 'coveredBy'; geometry.value = 'polygon'; coords.value = geom;
@@ -250,14 +349,22 @@
       });
     }
 
+    // Guards against out-of-order responses: rapid tracker/member changes
+    // fire concurrent fetches, and without the token the last response to
+    // ARRIVE would rebuild the select, which may belong to the first
+    // request SENT (a stale tracker's statuses shown for the current one).
+    var statusRequestToken = 0;
+
     function refreshIssueStatuses() {
       if (!issueStatusSelect || !issueStatusSelect.dataset.statusesUrl) { return; }
       var url = issueStatusSelect.dataset.statusesUrl +
         '?tracker_id=' + encodeURIComponent(trackerSelect ? trackerSelect.value : '') +
         '&member_id=' + encodeURIComponent(memberSelect ? memberSelect.value : '');
+      var token = ++statusRequestToken;
       fetch(url, { headers: { 'Accept': 'application/json' } })
         .then(function(response) { return response.json(); })
         .then(function(statuses) {
+          if (token !== statusRequestToken) { return; }
           var current = issueStatusSelect.value;
           var currentText = issueStatusSelect.selectedOptions[0] ?
             issueStatusSelect.selectedOptions[0].textContent : '';
@@ -312,10 +419,15 @@
     }
 
     var previewButton = document.getElementById('gtt-fiware-preview-button');
+    // Same out-of-order guard as the status refetch: a double-click fires
+    // two requests, and only the latest one may write the result box.
+    var previewRequestToken = 0;
     if (previewButton) {
       previewButton.addEventListener('click', function(e) {
         e.preventDefault();
         var out = document.getElementById('gtt-fiware-preview-result');
+        if (!out) { return; }
+        var token = ++previewRequestToken;
         out.style.display = '';
         out.textContent = previewButton.dataset.loading;
 
@@ -342,6 +454,7 @@
         }).then(function(response) {
           return response.json().then(function(json) { return { ok: response.ok, json: json }; });
         }).then(function(result) {
+          if (token !== previewRequestToken) { return; }
           if (!result.ok) {
             out.textContent = result.json.error || previewButton.dataset.error;
             return;
@@ -359,6 +472,7 @@
             (result.json.has_geometry ? ', ' + previewButton.dataset.geometryLabel : '');
           out.appendChild(em);
         }).catch(function() {
+          if (token !== previewRequestToken) { return; }
           out.textContent = previewButton.dataset.error;
         });
       });

assets/javascripts/gtt_fiware_wizard.js

@@ -58,7 +58,7 @@
         li.classList.toggle('active', Number(li.dataset.step) === step);
       });
       var activeItem = nav.querySelector('li[data-step="' + step + '"]');
-      help.textContent = activeItem ? activeItem.dataset.help : '';
+      if (help) { help.textContent = activeItem ? activeItem.dataset.help : ''; }
       back.disabled = step === 1;
       next.style.display = step === TOTAL_STEPS ? 'none' : '';
       // The preview result manages its own visibility (the preview button

test/javascripts/issue_details.test.js

@@ -125,4 +125,34 @@ describe('status refetch', () => {
     await tick();
     expect(Array.from(statusSelect().options)).toHaveLength(2);
   });
+
+  // Rapid tracker changes fire concurrent fetches. When the first request's
+  // response arrives last, it must not overwrite the newer one: the select
+  // would show a stale tracker's statuses for the current tracker.
+  it('ignores an out-of-order response', async () => {
+    buildForm();
+    initForm();
+    const pending = [];
+    vi.stubGlobal('fetch', (url) => new Promise((resolve) => {
+      pending.push({
+        url,
+        respond(statuses) {
+          resolve({ json: () => Promise.resolve(statuses) });
+        }
+      });
+    }));
+
+    changeTracker('2');
+    changeTracker('1');
+    expect(pending).toHaveLength(2);
+
+    pending[1].respond([{ id: 5, name: 'Current Tracker Status' }]);
+    await tick();
+    pending[0].respond([{ id: 9, name: 'Stale Tracker Status' }]);
+    await tick();
+
+    const names = Array.from(statusSelect().options).map((o) => o.textContent);
+    expect(names).toContain('Current Tracker Status');
+    expect(names).not.toContain('Stale Tracker Status');
+  });
 });

test/javascripts/json_mode.test.js

@@ -2,7 +2,7 @@
 // field, hides the rows, and switching back restores the picker.
 
 import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
-import { buildForm, initForm, loadScript } from './support/form_fixture.js';
+import { buildForm, initForm, loadScript, submitForm } from './support/form_fixture.js';
 
 beforeAll(loadScript);
 
@@ -33,12 +33,39 @@ describe('entities JSON mode', () => {
     expect(rows.style.display).toBe('');
   });
 
-  it('does not overwrite hand-edited JSON when toggling back and forth', () => {
+  it('rebuilds the rows from hand-edited JSON when toggling back', () => {
+    toggle('entities');
+    const field = document.getElementById('subscription_template_entities_string');
+    field.value = '[{"type":"HandEdited","id":"urn:x:1"}]';
+    toggle('entities');
+    const row = document.querySelector('.gtt-fiware-entity-row');
+    expect(row.querySelector('.js-entity-type').value).toBe('HandEdited');
+    expect(row.querySelector('.js-entity-match-kind').value).toBe('id');
+    expect(row.querySelector('.js-entity-match-value').value).toBe('urn:x:1');
+  });
+
+  // The regression this pins: the field used to survive the toggle-back
+  // only until submit, when the stale rows overwrote it.
+  it('keeps hand-edited JSON through toggle-back and submit', () => {
     toggle('entities');
     const field = document.getElementById('subscription_template_entities_string');
     field.value = '[{"type":"HandEdited"}]';
-    toggle('entities'); // back to rows: serialize skips, JSON mode was on
-    expect(field.value).toBe('[{"type":"HandEdited"}]');
+    toggle('entities');
+    submitForm();
+    expect(JSON.parse(field.value)).toEqual([{ type: 'HandEdited' }]);
+  });
+
+  // JSON the picker cannot show (extra members) must never be replaced by
+  // stale rows: the form refuses to leave JSON mode.
+  it('stays in JSON mode when the JSON is not representable by the picker', () => {
+    toggle('entities');
+    const field = document.getElementById('subscription_template_entities_string');
+    field.value = '[{"type":"Sensor","id":"urn:x:1","extra":"member"}]';
+    toggle('entities');
+    const json = document.getElementById('gtt-fiware-entities-json');
+    expect(json.classList.contains('hidden')).toBe(false);
+    submitForm();
+    expect(field.value).toBe('[{"type":"Sensor","id":"urn:x:1","extra":"member"}]');
   });
 });
 

test/javascripts/notifications.test.js

@@ -0,0 +1,36 @@
+// The shared page helper (gtt_fiware.js): showNotification is a global the
+// publish/unpublish/copy/sync responses call by name. It is loaded on every
+// page by the layout hook, so it must be a no-op (not a TypeError) on pages
+// without the #temporaryNotification box.
+
+import { readFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+import { dirname, join } from 'node:path';
+import { beforeAll, describe, expect, it, vi } from 'vitest';
+
+const here = dirname(fileURLToPath(import.meta.url));
+
+beforeAll(() => {
+  window.eval(readFileSync(join(here, '../../assets/javascripts/gtt_fiware.js'), 'utf8'));
+});
+
+describe('showNotification', () => {
+  it('is a no-op on pages without the notification box', () => {
+    document.body.innerHTML = '';
+    expect(() => window.showNotification('hello')).not.toThrow();
+  });
+
+  it('shows the message and hides it again after the timeout', () => {
+    vi.useFakeTimers();
+    document.body.innerHTML = '<div id="temporaryNotification"></div>';
+    window.showNotification('Command copied');
+
+    const box = document.getElementById('temporaryNotification');
+    expect(box.textContent).toBe('Command copied');
+    expect(box.classList.contains('visible')).toBe(true);
+
+    vi.advanceTimersByTime(3000);
+    expect(box.classList.contains('visible')).toBe(false);
+    vi.useRealTimers();
+  });
+});