-
Notifications
You must be signed in to change notification settings - Fork 21
[TSPS-728] Add Timeline to Job Details page #5473
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
[TSPS-728] Add Timeline to Job Details page #5473
Conversation
832342a to
69481a7
Compare
jsotobroad
left a comment
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.
im not sure we have the data available to make the nice timeline you have here :(
| // If an error message indicates 'QC failure', we know QC checks failed. | ||
| // If quota has been charged, we know the pipeline has progressed past QC checks | ||
| export const getQcEvent = (pipelineRunResult: PipelineRunResponse): PipelineTimelineEvent | undefined => { | ||
| const qcFailed = pipelineRunResult.errorReport?.message?.includes('failed QC'); |
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.
is this case insensitive? I see this from our qc validation step
if (!passesQc) {
// extract error messages
String qcMessages = (String) Objects.requireNonNull(inputQcOutputs.get("qcMessages"));
return new StepResult(
StepStatus.STEP_RESULT_FAILURE_FATAL,
new PipelineCheckFailedException(
"User input failed QC: "
+ qcMessages
+ " To troubleshoot, please see documentation at https://broadscientificservices.zendesk.com."));
}
if its not, i wonder which string you are keying off of
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.
as written it is case sensitive and is using that string you shared
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.
oh im omega blind lol
| if (pipelineRunning && !quotaCharged) { | ||
| return { label, status: 'PENDING', moreInfo: 'Quality checks pending' }; | ||
| } | ||
| if (quotaCharged || pipelineRunResult.jobReport.completed) { |
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.
so we actually charge quota before we run the QC checks so unfortunately I dont think we can make as clean of a separation between "qc is running" and "the pipeline is running"
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.
oh shoot.
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.
I guess we can just swap the order of these 2 events in the timeline and then just leave the QC checks as pending until completion. It's not quite as useful but still gives a quick visual for a user to see "cool, my workflow went through some sort of QC checks", and if it fails, then that'll just be an additional indicator that it failed due to bad data.
The downside is it could make users think their workflow is stuck on the QC check step, but I think we can solve that with the right language choice
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.
After discussing in post, i've removed the qc event entirely. users can see the qc error on the same page anyway. once we have more granular run statuses we can add it back.
|
jsotobroad
left a comment
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.
sorry our service couldnt get the original timeline working =/
a91485c
into
mb-tsps-635-details-page-latest



Jira Ticket: https://broadworkbench.atlassian.net/browse/TSPS-728
Summary of changes:
Adds a new timeline component to the Job Details page.
NOTE: This is a PR into the Job Details page feature branch.
What
Failed pipeline:

Successful run:

In progress run:

Full page view:

Why
Testing strategy