- Notifications
You must be signed in to change notification settings - Fork926
feat(coderd): allow workspace owners to mark workspaces as favorite#11791
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
| 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.
review: fixes pre-existing ordering issue in dbmem that diverges from postgres behaviour
coderd/workspaces.go Outdated
| httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ | ||
| Message: "You can only favorite workspaces that you own", | ||
| }) |
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.
review: open to changing this response code if forbidden is not what we want here
coderd/workspaces.go Outdated
| // 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 |
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.
review: this is part of how we ensure that 'favorite' workspaces only come up first for their respective owners.
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 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?
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.
How about justRequesterID?
Uh oh!
There was an error while loading.Please reload this page.
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.
Minor comments or questions
| r.Put("/favorite", api.putFavoriteWorkspace) | ||
| r.Delete("/favorite", api.deleteFavoriteWorkspace) |
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.
nit: as we modify a single record inworkspaces, I would consider single PATCH operation. This form is also correct but longer.
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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{} | |||
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.
What do you think about adding a precondition here to fail fast if uuid.UUID is empty/null/zero?
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.
What would be the purpose of this?
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.
To catch misuse ofconvertWorkspaces where it is called without user context.
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.
Will address in a follow-up PR.
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 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.
| @@ -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) | |||
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.
Was this added by code gen? I thought we only add it for whenarg 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.
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 believe you're correct, it's auto-added by codegen.
coderd/database/dbmem/dbmem.go Outdated
| @@ -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 { | |||
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.
If both are favorite, this might not produce the desired 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.
Yeah that's possible, but as it's justdbmem I'm going to defer caring about it until it makes a test fail ;-)
coderd/workspaces.go Outdated
| // 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 |
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 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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Part of#7912
Continues from#11673 and rebased on latest main
Notable changes:
favoritenow a boolean