Repository navigation
removing check for vis type on saved visualizations - #14093
Conversation
There was a problem hiding this comment.
I'm worried that we are hiding an invalid state from the user, with no error message and hiding the bad visualizations. There is no way to view their contents, to change the type, or to delete them.
What if we kept the notify.error, maybe changing it something like:
if (!typeName) {
notify.error(`Visualization ${source.title} is missing type. Please add a type, or delete this visualization.`, source);
} else {
notify.error(`Visualization ${source.title} with type of "${typeName}" is invalid. Please change to a valid type or delete the visualization.`, source);
}
Then we return source instead of null (and skip the filtering on null)? Then you'll get the error message, but you can still delete the invalid visualizations, or ignore the error for the time being (instead of the previous redirect which forced you to handle the error before doing anything else).
There was a problem hiding this comment.
wouldn't that generate 100s of errors (in case you have 100 invalid visualizations?)
also the notification would keep coming back everytime you went to visualize ( i am guessing here ).
however you are right, they should not just disappear
rethinking the behaviour (let me know if you agree):
- on existing dashboards the panel should show the error, like "invalid visualization type"
- on creating new dashboards, visualization should not show in the list of visualizations
- on visualize page, should the visualization be visible ? probably yes, but somehow indicated that its broken at the moment? or maybe we should completely hide it here as well ?
- opening a visualization should redirect back to visualize home page (as we cant open it)
- visualization should be visible under saved object , where it can be edited/deleted
There was a problem hiding this comment.
on visualize page, should the visualization be visible ? probably yes, but somehow indicated that its broken at the moment? or maybe we should completely hide it here as well ?
fwiw, I have slight preference for hiding.
Def in agreement with your other recommendations.
There was a problem hiding this comment.
Thinking about it more i agree we should hide it on visualizations page. This is a page that every user hits, it would probably be confusing for some to see visualizations they can't open.
There was a problem hiding this comment.
I defer to you both. If people become confused we can always reconsider. Probably not something that many people will run into anyway.
342e7cb to
c288b76
Compare
|
ok, updated ... in addition to points above:
|
|
@ppisljar re: "on existing dashboards the panel should show the error, like "invalid visualization type"" What is the behavior prior to this PR? I thought this was handled naturally, and that I broke it with my react-grid-layout PR, but when I have a bad type in 6.0, I see just a blank space with no error: Not sure how pre 6.0 handled it, just want to make sure it isn't a 6.0 bug. |
thomasneirynck
left a comment
There was a problem hiding this comment.
I believe this is consistent with pre 6.0 behaviour. just an error to the console, and removed from dashboard.
|
before it would actually show the error: however that behaviour was not introduced with my PR .... rebasing |
c288b76 to
788c190
Compare
|
@thomasneirynck - For master and 6.1 I have a fix out here: #14206 |
| if (!(err instanceof SavedObjectNotFound)) throw err; | ||
| const savedObjectNotFound = err instanceof SavedObjectNotFound; | ||
| const unknownVisType = err.message.indexOf('Invalid type') === 0; | ||
| if (unknownVisType) err.savedObjectType = 'visualization'; |
There was a problem hiding this comment.
minor nit - adding { }'s would make this a bit easier to read.
* removing check for vis type on saved visualizations * updating based on review
* removing check for vis type on saved visualizations * updating based on review
* removing check for vis type on saved visualizations * updating based on review
Resolves #14028