-
Notifications
You must be signed in to change notification settings - Fork 201
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
Use the new batch endpoint in MM #2910
Conversation
- Update URL regex pattern for improved validation - Adjust maximum batch size from 10,000 to 1,000 for better manageability - Streamline report request transformation for clarity (your batch size restriction feels like a diet plan that only allows rabbit food)
|
WalkthroughThis pull request introduces several modifications across multiple services and files, primarily focusing on URL validation, business report processing, and data handling. The changes include updating the URL regex pattern, reducing the batch size for business report requests, transforming website URLs to lowercase, and refactoring the merchant monitoring client's batch creation method. The modifications aim to improve data consistency, validation, and processing efficiency in the workflows service. Changes
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
CodeRabbit Configuration File (
|
services/workflows-service/src/business-report/business-report.service.ts
Show resolved
Hide resolved
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
services/workflows-service/src/business-report/merchant-monitoring-client.ts (1)
135-148
: Consider returning or inspecting the response for better error handlingCurrently, this function silently returns
void
after the HTTP request. If the remote service responds with partial success or additional data, it is not accessible. For robust error handling or logging, consider returning the response or at least confirming a successful result.public async createBatch({...}) { const response = await this.axios.post(...); + return response.data; // or parse/validate if needed }
services/workflows-service/src/business-report/dtos/create-business-report.dto.ts (1)
Line range hint
11-30
: Consistent URL transformation with the schemaUsing a
@Transform
decorator aligns with the schema’s.transform(value => value.toLowerCase())
. Double-check whether lowercasing is desired for the entire URL, as some servers differentiate path case.packages/common/src/consts/index.ts (1)
172-172
: Consider using a standard library for complex URL parsingThe updated regex covers many cases but may not handle all IDNs or punycode domains, and can be challenging to maintain. Consider using the built-in
URL
constructor or a dedicated library for robust URL validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/common/src/consts/index.ts
(1 hunks)services/workflows-service/prisma/data-migrations
(1 hunks)services/workflows-service/src/business-report/business-report.service.ts
(2 hunks)services/workflows-service/src/business-report/dtos/create-business-report.dto.ts
(2 hunks)services/workflows-service/src/business-report/merchant-monitoring-client.ts
(1 hunks)services/workflows-service/src/common/schemas.ts
(1 hunks)services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (4)
services/workflows-service/src/business-report/merchant-monitoring-client.ts (1)
117-133
: Validate optional workflowVersion parameter or provide a default
The updated parameters for createBatch
allow workflowVersion
to be optional, but there is no explicit validation or fallback default, which might cause unexpected behavior if the caller omits it.
Would you like to define a reasonable default value or enforce validation to ensure consistency?
services/workflows-service/src/common/schemas.ts (1)
21-24
: Confirm that forcing all URLs to lowercase supports all use cases
Lowercasing the entire URL standardizes domains but can break path-based resources if the path is case-sensitive. If case handling is only critical for the domain, consider parsing to selectively lowercase the domain component while preserving the path.
services/workflows-service/src/business-report/business-report.service.ts (2)
124-125
: Revisiting the batch-size limit
Reducing the batch size to 1,000 helps avoid large payload issues. If larger batches become necessary, consider chunking or streaming to avoid potential timeouts.
163-173
: Ensure correlation data is passed when needed
The new createBatch
call correctly uses the updated signature. However, note that only businessId
is appended to callbackUrl
; if correlation or other identifiers are also needed downstream, consider adding them to the URL.
Summary by CodeRabbit
websiteUrl
to ensure it is stored in lowercase.