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

GH integration and express interest #156

Closed
wants to merge 3 commits into from
Closed

GH integration and express interest #156

wants to merge 3 commits into from

Conversation

mbeacom
Copy link
Contributor

@mbeacom mbeacom commented Dec 21, 2017

Description

The goal of this PR is to provide rough github integration to validate Profile handles and email addresses to ensure we have a valid email address to send tips/notifications. Additionally, this PR implements Express Interest functionality that will add users to a list of users that have expressed interest (displayed on the bounty page) and comments on the github issue with a list of user handles and links back to their gitcoin profile pages.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

Tips and Bounties

Refers/Fixes

Ref: #62
Ref: #98

@codecov
Copy link

codecov bot commented Dec 21, 2017

Codecov Report

Merging #156 into master will increase coverage by 1.53%.
The diff coverage is 18.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   12.21%   13.74%   +1.53%     
==========================================
  Files          66       67       +1     
  Lines        3120     3282     +162     
  Branches      343      366      +23     
==========================================
+ Hits          381      451      +70     
- Misses       2739     2831      +92
Impacted Files Coverage Δ
app/app/urls.py 0% <ø> (ø) ⬆️
app/dashboard/apps.py 0% <0%> (ø) ⬆️
app/dashboard/helpers.py 0% <0%> (ø) ⬆️
app/dashboard/views.py 0% <0%> (ø) ⬆️
...pp/economy/management/commands/refresh_bounties.py 0% <0%> (ø) ⬆️
app/economy/models.py 76.66% <100%> (ø) ⬆️
app/dashboard/notifications.py 7.72% <11.11%> (+7.72%) ⬆️
app/app/github.py 26.82% <20.31%> (+26.82%) ⬆️
app/dashboard/models.py 42.59% <41.66%> (+4.48%) ⬆️
app/dashboard/signals.py 58.33% <58.33%> (ø)
... and 3 more

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 417b692...040e018. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Dec 22, 2017

good progress! submitting my comments now

@@ -92,6 +92,7 @@ GITHUB_CLIENT_ID = 'TODO'
GITHUB_CLIENT_SECRET = 'TODO'
GITHUB_API_USER = 'TODO'
GITHUB_API_TOKEN = 'TODO'
GITHUB_APP_NAME = 'TODO'
Copy link
Contributor

Choose a reason for hiding this comment

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

GITHUB_API_BASE_URL
GITHUB_SCOPE

these should probably go in the .dist, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

GITHUB_APP_NAME too

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 was considering GITHUB_SCOPE, GITHUB_API_BASE_URL, and GITHUB_TOKEN_URL to be more repo specific than local dev specific. Realistically, I could eliminate the api/token urls since they shouldn't change. Scope would only change if the project as a whole was changing to require more permissions from the user (so a dev would want to commit that change in order to allow any added functionality to work for everyone). Do you want me to move them?

} else {
var entry = {
href: '/interested',
text: 'Express Interest',
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like if i am not authenticated, this (and remove interest above) should present the user with a dialog asking them to login, no? http://bits.owocki.com/3z0K3v0r3c1c/Screen%20Recording%202017-12-22%20at%2012.26%20PM.gif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! I'll push a fix for this shortly.

@@ -337,12 +340,30 @@ window.addEventListener('load', function() {
actions.push(entry);
if(result['status']=='open' && !isBountyOwner(result) ){
var entry = {
href: '/funding/claim?source='+result['github_url'],
href: '/_github/auth?redirect_uri=/funding/claim?source=' + result['github_url'],
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be great if the username was prefilled after i got to this page http://bits.owocki.com/2M3U3m3n0k29/Screen%20Shot%202017-12-22%20at%2012.32.53%20PM.png

Copy link
Contributor

Choose a reason for hiding this comment

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

(claim page)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an oversight on my part. I'll add that.

var entry = {
href: '/uninterested',
text: 'Remove Interest',
parent: 'right_actions',
Copy link
Contributor

Choose a reason for hiding this comment

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

two things

  1. it'd be great if 'interested profiles updated AJAX'ily when i added / removed my interest
  2. not sure how much thought was put into the design here.. ( i can ask someone who does design to look into it if we want).. but maybe for now we can just remove the list bullet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I didn't add any method of updating the view with the current user after they express interest... Sorry about that.

return 'https://api.github.com/repos/' + url_path

def fetch_issue_item(self, item_type='body'):
"""Fetch the item type of an issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice little refactor :)

@@ -75,6 +75,22 @@ <h5>Claimee Info</h5>
Github Profile: <span id="claimee_github_username"></span>

</div>
<div id="interest_list">
<br>
<h5>Interested Profiles</h5>
Copy link
Contributor

Choose a reason for hiding this comment

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

if we ever decide to do this with javascript, theres some examples if you search text/x-jsrender

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I tend to drink the django kool-aid when it comes to handling data interaction with the template.

@@ -55,11 +55,11 @@ <h1>Send Tip.</h1>
</div>
<div>
From Name (optional):
<input type="text" placeholder="Bobby Tables" id="fromName" value="" style="width: 400px; margin-left: auto; margin-right: auto;">
<input type="text" placeholder="Bobby Tables" id="fromName" value="{% if from_handle %}{{ from_handle }}{% endif %}" style="width: 400px; margin-left: auto; margin-right: auto;">
Copy link
Contributor

Choose a reason for hiding this comment

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

strangely enough.. i cant get this information to prepopulate after i auth with github.. wonder what im doing wrong

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'll take a look when I'm going the refactor and include setup steps/gifs of functionality.

@@ -40,8 +44,53 @@
confirm_time_minutes_target = 3


def send_tip(request):
@require_GET
def github_callback(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

i haven't been horribly consistent about this myself... so take with a grain of salt.. but it could be worht moving all the github auth views into their own view file

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 agree. I originally added an app specifically for this, but figured it should conform to the rest of the project for the moment. 😂 I'll move it over to it's own app.

@owocki
Copy link
Contributor

owocki commented Dec 27, 2017

donno if you're on the grid this week (some are, some arent) so no pressure if not.. but im antsy to see this move forward! let me know when to expect another pass

@mbeacom
Copy link
Contributor Author

mbeacom commented Dec 27, 2017

@owocki The holidays have set me back on this a bit. Mostly been keeping updated on GH from my phone, but I should have the updates in today. I'll ping you once they're in.

@owocki owocki mentioned this pull request Dec 28, 2017
@owocki
Copy link
Contributor

owocki commented Dec 28, 2017

kk np

@owocki
Copy link
Contributor

owocki commented Jan 2, 2018

happy new year! let me know what your new schedule looks like here...

also, mind resolving the merge conflcits?

@kamescg
Copy link

kamescg commented Jan 8, 2018

@owocki thanks for sharing the link in the Slack focus-dev-general channel

Want to join the discussion and open up the possibility of using uPort and self-sovereign identity systems as the consistent identifier within the Gitcoin ecosystem.

Additionally, we could have participants "attest" to their Github account, make a clear link between self-sovereign identity and third-parties (github, twitter, etc...), but still maintaining a decentralized foundation.

The added benefit of using uPort might be easily associated wallet address, verifiable voting capabilities, and additional features like badges and accomplishments.

@owocki
Copy link
Contributor

owocki commented Jan 12, 2018

Want to join the discussion and open up the possibility of using uPort and self-sovereign identity systems as the consistent identifier within the Gitcoin ecosystem.

its something we should consider.. but is probably out of scope of this PR @kamescg -- could you start another issue and lets discuss what youre thinking!?

@owocki
Copy link
Contributor

owocki commented Jan 15, 2018

per https://gitcoincommunity.slack.com/archives/C7A4F4UHK/p1515876720000097 i wonder if we should rename 'express interest' into claim work and the current claim functionality to 'claim funds'.

Claim Work: you’ve claimed it and going to do it
Claim Bounty: like a bounty hunter you take the bounty to find the person and bring them in. Also you might not be able to finish it, for whatever reason so you “sell” it to someone else, or you need help so you split the bounty with another hunter to help. So I think in a deeper thought and probably stretching for meaning here lol I’m going with Claim Bounty

@owocki
Copy link
Contributor

owocki commented Jan 16, 2018

closing this as it's been moved over to january2018_feature_integration

@owocki owocki closed this Jan 16, 2018
@owocki
Copy link
Contributor

owocki commented Jan 17, 2018

hey @mbeacom it looks like most of my feedback above isnt turned around.. do you want to move it over to https://github.com/gitcoinco/web/projects/1 or just turn it around on the new branch?

@mbeacom
Copy link
Contributor Author

mbeacom commented Jan 17, 2018

@owocki I moved the remaining issues over to the project yesterday evening. I'll move any additional notes on the new PR over to GH project cards.

@mbeacom mbeacom deleted the gh-int-express-int branch March 2, 2018 06:03
owocki pushed a commit that referenced this pull request Apr 30, 2021
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