-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Show package count in header #26467
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
Show package count in header #26467
Conversation
git_model "code.gitea.io/gitea/models/git" | ||
issues_model "code.gitea.io/gitea/models/issues" | ||
packages_model "code.gitea.io/gitea/models/packages" | ||
access_model "code.gitea.io/gitea/models/perm/access" | ||
repo_model "code.gitea.io/gitea/models/repo" |
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.
This file needs a serious refactoring.
A modules/context
file, which is one of the most essential files in the hierarchy, should not depend on models
and services
…
But that's not a problem for this PR.
func ToOrganization(ctx context.Context, org *organization.Organization) *api.Organization { | ||
numPackages, _ := packages_model.CountLatestVersions(ctx, &packages_model.PackageSearchOptions{OwnerID: org.ID, IsInternal: 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.
This change looks really strange and not at all how the code should be structured.
This method should not make database queries.
It's a much better idea to store the information inside organization.Organization
, if at all:
After all, there are already API routes for getting this information (-> X-Total-Count
).
Furthermore, if we do that, I can already see the requests to also add it for issues, labels, milestones, PRs, releases, tags, projects, …
So in summary, I'm against modifying the API.
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.
To the API response in general: The Number is shown on the Profile, so it should also be returned by the API, so 3rd Party Apps can add this Feature easily. As we don't show Issues or Milestones on the profile, there is no need to return a count.
To the location: There are also other places in the convert API ehere databse querrys are done, but I can move it to the User7org Modell and add a LoadCount
function.
@@ -193,6 +201,7 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR | |||
OpenIssues: repo.NumOpenIssues, | |||
OpenPulls: repo.NumOpenPulls, | |||
Releases: int(numReleases), | |||
Packages: int(numPackages), |
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.
See comment above.
@@ -62,6 +66,7 @@ func toUser(ctx context.Context, user *user_model.User, signed, authed bool) *ap | |||
Followers: user.NumFollowers, | |||
Following: user.NumFollowing, | |||
StarredRepos: user.NumStars, | |||
Packages: int(numPackages), |
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.
See comment above.
I'd like to say that we shouldn't do so. Actually we should remove most useless numbers from the tab titles, also reduce unnecessary database queries.
And this PR is stale for long time, so I think we can close it. Feel free to reopen if there are better designs. |
This shows the number of packages in the header. This applies to repos, users and orgs. The count is also returned from the API.
I haven't found a way to connect a package to a repo through the API, so I couldn't write a test for that.