-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Fix 500 error caused by deleted users on issues (#3675) #18
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
Conversation
hihi, I think one should not LGTM himself's PR. |
Needs to be rebased, otherwise LGTM |
Please reopen against master. |
@tboerger you can do that by editing the PR 😉 (found that out today actually 😆 https://github.com/blog/2224-change-the-base-branch-of-a-pull-request ) |
Current coverage is 2.18% (diff: 0.00%)
|
issue.PosterID = -1 | ||
issue.Poster = NewGhostUser() | ||
} else { | ||
return fmt.Errorf("getUserByID.(poster) [%d]: %v", issue.PosterID, err) |
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.
Please still print this error if the User infact doesn't exist 🙂
An automated test would be great here |
@lunny can't lgtm check for self-lgtm ? (and I think your comment also counted) |
lgtm can check but not the default. I have configed it correctly but the problem is lgtm don't know |
@bkcsoft Would this work? |
It seems it's no need this PR via |
Having an automated test and instructions on how to reproduce the bug would help evaluating the need for change. |
Yes |
This is in reference to gogs/gogs#3675 |
I've just tried reproducing with these steps:
Logs say:
|
I tried this but it doesn't fix the error. |
Current coverage is 3.14% (diff: 0.00%)
|
Sorry, took me awhile but this should fix the error completely, as I forgot to reset the |
The |
Tested, now works, but the link to the ghost author is a 404 (maybe should be a page explaining what "Gost" is ? Or not be a link at all ?) |
Also, the "Ghost" user avatar seems to be the default avatar, maybe we want a Ghost-ed gopher ? :) |
btw, I noticed the user does not turn into a link while in the Issue page, so it's only in the "Issues List" page that it needs be fixed. (OT: the urls to user profile should really be changed IMHO - but it's for another discussion) |
LGTM |
Fixed typos on CONTRIBUTING based on work of @thehowl
This PR will resolve 500s caused by deleted users.