fix(documents): Sort by publication v1 and v2 by default #17157
Conversation
WalkthroughThe pull request introduces updates to the sorting capabilities of document-related input types across several files. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/clients/documents/src/lib/models/DocumentInput.ts (1)
8-8: Add JSDoc comment to document the new sort option.The addition of 'Publication' sort option is correct, but it would be helpful to add documentation explaining what this sort option represents and when it was introduced.
+ /** Sort options for document list. 'Publication' sorts by document publication date (added in Oct 2024) */ sortBy?: 'Date' | 'Category' | 'Type' | 'Subject' | 'Sender' | 'Publication'libs/clients/documents-v2/src/lib/dto/listDocuments.input.ts (1)
11-11: Maintain documentation consistency with v1 client.Add the same JSDoc documentation as in the v1 client to maintain consistency across versions.
+ /** Sort options for document list. 'Publication' sorts by document publication date (added in Oct 2024) */ sortBy?: 'Date' | 'Category' | 'Type' | 'Sender' | 'Subject' | 'Publication'libs/api/domains/documents/src/lib/dto/getDocumentListInput.ts (1)
24-24: Enhance GraphQL field documentation and validation.Consider these improvements for the GraphQL field:
- Add description to the @field decorator
- Consider using an enum for better type safety and GraphQL schema documentation
+ /** Sort options for the document list */ + @Field(() => DocumentSortBy, { + nullable: true, + description: 'Determines the sort order of documents. Publication sorts by document publication date.' + }) sortBy?: 'Date' | 'Category' | 'Type' | 'Subject' | 'Sender' | 'Publication'Consider creating an enum:
export enum DocumentSortBy { Date = 'Date', Category = 'Category', Type = 'Type', Subject = 'Subject', Sender = 'Sender', Publication = 'Publication' }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
libs/api/domains/documents/src/lib/dto/getDocumentListInput.ts(1 hunks)libs/api/domains/documents/src/lib/models/v2/documents.input.ts(2 hunks)libs/clients/documents-v2/src/lib/dto/listDocuments.input.ts(1 hunks)libs/clients/documents/src/lib/documentClient.ts(1 hunks)libs/clients/documents/src/lib/models/DocumentInput.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/clients/documents/src/lib/documentClient.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/documents-v2/src/lib/dto/listDocuments.input.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/documents/src/lib/models/DocumentInput.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/documents/src/lib/models/v2/documents.input.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/documents/src/lib/dto/getDocumentListInput.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/api/domains/documents/src/lib/dto/getDocumentListInput.ts (1)
24-24: Verify completeness of sort option updates.
The 'Publication' sort option has been consistently added across the codebase. Let's verify no other files need this update.
✅ Verification successful
'Publication' sort option is consistently implemented
All files containing sort options have been updated to include the 'Publication' option:
- libs/clients/documents/src/lib/models/DocumentInput.ts
- libs/clients/documents-v2/src/lib/dto/listDocuments.input.ts
- libs/api/domains/documents/src/lib/dto/getDocumentListInput.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other files that might need the sort option update
rg -t typescript "sortBy.*('Date'|'Category'|'Type'|'Subject'|'Sender')" --glob '!**/test/**'
Length of output: 160
Script:
#!/bin/bash
# Search for files containing sort options, using a more general pattern
rg "sortBy.*['\"](?:Date|Category|Type|Subject|Sender)" -g "*.ts" -g "!**/test/**" -g "!**/node_modules/**"
Length of output: 527
libs/api/domains/documents/src/lib/models/v2/documents.input.ts (1)
20-21: LGTM! Clear documentation for the Publication sort option.
The added comment effectively explains that Publication represents the document publication date and is now the default sorting option.
libs/clients/documents/src/lib/documentClient.ts (1)
136-136: Verify backward compatibility with existing clients.
While the change to default to 'Publication' aligns with the requirements, we should ensure existing clients are prepared for this change.
Let's verify the impact:
✅ Verification successful
Change to default sort order is safe to proceed
Based on the codebase analysis:
- No direct dependencies were found relying on the default 'Date' sorting
- All clients using
getDocumentListpass their sort parameters explicitly or accept the default - The only reference to 'Publication' sorting is in the changed code itself
- No test cases were found that assume a specific default sort order
The change appears to be an implementation detail that won't impact existing clients since:
- Clients that need specific sorting already specify it explicitly
- The default is only used when no sort parameter is provided
- The API contract remains unchanged as both sort options are valid values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for clients that might be affected by the sort change
# Look for direct API calls that might expect Date sorting
rg -A 3 "messages\?" --type ts
# Look for tests that might need updating
rg -l "sortBy.*Date.*test" --type ts
Length of output: 3694
Script:
#!/bin/bash
# Let's search for any usage of the DocumentClient's getDocumentList method
# and references to sortBy parameter to understand the impact
# Search for getDocumentList usage
rg -A 5 "getDocumentList" --type ts
# Search for DocumentSortBy enum usage
rg -A 2 "DocumentSortBy\." --type ts
# Search for test files that might be affected
fd "document.*test" -e ts -x cat {}
Length of output: 8448
Script:
#!/bin/bash
# Let's check for any references to the old default sort value and the new one
# Search for any references to sortBy=Date in tests or implementation
rg -A 2 "sortBy.*=.*Date" --type ts
# Search for any references to sortBy=Publication in tests or implementation
rg -A 2 "sortBy.*=.*Publication" --type ts
# Search for the DocumentSortBy enum definition to understand the available options
rg -A 5 "enum DocumentSortBy" --type ts
Length of output: 814
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17157 +/- ##
==========================================
- Coverage 35.73% 35.73% -0.01%
==========================================
Files 6926 6926
Lines 147678 147673 -5
Branches 42063 42060 -3
==========================================
- Hits 52775 52772 -3
+ Misses 94903 94901 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 3 Passed Test Services
|
* Sort by publication v1 and v2 by default * Remove logg --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Sort by publication in documents v1 and v2 by default
Why
After client config update in october,
Publicationhas been added. UI already sorts by publication, so the service should sort by publication by default.Checklist: