feat: remove POST /api/v1/generate-reports endpoint
#242
Conversation
This now can be achieved by running `curl -X POST http://localhost:3000/api/v1/workflow/generate-reports/run -H "Content-Type: application/json"`
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router
participant WorkflowRunner
Client->>API_Router: POST /api/v1/workflow/:id/run (with data)
API_Router->>WorkflowRunner: runWorkflow({workflowName, knex, data})
WorkflowRunner-->>API_Router: Workflow execution result
API_Router-->>Client: Response (success or error)
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/httpServer/routers/apiV1.js (1)
95-103:⚠️ Potential issueError message is prefixed twice – remove the second prefix
runWorkflowalready wraps failures withFailed to run workflow: ….
The router adds the same prefix again, resulting in messages like
Failed to run workflow: Failed to run workflow: Something went wrong.- errors: [{ message: `Failed to run workflow: ${error.message}` }] + errors: [{ message: error.message }]This keeps the response concise and avoids redundant wording.
♻️ Duplicate comments (1)
__tests__/httpServer/apiV1.test.js (1)
219-221: Duplicate of the previous call-argument assertionSame remark as above; the verification for the failure path mirrors the success path.
If you add the extra check suggested earlier, replicate it here to keep both branches symmetrical.
🧹 Nitpick comments (2)
__tests__/httpServer/apiV1.test.js (1)
185-187: Assertion adapts well to new signature – consider also validating the first arg👍 The shift from
calls[0][0]→calls[0][1]correctly reflects the insertedknexparameter.
If feasible, assert thatcalls[0][0]is indeed a Knex instance to guard against accidental argument re-ordering in future refactors:expect(mockWorkflowFn.mock.calls[0][0]).toHaveProperty('transaction') // rudimentary Knex shape checksrc/httpServer/routers/apiV1.js (1)
10-27:runWorkflowcould be simplified & made safer with async/awaitThe hand-rolled
new Promisewrapper plus manual timeout handling works but is unnecessarily verbose and easy to get wrong (e.g. unhandled rejection ifworkflow.workflowthrows synchronously beforetimeoutis set).Consider refactoring to an
asyncfunction and leveragingPromise.racefor the timeout:-const runWorkflow = ({ workflowName, knex, data } = {}) => new Promise((resolve, reject) => { - const { workflows } = getWorkflowsDetails() - const workflow = workflows[workflowName] - if (!workflow || typeof workflow.workflow !== 'function') { - return reject(new Error('Invalid Workflow')) - } - const timeout = setTimeout(() => { - reject(new Error('Workflow default timeout reached')) - }, HTTP_DEFAULT_TIMEOUT) - - Promise.resolve() - .then(() => workflow.workflow(knex, data)) - .then(() => resolve(workflow)) - .catch(err => reject(new Error(`Failed to run workflow: ${err.message}`))) - .finally(() => clearTimeout(timeout)) -}) +async function runWorkflow ({ workflowName, knex, data } = {}) { + const { workflows } = getWorkflowsDetails() + const wf = workflows[workflowName] + if (!wf || typeof wf.workflow !== 'function') { + throw new Error('Invalid Workflow') + } + + const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error('Workflow default timeout reached')), HTTP_DEFAULT_TIMEOUT) + ) + + await Promise.race([ + (async () => { await wf.workflow(knex, data) })(), + timeoutPromise + ]) + + return wf +} </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5012b928edc888e776b2a77176e0be4cb58ef6de and 2efd18c99cc38872cbd5b0434d0c6a80d6925242. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `__tests__/httpServer/apiV1.test.js` (2 hunks) * `src/httpServer/routers/apiV1.js` (3 hunks) * `src/httpServer/swagger/api-v1.yml` (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * src/httpServer/swagger/api-v1.yml </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (3)</summary> * GitHub Check: Analyze * GitHub Check: Playwright Tests * GitHub Check: build </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Related #216
This now can be achieved by running
curl -X POST http://localhost:3000/api/v1/workflow/generate-reports/run -H "Content-Type: application/json". Implemented in #241Summary by CodeRabbit
New Features
Bug Fixes
Documentation
/api/v1/generate-reportsendpoint from the API specification.Refactor
Chores
/api/v1/generate-reportsendpoint and its related tests.