-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CORE-1952 Display limit check errors from instant launch actions #549
CORE-1952 Display limit check errors from instant launch actions #549
Conversation
* Updated the `InstantLaunchButtonWrapper` to check for error codes related to VICE permission or limit errors, and to display the same related dialog as displayed from the `AppName` in the apps listing. * Since `POST /analyses` does not return `limitChecks` info in errors, refactored the `instantlyLaunch` service facade to parse `limitChecks` info from the `/quicklaunches/{id}/app-info` call it already makes, and to reject the call if the `canRun` flag is set to `false`. * Removed `catch` from `instantlyLaunch` promises so that errors from those endpoints can be properly displayed in the UI (rather than causing undefined `ql` or `app` exceptions). * Refactored `RunErrorDialog` from `AppName` into its own component. * Removed duplicate `usageSummary*Response` mocks from data stories. * Removed duplicate template args from data listing stories. * Fixed a deprecated `rows` warning in `FormMultilineTextField`. * Added missing and updated vice i18n namespaces to pages.
@@ -358,14 +356,29 @@ export const instantlyLaunch = ({ instantLaunch, resource, output_dir }) => { | |||
// Contains the Promises that resolve to the data needed to perform a job submission. | |||
const promiseList = [ | |||
savedLaunchPromise, | |||
getAppInfo({ launchId: savedLaunchId }).catch((e) => console.log(e)), | |||
getAppInfo({ launchId: savedLaunchId }), |
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.
Not sure why the errors from these promises were getting caught and ignored, but I removed those catches so that errors from those endpoints can be properly displayed in the UI (rather than causing undefined ql
or app
exceptions below).
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.
Looks good to me! 👍
const SupportLink = (props) => ( | ||
<Link | ||
variant="body1" | ||
component="button" | ||
onClick={intercomShow} | ||
{...props} | ||
/> | ||
); | ||
|
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.
👍
Thanks for the review! 👍 |
This PR will update the
InstantLaunchButtonWrapper
to check for error codes related to VICE permission or limit errors, and to display the same related dialog as displayed from theAppName
in the apps listing.Since
POST /analyses
does not returnlimitChecks
info in errors, I had to refactor theinstantlyLaunch
service facade to parselimitChecks
info from the/quicklaunches/{id}/app-info
call it already makes, and to reject the analysis submission if thecanRun
flag is set tofalse
.