-
Notifications
You must be signed in to change notification settings - Fork 49
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
replaced Minio with Document Record Service #760
replaced Minio with Document Record Service #760
Conversation
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.
Looks good.
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.
Looks good Brandon!
I have a few questions around the state management of consumerDocumentId
.
Are we storing this in state because we are using it in subsequent document requests?
ie the first POST generates the docId and we use that affiliate all the following documents to that singular docId?
If so, it makes sense in state, that being said we may want to consider that this property will need to be cleared before the Cont-In flow is launched or exited etc.
Either way, we will want to make sure this ID doesn't persist between Cont In Filings.
Edge case for sure and maybe the state IS already reset between, just wanted to call it out!
src/interfaces/store-interfaces/state-interfaces/continuation-in-state-interface.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.
LGTM 👍
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.
There's one outstanding comment that I'd like to see resolved but other than that, LGTM.
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.
DRS integration looks good. FYI CNTA is a legacy/existing document type possibly also used in the past by the scanning application.
@@ -4,4 +4,5 @@ export interface AuthorizationProofIF { | |||
fileKey: string | |||
fileName: string | |||
}> | |||
consumerDocumentId?: 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.
Can we make this name consistent with documentServiceId
(in filing-interfaces.ts and elsewhere)?
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.
We have both documentServiceId and consumerDocumentId as well.
Thanks
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 was thinking about this documentServiceId + documentConsumerId.
Is the first one "service id" and second one "document id"? If yes then what you have makes sense.
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.
LGTM
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.
Approved.
4d640b7
into
bcgov:feature-drs-integration
Issue #: /bcgov/entity#23446
Description of changes:
AuthorizationProof
componentUnlimitedLiabilityCorporationInformation
componentconsumerDocumentId
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).