-
Notifications
You must be signed in to change notification settings - Fork 17
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
21297 - Business Tombstone updates #11
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.
Hey really nice work this is a lot of stuff you had to add, I've just some got minor comments. Also just needs some tests
src/services/legal-s.ts
Outdated
} | ||
|
||
/** Response object from LegalServices.fetchDocuments(). */ | ||
export interface FetchDocumentsI { |
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.
These interfaces aren't being used here, do we need them? Also the comment should be updated since its referencing class api stuff
|
||
const blob = await fetchDocuments(summaryDocument.link) // todo: show alert box on error | ||
if (blob) { | ||
if (window.navigator && window.navigator.msSaveOrOpenBlob) { |
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 you use the 'download-file' util ? I think it is the same code. NOTE: downloadFile is used by director search as well
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.
sorry I remember you asked if we had something like this before haha
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.
OK, moved the code a bit, and used the util file to download and save it.
export interface BusinessWarningI { | ||
code: string // FUTURE: use an enum | ||
filing?: string // not used | ||
message: string | ||
warningType: WarningTypesE | ||
data?: any // optional extra properties (eg, amalgamationDate) | ||
} |
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.
The WarningTypesE
here and the WarningType
in the warning-type-e.ts
are not consistent. Maybe we can use WarningTypeE
?
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 renamed and reorganized this, nice spotting it.
This happened as I introduced some of existing interfaces/enums from entities.
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-11-6uggeve8.web.app |
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.
Very nice work
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-11-6uggeve8.web.app |
*Issue:21297 *bcgov/entity#21297
Description of changes:
Missing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).