Skip to content

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