-
Notifications
You must be signed in to change notification settings - Fork 392
Handle ghost users (deleted accounts) #2100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2100 +/- ##
==========================================
+ Coverage 92.59% 92.65% +0.05%
==========================================
Files 207 207
Lines 12036 12045 +9
Branches 1755 1764 +9
==========================================
+ Hits 11145 11160 +15
+ Misses 891 885 -6
Continue to review full report at Codecov.
|
... and not locally 🤔
React (and Relay) discourage you from modifying objects that are passed to components as props: https://reactjs.org/docs/components-and-props.html#props-are-read-only. The "data" passed to the Issueish constructor is sent directly from the props passed to IssueishListController, so we should treat them as immutable. I thought that React froze objects passed as props in development mode to catch these issues. That's what's failing on CI - I have no idea why it isn't failing locally for us.
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.
✨ A few random comments, no blockers. Thanks for taking care of this! It annoyed the hell out of me every time I did anything in find-and-replace 😆
@@ -67,6 +67,7 @@ export default createPaginationContainer(BareReviewCommentsAccumulator, { | |||
author { | |||
avatarUrl | |||
login | |||
url |
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 interesting, we were just URL-hacking this in before? Yeah, much cleaner to get it from GraphQL 👍
test/builder/graphql/issue.js
Outdated
@@ -18,7 +18,7 @@ export const IssueBuilder = createSpecBuilderClass('Issue', { | |||
number: {default: 123}, | |||
state: {default: 'OPEN'}, | |||
bodyHTML: {default: '<h1>HI</h1>'}, | |||
author: {linked: UserBuilder}, | |||
author: {linked: UserBuilder, default: null}, |
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 really like defaulting these to null
in the builder. This way we'll catch anywhere that an author is fetched from GraphQL but we don't account for it being nullable 👍
test/builder/graphql/timeline.js
Outdated
@@ -32,7 +32,7 @@ export const CommitBuilder = createSpecBuilderClass('Commit', { | |||
|
|||
export const CommitCommentBuilder = createSpecBuilderClass('CommitComment', { | |||
id: {default: nextID}, | |||
author: {linked: UserBuilder}, | |||
author: {linked: UserBuilder, default: null}, |
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.
You could add nullable: true
here too. All that really does is let you use the .nullAuthor()
function on the generated builder, though, which is functionally equivalent to .author(null)
.
test/builder/graphql/timeline.js
Outdated
@@ -61,7 +61,7 @@ export const HeadRefForcePushedEventBuilder = createSpecBuilderClass('HeadRefFor | |||
}, 'Node'); | |||
|
|||
export const IssueCommentBuilder = createSpecBuilderClass('IssueComment', { | |||
author: {linked: UserBuilder}, | |||
author: {linked: UserBuilder, default: null}, |
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.
Same here ☝️
@@ -6,6 +6,7 @@ export function createCommitComment(opts = {}) { | |||
const o = { | |||
id: idGen.generate('comment-comment'), | |||
commitOid: '1234abcd', | |||
includeAuthor: true, |
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've been trying to phase out these test/fixtures/factories
methods in favor of GraphQL builders - the GraphQL builders are easier to write against (the builder structure maps directly to the query!) and they keep up with changes to our queries and the schema. It's tedious as hell, though, so I don't blame you for patching this in 😛
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Handle ghost users (deleted accounts)
Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.
Requirements
Description of the Change
This PR introduces changes to handle ghost users throughout our UI. When a user account has been deleted, activity by them is replaced to be a "Deleted user" account -- https://github.com/ghost
Prior to this change, users were seeing errors thrown when looking up properties on a
null
user.To address this, I created a GHOST_USER object in our
lib/helpers.js
file and used it throughout the codebase when we'd get backnull
from the GitHub graphql API.Screenshot/Gif
N/A
Alternate Designs
N/A
Benefits
No more errors!
Possible Drawbacks
None.
Applicable Issues
Fixes #2014
Fixes #1921
Metrics
N/A
Tests
TODO
Documentation
N/A
Release Notes
User Experience Research (Optional)
N/A