Skip to content
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

[Backport] [Fix] use default avatar for ghost user (fix 500 error) (#9536) #9537

Merged
merged 1 commit into from
Dec 30, 2019

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 29, 2019

backports #9536

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@techknowlogick / @lunny can you add Milestone 1.10.2 and lables?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 29, 2019
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 29, 2019
@zeripath
Copy link
Contributor

I'm not much of a fan of requerying before saving - however I guess it's the simplest thing to do and we don't want posterID to be -1.

In master we should find the code that sets this and stop that

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@zeripath I know where it come from: from loadPoster() and only API is afected becouse of the technike wich is used to update issues :(

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

We have three options:

  • first = simplest = in this PR (check if posterID <= 0 then dont change)
  • let the system habdle -1 as ghost user (we need to change some functions)
  • change how the api can update issue (for each field or specify the rows somehow)

@zeripath
Copy link
Contributor

If it's only loadPoster - fix that. Then make the update method log an error, but keep the sanity saving step in as we don't want to lose data.

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@zeripath log errer as you suggest - I'll make a smal refactor on main branch wich address -1 id issue

EDIT: here: #9539

@6543 6543 changed the title [Backport] [BugFix] for ghost user (2 in 1) (#9536) [Backport] [BugFix] for ghost user (2 in 1) (#9536) (#9539) Dec 29, 2019
@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@lafriks would be nice if you look at it - thanks

models/issue.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

As it's a backport it's fine though

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@zeripath #9539 is for upstream

@zeripath
Copy link
Contributor

The IsGhost function in that differs quite a lot from this one, for example you're still going to update the posterID to -1 for anything that isn't Ghost in that. Is that still intended?

@6543
Copy link
Member Author

6543 commented Dec 29, 2019

@zeripath can we switch conversation about this to #9539 ...

@6543 6543 changed the title [Backport] [BugFix] for ghost user (2 in 1) (#9536) (#9539) [BugFix] use default avatar for ghost user (fix 500 error) (#9536) Dec 30, 2019
@6543
Copy link
Member Author

6543 commented Dec 30, 2019

@zeripath INFO: split PR to single backport of (#9536)

because the second part differ to mouch from new PR

@6543
Copy link
Member Author

6543 commented Dec 30, 2019

@lunny this is now a "real" backport of #9536 - do you have time to approve?

@6543 6543 changed the title [BugFix] use default avatar for ghost user (fix 500 error) (#9536) [Backport] [Fix] use default avatar for ghost user (fix 500 error) (#9536) Dec 30, 2019
@techknowlogick techknowlogick added this to the 1.10.2 milestone Dec 30, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 30, 2019
@lunny lunny merged commit b4b8c96 into go-gitea:release/v1.10 Dec 30, 2019
@6543
Copy link
Member Author

6543 commented Dec 30, 2019

Thanks

@6543 6543 deleted the backports-9536 branch December 30, 2019 11:33
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 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.

6 participants