-
Notifications
You must be signed in to change notification settings - Fork 125
feat(app-deployments): add server-side sorting by created, activated, and last used #7700
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b020bcb
bcd6119
62f2f57
41bf13e
da4ff0c
80a4307
11566c0
24d3616
7580142
f3003f8
50da1f5
c0b9111
c355931
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'hive': patch | ||
| --- | ||
|
|
||
| Add server-side sorting to app deployments table (Created, Activated, Last Used). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,13 +222,38 @@ export class AppDeploymentsManager { | |
|
|
||
| async getPaginatedAppDeploymentsForTarget( | ||
| target: Target, | ||
| args: { cursor: string | null; first: number | null }, | ||
| args: { | ||
| cursor: string | null; | ||
| first: number | null; | ||
| sort: { | ||
| field: GraphQLSchema.AppDeploymentsSortField; | ||
| direction: GraphQLSchema.SortDirectionType; | ||
| } | null; | ||
| }, | ||
| ) { | ||
| return await this.appDeployments.getPaginatedAppDeployments({ | ||
| targetId: target.id, | ||
| cursor: args.cursor, | ||
| first: args.first, | ||
| }); | ||
| const { sort, cursor, first } = args; | ||
|
|
||
| const pagePromise = | ||
| sort?.field === 'LAST_USED' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have an exported enum from codegen we could use?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. codegen produces string union types, not typescript enums, so there's no enum member to reference |
||
| ? this.appDeployments.getPaginatedAppDeploymentsSortedByLastUsed({ | ||
| targetId: target.id, | ||
| cursor, | ||
| first, | ||
| direction: sort.direction, | ||
| }) | ||
| : this.appDeployments.getPaginatedAppDeployments({ | ||
| targetId: target.id, | ||
| cursor, | ||
| first, | ||
| sort: sort ? { field: sort.field, direction: sort.direction } : null, | ||
| }); | ||
|
|
||
| const [page, total] = await Promise.all([ | ||
| pagePromise, | ||
| this.appDeployments.countAppDeployments(target.id), | ||
| ]); | ||
|
|
||
| return { ...page, total }; | ||
| } | ||
|
|
||
| async getActiveAppDeploymentsForTarget( | ||
|
|
@@ -243,12 +268,17 @@ export class AppDeploymentsManager { | |
| }; | ||
| }, | ||
| ) { | ||
| return await this.appDeployments.getActiveAppDeployments({ | ||
| targetId: target.id, | ||
| cursor: args.cursor, | ||
| first: args.first, | ||
| filter: args.filter, | ||
| }); | ||
| const [page, total] = await Promise.all([ | ||
| this.appDeployments.getActiveAppDeployments({ | ||
| targetId: target.id, | ||
| cursor: args.cursor, | ||
| first: args.first, | ||
| filter: args.filter, | ||
| }), | ||
| this.appDeployments.countAppDeployments(target.id), | ||
| ]); | ||
|
|
||
| return { ...page, total }; | ||
| } | ||
|
|
||
| getDocumentCountForAppDeployment = batch<AppDeploymentRecord, number>(async args => { | ||
|
|
||
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: I like to set the default in the SDL so that it's clearly visible to API consumer and so that although the argument doesnt need passed, youll always get a value in the resolver.
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 agree but, there's already a code check for this that clamps and sets it to 20 when null. If there's any needed change, it has to also change all fields that utilize the (nullable, no default)
firstparameter pattern and not justappDeployments. For consistency, this should be a codebase-wide decision, not a one-off on this field.This is based on the project patterns document: https://www.notion.so/theguildoss/Hive-Console-GraphQL-API-Design-Patterns-18ab6b71848a80678939c669699fe8e6#18ab6b71848a80d5a1b1d16abd881d32