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

display last 3 achievements #1134

Closed diananova closed 3 years ago

diananova commented 3 years ago

Description

The achievements list retrieved from /home endpoint retrieves the latest three achievements, instead of the first three.

Fixes #393

Type of Change:

Code
Quality Assurance

Code/Quality Assurance Only

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I added a new test in test_api_home_statistics.py
I also tested it manually by creating 5 tasks (and set them to complete). The result at GET /home should be the last three achievements (ex. id 5,4,3 instead of 1,2,3).

Checklist:

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

Code/Quality Assurance Only

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

diananova commented 3 years ago

I'm not sure why the build/coverage report is failing. It doesn't seem related to my changes. I'm also getting a different error locally. Local: FAILED tests/mentorship_relation/test_dao_creation.py::TestMentorshipRelationCreationDAO::test_dao_create_mentorship_relation_with_good_args_but_invalid_timestamp Github: FAILED tests/task_comments/test_api_get_task_comments.py::TestGetTaskCommentsApi::test_task_comment_listing_api_with_task_not_existing

epicadk commented 3 years ago

I think you need to make a change to the existing test.

codecov[bot] commented 3 years ago

Codecov Report

Merging #1134 (8d4fe3b) into develop (4193589) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1134   +/-   ##
========================================
  Coverage    92.91%   92.91%           
========================================
  Files           38       38           
  Lines         2062     2062           
========================================
  Hits          1916     1916           
  Misses         146      146

Impacted Files	Coverage Δ
app/api/dao/user.py	`85.82% <100.00%> (ø)`

diananova commented 3 years ago

@epicadk @isabelcosta Ready for review. I didn't make any changes, the tests are flaky.

epicadk commented 3 years ago

@epicadk @isabelcosta Ready for review. I didn't make any changes, the tests are flaky.

Which one exactly? This seems to be the first time it's happened.

diananova commented 3 years ago

@epicadk @isabelcosta Ready for review. I didn't make any changes, the tests are flaky.

Which one exactly? This seems to be the first time it's happened.

tests/task_comments/test_api_get_task_comments.py::TestGetTaskCommentsApi::test_task_comment_listing_api_with_task_not_existing. It's an assertion error (we get code 403 instead of 404). Might have been a one time problem.

isabelcosta commented 3 years ago

Interesting 🤔 I saw your changes yesterday, and thought you fixed the tests when you added str(i) while concatenating a string. The coverage report steps have been failing at times. So I would not worry about that, this should be an issue to fix aside from your PR @diananova :)

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:
- GET /home endpoint
  Screenshot/gif: left (this branch deployed) / right (develop branch deployed)
  
  Expected Result: tasks returned are the oldest ones Actual Result: tasks returned are the most recent ones
Additional Comments: Tested on https://ms-backend-review-pr-1134.herokuapp.com/
Status of PR Changed to: Ready to Merge.

devkapilbansal commented 3 years ago

I'm not sure why the build/coverage report is failing. It doesn't seem related to my changes. I'm also getting a different error locally. Local: FAILED tests/mentorship_relation/test_dao_creation.py::TestMentorshipRelationCreationDAO::test_dao_create_mentorship_relation_with_good_args_but_invalid_timestamp Github: FAILED tests/task_comments/test_api_get_task_comments.py::TestGetTaskCommentsApi::test_task_comment_listing_api_with_task_not_existing

@diananova consider opening an issue for both errors including the build id in which they occur. If tests are flaky, we need to fix them too. :thinking: