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

backend db & API support to show how many comments exist for an issue #219

Merged
merged 8 commits into from
Jan 15, 2018

Conversation

amites
Copy link
Contributor

@amites amites commented Jan 6, 2018

Description
  • add support to bounties API for github comment count
  • add function to Bounty model to pull github comments
  • extend refresh_bounties command to pull github comments & update
Checklist
  • linter status: 100% pass
    • Didn't touch anything in nodejs land, new code would pass pylint -- existing gonna take some effort to clean -- separate PR
  • changes don't break existing behavior
  • commit message follows [commit guidelines]
Affected core subsystem(s)
  • dashboard
  • bounty API
Refers/Fixes

Works on: #206

- add function to Bounty model to pull github comments
- extend refresh_bounties command to pull github comments & update

Refs: gitcoinco#206
@@ -45,12 +45,15 @@ def search(q):
return response.json()


def get_issue_comments(owner, repo):
def get_issue_comments(owner, repo, issue=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't find any existing usages of this function

method signature remains consistent but allows for retrieving comments of a single issue

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Some of the functionality has changed in the GH integration PR.

@@ -200,6 +200,10 @@
SECURE_HSTS_PRELOAD = True
SECURE_HSTS_SECONDS = 3600

# List of github usernames to not count as comments on an issue
IGNORE_COMMENTS_FROM = ['gitcoinbot', ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

put this here to make it easy to override

Copy link
Contributor

Choose a reason for hiding this comment

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

this is very nice.. thanks

name='github_comments',
field=models.IntegerField(default=0),
),
migrations.AlterField(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added help_text to 2 of the fields -- keeping them here to avoid generating extra migrations

@@ -252,6 +254,30 @@ def fetch_issue_description(self):
if body:
self.issue_description = body

def fetch_issue_comments(self, save=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

uses the app.github method to pull comments via API vs parsing html

Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed in: #156

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure which PR will land first, easy enough to pull your changes in but I'll wait till this has had a 2nd review

digging what I see in #156

@amites amites changed the title Provides the backend db & API support for showing how many comments exist for a given issue backend db & API support to show how many comments exist for an issue Jan 6, 2018
@amites
Copy link
Contributor Author

amites commented Jan 6, 2018

Travis CI failures are unrelated to this PR -- have a question about process to fix in Slack

https://gitcoincommunity.slack.com/archives/C7BQHUPAN/p1515202126000159

@@ -252,6 +254,30 @@ def fetch_issue_description(self):
if body:
self.issue_description = body

def fetch_issue_comments(self, save=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed in: #156

github_user, github_repo, _, github_issue = parsed_url.path.split('/')[1:5]
except ValueError:
# TODO: update print statements to logger
print('Invalid 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.

😉

Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if it's worth logging and error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably, the repo isn't setup for using the built-in logger that I saw -- easy enough to add but was aiming for a clean PR to make it easy to review

Copy link
Contributor

Choose a reason for hiding this comment

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

there is some of the built in logging system being done in notifications.py ... not sure if that qualifies or helps or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

want me to throw a logging config into settings?

pretty much copy and paste, can dial it in later but will open up using logging.getLogger

@@ -45,12 +45,15 @@ def search(q):
return response.json()


def get_issue_comments(owner, repo):
def get_issue_comments(owner, repo, issue=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Some of the functionality has changed in the GH integration PR.

@owocki
Copy link
Contributor

owocki commented Jan 8, 2018

this looks good ...

as @mbeacom notes.. well have to figure out how to merge it with his work on the 'express interest' pr. donno if thatll get merged first or this will.

cc @ethikz who has been doing some of the frontend stuff

@owocki
Copy link
Contributor

owocki commented Jan 12, 2018

hey @amites can you please merge the frontend code into this branch? looking fwd to getting it live

@amites
Copy link
Contributor Author

amites commented Jan 12, 2018

I'll start building it out, @ethikz had a couple snippets but far from a complete solution

wasn't sure what was in mind for process

@amites
Copy link
Contributor Author

amites commented Jan 15, 2018

updated UI -- #156 is still open to hesitant to pull in changes

another example of a task from github _ tester funded issue detail _ gitcoin _ gitcoin

issue explorer _ gitcoin

issue explorer _ gitcoin 1

@owocki
Copy link
Contributor

owocki commented Jan 15, 2018

thanks. screenshots look good! i will review once the mobile UI stuff has been committed

@amites
Copy link
Contributor Author

amites commented Jan 15, 2018

ended up fixing the mobile UI last night -- merge conflicts

@@ -650,8 +650,12 @@ input[type=text].loading {
width: 45%;
display: inline-block;
}
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

@amites merge conflict :)

background: url(/static/v2/images/icon-msg-red-16.png) no-repeat left transparent;
}
body.bounty_details a.btn-darkGrey{
=======
Copy link
Contributor

Choose a reason for hiding this comment

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

a few more down here too

@owocki
Copy link
Contributor

owocki commented Jan 15, 2018

pulled down the code and ran ./manage.py refresh_bounties --remote but didnt see the UI updated at all... maybe because of the merge conflicts? what else do i need to do to test?

@owocki
Copy link
Contributor

owocki commented Jan 15, 2018

just tested this again after chatting with @amites on slack and it LGTM... @mbeacom 👍 from you?

expires_date = models.DateTimeField()
raw_data = JSONField()
metadata = JSONField(default={})
claimee_metadata = JSONField(default={})
current_bounty = models.BooleanField(default=False) # whether this bounty is the most current revision one or not
current_bounty = models.BooleanField(default=False,
help_text='whether this bounty is the most current revision one or not')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor nitpick... Should we move forward with a standard of capitalizing the first letter in help_text or keep it all lowercase?

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Other than the comment pertaining to the superfluous help_text question, LGTM! :shipit:

@amites
Copy link
Contributor Author

amites commented Jan 15, 2018

nice nit-pick for consistency -- updated

@owocki owocki merged commit 1fed033 into gitcoinco:master Jan 15, 2018
ethikz pushed a commit to ethikz/web that referenced this pull request Jan 24, 2018
backend db &  API support to show how many comments exist for an issue
thelostone-mc added a commit that referenced this pull request May 24, 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