Skip to content

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

Merged
merged 3 commits into from Nov 9, 2016
Merged

Fix 500 error caused by deleted users on issues (#3675) #18

merged 3 commits into from Nov 9, 2016

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2016

This PR will resolve 500s caused by deleted users.

@ghost ghost added the type/bug label Nov 3, 2016
@lunny
Copy link
Member

lunny commented Nov 3, 2016

hihi, I think one should not LGTM himself's PR.

@tboerger
Copy link
Member

tboerger commented Nov 3, 2016

Needs to be rebased, otherwise LGTM

@tboerger tboerger closed this Nov 3, 2016
@tboerger
Copy link
Member

tboerger commented Nov 3, 2016

Please reopen against master.

@tboerger tboerger added invalid and removed type/bug labels Nov 3, 2016
@bkcsoft bkcsoft changed the base branch from develop to master November 4, 2016 03:54
@bkcsoft
Copy link
Member

bkcsoft commented Nov 4, 2016

@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 )

@bkcsoft bkcsoft reopened this Nov 4, 2016
@bkcsoft bkcsoft added type/bug issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail and removed reviewed/invalid labels Nov 4, 2016
@codecov-io
Copy link

codecov-io commented Nov 4, 2016

Current coverage is 2.18% (diff: 0.00%)

No coverage report found for master at 5c54243.

Powered by Codecov. Last update 5c54243...fd6be0d

bkcsoft
bkcsoft previously requested changes Nov 4, 2016
issue.PosterID = -1
issue.Poster = NewGhostUser()
} else {
return fmt.Errorf("getUserByID.(poster) [%d]: %v", issue.PosterID, err)
Copy link
Member

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 🙂

@strk
Copy link
Member

strk commented Nov 4, 2016

An automated test would be great here

@strk
Copy link
Member

strk commented Nov 4, 2016

@lunny can't lgtm check for self-lgtm ? (and I think your comment also counted)

@lunny
Copy link
Member

lunny commented Nov 4, 2016

lgtm can check but not the default. I have configed it correctly but the problem is lgtm don't know not LGTM.

@tboerger tboerger added this to the 1.0.0 milestone Nov 4, 2016
@ghost ghost dismissed bkcsoft’s stale review November 4, 2016 18:48

Fixed

@ghost
Copy link
Author

ghost commented Nov 4, 2016

@bkcsoft Would this work?

@lunny
Copy link
Member

lunny commented Nov 5, 2016

It seems it's no need this PR via Files changed.

@strk
Copy link
Member

strk commented Nov 5, 2016

Having an automated test and instructions on how to reproduce the bug would help evaluating the need for change.

@lunny
Copy link
Member

lunny commented Nov 5, 2016

Yes

@ghost
Copy link
Author

ghost commented Nov 6, 2016

This is in reference to gogs/gogs#3675

@strk
Copy link
Member

strk commented Nov 6, 2016

I've just tried reproducing with these steps:

  • Create a user X
  • Have user X file issue on project Y
  • Delete user X
  • Visit issues of project Y
    Got the 505 internal error, before and after applying this patch.

Logs say:

2016/11/06 19:53:59 [...outers/repo/issue.go:188 Issues()] [E] Issues: LoadAttributes [42705]: user does not exist [uid: 3, name: ]
2016/11/06 19:53:59 [D] Template: status/500

@DblK DblK added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Nov 8, 2016
@andreynering
Copy link
Contributor

I tried this but it doesn't fix the error.

@codecov-io
Copy link

codecov-io commented Nov 9, 2016

Current coverage is 3.14% (diff: 0.00%)

No coverage report found for master at 5c54243.

Powered by Codecov. Last update 5c54243...a6c487f

@ghost
Copy link
Author

ghost commented Nov 9, 2016

@andreynering @strk

Sorry, took me awhile but this should fix the error completely, as I forgot to reset the err variable.

@strk
Copy link
Member

strk commented Nov 9, 2016

The lgtm service is somewhat lame in that it doesn't reset upon pushing new commits.
@LefsFlarey: an automated test would still be great here, would you consider giving it a try ?

@strk
Copy link
Member

strk commented Nov 9, 2016

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 ?)

@strk
Copy link
Member

strk commented Nov 9, 2016

Also, the "Ghost" user avatar seems to be the default avatar, maybe we want a Ghost-ed gopher ? :)

@strk
Copy link
Member

strk commented Nov 9, 2016

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)

@lunny
Copy link
Member

lunny commented Nov 9, 2016

LGTM

@lunny lunny merged commit c511f1c into go-gitea:master Nov 9, 2016
@tboerger tboerger removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail labels Nov 11, 2016
@ghost ghost deleted the issue/3675 branch November 25, 2016 01:38
@tboerger tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Nov 29, 2016
@ghost ghost restored the issue/3675 branch December 11, 2016 05:39
lunny referenced this pull request in lunny/gitea Feb 7, 2019
Fixed typos on CONTRIBUTING based on work of @thehowl
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants