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

Suggested Contributor #4137

Merged
merged 37 commits into from
Apr 24, 2019
Merged

Suggested Contributor #4137

merged 37 commits into from
Apr 24, 2019

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Apr 8, 2019

Description

Suggested Contributor

Checklist
Affected core subsystem(s)
Refers/Fixes
Testing and Sign-off
Contributor
  • Read and followed the Contributor Guidelines
  • Tested all changes locally
  • Verified existing functionality
  • Ran make test and everything passed!
Reviewer
  • Affirm contributor guidelines have been followed and requested changes made
  • CI tests and linting pass
  • No conflicts (migrations, files, etc)
  • Regression tested against staging or local deployment
Funder
  • Validated requested changes were made to specification
  • Bounty payout released to the contributor

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #4137 into master will decrease coverage by <.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4137      +/-   ##
==========================================
- Coverage   30.26%   30.25%   -0.01%     
==========================================
  Files         206      206              
  Lines       16361    16365       +4     
  Branches     2154     2155       +1     
==========================================
+ Hits         4951     4952       +1     
- Misses      11234    11237       +3     
  Partials      176      176
Impacted Files Coverage Δ
app/dashboard/views.py 12.25% <25%> (+0.04%) ⬆️

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 67c3a5d...d82ac5f. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #4137 into master will decrease coverage by 0.06%.
The diff coverage is 7.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4137      +/-   ##
==========================================
- Coverage   30.19%   30.13%   -0.07%     
==========================================
  Files         209      209              
  Lines       16655    16692      +37     
  Branches     2224     2230       +6     
==========================================
+ Hits         5029     5030       +1     
- Misses      11445    11481      +36     
  Partials      181      181
Impacted Files Coverage Δ
app/app/urls.py 90% <ø> (ø) ⬆️
app/dashboard/helpers.py 14.17% <0%> (-0.9%) ⬇️
app/dashboard/views.py 13.64% <13.33%> (-0.08%) ⬇️
app/marketing/mails.py 11.52% <50%> (ø) ⬆️

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 53fc46b...06b0453. Read the comment docs.

@@ -295,7 +384,8 @@ $(document).ready(function() {
repo_type: data.repo_type,
featuring_date: data.featuredBounty && ((new Date().getTime() / 1000) | 0) || 0,
reservedFor: reservedFor ? reservedFor.text : '',
tokenName
tokenName,
invite: inviteContributors
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sending an array of usernames ['octavioamu', 'saptak']
I'm suppose you are going to get this data to make the email sending.

.order_by('-fulfillment_count')

keywords_filter = Q()
for keyword in keywords:
Copy link
Contributor

Choose a reason for hiding this comment

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

I had changed this query to work with more than one keyword and also changed the front end to send is as param, since is a GET and there is no body data.

Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

@SaptakS I made some changes so left some explanation in the comments.

@SaptakS SaptakS changed the title [WIP] Suggested Contributor Suggested Contributor Apr 16, 2019
octavioamu
octavioamu previously approved these changes Apr 17, 2019
$.ajax(settings).done(function(response) {
let groups = {
'contributors': 'Recently worked with you',
'recommended_developers': 'Recomended based on skills',
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here Recomended -> Recommended

Copy link
Contributor

@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 pretty good! I loved the demo last week - can we get this finished up for the deploy this week? I know some folks are really excited to get this into production.

@@ -504,6 +504,41 @@ def create_new_bounty(old_bounties, bounty_payload, bounty_details, bounty_id):
except Exception as e:
logger.error(e)

bounty_invitees = metadata.get('invite', '')
print (bounty_invitees)
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover print statement

bounty_invite.inviter.add(inviter.user)
bounty_invite.invitee.add(profile.user)
emails.append(profile.email)
print(emails)
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover prints here

emails = []
inviter = Profile.objects.get(handle=new_bounty.bounty_owner_github_username)
invite_url = get_bounty_invite_url(inviter, new_bounty.id)
msg = "Check out this bounty that pays out" + \
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy with this copy here? @PixelantDesign @frankchen07 - I think the formatting here needs a space between the word out and the bounty value.

app/dashboard/helpers.py Show resolved Hide resolved
@SaptakS
Copy link
Contributor Author

SaptakS commented Apr 23, 2019

@danlipert addressed your concerns. Please review.

@thelostone-mc thelostone-mc requested a review from danlipert April 24, 2019 13:29
danlipert
danlipert previously approved these changes Apr 24, 2019
@@ -38,6 +38,94 @@ function doShowQuickstart(url) {
return true;
}

var procesedData;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: procesedData should be processedData

@danlipert danlipert merged commit f700ddb into master Apr 24, 2019
@thelostone-mc thelostone-mc deleted the suggest-contributor branch July 4, 2019 14:57
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