Skip to content

Merge explore orgs into explore users #29816

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

Closed
wants to merge 7 commits into from

Conversation

delvh
Copy link
Member

@delvh delvh commented Mar 15, 2024

Now, there is only one explore page to search for accounts.

The existing setting to define the redirect behavior of / has been changed so that it no longer redirects to the no longer existing /explore/organizations.
As such, this refactoring is not breaking.

Additionally, fix incorrect visibility behavior for /explore/users.
It was correct for /explore/organizations.

Fixes #29803

After

grafik

(Note that both tester and test-org are orgs, test2 is a bot, and test is a normal user)

Now, there is only one `explore` page to search for accounts.

The existing setting to define the redirect behavior of `/` has been
changed so that it no longer redirects to the no longer existing `/explore/organizations`.
As such, this refactoring is not breaking.

Additionally fix incorrect visibility behavior that was correct for
`/explore/organizations` but incorrect for `/explore/users`.

Fixes go-gitea#29803
@delvh delvh added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 15, 2024
@delvh delvh self-assigned this Mar 15, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 15, 2024
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Mar 15, 2024
Comment on lines 49 to 62
if opts.Type.Has() {
cond = builder.Eq{"type": opts.Type}
if opts.IncludeReserved {
if opts.Type.Value() == UserTypeIndividual {
cond = cond.Or(builder.Eq{"type": UserTypeUserReserved}).Or(
builder.Eq{"type": UserTypeBot},
).Or(
builder.Eq{"type": UserTypeRemoteUser},
)
} else if opts.Type.Value() == UserTypeOrganization {
cond = cond.Or(builder.Eq{"type": UserTypeOrganizationReserved})
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Review hint: Hide whitespace changes for this file

@lunny
Copy link
Member

lunny commented Mar 15, 2024

I don't think this is better than before. It's confusing mixed users and orgs.

@silverwind
Copy link
Member

Maybe add a https://primer.github.io/octicons/organization-16 besides the org name to distinguish?

@delvh
Copy link
Member Author

delvh commented Mar 15, 2024

@silverwind

Additionally, I'd like to keep the refactoring clean, so that we add in these enhancements afterward.
What do you think?

I can also add it in this PR, but I would have liked to first make a refactoring and then a UX enhancement.
What's your preference on that?

@silverwind
Copy link
Member

Don't really care, these are seldomly used pages. But I do have a slight preference because landing something "unfinished" into main branch is not so great.

@delvh
Copy link
Member Author

delvh commented Mar 15, 2024

@silverwind Done.
I've adapted the screenshots as well.

@silverwind
Copy link
Member

Icon and text seem slightly misalign. Maybe it needs a gt-df gt-ac gt-fw (or equivalent tailwind classes) on the parent?

@delvh
Copy link
Member Author

delvh commented Mar 15, 2024

Done

@@ -7,11 +7,11 @@
<div class="flex-item-main">
<div class="flex-item-title">
{{if .IsOrganization}}
<span data-tooltip-content title="{{ctx.Locale.Tr "explore.user_is_organization" .DisplayName}}">{{svg "octicon-organization"}}</span>
<span class="tw-flex tw-justify-center tw-flex-wrap" data-tooltip-content title="{{ctx.Locale.Tr "explore.user_is_organization" .DisplayName}}">{{svg "octicon-organization"}}</span>
Copy link
Member

@silverwind silverwind Mar 15, 2024

Choose a reason for hiding this comment

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

Not tw-items-center instead of tw-justify-center?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea.
I simply translated your suggestion into tailwind.
Also, I see no improvement when adding tw-items-center

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry gt-ac is actually tw-content-center.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still no difference

Copy link
Member

Choose a reason for hiding this comment

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

I will check later. Maybe we should also move the icon into second row and add a User/Organization/Bot text besides it.

Copy link
Member Author

Choose a reason for hiding this comment

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

(the text is currently a tooltip)

@lafriks
Copy link
Member

lafriks commented Mar 15, 2024

I don't think it should be called users, imho the most common use case is to search for organizations not users. Personally I would just drop user search from there but if combined it would be preferable to call the tab Organizations

@delvh
Copy link
Member Author

delvh commented Mar 15, 2024

(Alternatively, the distinction could also be repos, owner.
However, I don't think that's any better either as owner is only a code-internal concept we haven't communicated to our end users, and orgs are on all accounts users too)

Furthermore, you can still query users with it, so it would be false advertising since an org is a user but a user is not necessarily an org.

Or how do you see it?

@lafriks
Copy link
Member

lafriks commented Mar 15, 2024

(Alternatively, the distinction could also be repos, owner.
However, I don't think that's any better either as owner is only a code-internal concept we haven't communicated to our end users, and orgs are on all accounts users too)

Furthermore, you can still query users with it, so it would be false advertising since an org is a user but a user is not necessarily an org.

Or how do you see it?

If you look from business side of view (not how it's technically saved in database) these are both different things. But from use case scenario in this place you would most commonly look for organizations to get to it's repositories not user profile

@lunny
Copy link
Member

lunny commented Mar 16, 2024

If you think both UI are similar because we haven't displayed more information on the list pages. I think the direction is displaying more user or org-related information on the list page but not sharing it.

@wxiaoguang
Copy link
Contributor

I also agree that the concept "User" and "Org" are fundamentally different. Although they share the same base table, but they are different things to end users, and it should avoid mixing them together.

@eeyrjmr
Copy link
Contributor

eeyrjmr commented Mar 16, 2024

just to add my 2c ...
while the backend may mean ORG and USERS are (almost) synonymous, to a user they are not.

From a UX perspective having the tab just called "users" will confuse people ... "I want to find the project page, where do I search".

From people behaviour there is a mental distinction between what a USER is and what an ORG is since an ORG is a collection of related repositories to meet a common goal . A user however is someone that interacts with gitea/git, is associated with a project (aka ORG) and may have a collection of repositories that at face value would appear to be randomly associated but means something to a user (forks of associated repositories [users or org], their own pet projects).

Likewise users are associated to org's via teams but orgs are not associated to a user.

NOW... from a streamlining of the UI... this might make sense but

  1. it can't be called users BECAUSE users use this page, orgs do not and users use this page to find orgs or users
  2. clearly indicate whether it is a USER or a ORG, especially as people use odd names and name orgs oddly (from the example in the OP you cannot tell if the returns are a USER or an ORG... you can guess from the name or if you know)

@yp05327
Copy link
Contributor

yp05327 commented Mar 27, 2024

At least, add an option next to sort, then we can select which type of user to search, individual or organization.
Then, at least, we can:
If I want to search individual account, then no organization account display. (Same as before)
If I want to search organization account, then no individual account display. (Same as before)
If I don't know which kind of it is, then search both individual and organization account. (New option)

@lunny
Copy link
Member

lunny commented Mar 27, 2024

I cannot imagine we have the requirement to search both users and orgs.

@wxiaoguang
Copy link
Contributor

According to #29816 (comment) , should we still keep this PR?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jan 1, 2025
@delvh
Copy link
Member Author

delvh commented Jan 1, 2025

Probably not.
I still understand the reason only partially, but apparently others are using this feature differently from me…

@delvh delvh closed this Jan 1, 2025
@delvh delvh deleted the merge-explore-pages branch January 1, 2025 16:46
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Apr 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge explore users with explore orgs
8 participants