anitab-org / mentorship-backend

Mentorship System is an application that matches women in tech to mentor each other, on career development, through 1:1 relations during a certain period of time. This is the backend of this system.

https://mentorship-backend-temp.herokuapp.com/

GNU General Public License v3.0

196 stars 449 forks source link

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

Open aakankshaagr opened 3 years ago

aakankshaagr commented 3 years ago

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[bot] commented 3 years ago

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%)`	:arrow_up:

aakankshaagr commented 3 years ago

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

aakankshaagr commented 3 years ago

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

aakankshaagr commented 3 years ago

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

devkapilbansal commented 3 years ago

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 commented 3 years ago

Sorry for the inconvenience

devkapilbansal commented 3 years ago

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

vj-codes commented 3 years ago

@aakankshaagr any updates about this PR?

aakankshaagr commented 3 years ago

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

devkapilbansal commented 3 years ago

@aakankshaagr make changes in this line https://github.com/anitab-org/mentorship-backend/blob/c154317b266ffb258f6a7b2ab5ea95de45c5de81/tests/users/test_dao_update_user.py#L30

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

aakankshaagr commented 3 years ago

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

isabelcosta commented 3 years ago

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 :/