Skip to content

Remove free text search on contextURL #8503

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Remove free text search on contextURL #8503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/dashboard/src/admin/ProjectsSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export function ProjectsSearch() {
<path fillRule="evenodd" clipRule="evenodd" d="M6 2a4 4 0 100 8 4 4 0 000-8zM0 6a6 6 0 1110.89 3.477l4.817 4.816a1 1 0 01-1.414 1.414l-4.816-4.816A6 6 0 010 6z" fill="#A8A29E" />
</svg>
</div>
<input type="search" placeholder="Search Projects" onKeyDown={(k) => k.key === 'Enter' && search()} onChange={(v) => { setSearchTerm(v.target.value) }} />
<input type="search" placeholder="Search Projects" onKeyDown={(k) => k.key === 'Enter' && search()} onChange={(v) => { setSearchTerm((v.target.value).trim()) }} />
</div>
<button disabled={searching} onClick={search}>Search</button>
</div>
Expand Down
2 changes: 1 addition & 1 deletion components/dashboard/src/admin/TeamsSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export function TeamsSearch() {
<path fillRule="evenodd" clipRule="evenodd" d="M6 2a4 4 0 100 8 4 4 0 000-8zM0 6a6 6 0 1110.89 3.477l4.817 4.816a1 1 0 01-1.414 1.414l-4.816-4.816A6 6 0 010 6z" fill="#A8A29E" />
</svg>
</div>
<input type="search" placeholder="Search Teams" onKeyDown={(k) => k.key === 'Enter' && search()} onChange={(v) => { setSearchTerm(v.target.value) }} />
<input type="search" placeholder="Search Teams" onKeyDown={(k) => k.key === 'Enter' && search()} onChange={(v) => { setSearchTerm((v.target.value).trim()) }} />
</div>
<button disabled={searching} onClick={search}>Search</button>
</div>
Expand Down
2 changes: 1 addition & 1 deletion components/dashboard/src/admin/UserSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default function UserSearch() {
<path fillRule="evenodd" clipRule="evenodd" d="M6 2a4 4 0 100 8 4 4 0 000-8zM0 6a6 6 0 1110.89 3.477l4.817 4.816a1 1 0 01-1.414 1.414l-4.816-4.816A6 6 0 010 6z" fill="#A8A29E" />
</svg>
</div>
<input type="search" placeholder="Search Users" onKeyDown={(ke) => ke.key === 'Enter' && search() } onChange={(v) => { setSearchTerm(v.target.value) }} />
<input type="search" placeholder="Search Users" onKeyDown={(ke) => ke.key === 'Enter' && search() } onChange={(v) => { setSearchTerm((v.target.value).trim()) }} />
</div>
<button disabled={searching} onClick={search}>Search</button>
</div>
Expand Down
22 changes: 8 additions & 14 deletions components/dashboard/src/admin/WorkspacesSearch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,27 +68,19 @@ export function WorkspaceSearch(props: Props) {
const search = async () => {
setSearching(true);
try {
let searchTerm: string | undefined = queryTerm;
const query: AdminGetWorkspacesQuery = {
ownerId: props?.user?.id,
};
if (matchesInstanceIdOrLegacyWorkspaceIdExactly(searchTerm)) {
query.instanceIdOrWorkspaceId = searchTerm;
} else if (matchesNewWorkspaceIdExactly(searchTerm)) {
query.workspaceId = searchTerm;
}
if (query.workspaceId || query.instanceId || query.instanceIdOrWorkspaceId) {
searchTerm = undefined;
const query: AdminGetWorkspacesQuery = {};
if (matchesInstanceIdOrLegacyWorkspaceIdExactly(queryTerm)) {
query.instanceIdOrWorkspaceId = queryTerm;
} else if (matchesNewWorkspaceIdExactly(queryTerm)) {
query.workspaceId = queryTerm;
}

// const searchTerm = searchTerm;
const result = await getGitpodService().server.adminGetWorkspaces({
limit: 100,
orderBy: 'instanceCreationTime',
offset: 0,
orderDir: "desc",
...query,
searchTerm,
});
setSearchResult(result);
} finally {
Expand All @@ -104,7 +96,9 @@ export function WorkspaceSearch(props: Props) {
<path fillRule="evenodd" clipRule="evenodd" d="M6 2a4 4 0 100 8 4 4 0 000-8zM0 6a6 6 0 1110.89 3.477l4.817 4.816a1 1 0 01-1.414 1.414l-4.816-4.816A6 6 0 010 6z" fill="#A8A29E" />
</svg>
</div>
<input type="search" placeholder="Search Workspaces" onKeyDown={(ke) => ke.key === 'Enter' && search() } onChange={(v) => { setQueryTerm(v.target.value) }} />
<input type="search" placeholder="Search Workspace IDs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I still think that long-term using more descriptive placeholders like Search by name as described in the last point in #8453 (comment) could be better. The ID, besides being a technical term, it's known within the team building the product but could be scary or ignored by users. We also scarcely reference the term workspace or instance ID in our docs.

suggestion: However, let's go with the current change as this looks obviously good and helpful and improve this if needed in a future iteration. ✔️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also scarcely reference the term workspace or instance ID in our docs.

Initially I thought that Admins surely know that those are workspace IDs, but I realize that that was my biased assumption and the fact that we don't reference the term ID anywhere makes me see your point.
Let's see how this goes - especially when self-hosted users give us feedback, and yes we can improve this anytime. 🙏🏽

onKeyDown={(ke) => ke.key === 'Enter' && search() }
onChange={(v) => { setQueryTerm((v.target.value).trim()) }} />
</div>
<button disabled={searching} onClick={search}>Search</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Although out of the scope of the changes in this PR, what do you think of dropping this search button here and other pages within the admin dashboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking When there is no button, would the expectation be that the search result gets updated with every key change? Or would the user be expected to hit Enter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: There are cases where searching or filtering on key change is valid and useful like projects or members which usually are a finite and small data set. However, it sounds optimal to not search on key change when filtering large datasets like instance users, workspaces, etc but require enter to perform the search. 💭

Thinking out loud, we added the search button back in #3723 because we require the enter key to perform a search here but seems harmless to drop the button. Your call, feel free to move this discussion in a follow up issue. 🏓

</div>
Expand Down
17 changes: 4 additions & 13 deletions components/gitpod-db/src/typeorm/workspace-db-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ export abstract class AbstractTypeORMWorkspaceDBImpl implements WorkspaceDB {
return await queryBuilder.getCount();
}

public async findAllWorkspaceAndInstances(offset: number, limit: number, orderBy: keyof WorkspaceAndInstance, orderDir: "ASC" | "DESC", query?: AdminGetWorkspacesQuery, searchTerm?: string): Promise<{ total: number, rows: WorkspaceAndInstance[] }> {
public async findAllWorkspaceAndInstances(offset: number, limit: number, orderBy: keyof WorkspaceAndInstance, orderDir: "ASC" | "DESC", query?: AdminGetWorkspacesQuery): Promise<{ total: number, rows: WorkspaceAndInstance[] }> {
let whereConditions = [];
let whereConditionParams: any = {};
let instanceIdQuery: boolean = false;
Expand All @@ -765,28 +765,19 @@ export abstract class AbstractTypeORMWorkspaceDBImpl implements WorkspaceDB {
whereConditions.push("(wsi.id = :instanceId OR ws.id = :workspaceId)");
whereConditionParams.instanceId = query.instanceIdOrWorkspaceId;
whereConditionParams.workspaceId = query.instanceIdOrWorkspaceId;
} else if (query.workspaceId) {
whereConditions.push("ws.id = :workspaceId");
whereConditionParams.workspaceId = query.workspaceId;
} else if (query.instanceId) {
// in addition to adding "instanceId" to the "WHERE" clause like for the other workspace-guided queries,
// we modify the JOIN condition below to a) select the correct instance and b) make the query faster
instanceIdQuery = true;

whereConditions.push("wsi.id = :instanceId");
whereConditionParams.instanceId = query.instanceId;
} else if (query.workspaceId) {
whereConditions.push("ws.id = :workspaceId");
whereConditionParams.workspaceId = query.workspaceId;
} else if (query.ownerId) {
// If an owner id is provided only search for workspaces belonging to that user.
whereConditions.push("ws.ownerId = :ownerId");
whereConditionParams.ownerId = query.ownerId;
}
}

if (searchTerm) {
// If a search term is provided perform a wildcard search in the context url or exact match on the workspace id (aka workspace name) or the instance id.
whereConditions.push(`ws.contextURL LIKE '%${searchTerm}%'`);
}

let orderField: string = orderBy;
switch (orderField) {
case "workspaceId": orderField = "ws.id"; break;
Expand Down
28 changes: 3 additions & 25 deletions components/gitpod-db/src/workspace-db.spec.db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,28 +314,6 @@ import { DBWorkspaceInstance } from './typeorm/entity/db-workspace-instance';
expect(dbResult.total).to.eq(1);
}

@test(timeout(10000))
public async testFindAllWorkspaceAndInstances_contextUrl() {
await Promise.all([
this.db.store(this.ws),
this.db.storeInstance(this.wsi1),
this.db.storeInstance(this.wsi2),
this.db.store(this.ws2),
this.db.storeInstance(this.ws2i1),
]);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "contextURL", "DESC", undefined, this.ws.contextURL);
// It should only find one workspace instance
expect(dbResult.total).to.eq(1);

const workspaceAndInstance = dbResult.rows[0]

// It should find the workspace that uses the queried context url
expect(workspaceAndInstance.workspaceId).to.eq(this.ws.id)

// It should select the workspace instance that was most recently created
expect(workspaceAndInstance.instanceId).to.eq(this.wsi2.id)
}

@test(timeout(10000))
public async testFindAllWorkspaceAndInstances_workspaceId() {
await Promise.all([
Expand All @@ -344,7 +322,7 @@ import { DBWorkspaceInstance } from './typeorm/entity/db-workspace-instance';
this.db.store(this.ws2),
this.db.storeInstance(this.ws2i1),
]);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "workspaceId", "DESC", { workspaceId: this.ws2.id }, undefined);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "workspaceId", "DESC", { workspaceId: this.ws2.id });
// It should only find one workspace instance
expect(dbResult.total).to.eq(1);

Expand All @@ -361,7 +339,7 @@ import { DBWorkspaceInstance } from './typeorm/entity/db-workspace-instance';
this.db.store(this.ws2),
this.db.storeInstance(this.ws2i1),
]);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "workspaceId", "DESC", { instanceIdOrWorkspaceId: this.ws2.id }, undefined);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "workspaceId", "DESC", { instanceIdOrWorkspaceId: this.ws2.id });
// It should only find one workspace instance
expect(dbResult.total).to.eq(1);

Expand All @@ -379,7 +357,7 @@ import { DBWorkspaceInstance } from './typeorm/entity/db-workspace-instance';
this.db.store(this.ws2),
this.db.storeInstance(this.ws2i1),
]);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "instanceId", "DESC", { instanceId: this.wsi1.id }, undefined);
const dbResult = await this.db.findAllWorkspaceAndInstances(0, 10, "instanceId", "DESC", { instanceId: this.wsi1.id });

// It should only find one workspace instance
expect(dbResult.total).to.eq(1);
Expand Down
2 changes: 1 addition & 1 deletion components/gitpod-db/src/workspace-db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export interface WorkspaceDB {
findWorkspacesForContentDeletion(minSoftDeletedTimeInDays: number, limit: number): Promise<WorkspaceOwnerAndSoftDeleted[]>;
findPrebuiltWorkspacesForGC(daysUnused: number, limit: number): Promise<WorkspaceAndOwner[]>;
findAllWorkspaces(offset: number, limit: number, orderBy: keyof Workspace, orderDir: "ASC" | "DESC", ownerId?: string, searchTerm?: string, minCreationTime?: Date, maxCreationDateTime?: Date, type?: WorkspaceType): Promise<{ total: number, rows: Workspace[] }>;
findAllWorkspaceAndInstances(offset: number, limit: number, orderBy: keyof WorkspaceAndInstance, orderDir: "ASC" | "DESC", query?: AdminGetWorkspacesQuery, searchTerm?: string): Promise<{ total: number, rows: WorkspaceAndInstance[] }>;
findAllWorkspaceAndInstances(offset: number, limit: number, orderBy: keyof WorkspaceAndInstance, orderDir: "ASC" | "DESC", query?: AdminGetWorkspacesQuery): Promise<{ total: number, rows: WorkspaceAndInstance[] }>;
findWorkspaceAndInstance(id: string): Promise<WorkspaceAndInstance | undefined>;
findInstancesByPhaseAndRegion(phase: string, region: string): Promise<WorkspaceInstance[]>;

Expand Down
1 change: 0 additions & 1 deletion components/gitpod-protocol/src/admin-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,5 +116,4 @@ export type AdminGetWorkspacesQuery = {
instanceIdOrWorkspaceId?: string;
instanceId?: string;
workspaceId?: string;
ownerId?: string;
};
2 changes: 1 addition & 1 deletion components/server/ee/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {

await this.guardAdminAccess("adminGetWorkspaces", { req }, Permission.ADMIN_WORKSPACES);

return await this.workspaceDb.trace(ctx).findAllWorkspaceAndInstances(req.offset, req.limit, req.orderBy, req.orderDir === "asc" ? "ASC" : "DESC", req, req.searchTerm);
return await this.workspaceDb.trace(ctx).findAllWorkspaceAndInstances(req.offset, req.limit, req.orderBy, req.orderDir === "asc" ? "ASC" : "DESC", req);
}

async adminGetWorkspace(ctx: TraceContext, workspaceId: string): Promise<WorkspaceAndInstance> {
Expand Down