Skip to content
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

Invite users to a bounty based on skills #5060

Merged
merged 20 commits into from
Sep 19, 2019
Merged

Conversation

danlipert
Copy link
Contributor

@danlipert danlipert commented Aug 23, 2019

Description

image

Refers/Fixes

#5045

Testing

Tested locally with all filters, getting emails with invites perfectly according to the query

@thelostone-mc
Copy link
Member

left a comment over at #5048 (comment)

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1cb0949). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5060   +/-   ##
=========================================
  Coverage          ?   18.57%           
=========================================
  Files             ?      206           
  Lines             ?    15985           
  Branches          ?     2162           
=========================================
  Hits              ?     2969           
  Misses            ?    13006           
  Partials          ?       10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cb0949...45dc8ad. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #5060 into master will decrease coverage by 0.03%.
The diff coverage is 37.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5060      +/-   ##
==========================================
- Coverage   30.78%   30.75%   -0.04%     
==========================================
  Files         221      221              
  Lines       17829    17958     +129     
  Branches     2464     2530      +66     
==========================================
+ Hits         5489     5523      +34     
- Misses      12114    12195      +81     
- Partials      226      240      +14
Impacted Files Coverage Δ
app/app/settings.py 78.96% <ø> (ø) ⬆️
app/app/urls.py 89.36% <ø> (ø) ⬆️
app/dashboard/views.py 14.23% <37.28%> (-0.06%) ⬇️
app/app/middleware.py 41.17% <0%> (-35.3%) ⬇️
...rketing/management/commands/no_applicants_email.py 0% <0%> (ø) ⬆️
app/marketing/mails.py 17.01% <0%> (+2.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f5a548...2b69f46. Read the comment docs.

app/dashboard/views.py Outdated Show resolved Hide resolved
app/dashboard/views.py Show resolved Hide resolved
@octavioamu octavioamu marked this pull request as ready for review September 18, 2019 23:51
@octavioamu
Copy link
Contributor

@danlipert @thelostone-mc When reviewing check the users_fetch_filters also, for some reason github is not showing diff there, but the def was introduced in this PR

else:
profile_list = Profile.objects.prefetch_related(
'fulfilled', 'leaderboard_ranks', 'feedbacks_got'
).exclude(hide_profile=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh why did we move this from def users_fetch_filters ? to keep that more generic ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be DRY instead of copy paste the same filters, both users filters and bulk invite user users_fetch_filters to filter and have the same result

return;
}
vm.issueDetails = undefined;
let getIssue = fetchData(apiUrldetails, 'GET');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ?

},
getIssueDetails: function(url) {
let vm = this;
let apiUrldetails = `/actions/api/v0.1/bounties/?github_url=${encodeURIComponent(url)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

Copy link
Contributor Author

@danlipert danlipert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few comments and some potential fix we'll have to make in the future

@@ -1145,6 +1164,77 @@ def social_contribution_modal(request):
return TemplateResponse(request, 'social_contribution_modal.html', params)


@csrf_exempt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add the @staff_member_required decorator here to restrict access?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm can be, Im restricting it using request.user.is_staff, seems to me there are plans to do this a feature for users so not sure if worst changing it.

bounty_invite.bounty.add(bounty)
bounty_invite.inviter.add(inviter)
bounty_invite.invitee.add(profile.user)
# emails.append(profile.email)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover line

# emails.append(profile.email)
try:
msg = request.POST.get('msg', '')
share_bounty([profile.email], msg, inviter.profile, invite_url, False)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how many emails we can send within the request-response window before the request times out? I'm guessing this will be somewhat buggy, may have to refactor to use celery soon but for now we'll see since its internal only.

@thelostone-mc thelostone-mc merged commit 4731117 into master Sep 19, 2019
@thelostone-mc thelostone-mc deleted the skill-based-invites branch June 27, 2020 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants