Feat: Issue#80 PUT /user/additional_info api endpoints and test cases #81

mtreacy002 · 2020-07-15T13:50:42Z

Description

Allow user to update their additional information.

Fixes #80

Type of Change:

Code

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

tested manually by running the application and update user additional information.

on successful update:
Before update:

After update:

Success message on Swagger UI:

On invalid input (invalid timezone):

When user has no existing additional information data in database:

by running unittest and all test cases passed

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

Code/Quality Assurance Only

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

mtreacy002 · 2020-07-15T13:54:44Z

Update @ramitsawhney27 and @anitab-org/bridgeintech-maintainers . Here's the PUT /user/additional_info to update user's existing additional information. Can you please provide feedback? Thanks

mtreacy002 · 2020-07-20T01:07:59Z

Update @anitab-org/bridgeintech-maintainers . I've pushed the change requested on Timezone enum as our discussion last night. Now all logic on enum will be dealt by backend, frontend ui will only view enum value.

mtreacy002 · 2020-07-25T11:27:20Z

@anitab-org/bridgeintech-maintainers . This PR and all dependent PRs after this one have merge conflict that is caused by the recent merge from PR#77. I will leave the merge conflict in place (not going to solve it for now) until the next PR on the queue (PR#79) is merged. This way, the merge conflict serves as automatic blocker to this PR so we could prevent accidental merging of this PR before its depenceny/ies (blocker) PR/s get merged. 😉
However, mentors can still reeview and approve the PR with note saying something like "providing merge conflict gets resolved", so I know if the PR is Ok, or if I need to do any modification to it. You'll need to reapprove though once the merge conflict is resolved and force pushed. Thanks

isabelcosta · 2020-07-25T16:16:46Z

@mtreacy002 can you solve this now, that #79 was merged? Ping me when you need my review

mtreacy002 · 2020-07-26T01:04:09Z

Update @isabelcosta and @anitab-org/bridgeintech-maintainers . I've solved the merge conflict. Can you please re-review this PR? Ta

mtreacy002 · 2020-07-26T06:55:21Z

Update @anitab-org/bridgeintech-maintainers . I've fix the user_id bug mentioned in my zulip post.
I also placed a temporary solution for bug on issue #94 by set AUTH_COOKIE["user_id"] to == 0 when user send POST /login request.
Note that setting user_id cookie to 0 only works as soluton in local environment. In dev environment where remote heroku server is used and more than one user access the server at a time, this solution might not work (so, I'll keep issue #94 open as icebox to be revisited after GSoC).

Can you please re-review? Thanks

isabelcosta · 2020-07-27T19:50:22Z

app/api/request_api_utils.py

@@ -36,6 +36,7 @@ def post_request(request_string, data):
            access_expiry_cookie = response_message.get("access_expiry")
            AUTH_COOKIE["Authorization"] = f"Bearer {access_token_cookie}"
            AUTH_COOKIE["Authorization"]["expires"] = access_expiry_cookie
+            AUTH_COOKIE["user_id"] = 0


why is this 0? @mtreacy002

@isabelcosta. This is because if the server only just run for the first time, there'll be no AUTH_COOKIE["user_id"] ever initiated in the system. I tried once with if not AUTH_COOKIE["user_id"] to check if it's none but the system crash since there's no such attribute as user_id in the AUTH_COOKIE. But if the system already has someone logged in, their "user_id" need to be cleared out so another logged in person not going to be accidentally associated to it. I also tried to set AUTH_COOKIE["user_id"] = None, but that didn't work as well (forgot what thee error message was).
So here, I used 0 as a default on AUTH_COOKIE["user_id"] since the database will never give an index of 0 as an ID to an instance (it always starts with 1). So, it's sort of the safest way to instantiate the attribute "user_id" for AUTH_COOKIE but without associating it to any user.

I think we should keep it as None. We can look into the error, it would just be a TypeError, where you can cast None to 0 if needed

Got it. I changed it to None and use catch the ValueError exception. Perhaps that crash I encountered last time was on something else then 😊

ramitsawhney27 · 2020-07-29T05:36:38Z

app/api/dao/user_extension.py

+        """
+
+        user_id = int(AUTH_COOKIE["user_id"].value)
+        if user_id == 0:


If not user_id

Yep, I removed it completely as now I'm applying EAFP try catch instead 😁

Very happy to see more usage of EAFP!

Allow user to input Timezone in enum value Fix bug user id not retrieved Refactor reset user_id on login from 0 to None refactor user_extension dao with EAFP try catch

mtreacy002 · 2020-07-29T07:40:14Z

Update @ramitsawhney27 and @anitab-org/bridgeintech-maintainers . I've just pushed the requested changes. Can you please re-review? Thanks

foongminwong

The changes made in this PR were tested locally. Following are the results:

Code review - Done
All possible responses were tested as below:
- PR 81
  Screenshot/gif/url:
  
  Expected Result: As a user, I should be able to update my additional information.
  Actual Result: Same as expected
- Running PR81 test cases
  Screenshot/gif/url:
  
  Expected Result: Test cases are run successfully.
  Actual Result: Same as expected
OS Version: Windows 10

mtreacy002 requested review from ramitsawhney27 and a team July 15, 2020 13:50

mtreacy002 self-assigned this Jul 15, 2020

mtreacy002 added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Program: GSOC Related to work completed during the Google Summer of Code Program. labels Jul 15, 2020

mtreacy002 added this to the GSoc Coding Phase 2 milestone Jul 15, 2020

ramitsawhney27 previously approved these changes Jul 16, 2020

View reviewed changes

mtreacy002 dismissed ramitsawhney27’s stale review via bacaf74 July 18, 2020 13:15

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from 7daafbb to bacaf74 Compare July 18, 2020 13:15

mtreacy002 added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Jul 19, 2020

mtreacy002 changed the title ~~Feat: Issue#80 PUT /user/additional_info api endpoints and test cases~~ WIP | Feat: Issue#80 PUT /user/additional_info api endpoints and test cases Jul 19, 2020

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from bacaf74 to 144c12e Compare July 20, 2020 01:06

mtreacy002 requested review from ramitsawhney27 and a team July 20, 2020 01:06

mtreacy002 changed the title ~~WIP | Feat: Issue#80 PUT /user/additional_info api endpoints and test cases~~ Feat: Issue#80 PUT /user/additional_info api endpoints and test cases Jul 20, 2020

mtreacy002 removed the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Jul 20, 2020

mtreacy002 force-pushed the issue#80-update-user-additional-info branch 3 times, most recently from 3fe155a to cfcfa93 Compare July 21, 2020 04:38

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from cfcfa93 to 504b2c4 Compare July 26, 2020 01:03

mtreacy002 changed the title ~~Feat: Issue#80 PUT /user/additional_info api endpoints and test cases~~ Feat: Issue#80 PUT /user/additional_info api endpoints and test cases [DO NOT MERGE] Jul 26, 2020

mtreacy002 added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Jul 26, 2020

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from 504b2c4 to 2b9d33a Compare July 26, 2020 06:45

mtreacy002 changed the title ~~Feat: Issue#80 PUT /user/additional_info api endpoints and test cases [DO NOT MERGE]~~ Feat: Issue#80 PUT /user/additional_info api endpoints and test cases Jul 26, 2020

mtreacy002 removed the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Jul 26, 2020

isabelcosta reviewed Jul 27, 2020

View reviewed changes

isabelcosta requested a review from foongminwong July 27, 2020 19:52

ramitsawhney27 suggested changes Jul 29, 2020

View reviewed changes

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from 2b9d33a to 5a792f6 Compare July 29, 2020 07:30

Feat: PUT /user/additional_info api endpoint and test cases

2b766fe

Allow user to input Timezone in enum value Fix bug user id not retrieved Refactor reset user_id on login from 0 to None refactor user_extension dao with EAFP try catch

mtreacy002 force-pushed the issue#80-update-user-additional-info branch from 5a792f6 to 2b766fe Compare July 29, 2020 07:39

ramitsawhney27 approved these changes Jul 29, 2020

View reviewed changes

foongminwong approved these changes Jul 31, 2020

View reviewed changes

foongminwong merged commit fb52e21 into anitab-org:develop Jul 31, 2020

mtreacy002 deleted the issue#80-update-user-additional-info branch August 30, 2020 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Issue#80 PUT /user/additional_info api endpoints and test cases #81

Feat: Issue#80 PUT /user/additional_info api endpoints and test cases #81

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 20, 2020

mtreacy002 commented Jul 25, 2020 •

edited

Loading

isabelcosta commented Jul 25, 2020

mtreacy002 commented Jul 26, 2020

mtreacy002 commented Jul 26, 2020 •

edited

Loading

isabelcosta Jul 27, 2020

mtreacy002 Jul 28, 2020 •

edited

Loading

ramitsawhney27 Jul 29, 2020

mtreacy002 Jul 29, 2020

ramitsawhney27 Jul 29, 2020

mtreacy002 Jul 29, 2020

ramitsawhney27 Jul 29, 2020

mtreacy002 commented Jul 29, 2020

foongminwong left a comment

Feat: Issue#80 PUT /user/additional_info api endpoints and test cases #81

Feat: Issue#80 PUT /user/additional_info api endpoints and test cases #81

Conversation

mtreacy002 commented Jul 15, 2020

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 20, 2020

mtreacy002 commented Jul 25, 2020 • edited Loading

isabelcosta commented Jul 25, 2020

mtreacy002 commented Jul 26, 2020

mtreacy002 commented Jul 26, 2020 • edited Loading

isabelcosta Jul 27, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

ramitsawhney27 Jul 29, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 29, 2020

Choose a reason for hiding this comment

ramitsawhney27 Jul 29, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 29, 2020

Choose a reason for hiding this comment

ramitsawhney27 Jul 29, 2020

Choose a reason for hiding this comment

mtreacy002 commented Jul 29, 2020

foongminwong left a comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 25, 2020 •

edited

Loading

mtreacy002 commented Jul 26, 2020 •

edited

Loading

mtreacy002 Jul 28, 2020 •

edited

Loading