-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix wrong user star count #26544
base: main
Are you sure you want to change the base?
Fix wrong user star count #26544
Conversation
Wrong "fixes"? |
Now fixed. It's now reference the correct Issue. |
@@ -37,16 +39,23 @@ func ToUsers(ctx context.Context, doer *user_model.User, users []*user_model.Use | |||
|
|||
// ToUserWithAccessMode convert user_model.User to api.User | |||
// AccessMode is not none show add some more information | |||
func ToUserWithAccessMode(ctx context.Context, user *user_model.User, accessMode perm.AccessMode) *api.User { | |||
func ToUserWithAccessMode(ctx context.Context, user, owner *user_model.User, accessMode perm.AccessMode) *api.User { |
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.
func ToUserWithAccessMode(ctx context.Context, user, owner *user_model.User, accessMode perm.AccessMode) *api.User { | |
func ToUserWithAccessMode(ctx context.Context, user, doer *user_model.User, accessMode perm.AccessMode) *api.User { |
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.
I think they are not always the same
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.
@lunny any more feedback?
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.
I mean about the variables name. I think the second user should be the doer
and the first one is the owner
? I don't know the different between user
and owner
from the name themselves.
Needs a merge |
are we holding this pr as of now? |
Waiting on @lunny to review, also one merge conflict. |
should be fixed now |
Any update on this one. |
|
// toUser convert user_model.User to api.User | ||
// signed shall only be set if requester is logged in. authed shall only be set if user is site admin or user himself | ||
func toUser(ctx context.Context, user *user_model.User, signed, authed bool) *api.User { | ||
func toUser(ctx context.Context, user, doer *user_model.User, signed, authed bool) *api.User { |
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.
IIRC, I have tried to fix #26530, but I met some problems of dealing with signed
and authed
here.
Normally, we can easily know whether doer
is signed
and authed
, so it seems that signed
and authed
are unnecessary. But some callers don't have doer
as param but permission access mode.
I didn't deeply check these callers why they have permission access mode but not doer
. But it seems that it is a bit strange to me of simply add doer
here.
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.
We need to know the current User. I see no other way to solve this problem.
Looks like we forgot about this one, but I'd be happy to merge it once the conflicts are resolved. |
Another conflict and some lint errors.. |
Actor: ctx.Doer, | ||
Private: ctx.IsSigned, | ||
StarredByID: ctx.ContextUser.ID, | ||
Collaborate: util.OptionalBoolFalse, |
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.
Collaborate: util.OptionalBoolFalse, | |
Collaborate: optional.Some(false), |
As per #29329. And import code.gitea.io/gitea/modules/optional
above.
Actor: doer, | ||
Private: signed, | ||
StarredByID: user.ID, | ||
Collaborate: util.OptionalBoolFalse, |
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.
Collaborate: util.OptionalBoolFalse, | |
Collaborate: optional.Some(false), |
As per #29329. And import code.gitea.io/gitea/modules/optional
above.
Fixes #26530
Fixes #27385
At the moment, the count of the stared repos is the total count. This also includes private repos, that the visitor of the profile is not able to see, which can result in confusion.