Pin repos on profile #30961

carlosfelgueiras · 2024-05-13T10:24:50Z

This PR implements the ability for users and organizations to pin repositories to their profiles.

At a given time, each user or organization can have, at most, 6 repositories pinned. A user can pin any repository that it has read permissions to. Organizations can only have pinned to their profiles repositories owned by themselves, only administrators of an organization can pin repositories to the org's profile.

The pins are stored on a table called repo_pin. When a repository is deleted, all its pins are automatically deleted. When a user loses visibility to a repository, the pin is not automatically removed. Instead, invalid pins will automatically be removed the next time someone does a read or write operation to said user's pins.

When a different user is visiting someone's profile, only the repositories that the visiting user can see are shown on the pins, to avoid leaking private information.

Used assets (unpin icon and pined repo cards) from #19831.

Header of the main page of a repository, showing the option to pin the repo.

Header of the main page of a repository showing the disabled pin button, since the user has reached the max limit of pinned repos.

- Created the pin model, and basic CRUD operations. - Implemented checks for lost of visibility of owner, to automatically delete the repository. - Implemented check for the lack of visibility of a viewer, when requesting the repos of another user, excluding those repositories from the list that is returned. - Added the deletion of all the pins of a repository when it is deleted. - Implemented the ability for a user to pin on the profile of an org user, checking if the user is admin of the org, and the org is owner of the repo. Co-authored-by: Daniel Carvalho <daniel.m.carvalho@tecnico.ulisboa.pt>

- Added list and count of pinned repos to the context data of the routes of a user and org profile. - Added a template for the list of pinned repo cards. - Included said template in the user and org profile templates. Co-authored-by: Daniel Carvalho <daniel.m.carvalho@tecnico.ulisboa.pt>

- Added the unpin octicon svg from a previous attempt to implement this feature. - Added the template for the button/set of buttons for pinning/unpinning a repo. - Added the use of said template in the header of a repo main page. - Added the routes for the POST requests for pinning/unpinning to user/org. Co-authored-by: Daniel Carvalho <daniel.m.carvalho@tecnico.ulisboa.pt>

- Created unit tests that check the basic functionality of the model functions - Created integration tests that check the functionality of the POST routes, verifying the effects directly on the database. Co-authored-by: Daniel Carvalho <daniel.m.carvalho@tecnico.ulisboa.pt>

models/repo/pin.go

routers/web/repo/view.go

web_src/svg/octicon-custom-pin-off.svg

silverwind · 2024-05-13T18:22:07Z

templates/shared/pinned_repo_cards.tmpl

+								{{if .IsPrivate}}
+									<span class="ui basic label">{{ctx.Locale.Tr "repo.desc.private"}}</span>
+								{{end}}
+							{{end}}


Do we have this label sub-templated yet? If not, I suggest creating templates/shared/repo/label.tmpl which accepts a Repo as its only argument.

I don't think that sub-template exists yet. At least the repo main page's header does not use any template for that part and I don't find any label template related to repos. I will create it then.

templates/repo/pin_unpin.tmpl

silverwind · 2024-05-13T18:23:52Z

Can you reduce the space between these card to half of what it is currently?

Co-authored-by: silverwind <me@silverwind.io>

… used it with the already present octicon-pin-slash

carlosfelgueiras · 2024-05-15T14:57:45Z

@silverwind I managed to reduce the space between the cards to half the size on the latest commit, but I don't know if my changes follow the best practices. Can you comment on that?

carlosfelgueiras · 2024-05-15T15:04:27Z

This is how it looks now.

models/repo/pin.go

routers/web/repo/view.go

yp05327 · 2024-05-16T01:36:03Z

services/user/pin.go

+}
+
+// CleanupPins iterates over the repos pinned by a user and removes
+// the invalid pins. (Needs to be called everytime before we read/write a pin)


(Needs to be called everytime before we read/write a pin)

This function seems a bit strange. I think it is not the correct way to handle this problem.

This approach where we clean the invalid pins only when it is needed was suggested in the original issue thread (#10375 (comment)), so we just followed that suggestion since it seemed to be agreed upon. If people want to discuss this further, we can implement what is desired later, after a consensus is reached.

It is something like this #26530
At first, we do not have permission system, and stored all these permission related data in DB.
Actually, it should be generated dynamically or there will be more and more bugs.
The comment you mentioned is about 2 years ago, which is a solution for this problem, but I don't think it is a good solution.

Maybe we should wait for the maintainers to give their opinion. I guess they are currently focused on 1.22.0, but after the release they could comment on this.

yp05327 · 2024-05-16T01:37:14Z

routers/web/repo/view.go

@@ -1168,3 +1177,31 @@ func Forks(ctx *context.Context) {

 	ctx.HTML(http.StatusOK, tplForks)
 }
+
+func loadPinData(ctx *context.Context) error {


maybe we can cache pin data

I see a good reason for caching that data, but I'm not confident in implementing this myself. I'm not too accustomed to this codebase and, since some of the pin data can be sensitive data (since some pinned repos can be private), that would be a hard task to do without any guidance.

I will ask @lunny.

I think it's unnecessary to cache these pinned repositories.

services/user/pin.go

yp05327 · 2024-05-16T01:46:17Z

services/user/pin.go

+func HasPermsToPin(ctx *context.Context, u *user_model.User, r *repo_model.Repository) bool {
+	// If user is an organization, it can only pin its own repos
+	if u.IsOrganization() {
+		return r.OwnerID == u.ID


the user here is doer not the owner of the repo, also in CanPin, it is better to rename u to doer
so u.IsOrganization() will always be false here.

I'm not sure that I'm understanding what you are saying here. Since there are two pin actions (pin to the doer's profile and pin to the owner org's profile), the user passed can be an organization. Also, this function is called in CleanupPins to check if any repo pinned to the org's profile has been moved to another org.

doer is the logined user, and organization user is also defined as user in source codes, but we can not login as an organization user. So doer is always an individual user and u.IsOrganization() will always be false here.

Yes, the thing is that this function is not only called with a Doer. It's just a function that checks for the base requirements that a user needs to fulfill to have the repository pinned to their profile. When a user tries to pin a repository to their own profile, the user in this function will be the Doer. But that user can also pin the repo to an organization (meaning that the user in this function will be the org, since that is the target profile for the pin). This function is also used later to check if the user has lost visibility to the repo, or (in this case for organizations) if the repo has been transferred to another org.

But that user can also pin the repo to an organization (meaning that the user in this function will be the org, since that is the target profile for the pin).

This is not a permission check.
The permission check should be
(1) whether doer (the user tries to pin the repo to an org) is the owner/admin of the org (2) which should be the repo's owner.
You only checked the second one, so if there's a public org and a public repo (belongs to this org), the user who is not the member of this org, can access this repo's, and can pin this repo to the org, which should not happen.

Oh, maybe that is a naming issue. This function was intended only to check if a given user has perms to have a given repository pinned to their profile. The check that the Doer has perms on the org to pin in its profile is done in assertUserOrgPerms which is only called if the action done is pinning/unpinning on the org profile. Maybe this code can be refactored or at least the functions can be renamed.

…d of TxContext

lunny · 2024-05-21T14:04:13Z

Thank you for your contribution. I will do a review after 1.22.0 is released.

yp05327 · 2024-05-22T00:52:31Z

Can you also upload a screenshot of organization's home(profile) page?
The design in #10375 (comment) seems not good enough.
I think the block in the below should be moved to the top of this page, as pined repos should be a part of repositories tab which is similar to individual user's profile page.

carlosfelgueiras · 2024-05-26T14:54:28Z

Can you also upload a screenshot of organization's home(profile) page? The design in #10375 (comment) seems not good enough. I think the block in the below should be moved to the top of this page, as pined repos should be a part of repositories tab which is similar to individual user's profile page.

Yes, we also thought about that and changed it so the pinned repo cards are inside the repositories tab.

JakobDev · 2024-05-27T09:36:49Z

Is there a way to customize the order?

models/repo/pin.go

lunny · 2024-05-27T12:46:48Z

routers/web/repo/view.go

+
+func loadPinData(ctx *context.Context) error {
+	// First, cleanup any pins that are no longer valid
+	err := user_service.CleanupPins(ctx, ctx.Doer)


Why cleanup pins when user home page be visited? Looks wired.

We are cleaning the pins to avoid an edge case.
If we have a user1, with 6 pins, being one of those pins a repo belonging to user2. If user 2 changes the repo visibility to private, user1 can no longer have it as a pin. If user1 now visited a new repo page, without visiting his own homepage, the pin button would be disabled because he already has the maximum number of pins. However, if we clean the pins, the pin to the repo of user2 will be deleted, and the button will be enabled.

When changing the visibility and user has no permission to access this pin, means user2 forcibly unpinned it, so this should be handled when user2 changes the repo visibility, not the moment user visiting the repo home page.
Also for other cases, like remove the user as the member of an organization and so on.

lunny · 2024-05-27T12:49:29Z

services/user/pin.go

+// Checks if the user has permission to have the repo pinned in it's profile.
+func HasPermsToPin(ctx *context.Context, u *user_model.User, r *repo_model.Repository) bool {
+	// If user is an organization, it can only pin its own repos
+	if u.IsOrganization() {


I think u should be renamed doer. This should check whether the doer can pin the repository into doer's profile? Or we need another parameter which is the pinning target user/org.

I think the best approach would be to add a parameter that is the target and move all checks for permissions to this function.

DanielMatiasCarvalho · 2024-05-27T13:14:06Z

@JakobDev
No, we currently cannot change the order. We thought it would be better if there were a basic implementation of the pin feature, which is the focus of this PR. Later, more capabilities, like ordering, could be added to it.

JakobDev · 2024-05-27T13:16:54Z

I think ordering is a basic feature that belongs to this PR before merging

lunny · 2024-05-27T13:59:19Z

We also need to handle user/org deletions.

… Exist

carlosfelgueiras · 2024-05-28T10:36:58Z

We also need to handle user/org deletions.

We forgot about that. Now it's done.

carlosfelgueiras and others added 4 commits May 10, 2024 17:35

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 13, 2024

pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 13, 2024

github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels May 13, 2024

fix(lint): fixed and added copyright and license comment on top of files

f8b1756

lunny reviewed May 13, 2024

View reviewed changes

models/repo/pin.go Outdated Show resolved Hide resolved

lunny reviewed May 13, 2024

View reviewed changes

routers/web/repo/view.go Outdated Show resolved Hide resolved

lunny added this to the 1.23.0 milestone May 13, 2024

fix(table name): Changed repository pins database name

5fba27c

silverwind reviewed May 13, 2024

View reviewed changes

web_src/svg/octicon-custom-pin-off.svg Outdated Show resolved Hide resolved

silverwind reviewed May 13, 2024

View reviewed changes

templates/repo/pin_unpin.tmpl Outdated Show resolved Hide resolved

KN4CK3R changed the title ~~Pin repos on profile (Fixes #10375)~~ Pin repos on profile May 13, 2024

Update templates/repo/pin_unpin.tmpl

0887277

Co-authored-by: silverwind <me@silverwind.io>

silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 15, 2024

carlosfelgueiras added 2 commits May 15, 2024 12:47

fix(pin): removed octicon-custom-pin-off and replaced the places that…

c9d2f49

… used it with the already present octicon-pin-slash

feat(pin): made the gap between pin card half the previous size

dae3c8a

yp05327 reviewed May 16, 2024

View reviewed changes

models/repo/pin.go Outdated Show resolved Hide resolved

yp05327 reviewed May 16, 2024

View reviewed changes

routers/web/repo/view.go Show resolved Hide resolved

yp05327 reviewed May 16, 2024

View reviewed changes

DanielMatiasCarvalho added 2 commits May 17, 2024 15:38

fix(user pin service): converted max number of pins to a constant

2140062

fix(user pin service): fixed CanPin function to return an error

b941b04

DanielMatiasCarvalho added 3 commits May 17, 2024 16:14

fix(pin db): added a function to get the count of pinned repos

a019e44

fix(view): Added a missing return function

3704d23

fix(transaction): changed the use of the transaction to WithTx instea…

805dbc7

…d of TxContext

carlosfelgueiras requested review from lunny, silverwind and yp05327 May 21, 2024 12:54

lunny reviewed May 27, 2024

View reviewed changes

models/repo/pin.go Outdated Show resolved Hide resolved

lunny reviewed May 27, 2024

View reviewed changes

carlosfelgueiras added 2 commits May 28, 2024 10:23

refactor(pin): changed method used to check if pin exists from Get to…

859b1ce

… Exist

fix(pin): pins not being deleted when user gets deleted

764b08b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin repos on profile #30961

Pin repos on profile #30961

carlosfelgueiras commented May 13, 2024 •

edited by KN4CK3R

silverwind May 13, 2024 •

edited

carlosfelgueiras May 15, 2024

silverwind commented May 13, 2024

carlosfelgueiras commented May 15, 2024

carlosfelgueiras commented May 15, 2024

yp05327 May 16, 2024

carlosfelgueiras May 17, 2024

yp05327 May 22, 2024

carlosfelgueiras May 26, 2024

yp05327 May 16, 2024

carlosfelgueiras May 17, 2024

yp05327 May 22, 2024 •

edited

lunny May 27, 2024

yp05327 May 16, 2024

carlosfelgueiras May 17, 2024

yp05327 May 17, 2024

carlosfelgueiras May 17, 2024

yp05327 May 22, 2024

carlosfelgueiras May 26, 2024

lunny commented May 21, 2024

yp05327 commented May 22, 2024 •

edited

carlosfelgueiras commented May 26, 2024

JakobDev commented May 27, 2024

lunny May 27, 2024

DanielMatiasCarvalho May 27, 2024

yp05327 May 28, 2024

lunny May 27, 2024

carlosfelgueiras May 28, 2024

DanielMatiasCarvalho commented May 27, 2024

JakobDev commented May 27, 2024

lunny commented May 27, 2024

carlosfelgueiras commented May 28, 2024

Pin repos on profile #30961

Are you sure you want to change the base?

Pin repos on profile #30961

Conversation

carlosfelgueiras commented May 13, 2024 • edited by KN4CK3R

silverwind May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented May 13, 2024

carlosfelgueiras commented May 15, 2024

carlosfelgueiras commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yp05327 May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented May 21, 2024

yp05327 commented May 22, 2024 • edited

carlosfelgueiras commented May 26, 2024

JakobDev commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMatiasCarvalho commented May 27, 2024

JakobDev commented May 27, 2024

lunny commented May 27, 2024

carlosfelgueiras commented May 28, 2024

carlosfelgueiras commented May 13, 2024 •

edited by KN4CK3R

silverwind May 13, 2024 •

edited

yp05327 May 22, 2024 •

edited

yp05327 commented May 22, 2024 •

edited