- Notifications
You must be signed in to change notification settings - Fork925
feat(coderd): add ability to mark workspaces as favorite#11673
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f154058 to321aca1Compare61da6b9 to4469c84Compare| CASE WHEN workspaces.favorite_of = @order_by_favorite THEN | ||
| workspaces.favorite_of = @order_by_favorite | ||
| END ASC, |
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.
self-review: this new parameterorder_by_favorite is passed in and contains the requestor's user ID. I tried multiple permutations without theCASE statement but all of them did not result in the desired sort order.
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.
Interesting that you do not need anELSE here.
I think we can drop the body of the case, and just puttrue. Just to remove a bit of duplication.
| CASE WHENworkspaces.favorite_of= @order_by_favorite THEN | |
| workspaces.favorite_of= @order_by_favorite | |
| ENDASC, | |
| CASE WHENworkspaces.favorite_of= @order_by_favorite THEN | |
| true | |
| ENDASC, |
| ifstrings.Compare(preloadedUsers[w1.ID].Username, preloadedUsers[w2.ID].Username) < 0 { | ||
| returntrue | ||
| } | ||
| // Order by: workspace names | ||
| returnsort.StringsAreSorted([]string{w1.Name, w2.Name}) | ||
| returnstrings.Compare(w1.Name, w2.Name) < 0 |
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.
Fixed a pre-existing sort ordering bug in dbmem.
| @@ -479,55 +479,85 @@ func TestAdminViewAllWorkspaces(t *testing.T) { | |||
| func TestWorkspacesSortOrder(t *testing.T) { | |||
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.
self-review: this test has been significantly beefed up
| 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() | ||
| })) |
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.
self-review: you need to be able to update a workspace in order to favorite it.
| 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.'; |
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.
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.
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.
Do you think there is any benefit to using a zero uuid here instead of null? It just makes you not need to useuuid.NullUUID{...}
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.
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?
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.
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.
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.
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:
Is this something you think is worthwhile? |
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.
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 :)
To be fair, the same can be said of any action an admin takes on a user's resources. |
Yeah, this was my first attempt as well. Suffice it to say that adding any sort of join to the |
... unless you identify API calls that truly require "favorites" data. Maybe it is just the 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 commentedJan 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 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 We can always normalize later too, that migration doesn't feel too hard. |
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 |
Ok, thanks for clarifying my doubts. Feel free to pick the implementation you prefer. |
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.
Left some nits, I think we should drop allowing admins to favorite other workspaces for now.
My nits are not blocking.
| CASE WHEN workspaces.favorite_of = @order_by_favorite THEN | ||
| workspaces.favorite_of = @order_by_favorite | ||
| END ASC, |
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.
Interesting that you do not need anELSE here.
I think we can drop the body of the case, and just puttrue. Just to remove a bit of duplication.
| CASE WHENworkspaces.favorite_of= @order_by_favorite THEN | |
| workspaces.favorite_of= @order_by_favorite | |
| ENDASC, | |
| CASE WHENworkspaces.favorite_of= @order_by_favorite THEN | |
| true | |
| ENDASC, |
| 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 | ||
| } |
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.
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.
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.
I'm happy to do that, it does smell like RBAC creep though.
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.
We do a lot of this outside rbac already unfortunately 😢. This stuff doesn't fit in rbac atm.
Some notes for posterity: Sorting by booleans is weird: There is |
| -- 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, |
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 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. |
Uh oh!
There was an error while loading.Please reload this page.
Part of#7912
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
favoriteto 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_favoritesin which we store(user_id, workspace_id)tuples and adding some joins to return the favorite status by user. This proved unwieldy forGetWorkspaceByIDas that changes the return signature to aGetWorkspaceByIDRowand breaks everything.I then changed tactics and instead added a nullable UUID column
favorite_of. This stores the UUID of the workspaceowner when marked as 'favorite', and isNULLotherwise. We can then pass in the UUID of the caller in order to control the ordering of 'favorites'.We convert the
Favoritestatus to a boolean value when converting between the respectivedatabaseandcodersdkstructs.I originally considered co-opting the
ownerparameter for this ordering but instead elected to add a separate parameter to avoid surprises (e.g. querying forowner:larryascurlywould then show Larry's favorite workspaces first).This means that: