feat(coderd): allow workspace owners to mark workspaces as favorite #11791

johnstcn · 2024-01-24T09:37:43Z

Part of #7912

Continues from #11673 and rebased on latest main

Notable changes:

favorite now a boolean
Workspaces can only be marked as 'favorite' by their owner. This can be migrated to its own table later if we need to allow favoriting other users' workspaces, but I suspect the rule of YAGNI may apply here. Migrating this data to a separate table in future should be a simple and reversible operation.

johnstcn · 2024-01-24T09:38:27Z

coderd/database/dbmem/dbmem.go

+		if strings.Compare(preloadedUsers[w1.ID].Username, preloadedUsers[w2.ID].Username) < 0 {
+			return true
 		}

 		// Order by: workspace names
-		return sort.StringsAreSorted([]string{w1.Name, w2.Name})
+		return strings.Compare(w1.Name, w2.Name) < 0


review: fixes pre-existing ordering issue in dbmem that diverges from postgres behaviour

johnstcn · 2024-01-24T09:40:23Z

coderd/workspaces.go

+		httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
+			Message: "You can only favorite workspaces that you own",
+		})


review: open to changing this response code if forbidden is not what we want here

johnstcn · 2024-01-24T09:41:28Z

coderd/workspaces.go

+	// To show the user's favorite workspaces first, we pass their userID and compare it to
+	// column favorite_of when ordering the rows.
+	filter.OrderByFavorite = apiKey.UserID


review: this is part of how we ensure that 'favorite' workspaces only come up first for their respective owners.

I think the naming is a bit confusing when read in query/code. I.e. "order by favorite" being the users ID.

How about something like (Prioritize|Order)FavoritesForUser?

How about just RequesterID?

coderd/database/queries/workspaces.sql

mtojek

Minor comments or questions

mtojek · 2024-01-24T11:23:19Z

coderd/coderd.go

+				r.Put("/favorite", api.putFavoriteWorkspace)
+				r.Delete("/favorite", api.deleteFavoriteWorkspace)


nit: as we modify a single record in workspaces, I would consider single PATCH operation. This form is also correct but longer.

What you say is correct. One can also make an argument for the simplicity of a handler that does one thing only instead of a complex handler that maybe does many things. I don't feel particularly strongly either way; the separate handler approach seems to be more in line with existing conventions.

coderd/database/queries/workspaces.sql

coderd/workspaces.go

mtojek · 2024-01-24T11:28:00Z

coderd/workspaces.go

@@ -1366,7 +1471,7 @@ func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspa
 	}, nil
 }

-func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]codersdk.Workspace, error) {
+func convertWorkspaces(requesterID uuid.UUID, workspaces []database.Workspace, data workspaceData) ([]codersdk.Workspace, error) {
 	buildByWorkspaceID := map[uuid.UUID]codersdk.WorkspaceBuild{}


What do you think about adding a precondition here to fail fast if uuid.UUID is empty/null/zero?

What would be the purpose of this?

To catch misuse of convertWorkspaces where it is called without user context.

Will address in a follow-up PR.

mafredri

I prefer this approach to the previous one, nice! A few small comments, and I wonder if we really need an own endpoint for the favorite, vs patching the workspace 🤔. Not a deal-breaker by any means.

mafredri · 2024-01-24T11:19:43Z

coderd/database/dbmem/dbmem.go

@@ -1315,6 +1316,25 @@ func (*FakeQuerier) DeleteTailnetTunnel(_ context.Context, arg database.DeleteTa
 	return database.DeleteTailnetTunnelRow{}, ErrUnimplemented
 }

+func (q *FakeQuerier) FavoriteWorkspace(_ context.Context, arg uuid.UUID) error {
+	err := validateDatabaseType(arg)


Was this added by code gen? I thought we only add it for when arg is a struct (i.e. database.*Params). If it works, I don't see any harm though, it's more complete with the check in-place.

I believe you're correct, it's auto-added by codegen.

mafredri · 2024-01-24T11:22:31Z

coderd/database/dbmem/dbmem.go

@@ -7713,7 +7753,15 @@ func (q *FakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 		w1 := workspaces[i]
 		w2 := workspaces[j]

-		// Order by: running first
+		// Order by: favorite first
+		if arg.OrderByFavorite == w1.OwnerID && w1.Favorite {


If both are favorite, this might not produce the desired order. 🤔

Yeah that's possible, but as it's just dbmem I'm going to defer caring about it until it makes a test fail ;-)

mafredri · 2024-01-24T11:26:45Z

coderd/workspaces.go

+	// To show the user's favorite workspaces first, we pass their userID and compare it to
+	// column favorite_of when ordering the rows.
+	filter.OrderByFavorite = apiKey.UserID


I think the naming is a bit confusing when read in query/code. I.e. "order by favorite" being the users ID.

How about something like (Prioritize|Order)FavoritesForUser?

coderd/workspaces.go

feat(coderd): allow workspace owners to mark workspaces as favorite

2404284

github-actions bot assigned johnstcn Jan 24, 2024

johnstcn commented Jan 24, 2024

View reviewed changes

coderd/database/queries/workspaces.sql Outdated Show resolved Hide resolved

johnstcn marked this pull request as ready for review January 24, 2024 09:43

johnstcn requested review from mafredri and mtojek January 24, 2024 09:46

johnstcn mentioned this pull request Jan 24, 2024

feat(cli): add favorite/unfavorite commands #11793

Merged

johnstcn requested a review from Emyrk January 24, 2024 10:58

mtojek reviewed Jan 24, 2024

View reviewed changes

mafredri reviewed Jan 24, 2024

View reviewed changes

address comments/nits

868fc52

mtojek approved these changes Jan 24, 2024

View reviewed changes

johnstcn merged commit f92336c into main Jan 24, 2024

johnstcn deleted the cj/favorite-workspaces-redux branch January 24, 2024 13:39

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coderd): allow workspace owners to mark workspaces as favorite #11791

feat(coderd): allow workspace owners to mark workspaces as favorite #11791

johnstcn commented Jan 24, 2024 •

edited

Loading

johnstcn Jan 24, 2024

johnstcn Jan 24, 2024

johnstcn Jan 24, 2024

mafredri Jan 24, 2024

johnstcn Jan 24, 2024

mtojek left a comment

mtojek Jan 24, 2024

johnstcn Jan 24, 2024

mtojek Jan 24, 2024

johnstcn Jan 24, 2024

mtojek Jan 24, 2024 •

edited

Loading

johnstcn Jan 24, 2024

mafredri left a comment

mafredri Jan 24, 2024

johnstcn Jan 24, 2024

mafredri Jan 24, 2024

johnstcn Jan 24, 2024

mafredri Jan 24, 2024

		r.Put("/favorite", api.putFavoriteWorkspace)
		r.Delete("/favorite", api.deleteFavoriteWorkspace)

feat(coderd): allow workspace owners to mark workspaces as favorite #11791

feat(coderd): allow workspace owners to mark workspaces as favorite #11791

Conversation

johnstcn commented Jan 24, 2024 • edited Loading

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

mtojek left a 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

mtojek Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri left a 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

johnstcn commented Jan 24, 2024 •

edited

Loading

mtojek Jan 24, 2024 •

edited

Loading