From 9daf8f53d96beb000d52a09b1c044eaeb1f88b9c Mon Sep 17 00:00:00 2001 From: John Cowen Date: Fri, 22 Mar 2019 17:10:33 +0000 Subject: [PATCH] ui: Adds blocking query support to the service detail page (#5479) This commit includes several pieces of functionality to enable services to be removed and the page to present information that this has happened but also keep the deleted information on the page. Along with the more usual blocking query based listing. To enable this: 1. Implements `meta` on the model (only available on collections in ember) 2. Adds new `catchable` ComputedProperty alongside a `listen` helper for working with specific errors that can be thrown from EventSources in an ember-like way. Briefly, normal computed properties update when a property changes, EventSources can additionally throw errors so we can catch them and show different visuals based on that. --- ui-v2/app/computed/catchable.js | 11 ++++++ ui-v2/app/controllers/dc/services/show.js | 19 +++++++++- .../app/instance-initializers/event-source.js | 6 +++ ui-v2/app/mixins/with-event-source.js | 31 +++++++++++++++- ui-v2/app/models/service.js | 1 + ui-v2/app/routes/dc/services/show.js | 8 ---- ui-v2/app/serializers/application.js | 25 +++++++------ ui-v2/app/services/repository/service.js | 14 +++++++ .../templates/dc/services/-notifications.hbs | 7 ++++ ui-v2/app/templates/dc/services/index.hbs | 3 +- ui-v2/app/templates/dc/services/show.hbs | 3 ++ ui-v2/config/environment.js | 2 + .../tests/acceptance/dc/list-blocking.feature | 37 +++++++++++++++---- ui-v2/tests/helpers/set-cookies.js | 1 + ui-v2/tests/helpers/type-to-url.js | 1 + .../services/repository/service-test.js | 4 ++ .../unit/controllers/dc/services/show-test.js | 2 +- 17 files changed, 144 insertions(+), 31 deletions(-) create mode 100644 ui-v2/app/computed/catchable.js create mode 100644 ui-v2/app/templates/dc/services/-notifications.hbs diff --git a/ui-v2/app/computed/catchable.js b/ui-v2/app/computed/catchable.js new file mode 100644 index 0000000000..9cc2bf008b --- /dev/null +++ b/ui-v2/app/computed/catchable.js @@ -0,0 +1,11 @@ +import ComputedProperty from '@ember/object/computed'; +import computedFactory from 'consul-ui/utils/computed/factory'; + +export default class Catchable extends ComputedProperty { + catch(cb) { + return this.meta({ + catch: cb, + }); + } +} +export const computed = computedFactory(Catchable); diff --git a/ui-v2/app/controllers/dc/services/show.js b/ui-v2/app/controllers/dc/services/show.js index 3f21a7b36b..8182d66203 100644 --- a/ui-v2/app/controllers/dc/services/show.js +++ b/ui-v2/app/controllers/dc/services/show.js @@ -1,9 +1,13 @@ import Controller from '@ember/controller'; import { get, set, computed } from '@ember/object'; +import { alias } from '@ember/object/computed'; import { inject as service } from '@ember/service'; import WithSearching from 'consul-ui/mixins/with-searching'; -export default Controller.extend(WithSearching, { +import WithEventSource, { listen } from 'consul-ui/mixins/with-event-source'; +export default Controller.extend(WithEventSource, WithSearching, { dom: service('dom'), + notify: service('flashMessages'), + items: alias('item.Nodes'), init: function() { this.searchParams = { serviceInstance: 's', @@ -17,6 +21,19 @@ export default Controller.extend(WithSearching, { // need this variable set(this, 'selectedTab', 'instances'); }, + item: listen('item').catch(function(e) { + if (e.target.readyState === 1) { + // OPEN + if (get(e, 'error.errors.firstObject.status') === '404') { + get(this, 'notify').add({ + destroyOnClick: false, + sticky: true, + type: 'warning', + action: 'update', + }); + } + } + }), searchable: computed('items', function() { return get(this, 'searchables.serviceInstance') .add(get(this, 'items')) diff --git a/ui-v2/app/instance-initializers/event-source.js b/ui-v2/app/instance-initializers/event-source.js index 705accdd5b..01265dfec5 100644 --- a/ui-v2/app/instance-initializers/event-source.js +++ b/ui-v2/app/instance-initializers/event-source.js @@ -34,6 +34,12 @@ export function initialize(container) { repo: 'repository/service/event-source', }, }, + { + route: 'dc/services/show', + services: { + repo: 'repository/service/event-source', + }, + }, ]) .forEach(function(definition) { if (typeof definition.extend !== 'undefined') { diff --git a/ui-v2/app/mixins/with-event-source.js b/ui-v2/app/mixins/with-event-source.js index 1fccd1dc83..f315f32f0b 100644 --- a/ui-v2/app/mixins/with-event-source.js +++ b/ui-v2/app/mixins/with-event-source.js @@ -1,12 +1,38 @@ import Mixin from '@ember/object/mixin'; +import { computed as catchable } from 'consul-ui/computed/catchable'; +import purify from 'consul-ui/utils/computed/purify'; -export default Mixin.create({ +import WithListeners from 'consul-ui/mixins/with-listeners'; +const PREFIX = '_'; +export default Mixin.create(WithListeners, { + setProperties: function(model) { + const _model = {}; + Object.keys(model).forEach(key => { + // here (see comment below on deleting) + if (typeof this[key] !== 'undefined' && this[key].isDescriptor) { + _model[`${PREFIX}${key}`] = model[key]; + const meta = this.constructor.metaForProperty(key) || {}; + if (typeof meta.catch === 'function') { + if (typeof _model[`${PREFIX}${key}`].addEventListener === 'function') { + this.listen(_model[`_${key}`], 'error', meta.catch.bind(this)); + } + } + } else { + _model[key] = model[key]; + } + }); + return this._super(_model); + }, reset: function(exiting) { if (exiting) { Object.keys(this).forEach(prop => { if (this[prop] && typeof this[prop].close === 'function') { this[prop].close(); // ember doesn't delete on 'resetController' by default + // right now we only call reset when we are exiting, therefore a full + // setProperties will be called the next time we enter the Route so this + // is ok for what we need and means that the above conditional works + // as expected (see 'here' comment above) delete this[prop]; } }); @@ -14,3 +40,6 @@ export default Mixin.create({ return this._super(...arguments); }, }); +export const listen = purify(catchable, function(props) { + return props.map(item => `${PREFIX}${item}`); +}); diff --git a/ui-v2/app/models/service.js b/ui-v2/app/models/service.js index cf98df3114..3d7e425928 100644 --- a/ui-v2/app/models/service.js +++ b/ui-v2/app/models/service.js @@ -30,6 +30,7 @@ export default Model.extend({ Node: attr(), Service: attr(), Checks: attr(), + meta: attr(), passing: computed('ChecksPassing', 'Checks', function() { let num = 0; // TODO: use typeof diff --git a/ui-v2/app/routes/dc/services/show.js b/ui-v2/app/routes/dc/services/show.js index 3757421cd8..670c607669 100644 --- a/ui-v2/app/routes/dc/services/show.js +++ b/ui-v2/app/routes/dc/services/show.js @@ -15,14 +15,6 @@ export default Route.extend({ const repo = get(this, 'repo'); return hash({ item: repo.findBySlug(params.name, this.modelFor('dc').dc.Name), - }).then(function(model) { - return { - ...model, - ...{ - // Nodes happen to be the ServiceInstances here - items: model.item.Nodes, - }, - }; }); }, setupController: function(controller, model) { diff --git a/ui-v2/app/serializers/application.js b/ui-v2/app/serializers/application.js index 8150179254..defe01efe8 100644 --- a/ui-v2/app/serializers/application.js +++ b/ui-v2/app/serializers/application.js @@ -15,15 +15,6 @@ export default Serializer.extend({ const headers = payload[HTTP_HEADERS_SYMBOL] || {}; delete payload[HTTP_HEADERS_SYMBOL]; const normalizedPayload = this.normalizePayload(payload, id, requestType); - const response = this._super( - store, - primaryModelClass, - { - [primaryModelClass.modelName]: normalizedPayload, - }, - id, - requestType - ); // put the meta onto the response, here this is ok // as JSON-API allows this and our specific data is now in // response[primaryModelClass.modelName] @@ -31,7 +22,7 @@ export default Serializer.extend({ // (which was the reason for the Symbol-like property earlier) // use a method modelled on ember-data methods so we have the opportunity to // do this on a per-model level - response.meta = this.normalizeMeta( + const meta = this.normalizeMeta( store, primaryModelClass, headers, @@ -39,7 +30,19 @@ export default Serializer.extend({ id, requestType ); - return response; + if (requestType === 'queryRecord') { + normalizedPayload.meta = meta; + } + return this._super( + store, + primaryModelClass, + { + meta: meta, + [primaryModelClass.modelName]: normalizedPayload, + }, + id, + requestType + ); }, normalizeMeta: function(store, primaryModelClass, headers, payload, id, requestType) { const meta = { diff --git a/ui-v2/app/services/repository/service.js b/ui-v2/app/services/repository/service.js index 61ef1d61cd..3e42c84f56 100644 --- a/ui-v2/app/services/repository/service.js +++ b/ui-v2/app/services/repository/service.js @@ -8,6 +8,18 @@ export default RepositoryService.extend({ findBySlug: function(slug, dc) { return this._super(...arguments).then(function(item) { const nodes = get(item, 'Nodes'); + if (nodes.length === 0) { + // TODO: Add an store.error("404", "message") or similar + // or move all this to serializer + const e = new Error(); + e.errors = [ + { + status: '404', + title: 'Not found', + }, + ]; + throw e; + } const service = get(nodes, 'firstObject'); const tags = nodes .reduce(function(prev, item) { @@ -16,6 +28,7 @@ export default RepositoryService.extend({ .uniq(); set(service, 'Tags', tags); set(service, 'Nodes', nodes); + set(service, 'meta', get(item, 'meta')); return service; }); }, @@ -36,6 +49,7 @@ export default RepositoryService.extend({ return service; } // TODO: Add an store.error("404", "message") or similar + // or move all this to serializer const e = new Error(); e.errors = [ { diff --git a/ui-v2/app/templates/dc/services/-notifications.hbs b/ui-v2/app/templates/dc/services/-notifications.hbs new file mode 100644 index 0000000000..9dee0a3ec9 --- /dev/null +++ b/ui-v2/app/templates/dc/services/-notifications.hbs @@ -0,0 +1,7 @@ +{{#if (eq type 'update')}} + {{#if (eq status 'warning') }} + This service has been deregistered and no longer exists in the catalog. + {{else}} + {{/if}} +{{/if}} + diff --git a/ui-v2/app/templates/dc/services/index.hbs b/ui-v2/app/templates/dc/services/index.hbs index 25885f12e5..ccc7e02e7e 100644 --- a/ui-v2/app/templates/dc/services/index.hbs +++ b/ui-v2/app/templates/dc/services/index.hbs @@ -1,7 +1,6 @@ {{#app-view class="service list"}} - {{!TODO: Look at the item passed through to figure what partial to show, also move into its own service partial, for the moment keeping here for visibility}} {{#block-slot 'notification' as |status type|}} - {{partial 'dc/acls/notifications'}} + {{partial 'dc/services/notifications'}} {{/block-slot}} {{#block-slot 'header'}}

diff --git a/ui-v2/app/templates/dc/services/show.hbs b/ui-v2/app/templates/dc/services/show.hbs index 07f19a0690..a648fd8337 100644 --- a/ui-v2/app/templates/dc/services/show.hbs +++ b/ui-v2/app/templates/dc/services/show.hbs @@ -1,4 +1,7 @@ {{#app-view class="service show"}} + {{#block-slot 'notification' as |status type|}} + {{partial 'dc/services/notifications'}} + {{/block-slot}} {{#block-slot 'breadcrumbs'}}
  1. All Services
    2. diff --git a/ui-v2/config/environment.js b/ui-v2/config/environment.js index 0d937aeafb..aaeed171fd 100644 --- a/ui-v2/config/environment.js +++ b/ui-v2/config/environment.js @@ -27,7 +27,9 @@ module.exports = function(environment) { injectionFactories: ['view', 'controller', 'component'], }, }; + // TODO: These should probably go onto APP ENV = Object.assign({}, ENV, { + CONSUL_UI_DISABLE_REALTIME: false, CONSUL_GIT_SHA: (function() { if (process.env.CONSUL_GIT_SHA) { return process.env.CONSUL_GIT_SHA; diff --git a/ui-v2/tests/acceptance/dc/list-blocking.feature b/ui-v2/tests/acceptance/dc/list-blocking.feature index 6385b86a21..21bb5ec990 100644 --- a/ui-v2/tests/acceptance/dc/list-blocking.feature +++ b/ui-v2/tests/acceptance/dc/list-blocking.feature @@ -10,8 +10,8 @@ Feature: dc / list-blocking consul:client: blocking: 1 --- - Scenario: - And 3 [Model] models + Scenario: Viewing the listing pages + Given 3 [Model] models And a network latency of 100 When I visit the [Page] page for yaml --- @@ -26,8 +26,31 @@ Feature: dc / list-blocking And an external edit results in 0 [Model] models And pause until I see 0 [Model] models Where: - -------------------------------------------- - | Page | Model | Url | - | services | service | services | - | nodes | node | nodes | - -------------------------------------------- + ------------------------------------------------ + | Page | Model | Url | + | services | service | services | + | nodes | node | nodes | + ------------------------------------------------ + Scenario: Viewing detail pages with a listing + Given 3 [Model] models + And a network latency of 100 + When I visit the [Page] page for yaml + --- + dc: dc-1 + service: service-0 + --- + Then the url should be /dc-1/[Url] + And pause until I see 3 [Model] models + And an external edit results in 5 [Model] models + And pause until I see 5 [Model] models + And an external edit results in 1 [Model] model + And pause until I see 1 [Model] model + And an external edit results in 0 [Model] models + And pause for 300 + And I see the text "deregistered" in "[data-notification]" + Where: + ------------------------------------------------ + | Page | Model | Url | + | service | instance | services/service-0 | + ------------------------------------------------ + diff --git a/ui-v2/tests/helpers/set-cookies.js b/ui-v2/tests/helpers/set-cookies.js index 0cfa58313b..e56b5d6a54 100644 --- a/ui-v2/tests/helpers/set-cookies.js +++ b/ui-v2/tests/helpers/set-cookies.js @@ -9,6 +9,7 @@ export default function(type, count, obj) { key = 'CONSUL_SERVICE_COUNT'; break; case 'node': + case 'instance': key = 'CONSUL_NODE_COUNT'; break; case 'kv': diff --git a/ui-v2/tests/helpers/type-to-url.js b/ui-v2/tests/helpers/type-to-url.js index 72c648f64d..4fc736c83d 100644 --- a/ui-v2/tests/helpers/type-to-url.js +++ b/ui-v2/tests/helpers/type-to-url.js @@ -5,6 +5,7 @@ export default function(type) { requests = ['/v1/catalog/datacenters']; break; case 'service': + case 'instance': requests = ['/v1/internal/ui/services', '/v1/health/service/']; break; case 'proxy': diff --git a/ui-v2/tests/integration/services/repository/service-test.js b/ui-v2/tests/integration/services/repository/service-test.js index 630aa38e4a..cac6967922 100644 --- a/ui-v2/tests/integration/services/repository/service-test.js +++ b/ui-v2/tests/integration/services/repository/service-test.js @@ -68,6 +68,10 @@ test('findBySlug returns the correct data for item endpoint', function(assert) { const service = payload.Nodes[0]; service.Nodes = nodes; service.Tags = payload.Nodes[0].Service.Tags; + service.meta = { + date: undefined, + cursor: undefined, + }; return service; }) diff --git a/ui-v2/tests/unit/controllers/dc/services/show-test.js b/ui-v2/tests/unit/controllers/dc/services/show-test.js index 4b4a50ef72..373d5bec9a 100644 --- a/ui-v2/tests/unit/controllers/dc/services/show-test.js +++ b/ui-v2/tests/unit/controllers/dc/services/show-test.js @@ -2,7 +2,7 @@ import { moduleFor, test } from 'ember-qunit'; moduleFor('controller:dc/services/show', 'Unit | Controller | dc/services/show', { // Specify the other units that are required for this test. - needs: ['service:search', 'service:dom'], + needs: ['service:search', 'service:dom', 'service:flashMessages'], }); // Replace this with your real tests.