feat(coderd): add ability to mark workspaces as favorite #11673

johnstcn · 2024-01-17T17:15:03Z

Part of #7912

Update DB schema + queries (this PR)
Add API endpoints to favorite/unfavorite workspace (this PR)
Show favorite status in UI/CLI (follow-up)
Add CLI commands to favorite/unfavorite workspace (follow-up)
Add UI support to favorite/unfavorite workspace (follow-up)

Goal:

When you query workspaces in the UI, your favorite workspaces should show up first (that is, favorite workspaces of other users should be sorted as they would if they were not favorited at all).

Approach:

The 'obvious' approach is to add a boolean column favorite to the workspaces table. This will work for one user, but then all users' favorites go to the top when filtering by multiple users. I decided to avoid this approach unless absolutely required.

Instead, I first tried adding a separate table user_favorites in which we store (user_id, workspace_id) tuples and adding some joins to return the favorite status by user. This proved unwieldy for GetWorkspaceByID as that changes the return signature to a GetWorkspaceByIDRow and breaks everything.

I then changed tactics and instead added a nullable UUID column favorite_of. This stores the UUID of the workspace owner when marked as 'favorite', and is NULL otherwise. We can then pass in the UUID of the caller in order to control the ordering of 'favorites'.

We convert the Favorite status to a boolean value when converting between the respective database and codersdk structs.

I originally considered co-opting the owner parameter for this ordering but instead elected to add a separate parameter to avoid surprises (e.g. querying for owner:larry as curly would then show Larry's favorite workspaces first).

This means that:

You must be able to write a workspsace in order to mark it as favorite.
Marking someone else's workspace as favorite will still make it show up as their favorite, and not yours.

…g order

johnstcn · 2024-01-23T11:02:18Z

coderd/database/queries/workspaces.sql

+	CASE WHEN workspaces.favorite_of = @order_by_favorite THEN
+		workspaces.favorite_of = @order_by_favorite
+	END ASC,


self-review: this new parameter order_by_favorite is passed in and contains the requestor's user ID. I tried multiple permutations without the CASE statement but all of them did not result in the desired sort order.

Interesting that you do not need an ELSE here.

I think we can drop the body of the case, and just put true. Just to remove a bit of duplication.

Suggested change

CASE WHEN workspaces.favorite_of = @order_by_favorite THEN

workspaces.favorite_of = @order_by_favorite

END ASC,

CASE WHEN workspaces.favorite_of = @order_by_favorite THEN

true

END ASC,

johnstcn · 2024-01-23T11:58:45Z

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


Fixed a pre-existing sort ordering bug in dbmem.

johnstcn · 2024-01-23T11:59:11Z

coderd/workspaces_test.go

@@ -479,55 +479,85 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
 func TestWorkspacesSortOrder(t *testing.T) {


self-review: this test has been significantly beefed up

johnstcn · 2024-01-23T11:59:48Z

coderd/database/dbauthz/dbauthz_test.go

+	s.Run("FavoriteWorkspace", s.Subtest(func(db database.Store, check *expects) {
+		u := dbgen.User(s.T(), db, database.User{})
+		ws := dbgen.Workspace(s.T(), db, database.Workspace{OwnerID: u.ID})
+		check.Args(ws.ID).Asserts(ws, rbac.ActionUpdate).Returns()
+	}))
+	s.Run("UnfavoriteWorkspace", s.Subtest(func(db database.Store, check *expects) {
+		u := dbgen.User(s.T(), db, database.User{})
+		ws := dbgen.Workspace(s.T(), db, database.Workspace{OwnerID: u.ID})
+		check.Args(ws.ID).Asserts(ws, rbac.ActionUpdate).Returns()
+	}))


self-review: you need to be able to update a workspace in order to favorite it.

johnstcn · 2024-01-23T12:00:47Z

coderd/database/migrations/000186_user_favorite_workspaces.up.sql

+ALTER TABLE ONLY workspaces
+ADD COLUMN favorite_of uuid DEFAULT NULL;
+COMMENT ON COLUMN workspaces.favorite_of IS 'FavoriteOf contains the UUID of the workspace owner if the workspace has been favorited.';


self-review: just storing a boolean value here doesn't provide enough context to the query to be able to know how to sort favorites.

Do you think there is any benefit to using a zero uuid here instead of null? It just makes you not need to use uuid.NullUUID{...}

Doesn't a workspace have the owner uuid already in the table? If so can't we just use a boolean + that? When I hear "favorite of" I kind of expect the data type to be an array and favorable by many.

If we don't have owner uuid in there, should we just add it vs. the current workaround?

I was originally wary of making that conditional on owner_id, but now it's looking like it makes more sense in its current shape.

mtojek

I then changed tactics and instead added a nullable UUID column favorite_of. This stores the UUID of the workspace owner when marked as 'favorite', and is NULL otherwise. We can then pass in the UUID of the caller in order to control the ordering of 'favorites'.

A comment on the design. Let's assume that Alice has workspace, and Bob is admin, but would like to mark Alice's workspace as favorite. Is it doable with this design?

johnstcn · 2024-01-23T12:51:24Z

I then changed tactics and instead added a nullable UUID column favorite_of. This stores the UUID of the workspace owner when marked as 'favorite', and is NULL otherwise. We can then pass in the UUID of the caller in order to control the ordering of 'favorites'.

A comment on the design. Let's assume that Alice has workspace, and Bob is admin, but would like to mark Alice's workspace as favorite. Is it doable with this design?

Bob can mark Alice's workspace as Alice's favorite on her behalf. Favoriting a workspace that you do not own isn't supported here, but is doable with some minor modifications:

favorite_of becomes a uuid[]
Add user_id to parameters of FavoriteWorkspace / UnfavoriteWorkspace

Is this something you think is worthwhile?

mtojek

Is this something you think is worthwhile?

I would say so, unless PMs have different preferences. Alice might be scared that somebody "hacked" her account and started interacting with "favorites" markers.

favorite_of becomes a uuid[]

I admit that I prefer to have a separate "favorites" table than expanding the workspaces table schema, but I'm not a database expert. I wouldn't like to mix concerns as there is always the risk of growing up the workspace table. You may want to consult our database guy @mafredri :)

johnstcn · 2024-01-23T14:03:16Z

Is this something you think is worthwhile?

I would say so, unless PMs have different preferences. Alice might be scared that somebody "hacked" her account and started interacting with "favorites" markers.

To be fair, the same can be said of any action an admin takes on a user's resources.

johnstcn · 2024-01-23T14:06:12Z

I admit that I prefer to have a separate "favorites" table than expanding the workspaces table schema, but I'm not a database expert. I wouldn't like to mix concerns as there is always the risk of growing up the workspace table.

Yeah, this was my first attempt as well. Suffice it to say that adding any sort of join to the GetWorkspaceByID query will break many, many things and requires some annoying workarounds. Migrating this to a separate table down the line is definitely possible if required.

mtojek · 2024-01-23T14:19:47Z

query will break many, many things and requires some annoying workarounds.

... unless you identify API calls that truly require "favorites" data. Maybe it is just the GetWorkspaces API call, so we can replace the database call with GetWorkspacesWithShares. Array field favorites_of []uuid could be an option too if we acknowledge risks above.

Anyway, I understand the complexity, and I don't intend to stop you. I'd like to save us from reimplementing this feature in the future if we make wrong product assumptions.

Emyrk · 2024-01-23T15:19:48Z

@mtojek Joining tables is more a limitation of our SQLc design. When you do any joins, the data structs become a mess.

So some of our database design, and likely this, is because of that. I think in a pure academic database setting, this could be normalized to another table. But the headache that gives up the stack makes a []uuid column simpler imo.

And table normalization would only really show value if we then use that table for some other queries somehow? Like "Show me the number of favorited workpaces for each template". Then the []uuid column starts to fall short.

We can always normalize later too, that migration doesn't feel too hard.

mtojek · 2024-01-23T15:31:15Z

Like "Show me the number of favorited workpaces for each template". Then the []uuid column starts to fall short.

I'm afraid that devs could perceive it as an invitation to add more joined arrays, and build the single table model.

One more question, I might have missed it in the review, but will it be possible to expose the "favorite" information only to the requester? It would be useless to leak the information about who really "likes" the workspace.

johnstcn · 2024-01-23T15:44:13Z

Like "Show me the number of favorited workpaces for each template". Then the []uuid column starts to fall short.

I'm afraid that devs could perceive it as an invitation to add more joined arrays, and build the single table model.

One more question, I might have missed it in the review, but will it be possible to expose the "favorite" information only to the requester? It would be useless to leak the information about who really "likes" the workspace.

I think this is covered by convertWorkspace taking the user ID of the requestor and only showing it as 'favorite' if the database struct favorite_of field matches the requestor's ID.

mtojek · 2024-01-23T15:45:54Z

Ok, thanks for clarifying my doubts. Feel free to pick the implementation you prefer.

Emyrk

Left some nits, I think we should drop allowing admins to favorite other workspaces for now.

My nits are not blocking.

Emyrk · 2024-01-23T15:43:17Z

coderd/database/queries/workspaces.sql

+	CASE WHEN workspaces.favorite_of = @order_by_favorite THEN
+		workspaces.favorite_of = @order_by_favorite
+	END ASC,


Interesting that you do not need an ELSE here.

I think we can drop the body of the case, and just put true. Just to remove a bit of duplication.

Suggested change

CASE WHEN workspaces.favorite_of = @order_by_favorite THEN

workspaces.favorite_of = @order_by_favorite

END ASC,

CASE WHEN workspaces.favorite_of = @order_by_favorite THEN

true

END ASC,

Emyrk · 2024-01-23T15:46:08Z

coderd/workspaces.go

+	err := api.Database.FavoriteWorkspace(ctx, workspace.ID)
+	if err != nil {
+		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error setting workspace as favorite",
+			Detail:  err.Error(),
+		})
+		return
+	}


It feels weird not to verify that the caller is the person. I understand this is so an admin can favorite a workspace for someone else, but still feels odd 🤔.

Should we just not allow anyone to favorite someone else's workspace for now? If a feature request is made, we can add it back.

I'm happy to do that, it does smell like RBAC creep though.

We do a lot of this outside rbac already unfortunately 😢. This stuff doesn't fit in rbac atm.

Emyrk · 2024-01-23T16:15:11Z

Some notes for posterity:

Sorting by booleans is weird:

SELECT * FROM (VALUES(true), (false), (null)) s(a) ORDER BY a DESC;

| a     |
| ----- |
| null  |
| true  |
| false |

SELECT * FROM (VALUES(true), (false), (null)) s(a) ORDER BY a ASC;

| a     |
| ----- |
| false |
| true  |
| null  |

There is ORDER BY x NULLS FIRST/LAST, but it is better to just remove the possibility of nulls.

Emyrk · 2024-01-23T16:32:01Z

coderd/database/queries/workspaces.sql

+	-- COALESCE because the result of the comparison is NULL if not true
+	-- and this messes up the ordering.
+	COALESCE(workspaces.favorite_of = @order_by_favorite, false) DESC,


johnstcn · 2024-01-23T17:55:48Z

This has gotten a bit big and unwieldy so I'm going to close this out and open a new PR with the feedback from this one.

johnstcn self-assigned this Jan 17, 2024

johnstcn force-pushed the cj/pin-workspaces branch 4 times, most recently from f154058 to 321aca1 Compare January 22, 2024 08:53

johnstcn changed the title ~~wip: feat(coderd): add ability to pin workspaces~~ wip: feat(coderd): add ability to mark workspaces as favo(u?)rite Jan 22, 2024

johnstcn force-pushed the cj/pin-workspaces branch 2 times, most recently from 61da6b9 to 4469c84 Compare January 22, 2024 22:33

johnstcn changed the title ~~wip: feat(coderd): add ability to mark workspaces as favo(u?)rite~~ wip: feat(coderd): add ability to mark workspaces as favorite Jan 22, 2024

johnstcn added 21 commits January 23, 2024 09:38

feat(coderd): add user_pinned_workspaces table

46ca00d

add unique index

015aabb

rename migration

0b0dce6

add queries to pin/unpin workspace

9878da7

add pinned status to GetWorkspaces/GetAuthorizedWorkspaces queries

4fba238

add API endpoints and test (currently fails)

83b03d9

rename to favorite workspace

0961298

move to top-level test

a814b65

s/favored/favorite/g

89d618d

move favorite status to workspaces table to avoid sqlc join sadness

7238b95

more wiring

4eef59a

improve test to include sort order of favorites

0f8904d

fix swagger summary

d921aa0

make gen

b68c18e

use dbfake for testing sort order

3d0545a

reduce scope of TestWorkspaceFavoriteUnfavorite to not include sortin…

46103a4

…g order

beef up sort order test

f6c0361

rm unnecessary fixture

89f0f50

fix sort ordering, dbfake still TODO

c9ed43c

remove unnecessary change to CreateFirstUser

ff3cde2

best-effort fix but testing dbmem sort order is a waste of time

f735b69

johnstcn force-pushed the cj/pin-workspaces branch from 4469c84 to f735b69 Compare January 23, 2024 10:47

johnstcn changed the title ~~wip: feat(coderd): add ability to mark workspaces as favorite~~ feat(coderd): add ability to mark workspaces as favorite Jan 23, 2024

johnstcn added 4 commits January 23, 2024 10:57

requestor->requester

f6e9531

fixup! remove unnecessary change to CreateFirstUser

34f2902

remove unused resource

99c7f96

fix sort ordering bug in dmem

d530546

johnstcn commented Jan 23, 2024

View reviewed changes

johnstcn marked this pull request as ready for review January 23, 2024 12:01

johnstcn requested review from Emyrk and mtojek January 23, 2024 12:02

mtojek reviewed Jan 23, 2024

View reviewed changes

add test case

6392db9

mtojek reviewed Jan 23, 2024

View reviewed changes

Emyrk approved these changes Jan 23, 2024

View reviewed changes

remove need for conditional ordering

9979c53

Emyrk reviewed Jan 23, 2024

View reviewed changes

johnstcn closed this Jan 23, 2024

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

github-actions bot deleted the cj/pin-workspaces branch July 24, 2024 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coderd): add ability to mark workspaces as favorite #11673

feat(coderd): add ability to mark workspaces as favorite #11673

johnstcn commented Jan 17, 2024 •

edited by matifali

Loading

johnstcn Jan 23, 2024

Emyrk Jan 23, 2024

johnstcn Jan 23, 2024

johnstcn Jan 23, 2024

johnstcn Jan 23, 2024

johnstcn Jan 23, 2024

Emyrk Jan 23, 2024 •

edited

Loading

mafredri Jan 23, 2024

johnstcn Jan 23, 2024

mtojek left a comment

johnstcn commented Jan 23, 2024

mtojek left a comment

johnstcn commented Jan 23, 2024

johnstcn commented Jan 23, 2024

mtojek commented Jan 23, 2024

Emyrk commented Jan 23, 2024 •

edited

Loading

mtojek commented Jan 23, 2024

johnstcn commented Jan 23, 2024

mtojek commented Jan 23, 2024

Emyrk left a comment

Emyrk Jan 23, 2024

Emyrk Jan 23, 2024

johnstcn Jan 23, 2024

Emyrk Jan 23, 2024

Emyrk commented Jan 23, 2024

Emyrk Jan 23, 2024

johnstcn commented Jan 23, 2024

		@@ -479,55 +479,85 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
		func TestWorkspacesSortOrder(t *testing.T) {

feat(coderd): add ability to mark workspaces as favorite #11673

feat(coderd): add ability to mark workspaces as favorite #11673

Conversation

johnstcn commented Jan 17, 2024 • edited by matifali 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

Choose a reason for hiding this comment

Emyrk Jan 23, 2024 • edited Loading

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

johnstcn commented Jan 23, 2024

mtojek left a comment

Choose a reason for hiding this comment

johnstcn commented Jan 23, 2024

johnstcn commented Jan 23, 2024

mtojek commented Jan 23, 2024

Emyrk commented Jan 23, 2024 • edited Loading

mtojek commented Jan 23, 2024

johnstcn commented Jan 23, 2024

mtojek commented Jan 23, 2024

Emyrk 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

Emyrk commented Jan 23, 2024

Choose a reason for hiding this comment

johnstcn commented Jan 23, 2024

johnstcn commented Jan 17, 2024 •

edited by matifali

Loading

Emyrk Jan 23, 2024 •

edited

Loading

Emyrk commented Jan 23, 2024 •

edited

Loading