-
Notifications
You must be signed in to change notification settings - Fork 69
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
fix: Wrong status shown in the Comparison View Page of duplicate record #7476
Conversation
This comment has been minimized.
This comment has been minimized.
regStatusMessages[ | ||
actualRegData.status as unknown as RegStatus | ||
] |
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.
I prefer changing the status's original type to RegStatus
here instead of this type assertion.
You'll need to change
opencrvs-core/packages/client/src/declarations/index.ts
Lines 1559 to 1563 in d46f47e
(eventData && | |
eventData.registration && | |
eventData.registration.status && | |
eventData.registration.status[0].type) || | |
'' |
const downloadedAppStatus: RegStatus | undefined =
eventData &&
eventData.registration &&
eventData.registration.status &&
eventData.registration.status[0].type
, but it would be a nice improvement to the technical debt we have in the front end.
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.
Thanks man, I wasn't sure so I avoided touching other parts of the codebase as much as possible but I get your point, my thinking was, we could deal with the tech debt as a separate ticket. I'm avoiding a case of effort spent on refactors bloating up the effort required to do the fix. would you agree?
I'm honestly a bit confused regarding downloadedAppStatus: RegStatus | undefined
does this mean the type of downloadedAppStatus
is of RegStatus
always? I'm asking because I saw the DOWNLOAD_STATUS enum with the values below, are they not related?
export enum DOWNLOAD_STATUS {
READY_TO_DOWNLOAD = 'READY_TO_DOWNLOAD',
DOWNLOADING = 'DOWNLOADING',
DOWNLOADED = 'DOWNLOADED',
FAILED = 'FAILED',
FAILED_NETWORK = 'FAILED_NETWORK',
READY_TO_UNASSIGN = 'READY_TO_UNASSIGN',
UNASSIGNING = 'UNASSIGNING'
}
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.
Thanks man, I wasn't sure so I avoided touching other parts of the codebase as much as possible but I get your point, my thinking was, we could deal with the tech debt as a separate ticket. I'm avoiding a case of effort spent on refactors bloating up the effort required to do the fix. would you agree?
Thanks for explaining your thought process, I see you 👍🏻 In our team refactors in pull requests like these are good, unless it's specifically an urgent bug fix. In my opinion, feel free to always apply the 1% improvement to the pull requests :)
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
declaration.registrationStatus === | ||
SUBMISSION_STATUS.CORRECTION_REQUESTED ? null : ( |
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.
@naftis This is an example of the type fields being compared
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.
I left a couple of comments regarding this, see if they help
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
CHANGELOG.md
Outdated
## Refactor | ||
|
||
- Remove dependency on openhim. The openhim db is kept for backwards compatibility reasons and will be removed in v1.6. It has brought some major changes | ||
in how the microservices are communicating among them. More on this can be found on the updated [sequence diagrams](https://github.com/opencrvs/opencrvs-core/tree/develop/sequence-diagrams/backend) |
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.
This doesn't seem related?
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.
Oh this was me resolving a merge conflict, The refactor changelog entry was from develop
which was conflicting with my bug changelog entry. I will look into it to see if I can resolve it differently
@@ -314,7 +314,7 @@ function FormAppBar({ | |||
} | |||
const [exitModalTitle, exitModalDescription] = | |||
isCorrection(declaration) || | |||
declaration.registrationStatus === SUBMISSION_STATUS.CORRECTION_REQUESTED | |||
declaration.submissionStatus === SUBMISSION_STATUS.CORRECTION_REQUESTED |
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.
This would probably change how the application behaves so it should rather be like this:
declaration.submissionStatus === SUBMISSION_STATUS.CORRECTION_REQUESTED | |
declaration.registrationStatus === RegStatus.CORRECTION_REQUESTED |
declaration.submissionStatus !== | ||
SUBMISSION_STATUS.CORRECTION_REQUESTED && ( |
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.
declaration.submissionStatus !== | |
SUBMISSION_STATUS.CORRECTION_REQUESTED && ( | |
declaration.registrationStatus !== | |
RegStatus.CORRECTION_REQUESTED && ( |
status: (props.declaration.data.history as unknown as History[]).find( | ||
(data) => data.action === null | ||
)?.regStatus, | ||
status: props.declaration.registrationStatus, |
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.
Should we not do the same for duplicateRegData.status
as well?
(declaration.registrationStatus as string) !== | ||
(SUBMISSION_STATUS.CORRECTION_REQUESTED as string) && ( |
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.
I think we can do the same here, basically use RegStatus
declaration.registrationStatus === | ||
SUBMISSION_STATUS.CORRECTION_REQUESTED ? null : ( |
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.
I left a couple of comments regarding this, see if they help
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
We were rendering a wrong status on the duplicate comparison tab #7439
Use RegStatus as a type instead of a generic string #7417
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
The declaration status bug fix was incorrectly listed under 1.5.0 section of the CHANGELOG so we had to move it to 1.6.0 #7439
When we were doing the refactor to ensure declaration.status was using RegStatus we incorrectly changed the field to declaration.submissionStatus instead of `registrationStatus` #7439
The way we determine status for duplicateRegData should be the same as the actualData. So we needed to keep things consistent #7439
We were not using the correct type when doing the comparison, so using the correct types without forced assertions is cleaner #7439
We were incorrectly reading the status from the actualData object instead of reading from duplicateRegData #7439
9b9ea0f
to
6d54cdf
Compare
@Zangetsu101 thanks for catching this, I've addressed it now |
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
Missed recording the changes done on this PR in the CHANGELOG.md file #7476
The createReviewDeclaration() function was using a incorrect type<string> instead of the RegStatus type. #7476
#7439