Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SubmissionController bug fix #3118

Merged
merged 7 commits into from May 23, 2022
Merged

SubmissionController bug fix #3118

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b71404
submissionController bug fix
yeasinhossain-dsi May 19, 2022
09f4fa9
Merge branch 'develop' of github.com:opencrvs/opencrvs-core into ocrv…
yeasinhossain-dsi May 19, 2022
5ee4640
refactor & fix in submission controller
yeasinhossain-dsi May 19, 2022
80ceb67
Merge branch 'develop' into ocrvs-2936
rikukissa May 19, 2022
4aec6e6
Merge branch 'develop' into ocrvs-2936
rikukissa May 21, 2022
8fefae1
FIX: Refactor in Submission controller
yeasinhossain-dsi May 23, 2022
f37c8ea
Merge branch 'ocrvs-2936' of github.com:opencrvs/opencrvs-core into o…
yeasinhossain-dsi May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 3 additions & 6 deletions packages/client/src/SubmissionController.ts
View file
Open in desktop
Expand Up @@ -259,10 +259,7 @@ export class SubmissionController {
}
}
const scopes = getScope(this.store.getState()) || []
//It needs some times to elasticSearch to update index
setTimeout(async () => {
await this.store.dispatch(updateRegistrarWorkqueue())
}, 100)
await this.store.dispatch(updateRegistrarWorkqueue())
await this.store.dispatch(modifyDeclaration(declaration))

if (
Expand All @@ -283,9 +280,9 @@ export class SubmissionController {
) {
delete declaration.data[attachmentSectionKey]
}
this.store.dispatch(writeDeclaration(declaration))
await this.store.dispatch(writeDeclaration(declaration))
Copy link
Collaborator

@rikukissa rikukissa May 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was happening before adding these awaits? Did some changes get overwritten by some action creator? writeDeclaration is quite widely used throughout the app. Do we need to await use await elsewhere too? If I recall right, action dispatching should be fire-and-forget, and waiting for the result is a bit of an anti-pattern

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, submissionStatus was getting overridden when there were multiple declarations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where exactly did it get overwritten? I'm thinking if we could fix it so writeDeclaration wouldn't overwrite the status of other declarations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. Instead of replacing all the declarations in the successActionCreator of WRITE_DECLARATION, only replacing the declaration we're writing should solve our issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We debugged this with Yeasin and it turns out, it's workqueue auto-update that causes this issue. Basically, it can happen that while the offline queue is processing, the logic meant to keep workqueues up-to-date triggers, messing the local state of the applications. That logic is also currently built into SubmissionController

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to get rid of the work-queue auto update in the next release. The user experience isnt great and if it is causing side effects like this, then it is another good reason to get rid

} else {
this.store.dispatch(deleteDeclaration(declaration))
await this.store.dispatch(deleteDeclaration(declaration))
}
} else {
await this.store.dispatch(writeDeclaration(declaration))
Expand Down