feat: added include_deleted to getWorkspaceByOwnerAndName #2164
Conversation
relates to #1955
There was a problem hiding this comment.
Nice! Just a couple of things:
- I think we don't need to actually check if the
include_deletedparameter is a valid boolean; just checking if it is equal to the stringtrueshould be enough here. I know this is taken from existing code though, so I'm willing to defer to others on this. - Would it be possible to add a unit test for this new behaviour in
workspace_test.go? I'm up for pairing on this with you if you'd like!
| workspace, err = api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ | ||
| OwnerID: owner.ID, | ||
| Name: workspaceName, | ||
| Deleted: includeDeleted, |
There was a problem hiding this comment.
I don't think this works as intended. The param here in the SQL matches for deleted = $includeDeleted.
So if you set this to true, the workspace will not show if the workspace is deleted. We will need to update the SQL too, or just always return deleted workspaces by default.
There was a problem hiding this comment.
See how it's handled here:
Lines 57 to 62 in 454aea7
I think the deleted query param is kinda annoying as you have to know the state of the workspace to query it.
There was a problem hiding this comment.
@Emyrk Hmm, testing it in the UI seems to work.
- if
include_deletedis false, then we never enter this code block (becauseerrors.Is(err, sql.ErrNoRows)is false - otherwise we enter this code block and try to query for a deleted workspace
In either case, if we get nothing back, we then go into the httpapi.Forbidden(rw) block below. I agree editing the SQL would also work. I just didn't know what other ramifications that would have.
Let me know if I'm misunderstanding your comment!
|
@johnstcn |
defining vars in the scope of conditional Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
avoid newline Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
| @@ -83,6 +83,7 @@ | |||
| "workspaceapp", | |||
| "workspaceapps", | |||
| "workspacebuilds", | |||
| "workspacename", | |||
shortening test name Co-authored-by: Cian Johnston <cian@coder.com>
* feat: added include_deleted relates to #1955 * Update coderd/workspaces.go defining vars in the scope of conditional Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> * Update coderd/workspaces.go avoid newline Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> * Update coderd/workspaces.go Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> * PR feedback * wrote test, added type * Update coderd/workspaces_test.go shortening test name Co-authored-by: Cian Johnston <cian@coder.com> * taking out api.ts change for now * casing Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> Co-authored-by: Cian Johnston <cian@coder.com>
relates to #1955
I'm working on #1955: a ticket that will show deleted workspaces in our UI.
@f0ssel made some helpful changes to
getWorkspacesin #2095 to help support the aforementioned UI change.However, @BrunoQuaresma recently updated the Workspace page so that it no longer uses the
getWorkspaceroute, instead favoringgetWorkspaceByOwnerAndName.This PR attempts to add an
include_deletedparam togetWorkspaceByOwnerAndNameso I can continue work on #1955. I've never touched Go before so please let me know what I've broken 🧹P.S. It would be great to generate a type for this addition but idk how to do that.