-
Notifications
You must be signed in to change notification settings - Fork 254
Bal 4370 flaky plugin rerun during submission in edit #3428
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
Bal 4370 flaky plugin rerun during submission in edit #3428
Conversation
|
WalkthroughThe changes introduce a conditional update to the collection flow status during form submission in the UI, add a utility method for removing plugin outputs in the backend service, and refactor the final submission controller logic to update workflow runtime data before updating collection flow state and triggering events. A subproject commit reference for data migrations was also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CollectionFlowUI
participant BackendAPI
participant CollectionFlowService
User->>CollectionFlowUI: Submit form
CollectionFlowUI->>CollectionFlowUI: Check current status
alt Status is pending
CollectionFlowUI->>CollectionFlowUI: Set status to inprogress
else Status is not pending
CollectionFlowUI->>CollectionFlowUI: Do not update status
end
CollectionFlowUI->>BackendAPI: Send submission data
BackendAPI->>CollectionFlowService: Call finalSubmission
CollectionFlowService->>CollectionFlowService: Remove plugin outputs (if needed)
CollectionFlowService->>BackendAPI: Update workflow runtime data
BackendAPI->>BackendAPI: Update collection flow state
BackendAPI->>BackendAPI: Trigger workflow event
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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 (
|
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: 3
🧹 Nitpick comments (3)
apps/kyb-app/src/pages/CollectionFlow/versions/v2/components/organisms/CollectionFlowUI/CollectionFlowUI.tsx (1)
197-199
: Consider capturing the return value ofsetCollectionFlowStatus
for clearer intent
setCollectionFlowStatus
mutates the context and returns the updated object, yet the returned value is discarded.
Although the current mutation-in-place behaviour works, explicitly re-assigning helps future maintainers understand that the function produces a new value and prevents surprises if its implementation ever changes to an immutable approach.- if (values.collectionFlow?.state?.status === CollectionFlowStatusesEnum.pending) { - setCollectionFlowStatus(values, CollectionFlowStatusesEnum.inprogress); - } + if (values.collectionFlow?.state?.status === CollectionFlowStatusesEnum.pending) { + values = setCollectionFlowStatus(values, CollectionFlowStatusesEnum.inprogress); + }services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (2)
181-187
: Update workflow data and collection-flow state inside a transaction
updateWorkflowRuntimeData
andupdateCollectionFlowState
execute independently.
If the first call succeeds and the second fails (or vice-versa) the database will hold inconsistent records.Consider wrapping both writes in a single Prisma transaction (or explicit DB transaction) to guarantee atomicity.
205-208
: PropagatearrayMergeOption
when triggering the final eventIf the workflow depends on deep-merge semantics (as used elsewhere in the controller) omitting
arrayMergeOption
here could yield different merge behaviour from the sync step above.Confirm whether this event should replicate the merge behaviour of
/sync/context
. If yes, include the option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/kyb-app/src/pages/CollectionFlow/versions/v2/components/organisms/CollectionFlowUI/CollectionFlowUI.tsx
(1 hunks)services/workflows-service/prisma/data-migrations
(1 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
(2 hunks)services/workflows-service/src/collection-flow/services/collection-flow.service.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/kyb-app/src/pages/CollectionFlow/versions/v2/components/organisms/CollectionFlowUI/CollectionFlowUI.tsx (2)
packages/common/src/utils/collection-flow/enums/collection-flow-status-enum.ts (1)
CollectionFlowStatusesEnum
(1-18)packages/common/src/utils/collection-flow/set-collection-flow-status.ts (1)
setCollectionFlowStatus
(10-29)
services/workflows-service/src/collection-flow/services/collection-flow.service.ts (1)
services/workflows-service/src/rule-engine/core/test/data-helper.ts (1)
context
(1-123)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_linux
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
services/workflows-service/prisma/data-migrations (1)
1-1
: Confirm submodule commit update
The data-migrations subproject reference was bumped to66f23f0003a77e37d353fe35b7fe5ec8526bfb72
. Ensure this commit contains the required migration scripts for recent schema changes.#!/bin/bash # Verify submodule HEAD matches the updated commit git submodule update --init --recursive services/workflows-service/prisma/data-migrations git -C services/workflows-service/prisma/data-migrations rev-parse HEADExpect output
66f23f0003a77e37d353fe35b7fe5ec8526bfb72
.services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts (1)
189-200
: Status is forced tocompleted
without checking current stateDuring an
edit
flow the status could legitimately berevision
orfailed
.
Blindly overriding tocompleted
may hide unresolved issues.Request verification that this is the intended business rule; if not, gate the change behind an explicit state check similar to the UI logic introduced in the PR.
services/workflows-service/src/collection-flow/services/collection-flow.service.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/controllers/collection-flow.controller.ts
Show resolved
Hide resolved
fae9452
to
36aba91
Compare
36aba91
to
ee066c6
Compare
* fix: fixed issue where edit status was updated to in progress (#3427) * feat: reworked final submission & added plugins output cleanup * fix: fixed types
* fix: fixed issue where edit status was updated to in progress (#3427) * fix: fixed checkbox styles in ui package (#3430) * Bal 4370 flaky plugin rerun during submission in edit (#3428) * fix: fixed issue where edit status was updated to in progress (#3427) * feat: reworked final submission & added plugins output cleanup * fix: fixed types * refactor(workflow.service): enhance final state detection logic * refactor(workflow.service): enhance final state detection logic --------- Co-authored-by: Illia Rudniev <[email protected]>
Summary by CodeRabbit