Skip to content

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

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