Add collaborative repositories to the dashboard #2205

Bwko · 2017-07-23T19:53:26Z

Remove some unused code from the Dashboard func
Fixes #2114

andreynering · 2017-07-23T21:33:21Z

LGTM

lafriks · 2017-07-23T22:11:02Z

LGTM

lafriks · 2017-07-23T22:53:17Z

One more thing - integration test for this would be great

ethantkoenig · 2017-07-23T22:58:32Z

models/repo_list.go

@@ -153,6 +154,10 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 		}

 		cond = cond.Or(builder.And(builder.Like{"lower_name", opts.Keyword}, builder.In("owner_id", ownerIds)))
+
+		if opts.Collaborate {
+			cond = cond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`, opts.Searcher.ID, opts.Searcher.ID))


This will match all collaborative repos, even ones that do not match the other options (e.g. opts.Private, opts.Keyword, ...)

ethantkoenig · 2017-07-23T23:03:14Z

routers/user/home.go

@@ -54,7 +54,7 @@ func getDashboardContextUser(ctx *context.Context) *models.User {
 }

 // retrieveFeeds loads feeds for the specified user
-func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isProfile bool, includeDeletedComments bool) {
+func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isProfile, includeCollaborate bool, includeDeletedComments bool) {


It might be time to add an options type for this function. If I see something like retrieveFeeds(ctx, ctxUser, true, false, true, false), I'll have no idea what it means without further investigating.

We could also reuse the GetFeedsOptions type instead of introducing a new options type, since most of the current argument map directly to GetFeedsOptions fields.

lafriks · 2017-07-26T06:01:58Z

Make LG-TM work

lunny · 2017-07-26T07:09:29Z

models/action.go

@@ -746,5 +747,8 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {

 	}

+	if opts.Collaborate {


This will drop other conditions like opts.IncludeDeleted. I think this method should refactor via builder otherwise it's still not correct.

lunny · 2017-07-26T07:10:10Z

models/repo_list.go

@@ -153,6 +154,10 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 		}

 		cond = cond.Or(builder.And(builder.Like{"lower_name", opts.Keyword}, builder.In("owner_id", ownerIds)))
+
+		if opts.Collaborate {
+			cond = cond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`, opts.Searcher.ID, opts.Searcher.ID))


Bwko · 2017-07-28T22:22:41Z

Fixed

lunny · 2017-07-29T04:50:59Z

models/action.go


 	}

-	return actions, sess.Find(&actions)
+	if opts.Collaborate {


I mean maybe we should move these 3 lines to line 737 ?

if opts.Collaborate { cond = builder.Eq{"user_id": opts.RequestedUser.ID}.Or( builder.Expr(`repo_id IN (SELECT repo_id FROM "access" WHERE access.user_id = ?)`, opts.RequestedUser.ID) ) } else { cond = builder.Eq{"user_id": opts.RequestedUser.ID} }

lunny · 2017-08-07T05:42:26Z

@Bwko do you have time to fix this PR?

Bwko · 2017-08-09T20:35:53Z

@lunny Done

lunny · 2017-08-10T00:28:06Z

models/action.go

@@ -728,23 +730,31 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
 	}

 	actions := make([]*Action, 0, 20)
+	cond := builder.NewCond()
 	sess := x.Limit(20).


Put this line to the end of the function is perfect.

ethantkoenig · 2017-08-10T17:47:03Z

models/repo_list.go

@@ -153,6 +154,11 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 		}

 		cond = cond.Or(builder.And(builder.Like{"lower_name", opts.Keyword}, builder.In("owner_id", ownerIds)))
+
+		if opts.Collaborate {
+			cond = cond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`, opts.Searcher.ID, opts.Searcher.ID),


This is still not correct. This will match repos that do not match the opts.Starred or opts.OwnerID options.

Apologies if I wasn't clear in my previous comment. Instead of relisting all the other conditions (which is fragile if we add more options), I think we should do something like

// compute ownerIds, existing code up to line 154 is fine searcherReposCond := builder.In("owner_id", ownerIds) if opts.Collaborate { searcherReposCond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`, opts.Searcher.ID, opts.Searcher.ID) } cond.And(searcherReposCond)

That's what I want to say.

But some bug

searcherReposCond := builder.In("owner_id", ownerIds) if opts.Collaborate { searcherReposCond = searcherReposCond.Or(builder.Expr(`id IN (SELECT repo_id FROM "access" WHERE access.user_id = ? AND owner_id != ?)`, opts.Searcher.ID, opts.Searcher.ID) } cond = cond.And(searcherReposCond)

lafriks · 2017-08-16T19:18:11Z

@Bwko do you have time to fix this?

lunny · 2017-08-17T01:34:38Z

~~Since #2285 has refactored the dashboard and fix #2114. This PR will only affect others places. I will move this to v1.3~~

Remove some unused code from the Dashboard func

andreynering · 2017-08-22T15:54:25Z

Trusted LGTM

ethantkoenig

@lunny

ethantkoenig · 2017-08-23T02:34:03Z

models/action.go

+		cond = builder.Eq{"user_id": opts.RequestedUser.ID}.Or(
+			builder.Expr(`repo_id IN (SELECT repo_id FROM "access" WHERE access.user_id = ?)`, opts.RequestedUser.ID))
+	} else {
+		cond = builder.Eq{"user_id": opts.RequestedUser.ID}


I don't think this is right. The assignments on lines 737 and 740 will overwrite the assignment on line 733.

Morlinest · 2017-08-23T11:47:08Z

@lunny @lafriks @andreynering @Bwko Hi, don't know if I am doing something wrong, but it looks like this PR actually removed all collaborative repositories instead of adding any (nice paradox :D) and doubled some feeds. Did anyone test it?

andreynering added modifies/api This PR adds API routes or modifies them type/bug topic/ui Change the appearance of the Gitea UI labels Jul 23, 2017

lafriks added this to the 1.2.0 milestone Jul 23, 2017

lafriks added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jul 23, 2017

ethantkoenig suggested changes Jul 23, 2017

View reviewed changes

lunny requested changes Jul 26, 2017

View reviewed changes

Bwko force-pushed the collaborate branch 3 times, most recently from 6de7ed7 to 47e8494 Compare July 28, 2017 20:13

lunny reviewed Jul 29, 2017

View reviewed changes

This was referenced Aug 9, 2017

Dashboard repo UI #2278

Closed

Fix and improve dashboard repo UI #2285

Merged

Bwko force-pushed the collaborate branch from 47e8494 to 9666f5f Compare August 9, 2017 20:31

lunny reviewed Aug 10, 2017

View reviewed changes

ethantkoenig suggested changes Aug 10, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.2.0 Aug 17, 2017

lafriks modified the milestones: 1.2.0, 1.3.0 Aug 17, 2017

Add collaborative repositories to the dashboard

d34f97a

Remove some unused code from the Dashboard func

fix some bug and some refactor

bdac5e0

lunny force-pushed the collaborate branch from e407c70 to bdac5e0 Compare August 22, 2017 14:21

fix tests

1b71b48

lunny approved these changes Aug 22, 2017

View reviewed changes

lunny merged commit 1a5fe43 into go-gitea:master Aug 23, 2017

ethantkoenig reviewed Aug 23, 2017

View reviewed changes

ethantkoenig mentioned this pull request Aug 23, 2017

Fix SQL condition bug in GetFeeds(..) #2360

Merged

This was referenced Aug 23, 2017

All collaborative repositories disappeared from dashboard repo panel #2365

Closed

Feeds from organization actions are doubled #2366

Closed

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collaborative repositories to the dashboard #2205

Add collaborative repositories to the dashboard #2205

Bwko commented Jul 23, 2017

andreynering commented Jul 23, 2017

lafriks commented Jul 23, 2017

lafriks commented Jul 23, 2017

ethantkoenig Jul 23, 2017

lunny Jul 26, 2017

ethantkoenig Jul 23, 2017

lafriks commented Jul 26, 2017

lunny Jul 26, 2017

lunny Jul 26, 2017

Bwko commented Jul 28, 2017

lunny Jul 29, 2017 •

edited

Loading

lunny commented Aug 7, 2017

Bwko commented Aug 9, 2017

lunny Aug 10, 2017

ethantkoenig Aug 10, 2017 •

edited

Loading

lunny Aug 10, 2017

lunny Aug 10, 2017

lafriks commented Aug 16, 2017

lunny commented Aug 17, 2017 •

edited

Loading

andreynering commented Aug 22, 2017

ethantkoenig left a comment

ethantkoenig Aug 23, 2017 •

edited

Loading

lunny Aug 23, 2017

Morlinest commented Aug 23, 2017

		@@ -746,5 +747,8 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {

		}

		if opts.Collaborate {

Add collaborative repositories to the dashboard #2205

Add collaborative repositories to the dashboard #2205

Conversation

Bwko commented Jul 23, 2017

andreynering commented Jul 23, 2017

lafriks commented Jul 23, 2017

lafriks commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bwko commented Jul 28, 2017

lunny Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

lunny commented Aug 7, 2017

Bwko commented Aug 9, 2017

Choose a reason for hiding this comment

ethantkoenig Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Aug 16, 2017

lunny commented Aug 17, 2017 • edited Loading

andreynering commented Aug 22, 2017

ethantkoenig left a comment

Choose a reason for hiding this comment

ethantkoenig Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest commented Aug 23, 2017

lunny Jul 29, 2017 •

edited

Loading

ethantkoenig Aug 10, 2017 •

edited

Loading

lunny commented Aug 17, 2017 •

edited

Loading

ethantkoenig Aug 23, 2017 •

edited

Loading