Feat: Issue#76 POST and GET /user/additional_info api endpoints #77

mtreacy002 · 2020-07-13T15:46:01Z

Description

Add POST and GET user/additional_info endpoints to create and get additional information of a user.

Fixes #76

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?

Steps to test functionalities:
For GET

run the app and go to swagger ui
login as existing user and get the access_token
go to GET /user/personal_details and get user details by providing token (this is a step that MUST be done before user can access their additional information page) since with this user_id can be reetrieved and saved as cookie.
go to GET /user/additional_info and provide the data.
if user is new and has not yet added their additional information, it should return 404 Not Found.

go to POST /user/additional_info to create additional information. Provide token and data in the relevant fields.
NOTE
- timezone value must be a string enum value of Timezone enum from bitschema_utils.py
- phone and mobile must follow the phone_regex pattern under app/utils/validation_utils.py
- no validation is currently placed for personal_website (icebox: can be a nice-to-have feature)
  if the response is successful, you should see the following
To confirm, go to GET /user//additional_information. Provide token in the specified area. The following is an example of successful request:

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials

Code/Quality Assurance Only

My changes generate no new warnings
New and existing unit tests pass locally with my changes

mtreacy002 · 2020-07-13T15:50:16Z

Update @anitab-org/bridgeintech-maintainers. I've just completed POST and GET /user/additional_info. I have no option but to put this 2 together as to have successful GET example, the additional information of the user must be created first. I put notes on how to test the functionality in this PR description above.
Note that the test cases for these functionalities will be dealt separately as this PR alreeady too big as it is.

ramitsawhney27 · 2020-07-13T15:54:58Z

app/api/dao/user_extension.py

        timezone = data["timezone"]
+
+        additional_info_data = {


Keep this as a default dict instead of initializing with empty strings

done. now default dict {}

ramitsawhney27 · 2020-07-13T15:56:38Z

app/api/dao/user_extension.py

+        if existing_additional_info:
+            return messages.ADDITIONAL_INFORMATION_OF_USER_ALREADY_EXIST, HTTPStatus.CONFLICT
+        user_extension = UserExtensionModel(user_id, timezone)
+        if "is_organization_rep" in data:


Use try except KeyError rather than if else to initialize dict elements
https://www.google.com/amp/s/www.geeksforgeeks.org/try-except-vs-if-in-python/amp/
EAFP principle

got it. Thanks for the tips. Learned something new here. 😁. Changed to try-except.

ramitsawhney27 · 2020-07-13T15:57:23Z

app/api/request_api_utils.py

-        logging.fatal(f"{response_message}")
-        return response_message, response_code
-
+        return ()


Why does this return an empty tuple?

reemoved. now simply ends, not returning anything if valid

ramitsawhney27 · 2020-07-13T15:57:50Z

app/api/resources/users.py

+        is_wrong_token = validate_token(token)
+
+        if not is_wrong_token:
+


Line spacing seems off

Got it. Fixed now 👍

mtreacy002

@ramitsawhney27 . I've just made the requested changes. Please re-review. Thanks for the feedback.

mtreacy002 · 2020-07-13T23:29:22Z

app/api/resources/users.py

+        is_wrong_token = validate_token(token)
+
+        if not is_wrong_token:
+


Got it. Fixed now 👍

mtreacy002 · 2020-07-13T23:29:46Z

app/api/request_api_utils.py

-        logging.fatal(f"{response_message}")
-        return response_message, response_code
-
+        return ()


reemoved. now simply ends, not returning anything if valid

mtreacy002 · 2020-07-13T23:31:04Z

app/api/dao/user_extension.py

+        if existing_additional_info:
+            return messages.ADDITIONAL_INFORMATION_OF_USER_ALREADY_EXIST, HTTPStatus.CONFLICT
+        user_extension = UserExtensionModel(user_id, timezone)
+        if "is_organization_rep" in data:


got it. Thanks for the tips. Learned something new here. 😁. Changed to try-except.

mtreacy002 · 2020-07-13T23:31:18Z

app/api/dao/user_extension.py

        timezone = data["timezone"]
+
+        additional_info_data = {


done. now default dict {}

ramitsawhney27

Approved

ramitsawhney27 · 2020-07-14T08:43:52Z

app/database/models/bit_schema/user_extension.py

@@ -2,7 +2,7 @@
 from sqlalchemy.dialects.postgresql import JSONB
 from app.database.sqlalchemy_extension import db
 from app.utils.bitschema_utils import Timezone
-
+# from app.database.models.ms_schema.user import UserModel


Is this needed

hehehe.... sorry... will remove it. 😊

mtreacy002 · 2020-07-14T09:02:00Z

@ramitsawhney27 I've removed the comment and pushed force again. Can you pls re-approved? Thanks.

foongminwong · 2020-07-14T17:29:06Z

Hey @mtreacy002 , are you able to resolve the conflicting files? I'm having some problem testing this PR (might be caused by those conflicting files hmm) let me know if you have any questions

mtreacy002 · 2020-07-14T23:07:41Z

@foongminwong , Done. Those merge conflict happens coz of the new merge to develop. It'll happen often in this dev since I carry across codes from PR to next ones. So, I have to fix it when the previous PR finally merge. But not to worry, it's an easy fix.
Hope this won't give you trouble anymore. Ta

foongminwong · 2020-07-15T16:34:37Z

This PR is tested locally and failed.

Error: File "C:\Users\foongmin\Desktop\bridge-in-tech-backend\app\api\dao\user_extension.py", line 60, in get_user_additional_data_info "user_id": result.user_id, AttributeError: 'NoneType' object has no attribute 'user_id'

@mtreacy002 hmm I followed your steps by running GET /user/personal_details first before GET /user/additional_info, as shown below:

Question:

Does the id on GET /user/personal_details correspond to the user_id on GET /user/additional_info?
Do you encounter this error before?

mtreacy002 · 2020-07-15T23:31:32Z

@foongminwong . Just curious. Which MS backend server did you run for this test? Do you use the same server that was used in PR #74 ? From 74 onwards we should use that ms-bitschema-backend-server or ms-backend-server from my fork repo as MS backend server. I worked on this PR running ms-bitschema-backend-server but actually prefer to start using ms-backend-server since this is the one I pushed yesterday to merge with MS develop branch.
If you already using the ms-bitschema-backend-server then maybe there's a problem somewhere else. Are you available for a catch up session? We can trooubleshoot it together. Let me know when.

mtreacy002 · 2020-07-15T23:52:17Z

To answer your questions:

Does the id on GET /user/personal_details correspond to the user_id on GET /user/additional_info?

Yes, it does.

Do you encounter this error before?

I have never encountered this issue before... 🤔

mtreacy002 · 2020-07-15T23:56:50Z

Another thing, @foongminwong . Your MS and BIT servers both are connected to the same db, right? bit_schema db?

mtreacy002 · 2020-07-16T00:50:52Z

@foongminwong . I spotted from your gif that you still have the userr details with organization field instead of the current_organization. The the db schema has changed as mentioned on PR#74 comment.
You should have this in GET /user/personal_details response body

This means you need to drop all tables that you currently have on bit_schema database and recreate them by running the app again python run.py (noticed that there's also no migrations folder anymore on bit backend repo with the recent PR).
Let me know how you go.

mtreacy002 · 2020-07-16T01:11:59Z

Not only updating the database, you should also pull the latest code for ms-bitschema-backend-server (or better, ms-backend-server) from my fork repo coz both the user schema and the api response body for user model has changed since that PR#74.

Again, I recommend to use the ms-backend-server for this as this is the one I'm planning to maintain moving forward.

isabelcosta · 2020-07-18T10:18:00Z

tests/users/test_api_get_user_details.py

+    @patch("requests.get")
+    @patch("requests.post")
+    def test_api_get_user_details_with_correct_token(self, mock_login, mock_get_user):
+        success_message = {"access_token": "this is fake token", "access_expiry": 1601478236}


Can you add a comment about that access_expiry? Is that past? future, what is the date represented there?

done, @isabelcosta . Now also with changed commit message as we discussed in the meeting before 😉

mtreacy002 · 2020-07-20T01:43:56Z

Additional error response 403 forbidden operation
Screenshots:
403 error when user has not retrieved their id using GET /user/personal_details

before sending GET /user/additional_info request

before sending POST /user/additional_info request

mtreacy002 · 2020-07-21T01:46:28Z

Update @anitab-org/bridgeintech-maintainers . I've added app reviews pipeline to BIT Heroku for your convenience.

This way if you want you can test the features of proposed PR without having to run the server locally.

When I have time (I'm not promising this will be asap since you can always pull my PR and run the servers locally). If I do make the push on the open PR currently being reviewed, I'll let you know.

Here's this PR Heroku test PR:
https://bit-developm-heroku-iss-fkp29j.herokuapp.com

@meenakshi-dhanani and @anitab-org/bridgeintech-maintainers team, if you want to test frontend features using the matching heroku backend PR, you can do this by adding the Heroku test PR link above to the config file yourself.

@isabelcosta , I think with Heroku free account, there is a limit to how many app we can put to the PR review pipeline. Bcoz when I tried to create this Review app PR before, it complained saying I have reach my limit of Heroku apps so I deleted the old heroku bridge-in-tech-bit-test server (the one connected to elephantsql, not heroku psql) to give way to this heroku test pr. Just so you are aware this might also apply to MS heroku pipeline.

mtreacy002 · 2020-07-21T01:56:00Z

PS: It'll be ready soon. I'll need to adjust something. Will ping you when I'm done

Add comment for human readable dates on access_expiry Allow timezone enum take from and return value to user Add check to make sure user_id is retrieved Add http error response 403 on post request Improve 403 error message to remove ambiguity Add other on some enum class

mtreacy002 · 2020-07-21T02:14:24Z

Ok, now it's ready... Go ahead and test it out 😉. Just added myself as first user who'll be the admin by default 😁

mtreacy002 · 2020-07-21T02:21:37Z

Note @anitab-org/bridgeintech-maintainers . I've refactored enum type to add others in some of enum types, so if you testing locally, drop all existing tables and its types (under bitschema > Types directory) and run the app again to recreate the db.
No need to do anything on Heroku PR app as I've done this for Heroku db

mtreacy002 · 2020-07-21T09:11:10Z

PS: The heroku servers might nor work as expected. I'm sttill figuring things out, it still best to use local servers imo

mtreacy002 · 2020-07-21T13:13:12Z

Update. It's too hard to try make the Review app work for PRs ( have to set another db and another ms backend server, plus have to do this for every open PR). So, I leave it aside for now. Theere is a develop branch already running on Heroku for both BIT and MS-for-BIT if you want to try out the merged PR. But for open/under review PRs, unfortunately, you just have to run local servers to test PRs.
Just want to let you know @anitab-org/bridgeintech-maintainers

foongminwong

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

Code review - Done
All possible responses were tested as below (tap/zoom in to see clearer gifs):

Test Case	GIF	Expected Result	Actual Result
GET /user/additional_info		As a user, I should be able to get my personal details.	✅
POST /user/additional_info		As a user, I should be able to create/update my personal details.	🔄

Additional comments:

hey @mtreacy002 , when I'm trying to do POST /user/additional_info, got a 500 Internal Server Error which shows ValueError: 'CENTRAL_STANDARD_TIME' is not a valid Timezone. Did you encounter this error before?

{
  "is_organization_rep": true,
  "timezone": "CENTRAL_STANDARD_TIME",
  "phone": "608-608-6008",
  "mobile": "",
  "personal_website": "http://habibo.com"
}

OS Version: Windows 10

mtreacy002 · 2020-07-22T16:05:46Z

@foongminwong . Sorry, I forgot to change the steps to tests on the description of this PR (now I've changed it). Based on mentors suggestions on the last BIT weekly meeting, I strictly limited enum logic to backend. This means frontend user (or backend swagger ui) can straight select the enum value instead of enum name. so in your case, you can change the Timezone input to "UTC-06:00/Central Standard Time" for central standard time. This will be the same case on enums in personal background PRs

foongminwong · 2020-07-22T21:44:52Z

@foongminwong . Sorry, I forgot to change the steps to tests on the description of this PR (now I've changed it). Based on mentors suggestions on the last BIT weekly meeting, I strictly limited enum logic to backend. This means frontend user (or backend swagger ui) can straight select the enum value instead of enum name. so in your case, you can change the Timezone input to "UTC-06:00/Central Standard Time" for central standard time. This will be the same case on enums in personal background PRs

Ok @mtreacy002 , just changed it to:

{
  "is_organization_rep": true,
  "timezone": "UTC-06:00/Central Standard Time",
  "phone": "608-608-6008",
  "mobile": "",
  "personal_website": "http://habibo.com"
}

hmm I got a following error:

sqlalchemy.exc.IntegrityError: (psycopg2.errors.ForeignKeyViolation) insert or update on table "users_extension" violates foreign key constraint "fk_users_extension_user_id_users"

DETAIL:  Key (user_id)=(4) is not present in table "users".

 [SQL: 'INSERT INTO bitschema.users_extension (user_id, is_organization_rep, additional_info, timezone) VALUES (%(user_id)s, %(is_organization_rep)s, %(additional_info)s, %(timezone)s) RETURNING bitschema.users_extension.id'] [parameters: {'user_id': '4', 'is_organization_rep': True, 'additional_info': '{"phone": "608-608-6008", "mobile": "", "personal_website": "http://habibo.com"}', 'timezone': 'CENTRAL_STANDARD_TIME'}] (Background on this error at: http://sqlalche.me/e/gkpj)

I wonder why Key (user_id)=(4) is not present in table "users". when tested locally. I am able to login and get the personal details for user habibo with id=4 that means the user should be in the users database 🤔

mtreacy002 · 2020-07-22T22:59:20Z

@foongminwong . May I ask which MS server you're running with this? If possible, can we meet on hangout so we can troubleshoot this together?

PS: Can you please also add as part as point 4 on your comment above which MS branch you're running the server from? Thanks

foongminwong · 2020-07-22T23:02:50Z

@foongminwong . May I ask which MS server you're running with this? If possible, can we meet on hangout so we can troubleshoot this together?

Yes, I'm running on mentorship-backend/bit-ms-backend-server branch. let me know what time that works for you meeting on google hangout

mtreacy002 · 2020-07-22T23:03:54Z

@foongminwong . May I ask which MS server you're running with this? If possible, can we meet on hangout so we can troubleshoot this together?

Yes, I'm running on mentorship-backend/bit-ms-backend-server branch. let me know what time that works for you meeting on google hangout

Are you available to go quickly now or is it too late at your time?

mtreacy002 · 2020-07-22T23:05:21Z

Just curious, have you dropped and recreate the db as I previously suggested?

mtreacy002 · 2020-07-22T23:06:01Z

BTW, are you already back in Malaysia or still at US timezone @foongminwong ?

foongminwong · 2020-07-22T23:06:30Z

@foongminwong . May I ask which MS server you're running with this? If possible, can we meet on hangout so we can troubleshoot this together?

Yes, I'm running on mentorship-backend/bit-ms-backend-server branch. let me know what time that works for you meeting on google hangout

Are you available to go quickly now or is it too late at your time?

yes, just sent you an invite, i'm still in US, packing 😂

mtreacy002 · 2020-07-22T23:07:43Z

I haven't receive the invite? Where did you send it to? email or Zulip?

foongminwong · 2020-07-22T23:10:04Z

I haven't receive the invite? Where did you send it to? email or Zulip?

sent invitation on Google Hangouts, also shared the link on Zulip

foongminwong · 2020-07-23T01:00:05Z

@mtreacy002 helped fixed the error in my local environment!! 🙌

Problems found + Fixes:

Problem: The MS and BIT database are not connected to each other. The local search_path is incorrect:
Fix: Run this command psql -c "ALTER DATABASE bit_schema SET search_path TO bitschema,public;" -U postgres -d bit_schema . Then, run the following command after the database alteration psql -c 'show search_path;' -U postgres -d bit_schema
Fix: We removed all the existing Tables and Types on pgAdmin in order to repopulate the latest tables. After removing all the tables and types, please open BIT backend first by going to localhost:5000 to repopulate the correct tables, instead of MS backend localhost:4000 (second)

Fix: Make sure you are running on ms-backend-server

Commands:

$ git checkout -b bit-ms-backend-server develop
$ git pull https://github.com/mtreacy002/mentorship-backend.git ms-backend-server

To-do:

Add the instructions to BIT backend Wiki about how to run the specific MS server to make sure MS and BIT connect to the same database (similar to pr43)

mtreacy002 · 2020-07-23T02:42:57Z

Update @ramitsawhney27 , currently there's a bug in the existing code (opened issue #94 to report this). Do you want me to deal with it in this PR or as separate issue (since this PR been hold up for a while now). Also should I fix the bug using this PR code base or the latest open PR code base (PR #86 ) PUT/personal_background because fixing it with the first PR on the queue means having to rebase from the next PR up to the latest one on the queue which means removing all the approvals from mentors on the PR (while we're on that topic, can you please re-review and re-approved this PR, as the latest force pushed removed your previous approval 😁). Whereas tackling it from the last PR on the queue (PR #86) will have no effect to the one before that)

isabelcosta

Good work @mtreacy002 👏

ramitsawhney27 suggested changes Jul 13, 2020

View reviewed changes

mtreacy002 self-assigned this Jul 13, 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 13, 2020

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

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch from a227411 to ced7814 Compare July 13, 2020 23:35

mtreacy002 commented Jul 13, 2020

View reviewed changes

mtreacy002 requested a review from ramitsawhney27 July 13, 2020 23:36

mtreacy002 changed the title ~~Feeat: Issue#76 POST and GET /user/additional_info api endpoints~~ Feat: Issue#76 POST and GET /user/additional_info api endpoints Jul 14, 2020

ramitsawhney27 previously approved these changes Jul 14, 2020

View reviewed changes

mtreacy002 dismissed ramitsawhney27’s stale review via 6aae1d9 July 14, 2020 09:00

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch from ced7814 to 6aae1d9 Compare July 14, 2020 09:00

mtreacy002 requested a review from ramitsawhney27 July 14, 2020 09:01

ramitsawhney27 previously approved these changes Jul 14, 2020

View reviewed changes

mtreacy002 requested a review from foongminwong July 14, 2020 09:11

mtreacy002 mentioned this pull request Jul 14, 2020

Issue#78 test cases for post get additional info #79

Merged

6 tasks

mtreacy002 dismissed ramitsawhney27’s stale review via b4dfee5 July 14, 2020 23:03

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch from 6aae1d9 to b4dfee5 Compare July 14, 2020 23:03

isabelcosta reviewed Jul 18, 2020

View reviewed changes

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch from b4dfee5 to 7432e4a Compare July 18, 2020 13:02

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch 3 times, most recently from f3d5c78 to b210624 Compare July 20, 2020 02:46

mtreacy002 force-pushed the issue#76-get-user-additional-info-api branch from b210624 to 41b015d Compare July 21, 2020 02:03

foongminwong reviewed Jul 22, 2020

View reviewed changes

foongminwong approved these changes Jul 23, 2020

View reviewed changes

isabelcosta approved these changes Jul 25, 2020

View reviewed changes

isabelcosta merged commit 06ab5c9 into anitab-org:develop Jul 25, 2020

mtreacy002 mentioned this pull request Jul 25, 2020

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

Merged

7 tasks

mtreacy002 deleted the issue#76-get-user-additional-info-api branch August 30, 2020 11:47

		is_wrong_token = validate_token(token)

		if not is_wrong_token:

Feat: Issue#76 POST and GET /user/additional_info api endpoints #77

Feat: Issue#76 POST and GET /user/additional_info api endpoints #77

Conversation

mtreacy002 commented Jul 13, 2020 • edited Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramitsawhney27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 14, 2020

foongminwong commented Jul 14, 2020

mtreacy002 commented Jul 14, 2020

foongminwong commented Jul 15, 2020

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 15, 2020

mtreacy002 commented Jul 16, 2020 • edited Loading

mtreacy002 commented Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 20, 2020

mtreacy002 commented Jul 21, 2020 • edited Loading

mtreacy002 commented Jul 21, 2020

mtreacy002 commented Jul 21, 2020 • edited Loading

mtreacy002 commented Jul 21, 2020 • edited Loading

mtreacy002 commented Jul 21, 2020 • edited Loading

mtreacy002 commented Jul 21, 2020 • edited Loading

foongminwong left a comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 22, 2020 • edited Loading

foongminwong commented Jul 22, 2020 • edited Loading

mtreacy002 commented Jul 22, 2020 • edited Loading

foongminwong commented Jul 22, 2020

mtreacy002 commented Jul 22, 2020

mtreacy002 commented Jul 22, 2020

mtreacy002 commented Jul 22, 2020 • edited Loading

foongminwong commented Jul 22, 2020

mtreacy002 commented Jul 22, 2020 • edited Loading

foongminwong commented Jul 22, 2020

foongminwong commented Jul 23, 2020 • edited Loading

mtreacy002 commented Jul 23, 2020

isabelcosta left a comment

Choose a reason for hiding this comment

mtreacy002 commented Jul 13, 2020 •

edited

Loading

mtreacy002 commented Jul 16, 2020 •

edited

Loading

mtreacy002 commented Jul 16, 2020 •

edited

Loading

mtreacy002 commented Jul 21, 2020 •

edited

Loading

mtreacy002 commented Jul 21, 2020 •

edited

Loading

mtreacy002 commented Jul 21, 2020 •

edited

Loading

mtreacy002 commented Jul 21, 2020 •

edited

Loading

mtreacy002 commented Jul 21, 2020 •

edited

Loading

mtreacy002 commented Jul 22, 2020 •

edited

Loading

foongminwong commented Jul 22, 2020 •

edited

Loading

mtreacy002 commented Jul 22, 2020 •

edited

Loading

mtreacy002 commented Jul 22, 2020 •

edited

Loading

mtreacy002 commented Jul 22, 2020 •

edited

Loading

foongminwong commented Jul 23, 2020 •

edited

Loading