Document Record Service #274
Conversation
cameron-eyds
requested review from
JazzarKarim,
severinbeauvais and
cameron-eyds
src/services/document-services.ts
Outdated
| @@ -0,0 +1,125 @@ | |||
| import Axios, { AxiosResponse } from 'axios' | |||
There was a problem hiding this comment.
I know the FeeServices imports Axios like this but I think it's bad practice.
Consider passing the existing Axios instance in as a prop, like this:
and
There was a problem hiding this comment.
The only catch i bumped into here is the interceptors from a local instance may not meet all requirements.
ie missing account-id insertion into the header etc
Work around is to create another local instance but it might also make sense to make a service or component atomic so it doesn't need to rely on that local axios etc.
Just considerations!
There was a problem hiding this comment.
You can still provide the necessary headers in this file using a passed-in Axios instance, ie, this should still work fine:
As for interceptors, well if they're needed in the parent project then they're probably needed when this shared component is part of that project, yes?
There was a problem hiding this comment.
For sure
Just calling out that passing in axios vs a local Axios instance with defined interceptors matters.
If a service or component here has somewhat unique headers for it's requests, than the argument could be made to just make it atomic on this end, is all.
There was a problem hiding this comment.
The thing is, I might need to use these functions in any project that has doc-related components.
However, I believe we use useBcrosFetch in Nuxt applications(e.g. dashboard ui), and in that case, I need to create a new Axios instance to pass as a prop.
There was a problem hiding this comment.
I'm still not sure whether a local Axios instance (in your component) is desirable.
I do see the problem with the unwanted interceptor. Does it actually cause a problem?
Also, is there no auth for DRS calls?
There was a problem hiding this comment.
Depending on how Axios is working currently with shared components, the local axios idea might be ideal or just as fraught with issues. I'm waiting for your response on my questions above to understand this and give an informed opinion. Thx.
There was a problem hiding this comment.
I need to override the intercepter because we get 401 response with JWT and we use apikey for authentication.
But yes I've included axios instance as a param for each function and I am writing unit test as requested.
Once the updated fucntions are fully test locally across create ui, edit ui, filings ui, and sbc-auth, i will make another commit and will inform you.
Thanks.
There was a problem hiding this comment.
Are you saying that import Axios, { AxiosResponse } from 'axios' returns the existing instance (with interceptors), and that's why you don't need to "create" it but you do need to override the interceptor?
In a way, that's good, as it means we don't have to pass the axios instance into the component; the code above just returns it.
In a way, that's bad, because the existing instance can have interceptors, and it's really unfortunate you need to hack the UI's axios instance in order to use an external component (which, ideally, would be "atomic" as Cameron put it, or I would say "independent").
If the above is true then you don't need to pass in the prop, and let's keep this simple for now -- I will accept your existing code.
If I still have my lines crossed, let me know 😄
There was a problem hiding this comment.
Happy Friday Severin!
I just pushed a new commit which includes the below updates:
- passed axios instance as a param
- implemented unit test
- confirmed fee-summary component is working on storybook
- tested on create-ui and edit-ui
- addressed additional issues.
This is the first time for me to work on npm package which is a great opportunity to learn new skills.
Please let me know if I missed anything.
Thank you for your help! ❤️ ❤️ ❤️
src/services/package.json
Outdated
| "publishConfig": { | ||
| "access": "public" | ||
| }, | ||
| "dependencies": { | ||
| "@bcrs-shared-components/enums": "^1.1.21", | ||
| "@bcrs-shared-components/enums": "^1.1.22", |
There was a problem hiding this comment.
As above, revert these changes. Lerna will do them automatically in step 14:
https://github.com/bcgov/bcrs-shared-components/blob/main/README.md
There was a problem hiding this comment.
To be clear: leave the extra dependency here but use current version numbers. Lerna will update the version numbers.
| PayApiUrl = 'PAY_API_URL' | ||
| PayApiUrl = 'PAY_API_URL', | ||
| DocApiUrl = 'DOC_API_URL', | ||
| DocApiKey = 'DOC_API_KEY' |
There was a problem hiding this comment.
Next time this file is updated, we should alphabetize these keys to make them easier to look through!
tests/utils.ts
Outdated
| return new Promise((resolve) => setTimeout(resolve, ms)) | ||
| } | ||
|
|
||
| export { instance as AxiosInstance } |
There was a problem hiding this comment.
Would you mind breaking this up into separate functions (named for their content)?
tests/utils.ts
Outdated
| instance.interceptors.response.use( | ||
| (response) => response, | ||
| (error) => Promise.reject(error) | ||
| ) |
There was a problem hiding this comment.
Why do you need this for the tests?
| COOP_RULES = 'COOP_RULES', // Rules | ||
| COU = 'COU', // Court Order | ||
| COOP_MISC = 'COOP_MISC', // Correction Filing | ||
| COFI = 'COFI', // Cooperatives Miscellaneous Documents |
There was a problem hiding this comment.
Please alphabetize :)
There was a problem hiding this comment.
(within each section is OK)
You can also add whitespace between sections to make it easy to read.
Issue #: /bcgov/entity#25849
Description of changes:
Added enums and services for document service integration.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).