From c3032614fe9646fb5399c4994ac55cc2d85f32e4 Mon Sep 17 00:00:00 2001 From: Sergio Padrino Date: Wed, 17 Nov 2021 12:44:57 +0100 Subject: [PATCH] Clean up and reuse some code --- app/src/lib/ci-checks/ci-checks.ts | 21 +++ .../ui/check-runs/ci-check-run-popover.tsx | 18 +-- .../pull-request-checks-failed.tsx | 122 ++++++++++-------- 3 files changed, 96 insertions(+), 65 deletions(-) diff --git a/app/src/lib/ci-checks/ci-checks.ts b/app/src/lib/ci-checks/ci-checks.ts index 592a963b14..b52d099986 100644 --- a/app/src/lib/ci-checks/ci-checks.ts +++ b/app/src/lib/ci-checks/ci-checks.ts @@ -12,6 +12,7 @@ import { import JSZip from 'jszip' import moment from 'moment' import { enableCICheckRunsLogs } from '../feature-flag' +import { GitHubRepository } from '../../models/github-repository' /** * A Desktop-specific model closely related to a GitHub API Check Run. @@ -562,3 +563,23 @@ export function getCheckRunDisplayName( : undefined return wfName !== undefined ? `${wfName} / ${checkRun.name}` : checkRun.name } + +export function getCheckRunStepURL( + checkRun: IRefCheck, + step: IAPIWorkflowJobStep, + repository: GitHubRepository, + pullRequestNumber: number +): string | null { + if (checkRun.htmlUrl === null && repository.htmlURL === null) { + // A check run may not have a url depending on how it is setup. + // However, the repository should have one; Thus, we shouldn't hit this + return null + } + + const url = + checkRun.htmlUrl !== null + ? `${checkRun.htmlUrl}/#step:${step.number}:1` + : `${repository.htmlURL}/pull/${pullRequestNumber}` + + return url +} diff --git a/app/src/ui/check-runs/ci-check-run-popover.tsx b/app/src/ui/check-runs/ci-check-run-popover.tsx index ca8aacd6b6..6684fd7305 100644 --- a/app/src/ui/check-runs/ci-check-run-popover.tsx +++ b/app/src/ui/check-runs/ci-check-run-popover.tsx @@ -6,6 +6,7 @@ import { getCheckRunConclusionAdjective, ICombinedRefCheck, IRefCheck, + getCheckRunStepURL, } from '../../lib/ci-checks/ci-checks' import { Octicon, syncClockwise } from '../octicons' import { Button } from '../lib/button' @@ -229,18 +230,13 @@ export class CICheckRunPopover extends React.PureComponent< checkRun: IRefCheck, step: IAPIWorkflowJobStep ): void => { - if (checkRun.htmlUrl === null && this.props.repository.htmlURL === null) { - // A check run may not have a url depending on how it is setup. - // However, the repository should have one; Thus, we shouldn't hit this - return + const { repository, prNumber, dispatcher } = this.props + + const url = getCheckRunStepURL(checkRun, step, repository, prNumber) + + if (url !== null) { + dispatcher.openInBrowser(url) } - - const url = - checkRun.htmlUrl !== null - ? `${checkRun.htmlUrl}/#step:${step.number}:1` - : `${this.props.repository.htmlURL}/pull/${this.props.prNumber}` - - this.props.dispatcher.openInBrowser(url) } private getCommitRef(prNumber: number): string { diff --git a/app/src/ui/notifications/pull-request-checks-failed.tsx b/app/src/ui/notifications/pull-request-checks-failed.tsx index d533149752..1d06771806 100644 --- a/app/src/ui/notifications/pull-request-checks-failed.tsx +++ b/app/src/ui/notifications/pull-request-checks-failed.tsx @@ -10,6 +10,7 @@ import { getLatestPRWorkflowRunsLogsForCheckRun, getCheckRunActionsJobsAndLogURLS, isFailure, + getCheckRunStepURL, } from '../../lib/ci-checks/ci-checks' import { Account } from '../../models/account' import { API, IAPIWorkflowJobStep } from '../../lib/api' @@ -104,14 +105,6 @@ export class PullRequestChecksFailed extends React.Component< ) - const selectedCheck = this.selectedCheck - - const failedChecks = this.state.checks.filter( - check => check.conclusion === 'failure' - ) - const pluralChecks = failedChecks.length > 1 ? 'checks' : 'check' - const pluralThem = failedChecks.length > 1 ? 'them' : 'it' - const loadingChecksInfo = this.loadingChecksInfo return ( @@ -125,39 +118,13 @@ export class PullRequestChecksFailed extends React.Component< loading={loadingChecksInfo || this.state.switchingToPullRequest} > - - - {failedChecks.length} {pluralChecks} failed in your pull request. - Do you want to switch to that Pull Request now and start fixing{' '} - {pluralThem}? - - + {this.renderSummary()}
-
- - {truncateWithEllipsis( - this.props.commitMessage, - MaxCommitMessageLength - )} - - {' '} - {this.props.commitSha.slice(0, 9)} - {this.renderRerunButton()} -
+ {this.renderCheckRunHeader()}
- - {selectedCheck !== undefined && - this.renderCheckRunSteps(selectedCheck)} + {this.renderCheckRunJobs()} + {this.renderCheckRunSteps()}
@@ -174,18 +141,66 @@ export class PullRequestChecksFailed extends React.Component< ) } - private renderCheckRunSteps(checkRun: IRefCheck) { + private renderSummary() { + const failedChecks = this.state.checks.filter(isFailure) + const pluralChecks = failedChecks.length > 1 ? 'checks' : 'check' + const pluralThem = failedChecks.length > 1 ? 'them' : 'it' + return ( + + {failedChecks.length} {pluralChecks} failed in your pull request. Do you + want to switch to that Pull Request now and start fixing {pluralThem}? + + ) + } + + private renderCheckRunHeader() { + return ( +
+ + {truncateWithEllipsis( + this.props.commitMessage, + MaxCommitMessageLength + )} + + {' '} + {this.props.commitSha.slice(0, 9)} + {this.renderRerunButton()} +
+ ) + } + + private renderCheckRunJobs() { + return ( + + ) + } + + private renderCheckRunSteps() { + const selectedCheck = this.selectedCheck + if (selectedCheck === undefined) { + return null + } + if (this.loadingChecksInfo) { // TODO: add nice loading indicator return null } const stepsContent = - checkRun.actionJobSteps === undefined ? ( + selectedCheck.actionJobSteps === undefined ? ( this.renderEmptyLogOutput() ) : ( ) @@ -195,7 +210,7 @@ export class PullRequestChecksFailed extends React.Component< ) } - private renderEmptyLogOutput = () => { + private renderEmptyLogOutput() { return (
@@ -212,24 +227,23 @@ export class PullRequestChecksFailed extends React.Component< } private onViewJobStep = (step: IAPIWorkflowJobStep): void => { - const repository = this.props.repository.gitHubRepository + const { repository, pullRequest, dispatcher } = this.props const checkRun = this.selectedCheck - if ( - checkRun === undefined || - (checkRun.htmlUrl === null && repository.htmlURL === null) - ) { - // A check run may not have a url depending on how it is setup. - // However, the repository should have one; Thus, we shouldn't hit this + if (checkRun === undefined) { return } - const url = - checkRun.htmlUrl !== null - ? `${checkRun.htmlUrl}/#step:${step.number}:1` - : `${repository.htmlURL}/pull/${this.props.pullRequest.pullRequestNumber}` + const url = getCheckRunStepURL( + checkRun, + step, + repository.gitHubRepository, + pullRequest.pullRequestNumber + ) - this.props.dispatcher.openInBrowser(url) + if (url !== null) { + dispatcher.openInBrowser(url) + } } public componentDidMount() {