Add flash messages when viewed model is deleted #5759
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,4 +20,6 @@ export default Service.extend({ | |
| setIndexFor(url, value) { | ||
| list[url] = +value; | ||
| }, | ||
|
|
||
| watchInTesting: false, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| .global-flash { | ||
| bottom: 0; | ||
| left: 12px; | ||
| margin: 10px; | ||
| max-width: 300px; | ||
| position: fixed; | ||
| width: 95%; | ||
| z-index: 300; | ||
|
|
||
| .message { | ||
| box-shadow: 0 12px 5px -7px rgba($black, 0.08), 0 11px 10px -3px rgba($black, 0.1); | ||
| } | ||
|
|
||
| .message { | ||
| border: 1px solid; | ||
| margin-bottom: 12px; | ||
| padding: 12px 16px 16px 12px; | ||
| position: relative; | ||
|
|
||
| .close-button { | ||
| background: transparent; | ||
| border: 0; | ||
| color: $grey; | ||
| cursor: pointer; | ||
| position: absolute; | ||
| right: 12px; | ||
| top: 16px; | ||
|
|
||
| .icon { | ||
| width: 9px; | ||
| height: 9px; | ||
| } | ||
| } | ||
|
|
||
| .message-title { | ||
| font-size: 16px; | ||
| font-weight: bold; | ||
| line-height: 1.25; | ||
| } | ||
|
|
||
| .close-button + .message-title { | ||
| padding-right: 16px; | ||
| } | ||
|
|
||
| .message-body { | ||
| border: 0; | ||
| margin-top: 4px; | ||
| } | ||
|
|
||
| p { | ||
| font-size: $size-7; | ||
| border: 0; | ||
| padding: 0; | ||
| } | ||
|
|
||
| .message-actions { | ||
| margin-top: 8px; | ||
|
|
||
| a, | ||
| a:not(.button):not(.file-delete-button):not(.tag) { | ||
| color: $blue; | ||
| font-weight: 600; | ||
| text-decoration: none; | ||
| } | ||
|
|
||
| > * + * { | ||
| margin-left: 8px; | ||
| } | ||
| } | ||
|
|
||
| &.is-highlight { | ||
| background: #fffdf6; | ||
| border: 1px solid #fdeeba; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import Ember from 'ember'; | ||
| import { get, getWithDefault } from '@ember/object'; | ||
| import RSVP from 'rsvp'; | ||
| import { task } from 'ember-concurrency'; | ||
| import wait from 'nomad-ui/utils/wait'; | ||
|
|
@@ -8,13 +8,14 @@ import config from 'nomad-ui/config/environment'; | |
|
|
||
| const isEnabled = config.APP.blockingQueries !== false; | ||
|
|
||
| export function watchRecord(modelName, { ignore404 } = {}) { | ||
| return task(function*(id, { throttle = 2000, runInTests = false } = {}) { | ||
| const token = new XHRToken(); | ||
| if (typeof id === 'object') { | ||
| id = get(id, 'id'); | ||
| } | ||
|
|
||
| while (isEnabled && (!Ember.testing || runInTests)) { | ||
| try { | ||
| yield RSVP.all([ | ||
| this.store.findRecord(modelName, id, { | ||
|
|
@@ -24,6 +25,16 @@ export function watchRecord(modelName) { | |
| wait(throttle), | ||
| ]); | ||
| } catch (e) { | ||
| if (!ignore404) { | ||
| const statusIs404 = getWithDefault(e, 'errors', []) | ||
| .mapBy('status') | ||
| .includes('404'); | ||
backspace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const errorIsEmptyResponse = e.message.includes('response did not have any data'); | ||
| if (statusIs404 || errorIsEmptyResponse) { | ||
| this.flashMessages.warning(`This ${modelName} no longer exists`, { sticky: true }); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. I recall talking about pushing this logic into the Did that not pan out? There was a problem hiding this comment. I tried that out in this branch, what do you think? There’s some awkwardness involving bundling the I also tried making it an encapsulated task so it could contain One significant problem is that the tests no longer pass:
That could be partially addressed by ignoring the uncaught exception, hopefully in a more targeted way to avoid how that burned me in the past. BUT it’s not just that; the assertion that a flash message is displayed is also failing, despite me being able to see that it’s there 🤔 (that’s the inscrutable I can hopefully address these problems but I wanted to check in on what you thought of the There was a problem hiding this comment. this could perhaps be revived as it’s not a huge change and would still be nice to have, maybe the above weirdness is a non-problem now 🤞 |
||
|
|
||
| yield e; | ||
| break; | ||
| } finally { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| import { run } from '@ember/runloop'; | ||
| import { currentURL, settled } from '@ember/test-helpers'; | ||
| import { assign } from '@ember/polyfills'; | ||
| import { module, test } from 'qunit'; | ||
| import { setupApplicationTest } from 'ember-qunit'; | ||
| import setupMirage from 'ember-cli-mirage/test-support/setup-mirage'; | ||
| import Allocation from 'nomad-ui/tests/pages/allocations/detail'; | ||
| import PageLayout from 'nomad-ui/tests/pages/layout'; | ||
| import moment from 'moment'; | ||
|
|
||
| let job; | ||
|
|
@@ -215,6 +216,42 @@ module('Acceptance | allocation detail', function(hooks) { | |
| }); | ||
| }); | ||
|
|
||
| // This is a separate module only because of the watchInTesting adjustment, hopefully that can change | ||
|
There was a problem hiding this comment. What is |
||
| module('Acceptance | allocation detail after stopping', function(hooks) { | ||
| setupApplicationTest(hooks); | ||
| setupMirage(hooks); | ||
|
|
||
| hooks.beforeEach(async function() { | ||
| server.create('agent'); | ||
|
|
||
| node = server.create('node'); | ||
| job = server.create('job', { groupsCount: 1, createAllocations: false }); | ||
| allocation = server.create('allocation', 'withTaskWithPorts', { clientStatus: 'running' }); | ||
|
|
||
| this.owner.lookup('service:watch-list').set('watchInTesting', true); | ||
| await Allocation.visit({ id: allocation.id }); | ||
| }); | ||
|
|
||
| test('when the allocation endpoint 404s, a flash message appears', async function(assert) { | ||
| assert.equal(PageLayout.flashMessages.length, 0); | ||
|
|
||
| const controller = this.owner.lookup('controller:allocations/allocation'); | ||
| const route = this.owner.lookup('route:allocations/allocation'); | ||
| controller.watcher = route.watch.perform(controller.model, { throttle: 0, runInTests: true }); | ||
|
There was a problem hiding this comment. As you know, these are distinctly not acceptance tests since they invoke methods on objects rather than treating the app like a black box. I'm fine with running with this as-is and adding an overarching acceptance testing + watching task to the backlog. To ramble for a minute... I think the best solution will make use of either ember-concurrency-test-waiter or some abstraction over The dream is to have a test that is easy to write, reads well, and works via a series of button clicks and waiting. Which is a tall order given the inner workings of Ember concurrency. |
||
|
|
||
| await this.server.db.allocations.remove(); | ||
| await settled(); | ||
|
|
||
| assert.equal(PageLayout.flashMessages.length, 1); | ||
|
|
||
| PageLayout.flashMessages[0].as(message => { | ||
| assert.ok(message.icon.isWarning); | ||
| assert.equal(message.title, 'Warning'); | ||
| assert.equal(message.body, 'This allocation no longer exists'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| module('Acceptance | allocation detail (rescheduled)', function(hooks) { | ||
| setupApplicationTest(hooks); | ||
| setupMirage(hooks); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are existing notification styles that at the very least the flash notifications should look like. Ideally they would use the same styles via some sass pattern or composition.