Add flash messages when viewed model is deleted #5759
Conversation
The element structure and CSS is taken from Vault; it’s a hack-job inclusion that still needs to be generalised.
This is only applied to jobs:job:index at the moment as the other places I tried it didn’t work… to be examined.
Unlike the job endpoint, the allocation endpoint doesn’t 404 when it’s been stopped, but instead times out. This requires different handling. I’ve inquired whether it’s possible to have the endpoint respond similarly.
This may need tweaking as the flash message refers to a “node” rather than a client…?? This has the same timeout situation as an allocation, but discussion suggests maybe that‘s not actually a problem.
This includes a hackish way to run watcher tests in testing. There’s surely a better approach. I’m actually unable to run the tests at all at the moment so I’m only making this commit to run them in CI!
I’m leaning toward extracting more extensive flash message tests into their own file or PR even, but for now I’m doing it all here.
This will allow another flag to be passed in.
It feels a bit awkward to be reaching into the route and controller like this in tests, but it seems preferable to me over mutating a global flag in a semi-related service.
Much better!
There was a problem hiding this comment.
This is coming together!
One last note that didn't fit anywhere inline: the toast/flash notifications represent a new component and should be added to freestyle.
| 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 with-watchers by making these tasks evented.
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 modelName along with the error because otherwise the error handler doesn’t know what to put in the flash message. Another approach would be to use the args on the task, as the model is passed in to perform, but given that the signature is id vs model, it seems questionable.
I also tried making it an encapsulated task so it could contain modelName for future reference, but that seemed to require passing in the Ember Data store because this would therefore be the task instance and not the containing model 😞
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 failed error 8 which I opened this PR to improve so we don’t need to write custom assertion failure messages)
I can hopefully address these problems but I wanted to check in on what you thought of the modelName weirdness and the general approach before sinking more time into it.
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 🤞
| @@ -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 watchInTesting used for? It doesn't seem wired up to anything.
|
|
||
| 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 run.cancelTimers like I experimented with over here: d5e4117
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.
| @@ -0,0 +1,76 @@ | |||
| .global-flash { | |||
There was a problem hiding this comment.
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.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is another approach to #5753, still a prototype. The flash message structure and CSS are hackishly taken from Vault and need to be refined.