Skip to content

Make sure author association is set on review events#1207

Merged
RMacfarlane merged 1 commit intomasterfrom
rmacfarlane/blankDescription
Jun 6, 2019
Merged

Make sure author association is set on review events#1207
RMacfarlane merged 1 commit intomasterfrom
rmacfarlane/blankDescription

Conversation

@RMacfarlane
Copy link
Contributor

@RMacfarlane RMacfarlane commented Jun 4, 2019

Fixes #1175

We do authorAssocation.toLowerCase() in the WebView, but were not correctly setting authorAssocation on review events when using the REST API. There's still an "any" that's destroying type safety in the function converting the REST response to normalized form, we need to update our octokit typings. I removed some casts in our GraphQL conversion function that were preventing type checking.

Copy link
Contributor

@kuychaco kuychaco left a comment

Choose a reason for hiding this comment

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

👍 :shipit:


export function parseGraphQLTimelineEvents(events: (GraphQL.MergedEvent | GraphQL.Review | GraphQL.IssueComment | GraphQL.Commit | GraphQL.AssignedEvent)[], githubRepository: GitHubRepository): Common.TimelineEvent[] {
let ret: Common.TimelineEvent[] = [];
const normalizedEvents: Common.TimelineEvent[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 for descriptive variable names

let commitEv = event as GraphQL.Commit;
ret.push({
normalizedEvents.push({
id: commitEv.databaseId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catches here!

@RMacfarlane RMacfarlane merged commit 5ae5f63 into master Jun 6, 2019
@RMacfarlane RMacfarlane deleted the rmacfarlane/blankDescription branch September 6, 2019 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Description page is blank after 0.7.0 upgrade

2 participants