-
Notifications
You must be signed in to change notification settings - Fork 80
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: issue#63 - GET /user/personal_details api endpoint and tests #71
Feat: issue#63 - GET /user/personal_details api endpoint and tests #71
Conversation
app/api/ms_api_utils.py
Outdated
|
||
def post_request(request_url, data): | ||
def post_request(request_string, data): | ||
request_url = f"{BASE_MS_API_URL}" + request_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.
Simplify as:
f”{BASE_MS_API_URL}{request_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.
done.
PS: I need to open a new branch for issue #62 and will need to carry this PR code changes across. I'll squashed merge it now so it won't be a problem later on. |
b2e01a2
to
663de70
Compare
Update @anitab-org/bridgeintech-maintainers . I've just forced push last fix for f-string referred by @ramitsawhney27 above that I missed (on get_request function). |
@ramitsawhney27 . Atm, this code change on this PR only getting user from user table on MS schema. do you want me to also add the do the BIT schema DAO side on this PR as well or open a new one for that? |
But perhaps, the BIT DAO can be done after update user profile, since BIT DAO will only have data after user update their profile which is for issue #62. I'll leave this PR as it is then (only GET /user on MS public) and will open a separate issue for GET /user that calls BIT DAO user_extension and personal_background |
Update @anitab-org/bridgeintech-maintainers. As per MVP mockups on User
This PR will only deal with GET /user/personal_details. I'll create separate issues for the other 2 GET sub-urls. |
663de70
to
4d9ab8c
Compare
Update @anitab-org/bridgeintech-maintainers . I've just pushed forced the changes to fix request url |
Now this PR is complete and any changes will be made in the next PR. Please re-review. @anitab-org/bridgeintech-maintainers |
Update: just came across a bug on this PR... will fix and push another commit soon @anitab-org/bridgeintech-maintainers |
4d9ab8c
to
e93f1d9
Compare
Update @anitab-org/bridgeintech-maintainers . I fixed the bug. Now it's ready for re-review 😉 |
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 changes made in this PR were tested locally. Following are the results:
-
Code review - Done by @ramitsawhney27
-
All possible responses were tested as below (tap/zoom in to see clearer gifs):
-
Additional testcases covered: N/A
-
Additional Comments:
- Tested locally with no errors @mtreacy002 !!
- OS Version: Windows 10
Thanks for the review, @foongminwong. @ramitsawhney27 . can you please re-approve this PR? your previous approval got dismissed when I did the force pushed 😁 |
Description
Add API endpoint for GET/user profile and test cases
Fixes #72 as well as part of issue #63
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Checklist:
Code/Quality Assurance Only