Skip to content
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

add review, approval info and linkify #831

Merged
merged 7 commits into from
Jan 29, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions preview-src/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ body .comment-container .review-comment-header {

body .comment-container .review-comment-header > span,
body .comment-container .review-comment-header > a,
body .commit .commit-message > a {
body .commit .commit-message > a,
body .merged .merged-message > a {
margin-right: 4px;
}

Expand Down Expand Up @@ -329,12 +330,13 @@ body .overview-title .button-group button {
fill: var(--vscode-foreground);
}

.comment-container.commit {
.comment-container.commit,
.comment-container.merged {
padding-left: 16px;
box-sizing: border-box
}

.commit, .review {
.commit, .review, .merged {
display: flex;
width: 100%;
border: none;
Expand All @@ -347,24 +349,28 @@ body .overview-title .button-group button {
padding: 4px 0;
}

.commit .commit-message {
.commit .commit-message,
.merged .merged-message {
display: flex;
align-items: center;
line-height: 18px;
overflow: hidden;
}

.commit .commit-message .avatar-container {
.commit .commit-message .avatar-container,
.merged .merged-message .avatar-container {
margin-right: 4px;
flex-shrink: 0;
}

.commit .avatar-container .avatar {
.commit .avatar-container .avatar,
.merged .avatar-container .avatar {
width: 18px;
height: 18px;
}

.commit .commit-message .message {
.commit .commit-message .message,
.merged .merged-message .message {
overflow: inherit;
text-overflow: ellipsis;
white-space: nowrap;
Expand All @@ -374,6 +380,19 @@ body .overview-title .button-group button {
margin-left: auto;
}

.merged .merged-message .message,
.merged .inline-sha {
margin: 0 4px 0 0;
}

.merged svg {
width: 14px;
height: auto;
margin-right: 8px;
flex-shrink: 0;
fill: var(--vscode-foreground);
}

.details {
display: flex;
flex-direction: column;
Expand Down
57 changes: 55 additions & 2 deletions preview-src/pullRequestOverviewRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
*--------------------------------------------------------------------------------------------*/

import { dateFromNow } from '../src/common/utils';
import { TimelineEvent, CommitEvent, ReviewEvent, CommentEvent, isCommentEvent, isReviewEvent, isCommitEvent } from '../src/common/timelineEvent';
import { TimelineEvent, CommitEvent, ReviewEvent, CommentEvent, isCommentEvent, isReviewEvent, isCommitEvent, isMergedEvent, MergedEvent } from '../src/common/timelineEvent';
import { PullRequestStateEnum } from '../src/github/interface';
import md from './mdRenderer';
import { MessageHandler } from './message';
import { getState, updateState, PullRequest } from './cache';
import { Comment } from '../src/common/comment';

const commitIconSvg = require('../resources/icons/commit_icon.svg');
const mergeIconSvg = require('../resources/icons/merge_icon.svg');
const editIcon = require('../resources/icons/edit.svg');
const deleteIcon = require('../resources/icons/delete.svg');
const checkIcon = require('../resources/icons/check.svg');
Expand Down Expand Up @@ -496,6 +497,50 @@ export function renderCommit(timelineEvent: CommitEvent): HTMLElement {
return commentContainer;
}

export function renderMergedEvent(timelineEvent: MergedEvent): HTMLElement {
const shaShort = timelineEvent.sha.substring(0, 7);

const mergedMessageContainer: HTMLDivElement = document.createElement('div');
mergedMessageContainer.classList.add('comment-container', 'merged');
const mergedMessage: HTMLDivElement = document.createElement('div');
mergedMessage.className = 'merged-message';
mergedMessage.insertAdjacentHTML('beforeend', mergeIconSvg);

const userIcon = renderUserIcon(timelineEvent.user.url, timelineEvent.user.avatarUrl);
mergedMessage.appendChild(userIcon);

const login: HTMLAnchorElement = document.createElement('a');
login.className = 'author';
login.href = timelineEvent.user.url;
login.textContent = timelineEvent.user.login!;
mergedMessage.appendChild(login);

const message: HTMLSpanElement = document.createElement('span');
message.className = 'message';
message.textContent = 'merged commit';
mergedMessage.appendChild(message);

const sha: HTMLAnchorElement = document.createElement('a');
sha.className = 'inline-sha';
sha.href = timelineEvent.commitUrl;
sha.textContent = shaShort;
mergedMessage.appendChild(sha);

const ref: HTMLSpanElement = document.createElement('span');
ref.className = 'message';
ref.textContent = `into ${timelineEvent.mergeRef}`;
mergedMessage.appendChild(ref);

const timestamp: HTMLAnchorElement = document.createElement('a');
timestamp.className = 'timestamp';
timestamp.href = timelineEvent.url;
timestamp.textContent = dateFromNow(timelineEvent.createdAt);
mergedMessage.appendChild(timestamp);

mergedMessageContainer.appendChild(mergedMessage);
return mergedMessageContainer;
}

function getDiffChangeClass(type: DiffChangeType) {
switch (type) {
case DiffChangeType.Add:
Expand Down Expand Up @@ -563,6 +608,9 @@ class ReviewNode {
userLogin.href = this._review.user.url;
userLogin.textContent = this._review.user.login;

const authorAssociation: HTMLSpanElement = document.createElement('span');
authorAssociation.textContent = `(${this._review.authorAssociation.toLocaleLowerCase()})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorAssociation seems to be undefined when using the REST API. Also, when the authorAssociation is none, I think we shouldn't render it


const reviewState = document.createElement('span');
switch (this._review.state.toLowerCase()) {
case 'approved':
Expand Down Expand Up @@ -590,6 +638,7 @@ class ReviewNode {

commentHeader.appendChild(userIcon);
commentHeader.appendChild(userLogin);
commentHeader.appendChild(authorAssociation);
commentHeader.appendChild(reviewState);
commentHeader.appendChild(timestamp);

Expand All @@ -605,7 +654,7 @@ class ReviewNode {
const reviewBody: HTMLDivElement = document.createElement('div');
reviewBody.className = 'review-body';
if (this._review.body) {
reviewBody.innerHTML = md.render(emoji.emojify(this._review.body));
reviewBody.innerHTML = this._review.bodyHTML ? this._review.bodyHTML : md.render(emoji.emojify(this._review.body));
reviewCommentContainer.appendChild(reviewBody);
}

Expand Down Expand Up @@ -707,6 +756,10 @@ export function renderTimelineEvent(timelineEvent: TimelineEvent, messageHandler
return renderComment(timelineEvent, messageHandler);
}

if (isMergedEvent(timelineEvent)) {
return renderMergedEvent(timelineEvent);
}

return undefined;
}

Expand Down
1 change: 1 addition & 0 deletions resources/icons/merge_icon.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
19 changes: 18 additions & 1 deletion src/common/timelineEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ export interface ReviewEvent {
comments: Comment[];
submittedAt: string;
body: string;
bodyHTML?: string;
htmlUrl: string;
user: IAccount;
authorAssociation: string;
state: string;
id: number;
}
Expand All @@ -58,7 +60,18 @@ export interface CommitEvent {
bodyHTML?: string;
}

export type TimelineEvent = CommitEvent | ReviewEvent | CommentEvent;
export interface MergedEvent {
graphNodeId: string;
user: IAccount;
createdAt: string;
mergeRef: string;
sha: string;
commitUrl: string;
event: string;
url: string;
}

export type TimelineEvent = CommitEvent | ReviewEvent | CommentEvent | MergedEvent;

export function isReviewEvent(event: TimelineEvent): event is ReviewEvent {
return Number(event.event) === EventType.Reviewed;
Expand All @@ -71,3 +84,7 @@ export function isCommitEvent(event: TimelineEvent): event is CommitEvent {
export function isCommentEvent(event: TimelineEvent): event is CommentEvent {
return Number(event.event) === EventType.Commented;
}

export function isMergedEvent(event: TimelineEvent): event is MergedEvent {
return Number(event.event) === EventType.Merged;
}
46 changes: 31 additions & 15 deletions src/github/githubRepository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import { PullRequestModel } from './pullRequestModel';
import { CredentialStore, GitHub } from './credentials';
import { AuthenticationError } from '../common/authentication';
import { QueryOptions, MutationOptions, ApolloQueryResult } from 'apollo-boost';
import { convertRESTPullRequestToRawPullRequest } from './utils';
import { convertRESTPullRequestToRawPullRequest, parseGraphQLPullRequest } from './utils';
import { PullRequestResponse } from './graphql';
const queries = require('./queries.gql');

export const PULL_REQUEST_PAGE_SIZE = 20;

Expand Down Expand Up @@ -263,22 +265,36 @@ export class GitHubRepository implements IGitHubRepository {
async getPullRequest(id: number): Promise<PullRequestModel> {
try {
Logger.debug(`Fetch pull request ${id} - enter`, GitHubRepository.ID);
const { octokit, remote } = await this.ensure();
let { data } = await octokit.pullRequests.get({
owner: remote.owner,
repo: remote.repositoryName,
number: id
});
Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID);

if (!data.head.repo) {
Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID);
return null;
}
const { octokit, query, remote } = await this.ensure();

let item = convertRESTPullRequestToRawPullRequest(data);
if (this.supportsGraphQl()) {
const { data } = await query<PullRequestResponse>({
query: queries.PullRequest,
variables: {
owner: remote.owner,
name: remote.repositoryName,
number: id
}
});

return new PullRequestModel(this, remote, item);
Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID);
return new PullRequestModel(this, remote, parseGraphQLPullRequest(data));
} else {
let { data } = await octokit.pullRequests.get({
owner: remote.owner,
repo: remote.repositoryName,
number: id
});
Logger.debug(`Fetch pull request ${id} - done`, GitHubRepository.ID);

if (!data.head.repo) {
Logger.appendLine('The remote branch for this PR was already deleted.', GitHubRepository.ID);
return null;
}

let item = convertRESTPullRequestToRawPullRequest(data);
return new PullRequestModel(this, remote, item);
}
} catch (e) {
Logger.appendLine(`GithubRepository> Unable to fetch PR: ${e}`);
return null;
Expand Down
52 changes: 51 additions & 1 deletion src/github/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ export interface MergedEvent {
mergeRef: {
name: string;
};
commit: {
oid: string;
commitUrl: string;
};
url: string;
}

export interface IssueComment {
Expand All @@ -27,6 +32,7 @@ export interface IssueComment {
};
url: string;
body: string;
bodyHTML: string;
updatedAt: string;
createdAt: string;
viewerCanUpdate: boolean;
Expand All @@ -46,6 +52,7 @@ export interface ReviewComment {
path: string;
originalPosition: number;
body: string;
bodyHTML: string;
diffHunk: string;
position: number;
state: string;
Expand Down Expand Up @@ -97,7 +104,7 @@ export interface AssignedEvent {
export interface Review {
id: string;
databaseId: number;
authorAssocation: string;
authorAssociation: string;
author: {
login: string;
avatarUrl: string;
Expand Down Expand Up @@ -172,4 +179,47 @@ export interface DeleteReviewResponse {
}
}
};
}

export interface PullRequestResponse {
repository: {
pullRequest: {
number: number;
url: string;
state: string;
body: string;
bodyHTML: string;
title: string;
author: {
login: string;
url: string;
avatarUrl: string;
}
createdAt: string;
updatedAt: string;
headRef: {
name: string;
repository: {
nameWithOwner: string;
url: string;
}
target: {
oid: string;
}
}
baseRef: {
name: string;
repository: {
nameWithOwner: string;
url: string;
}
target: {
oid: string
}
}
merged: boolean;
mergeable: boolean;
id: string;
}
};
}
5 changes: 3 additions & 2 deletions src/github/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,15 @@ export interface PullRequest {
number: number;
state: string;
body: string;
bodyHTML?: string;
title: string;
assignee: IAccount;
assignee?: IAccount;
createdAt: string;
updatedAt: string;
head: IGitHubRef;
base: IGitHubRef;
user: IAccount;
labels: ILabel[];
labels?: ILabel[];
merged: boolean;
mergeable?: boolean;
nodeId: string;
Expand Down
Loading