improved code of user.py with dict.get() and suggestions #1124

aakankshaagr · 2021-06-20T11:05:21Z

Description

Fixes #1065

Type of Change:

Code
Quality Assurance

How Has This Been Tested?

Tested on my local pc

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
Any dependent changes have been published in downstream modules

codecov · 2021-06-20T11:13:11Z

Codecov Report

Merging #1124 (6f6df83) into develop (c154317) will increase coverage by 1.12%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1124      +/-   ##
===========================================
+ Coverage    93.25%   94.38%   +1.12%     
===========================================
  Files           38       38              
  Lines         2062     2029      -33     
===========================================
- Hits          1923     1915       -8     
+ Misses         139      114      -25

Impacted Files	Coverage Δ
app/api/dao/user.py	`95.32% <100.00%> (+9.49%)`	⬆️

aakankshaagr · 2021-06-20T11:23:00Z

@devkapilbansal I have rebased my branch to master and have made all changes suggested by you. Please review my changes

epicadk · 2021-06-21T11:44:22Z

app/api/dao/user.py

-        password = data["password"]
-        email = data["email"]
-        terms_and_conditions_checked = data["terms_and_conditions_checked"]
+        name = data.get("name", None)


None is default right? I don't think you need this here

epicadk · 2021-06-21T11:44:35Z

app/api/dao/user.py

            user.username = username

-        if "name" in data and data["name"]:
-            user.name = data["name"]
+        user.name = data.get("name", None)


This will raise a key error if name entered is not found that's why none is required here

epicadk · 2021-06-21T11:45:20Z

app/api/dao/user.py

@@ -39,11 +39,11 @@ def create_user(data: Dict[str, str]):
            A tuple with two elements. The first element is a dictionary containing a key 'message' containing a string which indicates whether or not the user was created successfully. The second is the HTTP response code.
        """

-        name = data["name"]


Also please use a different branch for each pr. You've used develop currently, this may cause issues in the future.

devkapilbansal

@aakankshaagr please remove None as asked above. I remember asking for this in previous PR too. Also, please try to refrain from closing PR and opening a new one. You can ask for help if you stuck.

P.S.:- Please don't use default branch name from next time.

aakankshaagr · 2021-06-21T13:39:06Z

Sure @devkapilbansal @epicadk I will keep these things in mind.

aakankshaagr · 2021-06-21T14:51:18Z

Shall I merge all commit in a single commit as I haven't created a new branch ?

devkapilbansal · 2021-07-03T11:55:03Z

app/api/dao/user.py

            user.username = username

-        if "name" in data and data["name"]:
-            user.name = data["name"]
+        user.name = data.get("name", None)


If you use data["name"], then KeyError will be raised, not with data.get

Suggested change

user.name = data.get("name", None)

user.name = data.get("name")

devkapilbansal · 2021-07-03T11:55:11Z

app/api/dao/user.py

-                user.bio = data["bio"]
-            else:
-                user.bio = None
+        user.bio = data.get("bio", None)


Same as above

devkapilbansal · 2021-07-03T11:56:05Z

app/api/dao/user.py

-                user.organization = data["organization"]
-            else:
-                user.organization = None
+        user.organization = data.get("organization", None) or None


Same as above, same for other cases as well

Suggested change

user.organization = data.get("organization", None) or None

user.organization = data.get("organization")

As I have told earlier, on removing none in organization and occupation will fail test cases.

devkapilbansal · 2021-07-03T11:57:37Z

Shall I merge all commit in a single commit as I haven't created a new branch ?

No, you don't need to. Just keep this in mind from next time

aakankshaagr · 2021-07-04T15:22:33Z

Sorry for the inconvenience

devkapilbansal · 2021-07-06T20:25:41Z

@aakankshaagr there are some suggestions above. Once done, this PR may get completed. Look into them once you have some time.

vj-codes · 2021-07-24T14:00:07Z

@aakankshaagr any updates about this PR?

aakankshaagr · 2021-07-25T12:37:02Z

@devkapilbansal @vj-codes I have made changes as you have asked but on removing none in organization and occupation test cases are failing

devkapilbansal · 2021-07-25T21:35:38Z

@aakankshaagr make changes in this line

mentorship-backend/tests/users/test_dao_update_user.py

Line 30 in c154317

data = dict(occupation="", organization="")

Pass occupation and organization as None here. Then your tests will not fail

aakankshaagr · 2021-07-26T15:28:57Z

Thanks, @devkapilbansal. I thought I can't edit the test cases and have to code such that the test passes.

devkapilbansal

LGTM 👍
Thanks a lot @aakankshaagr

vj-codes

@aakankshaagr nice work 🎉
Do remember to commit the changes on a new branch other than develop while working on future issues as mentioned in the community guidelines:)

isabelcosta

Great work! Thank you for contributing @aakankshaagr 🤗

isabelcosta · 2021-07-29T22:20:23Z

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

Code review - Done
All possible responses (positive and negative tests) were tested as below:
- Update my user's location
  Screenshot/gif:
  
  my location was null
  
  I updated it
  
  then the location changed
  
  Expected Result: Only Location should change from "null" to "UK"
  Actual Result: When 1 field is sent and other fields are not being sent, it is setting the user attributes as None anyways. This is a breaking change 🤔
Additional Comments: tested at https://ms-backend-review-pr-1124.herokuapp.com/ This will break the current functionality.
Status of PR Changed to: Changes requested :/

isabelcosta

This changes the functionality of the API at the moment because if we don't send one of the values, these will be set with None 🤔 Could we find a better way to avoid setting these as null.
This also reminds me of the PUT vs PATCH usage in this project right?

cc @anitab-org/mentorship-maintainers

aakankshaagr added 2 commits June 20, 2021 16:03

enhanced code of user.py with dict.get()

4091169

reformatted users.py with black

ec66d78

aakankshaagr requested a review from isabelcosta as a code owner June 20, 2021 11:05

epicadk suggested changes Jun 21, 2021

View reviewed changes

epicadk requested a review from devkapilbansal June 21, 2021 11:45

epicadk added the Status: Changes Requested Changes are required to be done by the PR author. label Jun 21, 2021

devkapilbansal suggested changes Jun 21, 2021

View reviewed changes

aakankshaagr added 2 commits June 21, 2021 18:58

added sugesstions to users.py

29bf220

reformatted with black

747d858

removed keyerror

e706c05

aakankshaagr requested a review from epicadk June 21, 2021 14:48

devkapilbansal suggested changes Jul 3, 2021

View reviewed changes

Merge branch 'anitab-org:develop' into develop

66550c9

aakankshaagr and others added 2 commits July 25, 2021 17:03

Merge branch 'anitab-org:develop' into develop

02b982e

removed None as suggested

b5e5781

edited tests and user.py

6f6df83

devkapilbansal approved these changes Jul 26, 2021

View reviewed changes

devkapilbansal added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jul 26, 2021

vj-codes approved these changes Jul 29, 2021

View reviewed changes

vj-codes added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jul 29, 2021

isabelcosta approved these changes Jul 29, 2021

View reviewed changes

isabelcosta temporarily deployed to ms-backend-review-pr-1124 July 29, 2021 22:05 Inactive

isabelcosta added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jul 29, 2021

isabelcosta reviewed Jul 29, 2021

View reviewed changes

isabelcosta self-requested a review July 29, 2021 22:25

vj-codes added Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Aug 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved code of user.py with dict.get() and suggestions #1124

improved code of user.py with dict.get() and suggestions #1124

aakankshaagr commented Jun 20, 2021

codecov bot commented Jun 20, 2021 •

edited

Loading

aakankshaagr commented Jun 20, 2021

epicadk Jun 21, 2021

epicadk Jun 21, 2021

aakankshaagr Jun 21, 2021

epicadk Jun 21, 2021

devkapilbansal left a comment

aakankshaagr commented Jun 21, 2021

aakankshaagr commented Jun 21, 2021

devkapilbansal Jul 3, 2021

devkapilbansal Jul 3, 2021

devkapilbansal Jul 3, 2021

aakankshaagr Jul 25, 2021

devkapilbansal commented Jul 3, 2021

aakankshaagr commented Jul 4, 2021

devkapilbansal commented Jul 6, 2021

vj-codes commented Jul 24, 2021

aakankshaagr commented Jul 25, 2021

devkapilbansal commented Jul 25, 2021 •

edited

Loading

aakankshaagr commented Jul 26, 2021

devkapilbansal left a comment

vj-codes left a comment

isabelcosta left a comment

isabelcosta commented Jul 29, 2021

isabelcosta left a comment

	user.name = data.get("name", None)
	user.name = data.get("name")

	user.organization = data.get("organization", None) or None
	user.organization = data.get("organization")

improved code of user.py with dict.get() and suggestions #1124

Are you sure you want to change the base?

improved code of user.py with dict.get() and suggestions #1124

Conversation

aakankshaagr commented Jun 20, 2021

Description

Type of Change:

How Has This Been Tested?

Checklist:

codecov bot commented Jun 20, 2021 • edited Loading

Codecov Report

aakankshaagr commented Jun 20, 2021

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

devkapilbansal left a comment

Choose a reason for hiding this comment

aakankshaagr commented Jun 21, 2021

aakankshaagr commented Jun 21, 2021

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

devkapilbansal commented Jul 3, 2021

aakankshaagr commented Jul 4, 2021

devkapilbansal commented Jul 6, 2021

vj-codes commented Jul 24, 2021

aakankshaagr commented Jul 25, 2021

devkapilbansal commented Jul 25, 2021 • edited Loading

aakankshaagr commented Jul 26, 2021

devkapilbansal left a comment

Choose a reason for hiding this comment

vj-codes left a comment

Choose a reason for hiding this comment

isabelcosta left a comment

Choose a reason for hiding this comment

isabelcosta commented Jul 29, 2021

isabelcosta left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2021 •

edited

Loading

devkapilbansal commented Jul 25, 2021 •

edited

Loading