From f45880bffefeb6d8ca40916305514204fa23f3be Mon Sep 17 00:00:00 2001
From: John Cowen <johncowen@users.noreply.github.com>
Date: Fri, 31 Jan 2020 14:11:46 +0000
Subject: [PATCH] ui:  Fix sticky nspace menu (#7164)

* ui: Fix typo expanded > ariaExpanded

* ui: Add the things we need to test this

* ui: Add tests for testing the menu closes when clicked

* ui: Ensure the aria-menu closes on route change
---
 ui-v2/app/components/aria-menu.js             |  6 ++
 .../templates/components/hashicorp-consul.hbs |  2 +-
 .../app/templates/components/popover-menu.hbs |  2 +-
 .../acceptance/dc/nspaces/manage.feature      | 33 ++++++++++
 .../steps/dc/nspaces/manage-steps.js          | 10 +++
 ui-v2/tests/pages/components/page.js          |  8 ++-
 ui-v2/tests/steps/assertions/page.js          | 64 +++++++++++++++----
 7 files changed, 110 insertions(+), 15 deletions(-)
 create mode 100644 ui-v2/tests/acceptance/dc/nspaces/manage.feature
 create mode 100644 ui-v2/tests/acceptance/steps/dc/nspaces/manage-steps.js

diff --git a/ui-v2/app/components/aria-menu.js b/ui-v2/app/components/aria-menu.js
index 50f63afa4d..2c1daff70c 100644
--- a/ui-v2/app/components/aria-menu.js
+++ b/ui-v2/app/components/aria-menu.js
@@ -39,6 +39,7 @@ const MENU_ITEMS = '[role^="menuitem"]';
 export default Component.extend({
   tagName: '',
   dom: service('dom'),
+  router: service('router'),
   guid: '',
   expanded: false,
   orientation: 'vertical',
@@ -47,6 +48,7 @@ export default Component.extend({
     this._super(...arguments);
     set(this, 'guid', this.dom.guid(this));
     this._listeners = this.dom.listeners();
+    this._routelisteners = this.dom.listeners();
   },
   didInsertElement: function() {
     // TODO: How do you detect whether the children have changed?
@@ -54,10 +56,14 @@ export default Component.extend({
     this.$menu = this.dom.element(`#${COMPONENT_ID}menu-${this.guid}`);
     const labelledBy = this.$menu.getAttribute('aria-labelledby');
     this.$trigger = this.dom.element(`#${labelledBy}`);
+    this._routelisteners.add(this.router, {
+      routeWillChange: () => this.actions.close.apply(this, [{}]),
+    });
   },
   willDestroyElement: function() {
     this._super(...arguments);
     this._listeners.remove();
+    this._routelisteners.remove();
   },
   actions: {
     keypressClick: function(e) {
diff --git a/ui-v2/app/templates/components/hashicorp-consul.hbs b/ui-v2/app/templates/components/hashicorp-consul.hbs
index 37d73e1e5a..f6f844b717 100644
--- a/ui-v2/app/templates/components/hashicorp-consul.hbs
+++ b/ui-v2/app/templates/components/hashicorp-consul.hbs
@@ -12,7 +12,7 @@
 {{#if dc}}
                 <ul>
   {{#if (and (env 'CONSUL_NSPACES_ENABLED') (gt nspaces.length 0))}}
-                    <li>
+                    <li data-test-nspace-menu>
     {{#if (and (eq nspaces.length 1) (not canManageNspaces)) }}
                         <span data-test-nspace-selected={{nspace.Name}}>{{nspace.Name}}</span>
     {{ else }}
diff --git a/ui-v2/app/templates/components/popover-menu.hbs b/ui-v2/app/templates/components/popover-menu.hbs
index e6d31a34aa..9b7b4721f4 100644
--- a/ui-v2/app/templates/components/popover-menu.hbs
+++ b/ui-v2/app/templates/components/popover-menu.hbs
@@ -1,6 +1,6 @@
 {{yield (concat 'popover-menu-' guid)}}
 {{#aria-menu keyboardAccess=keyboardAccess as |change keypress ariaLabelledBy ariaControls ariaExpanded keypressClick|}}
-  {{#toggle-button checked=expanded onchange=(queue change (action 'change')) as |click|}}
+  {{#toggle-button checked=ariaExpanded onchange=(queue change (action 'change')) as |click|}}
     <button type="button" aria-haspopup="menu" onkeydown={{keypress}} onclick={{click}} id={{ariaLabelledBy}} aria-controls={{ariaControls}}>
       {{#yield-slot name='trigger'}}
         {{yield}}
diff --git a/ui-v2/tests/acceptance/dc/nspaces/manage.feature b/ui-v2/tests/acceptance/dc/nspaces/manage.feature
new file mode 100644
index 0000000000..368bc2e047
--- /dev/null
+++ b/ui-v2/tests/acceptance/dc/nspaces/manage.feature
@@ -0,0 +1,33 @@
+@setupApplicationTest
+Feature: dc / nspaces / manage : Managing Namespaces
+  Scenario:
+    Given settings from yaml
+    ---
+    consul:token:
+      SecretID: secret
+      AccessorID: accessor
+      Namespace: default
+    ---
+    And 1 datacenter models from yaml
+    ---
+      - dc-1
+    ---
+    And 6 service models
+    When I visit the services page for yaml
+    ---
+      dc: dc-1
+    ---
+    Then the url should be /dc-1/services
+    Then I see 6 service models
+    # In order to test this properly you have to click around a few times
+    # between services and nspace management
+    When I click nspace on the navigation
+    And I click manageNspaces on the navigation
+    Then the url should be /dc-1/namespaces
+    And I don't see manageNspacesIsVisible on the navigation
+    When I click services on the navigation
+    Then the url should be /dc-1/services
+    When I click nspace on the navigation
+    And I click manageNspaces on the navigation
+    Then the url should be /dc-1/namespaces
+    And I don't see manageNspacesIsVisible on the navigation
diff --git a/ui-v2/tests/acceptance/steps/dc/nspaces/manage-steps.js b/ui-v2/tests/acceptance/steps/dc/nspaces/manage-steps.js
new file mode 100644
index 0000000000..ba1093295f
--- /dev/null
+++ b/ui-v2/tests/acceptance/steps/dc/nspaces/manage-steps.js
@@ -0,0 +1,10 @@
+import steps from '../../steps';
+
+// step definitions that are shared between features should be moved to the
+// tests/acceptance/steps/steps.js file
+
+export default function(assert) {
+  return steps(assert).then('I should find a file', function() {
+    assert.ok(true, this.step);
+  });
+}
diff --git a/ui-v2/tests/pages/components/page.js b/ui-v2/tests/pages/components/page.js
index e8bbee753a..14742a504f 100644
--- a/ui-v2/tests/pages/components/page.js
+++ b/ui-v2/tests/pages/components/page.js
@@ -1,4 +1,4 @@
-import { clickable } from 'ember-cli-page-object';
+import { clickable, is } from 'ember-cli-page-object';
 const page = {
   navigation: ['services', 'nodes', 'kvs', 'acls', 'intentions', 'docs', 'settings'].reduce(
     function(prev, item, i, arr) {
@@ -24,4 +24,10 @@ const page = {
   ),
 };
 page.navigation.dc = clickable('[data-test-datacenter-menu] button');
+page.navigation.nspace = clickable('[data-test-nspace-menu] button');
+page.navigation.manageNspaces = clickable('[data-test-main-nav-nspaces] a');
+page.navigation.manageNspacesIsVisible = is(
+  ':checked',
+  '[data-test-nspace-menu] > input[type="checkbox"]'
+);
 export default page;
diff --git a/ui-v2/tests/steps/assertions/page.js b/ui-v2/tests/steps/assertions/page.js
index f7f4ae6bed..6458a2aeb0 100644
--- a/ui-v2/tests/steps/assertions/page.js
+++ b/ui-v2/tests/steps/assertions/page.js
@@ -1,4 +1,30 @@
 /* eslint no-console: "off" */
+const notFound = 'Element not found';
+const cannotDestructure = "Cannot destructure property 'context'";
+const cannotReadContext = "Cannot read property 'context' of undefined";
+// checking for existence of pageObjects is pretty difficult
+// errors are thrown but we should check to make sure its the error that we
+// want and not another real error
+// to make things more difficult depending on how you reference the pageObject
+// an error with a different message is thrown for example:
+
+// pageObject[thing]() will give you a Element not found error
+
+// whereas:
+
+// const obj = pageObject[thing]; obj() will give you a 'cannot destructure error'
+// and in CI it will give you a 'cannot read property' error
+
+// the difference in CI could be a difference due to headless vs headed browser
+// or difference in Chrome/browser versions
+
+// ideally we wouldn't be checking on error messages at all, but we want to make sure
+// that real errors are picked up by the tests, so if this gets unmanageable at any point
+// look at checking for the instance of e being TypeError or similar
+const isExpectedError = function(e) {
+  return [notFound, cannotDestructure, cannotReadContext].some(item => e.message.startsWith(item));
+};
+
 export default function(scenario, assert, find, currentPage) {
   scenario
     .then('I see $property on the $component like yaml\n$yaml', function(
@@ -64,23 +90,37 @@ export default function(scenario, assert, find, currentPage) {
       );
     })
     .then(["I don't see $property on the $component"], function(property, component) {
-      // Collection
-      var obj;
+      const message = `Expected to not see ${property} on ${component}`;
+      // Cope with collections
+      let obj;
       if (typeof currentPage()[component].objectAt === 'function') {
         obj = currentPage()[component].objectAt(0);
       } else {
         obj = currentPage()[component];
       }
-      assert.throws(
-        function() {
-          const func = obj[property].bind(obj);
-          func();
-        },
-        function(e) {
-          return e.message.startsWith('Element not found');
-        },
-        `Expected to not see ${property} on ${component}`
-      );
+      let prop;
+      try {
+        prop = obj[property];
+      } catch (e) {
+        if (isExpectedError(e)) {
+          assert.ok(true, message);
+        } else {
+          throw e;
+        }
+      }
+      if (typeof prop === 'function') {
+        assert.throws(
+          function() {
+            prop();
+          },
+          function(e) {
+            return isExpectedError(e);
+          },
+          message
+        );
+      } else {
+        assert.notOk(prop);
+      }
     })
     .then(["I don't see $property"], function(property) {
       assert.throws(