Fix eight contained defects found during the coverage work

addon/components/custom-field/input.js

@@ -3,7 +3,6 @@ import { tracked } from '@glimmer/tracking';
 import { inject as service } from '@ember/service';
 import { action } from '@ember/object';
 import { underscore } from '@ember/string';
-import isObject from '@fleetbase/ember-core/utils/is-object';
 import isModel from '@fleetbase/ember-core/utils/is-model';
 import getModelName from '@fleetbase/ember-core/utils/get-model-name';
 import getCustomFieldTypeMap from '../../utils/get-custom-field-type-map';
@@ -141,8 +140,10 @@ export default class CustomFieldInputComponent extends Component {
     }
 
     @action onChangeHandler(event, otherValue) {
+        // <MoneyInput> reports `onChange(storedValue, detail)` where storedValue is a number, so
+        // a money field is a raw input like any other. The old `isMoneyInput` arm required an
+        // object, could never run, and would have reported the formatted value instead of cents.
         const isRawInput = typeof event === 'string' || typeof event === 'number';
-        const isMoneyInput = this.customFieldComponent === 'money-input' && isObject(event);
         const isEventInput = event instanceof window.Event;
         const isDateTimeInput = this.customFieldComponent === 'date-time-input' && typeof otherValue === 'string';
         const isDatePicker = this.customFieldComponent === 'date-picker' && typeof otherValue === 'string';
@@ -166,14 +167,6 @@ export default class CustomFieldInputComponent extends Component {
             return;
         }
 
-        if (isMoneyInput) {
-            const value = event.newValue;
-            if (typeof this.args.onChange === 'function') {
-                this.args.onChange(value, this.customField);
-            }
-            return;
-        }
-
         if (isEventInput) {
             const value = event.target.value;
             this.value = value;

addon/components/query-builder/actions.js

@@ -4,19 +4,19 @@ import { action } from '@ember/object';
 export default class QueryBuilderActionsComponent extends Component {
     @action onExecute() {
         if (typeof this.args.onExecute === 'function') {
-            this.args.onExecute(this.queryObject);
+            this.args.onExecute(this.args.queryObject);
         }
     }
 
     @action onSave() {
         if (typeof this.args.onSave === 'function') {
-            this.args.onSave(this.queryObject);
+            this.args.onSave(this.args.queryObject);
         }
     }
 
     @action onClear() {
         if (typeof this.args.onClear === 'function') {
-            this.args.onClear(this.queryObject);
+            this.args.onClear(this.args.queryObject);
         }
     }
 }

addon/components/query-builder/column-select.hbs

@@ -27,11 +27,15 @@
                     {{#if (includes column.name (map-by "name" this.selectedColumns))}}
                         <div class="flex items-center space-x-1">
                             <label class="text-xs text-gray-600 dark:text-gray-400">Alias:</label>
-                            <Input
-                                @value={{get this.columnAliases column.name}}
+                            {{! One-way on purpose: a two-way `@value` wrote the raw text back
+                                AFTER updateAlias ran, undoing its trimming and mutating the
+                                aliases hash that had already been handed to `onChange`. }}
+                            <input
+                                type="text"
+                                value={{get this.columnAliases column.name}}
                                 placeholder={{column.name}}
                                 class="form-input form-input-sm column-alias-input"
-                                {{on "input" (fn this.updateAlias column.name)}}
+                                {{on "change" (fn this.updateAlias column.name)}}
                             />
                         </div>
                     {{/if}}

addon/helpers/transition-to.js

@@ -31,9 +31,9 @@ export default class TransitionToHelper extends Helper {
 }
 
 function prefixMountPoint(mountPoint, propValue) {
-    if (typeOf(propValue) !== 'string') {
-        assert('propValue argument must be an string', typeOf(propValue) !== 'string');
-    }
+    // The guard used to assert the same condition as the `if` that wrapped it, so it could never
+    // fire and a non-string route name was interpolated into the route instead of being rejected.
+    assert('propValue argument must be an string', typeOf(propValue) === 'string');
 
     if (typeOf(mountPoint) !== 'string' || isBlank(mountPoint)) {
         return propValue;

addon/modifiers/set-height.js

@@ -18,9 +18,22 @@ export default modifier(function setHeight(element, [height], { calculated = fal
         if (match !== null) {
             heightValue = match[1];
             unit = match[2] || '';
+        } else {
+            // A keyword like `auto`, `none` or `fit-content` has no numeric part, so the
+            // numbersOnly() line below would reduce it to the invalid string "px" and the browser
+            // would drop it. Apply it verbatim instead.
+            element.style.height = height;
+            return;
         }
     }
 
+    // A unit this modifier cannot convert (%, vh, vw, ch, …) used to be parsed off and then
+    // thrown away, so `100%` silently became `100px`. Honour it as written instead.
+    if (!['', 'px', 'em', 'rem', 'pt', 'pc'].includes(unit)) {
+        element.style.height = height;
+        return;
+    }
+
     // Convert the height value to pixels
     if (unit === 'em') {
         heightValue *= 16; // 1em = 16px

addon/modifiers/set-max-height.js

@@ -15,9 +15,22 @@ export default modifier(function setMaxHeight(element, [height]) {
         if (match !== null) {
             heightValue = match[1];
             unit = match[2] || '';
+        } else {
+            // A keyword like `auto`, `none` or `fit-content` has no numeric part, so the
+            // numbersOnly() line below would reduce it to the invalid string "px" and the browser
+            // would drop it. Apply it verbatim instead.
+            element.style.maxHeight = height;
+            return;
         }
     }
 
+    // A unit this modifier cannot convert (%, vh, vw, ch, …) used to be parsed off and then
+    // thrown away, so `100%` silently became `100px`. Honour it as written instead.
+    if (!['', 'px', 'em', 'rem', 'pt', 'pc'].includes(unit)) {
+        element.style.maxHeight = height;
+        return;
+    }
+
     // Convert the height value to pixels
     if (unit === 'em') {
         heightValue *= 16; // 1em = 16px

addon/services/leaflet.js

@@ -22,8 +22,10 @@ export default class LeafletService extends Service {
                     debug('Leaflet has been initialized.');
                     if (this.instance === undefined) {
                         this.setInstance(Leaflet);
-                        this.initialized = true;
                     }
+                    // `initialized` is assigned nowhere else, so leaving it false when an instance
+                    // was already set kept the polling interval running forever.
+                    this.initialized = true;
                 } else if (Leaflet !== this.instance && !this.instances.includes(Leaflet)) {
                     // Subsequent re-initializations
                     debug('Leaflet has been re-initialized!');

addon/services/resource-context-panel.js

@@ -46,6 +46,10 @@ export default class ResourceContextPanelService extends Service {
      * @returns {String} The overlay ID
      */
     @action open(definition) {
+        // Validate first: the checks below dereference `definition`, so a missing one used to die
+        // with a TypeError before ever reaching `#validateDefinition`'s own error.
+        this.#validateDefinition(definition);
+
         // Generate ID if not provided
         if (!definition.id) {
             definition.id = this.#generateId();
@@ -61,9 +65,6 @@ export default class ResourceContextPanelService extends Service {
             this.#registerCloseOnTransition(definition);
         }
 
-        // Validate definition
-        this.#validateDefinition(definition);
-
         // Set initial active tab if tabs are provided
         if (definition.tabs && definition.tabs.length > 0) {
             const initialTab = definition.initialTab || definition.tabs[0].key;

addon/utils/is-menu-item-active.js

@@ -13,10 +13,8 @@ export default function isMenuItemActive(section, slug, view = null) {
     let slugMatch = segments.includes(slug);
     let viewMatch = segments.includes(view) || getUrlParam('view') === view;
 
-    if (slugOnly && view) {
-        return slugMatch && viewMatch;
-    }
-
+    // `slugOnly` already requires `view === null`, so a `slugOnly && view` branch could never
+    // run; a caller that passes a view falls through to the section rules below.
     if (slugOnly) {
         return slugMatch;
     }

tests/integration/components/query-builder/actions-test.js

@@ -65,4 +65,25 @@ module('Integration | Component | query-builder/actions', function (hooks) {
 
         assert.dom('.query-builder-panel').hasAttribute('data-test-actions', 'yes');
     });
+    test('each handler receives the query object the component was given', async function (assert) {
+        const received = [];
+        const queryObject = { sql: 'select 1' };
+        this.set('queryObject', queryObject);
+        this.set('onExecute', (value) => received.push(['execute', value]));
+        this.set('onSave', (value) => received.push(['save', value]));
+        this.set('onClear', (value) => received.push(['clear', value]));
+
+        await render(hbs`<QueryBuilder::Actions @queryObject={{this.queryObject}} @onExecute={{this.onExecute}} @onSave={{this.onSave}} @onClear={{this.onClear}} />`);
+
+        for (const button of findAll('button')) {
+            await click(button);
+        }
+
+        assert.deepEqual(received.map(([name]) => name).sort(), ['clear', 'execute', 'save'], 'all three handlers fire');
+        assert.deepEqual(
+            received.map(([, value]) => value),
+            [queryObject, queryObject, queryObject],
+            'and each is handed the query object rather than undefined'
+        );
+    });
 });

tests/integration/components/query-builder/column-select-test.js

@@ -196,6 +196,20 @@ module('Integration | Component | query-builder/column-select', function (hooks)
             assert.true(selectedChips()[0].includes('as order_state'), 'the chip shows the alias');
         });
 
+        // Regression: the alias field used to be a two-way `<Input @value>`, whose write-back
+        // landed AFTER updateAlias and re-inserted the raw text into the hash the callback had
+        // already been handed.
+        test('a whitespace-only alias is not written back into the reported aliases', async function (assert) {
+            await render(TEMPLATE);
+            await click(checkboxAt(0));
+            await fillIn('.column-alias-input', '   ');
+
+            const reported = lastChange().aliases;
+
+            assert.deepEqual(reported, {}, 'the hash handed to onChange holds no alias');
+            assert.notOk('status' in reported, 'and the key is absent rather than holding whitespace');
+        });
+
         test('a whitespace-only alias is treated as no alias', async function (assert) {
             await render(TEMPLATE);
             await click(checkboxAt(0));

tests/integration/helpers/transition-to-test.js

@@ -2,6 +2,7 @@ import { module, test } from 'qunit';
 import { setupRenderingTest } from 'dummy/tests/helpers';
 import { render, click, settled } from '@ember/test-helpers';
 import { hbs } from 'ember-cli-htmlbars';
+import { helper } from '@ember/component/helper';
 
 /**
  * The helper transitions through `router:main`, so record the calls on the real router instance
@@ -116,16 +117,25 @@ module('Integration | Helper | transition-to', function (hooks) {
 
         assert.deepEqual(calls, [['orders'], ['places']], 'the second click uses the updated route name');
     });
-    // `prefixMountPoint` guards its argument with an assertion whose condition is inverted, so a
-    // non-string route name passes straight through it — see DEFECTS.md #156.
-    test('a non-string route name is still handed to the router', async function (assert) {
+    // The closure is captured and called directly: thrown from inside `{{on "click"}}` the
+    // assertion escapes as an uncaught global error that assert.throws cannot intercept.
+    test('a non-string route name is rejected rather than interpolated', async function (assert) {
         const calls = stubRouter(this.owner);
         this.owner.mountPoint = 'console.fleet-ops';
         this.set('route', 404);
 
-        await render(hbs`<button type="button" class="go" {{on "click" (transition-to this.route)}}>Go</button>`);
-        await click('.go');
+        const captured = [];
+        this.owner.register(
+            'helper:capture-value',
+            helper(function ([value]) {
+                captured.push(value);
+                return '';
+            })
+        );
+
+        await render(hbs`{{capture-value (transition-to this.route)}}`);
 
-        assert.deepEqual(calls, [['console.fleet-ops.404']], 'it is interpolated into the mount-point prefix');
+        assert.throws(captured[0], /propValue argument must be an string/, 'the caller is told about its own bug');
+        assert.deepEqual(calls, [], 'and no transition is attempted');
     });
 });

tests/integration/modifiers/set-height-test.js

@@ -103,13 +103,19 @@ module('Integration | Modifier | set-height', function (hooks) {
 
         assert.strictEqual(find('[data-test-el]').style.height, '', 'a negative height is invalid css and is not applied');
     });
-    // A keyword height never matches the number-with-unit pattern, so it reaches the final
-    // `${numbersOnly(value)}px` unchanged and produces the invalid string "px" — see DEFECTS.md #160.
-    test('a keyword height leaves the element unsized rather than applying it', async function (assert) {
+    test('a keyword height is applied verbatim', async function (assert) {
         this.set('height', 'auto');
 
         await render(hbs`<div data-test-el {{set-height this.height}}></div>`);
 
-        assert.strictEqual(find('[data-test-el]').style.height, '', 'the keyword is dropped, not honoured');
+        assert.strictEqual(find('[data-test-el]').style.height, 'auto', 'a value with no numeric part is passed straight through');
+    });
+
+    test('a percentage height is applied verbatim', async function (assert) {
+        this.set('height', '100%');
+
+        await render(hbs`<div data-test-el {{set-height this.height}}></div>`);
+
+        assert.strictEqual(find('[data-test-el]').style.height, '100%');
     });
 });

tests/integration/modifiers/set-max-height-test.js

@@ -94,13 +94,18 @@ module('Integration | Modifier | set-max-height', function (hooks) {
         assert.strictEqual(find('[data-test-el]').style.maxHeight, '', 'a negative max height is invalid css and is not applied');
         assert.dom('[data-test-el]').hasStyle({ maxHeight: 'none' }, 'element keeps the default max height');
     });
-    // Same shape as set-height: a keyword never matches the number-with-unit pattern and ends up
-    // as the invalid string "px" — see DEFECTS.md #160.
-    test('a keyword max height leaves the element unconstrained rather than applying it', async function (assert) {
+    test('a keyword max height is applied verbatim', async function (assert) {
         this.set('maxHeight', 'none');
 
         await render(hbs`<div data-test-el {{set-max-height this.maxHeight}}></div>`);
 
-        assert.strictEqual(find('[data-test-el]').style.maxHeight, '', 'the keyword is dropped, not honoured');
+        assert.strictEqual(find('[data-test-el]').style.maxHeight, 'none', 'a value with no numeric part is passed straight through');
+    });
+    test('a percentage max height is applied verbatim', async function (assert) {
+        this.set('maxHeight', '80%');
+
+        await render(hbs`<div data-test-el {{set-max-height this.maxHeight}}></div>`);
+
+        assert.strictEqual(find('[data-test-el]').style.maxHeight, '80%', 'a unit the modifier cannot convert is honoured, not turned into px');
     });
 });

tests/unit/services/leaflet-test.js

@@ -231,10 +231,7 @@ module('Unit | Service | leaflet', function (hooks) {
 
         assert.strictEqual(readyCount, 1, 'the callback is guarded by an exact tick count');
     });
-    // The first-initialization arm only claims the global when `instance` is still undefined, and
-    // it is the only thing that sets `initialized` — so a preset instance leaves the poll running
-    // forever. See DEFECTS.md #160.
-    test('an instance already set is kept, and the service never finishes initializing', function (assert) {
+    test('an instance already set is kept, and the service still finishes initializing', function (assert) {
         const existing = fakeLeaflet('preset');
         this.service.instance = existing;
         this.service.load();
@@ -244,7 +241,7 @@ module('Unit | Service | leaflet', function (hooks) {
         this.tick();
 
         assert.strictEqual(this.service.instance, existing, 'the preset instance is kept');
-        assert.false(this.service.initialized, 'but nothing ever marks the service initialized');
-        assert.deepEqual(this.service.instances, [], 'and the global is not recorded as a re-initialization');
+        assert.true(this.service.initialized, 'and the poll is marked done rather than running forever');
+        assert.strictEqual(this.service.instances.length, 1, 'a later global is now noticed as a re-initialization instead of being ignored forever');
     });
 });

tests/unit/services/resource-context-panel-test.js

@@ -916,4 +916,11 @@ module('Unit | Service | resource-context-panel', function (hooks) {
             assert.strictEqual(service.overlays.length, 0, 'invoking it dismisses the overlay');
         });
     });
+    test('opening nothing at all is refused with a useful error', function (assert) {
+        const service = this.owner.lookup('service:resource-context-panel');
+
+        assert.throws(() => service.open(), /Overlay definition is required/, 'with no argument');
+        assert.throws(() => service.open(null), /Overlay definition is required/, 'and with an explicit null');
+        assert.strictEqual(service.overlays.length, 0, 'nothing is left behind');
+    });
 });