Run Now UI Service Restoration #1234

Merged
merged 7 commits into from Aug 24, 2016

Conversation

Projects
None yet
3 participants
@Calvinp
Contributor

Calvinp commented Aug 19, 2016

Fixes the following:

  • Run now modal wasn't able to find the task being run to browse to its sandbox or tail the file.
  • Run now modal used to save in local storage the file you chose to tail and the after-run setting.
  • Compared to using a multi-input, a tagsinput made it difficult to properly enter multiple command line arguments as separate arguments if there was a space in one of them.
    This brings that functionality back.

cc @tpetr @wolfd

@Calvinp Calvinp changed the title from Run Now UI Service Restoration (WIP) to Run Now UI Service Restoration Aug 19, 2016

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Aug 19, 2016

Contributor

Looks like the poller is working properly.

Contributor

Calvinp commented Aug 19, 2016

Looks like the poller is working properly.

this.props.runNow(data).then((response) => {
if (response.error) {
Messenger().post({
message: '<p>This request cannot be run now. This is likely because it is already running.</p>',
type: 'error'
});
} else if (_.contains([RunNowModal.AFTER_TRIGGER.SANDBOX.value, RunNowModal.AFTER_TRIGGER.TAIL.value], data.afterTrigger)) {
+ const requestId = Utils.maybe(response, ['data', 'request', 'id']);

This comment has been minimized.

@wolfd

wolfd Aug 22, 2016

Contributor

What happens when this maybe returns undefined?

@wolfd

wolfd Aug 22, 2016

Contributor

What happens when this maybe returns undefined?

This comment has been minimized.

@Calvinp

Calvinp Aug 22, 2016

Contributor

The poller still tries to find the task, but never will because it's requesting /api/history/request/undefined/run/${runId}.
It might make sense to not even try to poll if that's undefined.
EDIT - This is implemented.

@Calvinp

Calvinp Aug 22, 2016

Contributor

The poller still tries to find the task, but never will because it's requesting /api/history/request/undefined/run/${runId}.
It might make sense to not even try to poll if that's undefined.
EDIT - This is implemented.

data.afterTrigger === RunNowModal.AFTER_TRIGGER.TAIL.value && data.fileToTail
);
}
});
}
+ getDefaultFileToTail() {
+ const previousFile = localStorage.getItem(LOCAL_STORAGE_TAIL_AFTER_TRIGGER_FILENAME);
+ if (previousFile) return previousFile;

This comment has been minimized.

@wolfd

wolfd Aug 22, 2016

Contributor

is this actually allowed in our linting? I'm not a fan of single line ifs without blocks.

@wolfd

wolfd Aug 22, 2016

Contributor

is this actually allowed in our linting? I'm not a fan of single line ifs without blocks.

This comment has been minimized.

@Calvinp

Calvinp Aug 22, 2016

Contributor

It is allowed, but I can change these ifs.

@Calvinp

Calvinp Aug 22, 2016

Contributor

It is allowed, but I can change these ifs.

@@ -35,22 +39,34 @@ class RunNowModal extends Component {
}
handleRunNow(data) {
+ localStorage.setItem(LOCAL_STORAGE_AFTER_TRIGGER_VALUE, data.afterTrigger);
+ const runId = uuid.v4();
+ data.runId = runId;

This comment has been minimized.

@wolfd

wolfd Aug 22, 2016

Contributor

what is this data object that we're modifying here?

@wolfd

wolfd Aug 22, 2016

Contributor

what is this data object that we're modifying here?

This comment has been minimized.

@Calvinp

Calvinp Aug 22, 2016

Contributor

That's the object that is returned by the FormModal. I'd be fine with cloning it first to avoid mutating the function argument.

@Calvinp

Calvinp Aug 22, 2016

Contributor

That's the object that is returned by the FormModal. I'd be fine with cloning it first to avoid mutating the function argument.

@ssalinas ssalinas modified the milestone: 0.10.1 Aug 22, 2016

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Aug 22, 2016

Contributor

Brought back the multi-input for command line arguments.

screenshot 2016-08-22 12 49 09

Contributor

Calvinp commented Aug 22, 2016

Brought back the multi-input for command line arguments.

screenshot 2016-08-22 12 49 09

@wolfd

This comment has been minimized.

Show comment
Hide comment
@wolfd

wolfd Aug 22, 2016

Contributor

LGTM

Contributor

wolfd commented Aug 22, 2016

LGTM

@ssalinas ssalinas merged commit 3f66506 into master Aug 24, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@ssalinas ssalinas deleted the run_now_ui_service_restoration branch Aug 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment