From 1625a0937291fc98b322078aa2e70f9498875923 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 000000000..9cc2bf008 --- /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 3f21a7b36..8182d6620 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 705accdd5..01265dfec 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 1fccd1dc8..f315f32f0 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 cf98df311..3d7e42592 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 3757421cd..670c60766 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 815017925..defe01efe 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 61ef1d61c..3e42c84f5 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 000000000..9dee0a3ec --- /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 25885f12e..ccc7e02e7 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 07f19a069..a648fd833 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 0d937aeaf..aaeed171f 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 6385b86a2..21bb5ec99 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 0cfa58313..e56b5d6a5 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 72c648f64..4fc736c83 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 630aa38e4..cac696792 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 4b4a50ef7..373d5bec9 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.