-
-
Notifications
You must be signed in to change notification settings - Fork 775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sorts user directory by previously worked and leaderboard #4475
Sorts user directory by previously worked and leaderboard #4475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small style suggestions but otherwise looks good
app/dashboard/views.py
Outdated
bounties_completed = request.GET.get('bounties_completed', '').strip().split(',') | ||
leaderboard_rank = request.GET.get('leaderboard_rank', '').strip().split(',') | ||
rating = int(request.GET.get('rating', '0')) | ||
organisation = request.GET.get('organisation', '') | ||
|
||
user_id = request.GET.get('user', None) | ||
if user_id: | ||
profile = Profile.objects.get(id=int(user_id)) | ||
profile = User.objects.get(id=int(user_id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets not call an instance of a User
object profile
so we can avoid confusion here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. makes sense. It's only that in the loop below we were using the variable user
to loop through the user list. I will rename this as current_user
app/dashboard/views.py
Outdated
|
||
context = {} | ||
if not settings.DEBUG: | ||
network = 'mainnet' | ||
else: | ||
network = 'rinkeby' | ||
|
||
user_list = Profile.objects.prefetch_related('fulfilled', 'leaderboard_ranks', 'feedbacks_got').order_by(order_by) | ||
|
||
user_list = Profile.objects.prefetch_related( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly here, can we can this profile_list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I mean it's called User Directory. So I thought user_list is good. 🤔
Codecov Report
@@ Coverage Diff @@
## master #4475 +/- ##
=========================================
- Coverage 30.08% 29.8% -0.29%
=========================================
Files 209 209
Lines 16850 16848 -2
Branches 2267 2266 -1
=========================================
- Hits 5070 5022 -48
- Misses 11582 11645 +63
+ Partials 198 181 -17
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #4475 +/- ##
==========================================
- Coverage 30.02% 30.02% -0.01%
==========================================
Files 209 209
Lines 16884 16886 +2
Branches 2277 2278 +1
==========================================
Hits 5070 5070
- Misses 11618 11619 +1
- Partials 196 197 +1
Continue to review full report at Codecov.
|
e21cef6
to
5866371
Compare
@SaptakS looks like Travis is failing for these changes |
app/dashboard/views.py
Outdated
previously_worked_with = BountyFulfillment.objects.filter( | ||
bounty__bounty_owner_github_username__iexact=profile.handle, | ||
bounty__bounty_owner_github_username__iexact=current_user.username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use the username here instead of handle? If we are expecting a github username, then I think handle should be used, since non-github users could have username that is someone else's github handle (theoretically, at least...)
https://github.com/gitcoinco/web/blob/master/app/dashboard/models.py#L2352
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the built-in User model in django has only username and no handle column, hence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaptakS I understand that, but we can easily access the profile from the current_user
object, right? So why not use the handle? Is this a performance tradeoff?
e4a0a28
to
c664e60
Compare
Description
Refers/Fixes
Fixes #4398
Testing