Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(GIFT-3546): add endpoint to fetch customer data #1127
base: feat/FN-3543/integrate-with-ods
Are you sure you want to change the base?
feat(GIFT-3546): add endpoint to fetch customer data #1127
Changes from all commits
b968c06
dfbadbc
1c2dd48
1b92da1
b684f32
994a243
59f584d
4fc4b0c
efd22ed
44331c3
78446fb
27fcf9d
1f34c07
388e3bb
9d57205
6857a12
dc437ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
temporarily leaving this in while the feature branch is open to run tests, will remove before feature branch is merged to main
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 know this exists else where, will raise a seprate PR to rectify this minor typo.
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 this be a number?
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.
Because a lot of urns start with 00 we don't want to cast this to number, has to stay a 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.
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.
Number?
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.
ditto other comment: Because a lot of urns start with 00 we don't want to cast this to number, has to stay a 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.
Is there a constant for this? If not then don't worry will have to be a separate PR.
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 is a constant I've defined to have all the different entities that can be queried in ODS
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 wanted a test to check the validation of the dto (e.g. valid when a urn matching the regex is passed through) but that would require setting up a validationPipe for this test which no other test in the codebase has done. For now I'm going to leave out to stay consistent with the other test suites but one to look at across the board
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.
At the moment DTO validations are tested as part of
api/e2e
tests (root/tests
). Most likely these tests are not part of github pipelines yet.Would be nice to create
api/e2e
test forosd
.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.
Yep will add this in a new PR straight after this one (don't have a tonne of time today/tomorrow but keen to get my feature branch tested in dev so have split up the work)
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.
Could you please replace
200
with HttpStatusCode through out your PR.Know this exists else where, so please ignore the old code.
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.
Not requesting to change, but just an fyi that there is a more specific
@ApiOkResponse
that we could use here, like with the error responses.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 add Internal server error response or will that be caught/thrown by a global middleware?
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 didn't want to have to match indentation in the string matching so there is a regex match instead. Thoughts @toddym42 ?
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.
Not sure I can think of anything better if we want to match the whole string, but I'm wondering if there's much value asserting on the whole thing, as it's just a hard-coded value that we're repeating. Could just expect any string, or something more minimal like
expect.stringContaining('EXEC t_apim.sp_ODS_query')
?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 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.
Missed this one.