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.

Sign up for GitHub

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

Jump to bottom

Refactor GH issue details endpoint #1780

Merged
merged 6 commits into from
Jul 18, 2018
Merged

Refactor GH issue details endpoint #1780

merged 6 commits into from
Jul 18, 2018

Conversation

mbeacom
Copy link
Contributor

@mbeacom mbeacom commented Jul 18, 2018 •
edited
Loading

Description

The goal of this PR is to introduce pygithub and refactor the dashboard.helpers.issue_details view.

  • Removes BS4 for finding keywords
  • Slims up the response from get_issue_details
  • Drops the requests to GH significantly
  • Fixes bug in get_issue_details where /pull urls wouldn't return results, but rather an error.
  • Removes some dead imports
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

dashboard helpers, issue details

Testing

Locally

Refers/Fixes

Ref #1772

@mbeacom mbeacom added the backend This needs backend expertise. label Jul 18, 2018
@mbeacom mbeacom self-assigned this Jul 18, 2018
@ghost ghost added the in progress label Jul 18, 2018
stickler-ci
stickler-ci reviewed Jul 18, 2018
View reviewed changes
app/avatar/models.py Outdated
@@ -29,7 +29,7 @@
from django_extensions.db.fields import AutoSlugField
from easy_thumbnails.fields import ThumbnailerImageField
from economy.models import SuperModel
from github.utils import get_user
from git.utils import get_user
Copy link

Choose a reason for hiding this comment

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

F401 'git.utils.get_user' imported but unused

app/dashboard/helpers.py Outdated
@@ -38,7 +38,7 @@
)
from dashboard.tokens import addr_to_token
from economy.utils import convert_amount
from github.utils import _AUTH
from git.utils import _AUTH, get_gh_issue_details, get_url_dict, github_connect, issue_number, org_name, repo_name
Copy link

Choose a reason for hiding this comment

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

F401 'git.utils._AUTH' imported but unused
F401 'git.utils.github_connect' imported but unused
F401 'git.utils.issue_number' imported but unused
F401 'git.utils.org_name' imported but unused
F401 'git.utils.repo_name' imported but unused

app/dashboard/tip_views.py Outdated
@@ -32,7 +32,7 @@
from dashboard.utils import generate_pub_priv_keypair, get_web3, has_tx_mined
from dashboard.views import record_user_action
from gas.utils import recommend_min_gas_price_to_confirm_in_time
from github.utils import (
from git.utils import (
Copy link

Choose a reason for hiding this comment

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

F401 'git.utils.get_auth_url' imported but unused
F401 'git.utils.get_github_user_data' imported but unused
F401 'git.utils.is_github_token_valid' imported but unused

app/dashboard/views.py Outdated
@@ -42,7 +42,7 @@
from avatar.utils import get_avatar_context
from economy.utils import convert_amount
from gas.utils import conf_time_spread, gas_advisories, gas_history, recommend_min_gas_price_to_confirm_in_time
from github.utils import (
from git.utils import (
Copy link

Choose a reason for hiding this comment

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

F401 'git.utils.get_github_emails' imported but unused
F401 'git.utils.get_github_primary_email' imported but unused

app/gitcoinbot/management/commands/get_notifications.py Outdated
@@ -22,7 +22,7 @@
from django.core.management.base import BaseCommand
from django.utils import timezone

from github.utils import (
from git.utils import (
Copy link

Choose a reason for hiding this comment

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

F401 'git.utils.post_issue_comment' imported but unused

@codecov
Copy link

codecov bot commented Jul 18, 2018 •
edited
Loading

Codecov Report

Merging #1780 into master will decrease coverage by 0.28%.
The diff coverage is 27.58%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1780      +/-   ##
=========================================
- Coverage   28.88%   28.6%   -0.29%     
=========================================
  Files         128     128              
  Lines       10073   10051      -22     
  Branches     1326    1324       -2     
=========================================
- Hits         2910    2875      -35     
- Misses       7057    7075      +18     
+ Partials      106     101       -5
Impacted Files Coverage Δ
app/app/settings.py 81.14% <ø> (ø) ⬆️
app/avatar/models.py 42.85% <ø> (-0.74%) ⬇️
app/git/apps.py 0% <0%> (ø)
app/marketing/management/commands/pull_github.py 0% <0%> (ø) ⬆️
app/marketing/management/commands/sync_github.py 0% <0%> (ø) ⬆️
...itcoinbot/management/commands/get_notifications.py 0% <0%> (ø) ⬆️
...ies/management/commands/sync_known_github_repos.py 0% <0%> (ø) ⬆️
app/marketing/stats.py 0% <0%> (ø) ⬆️
app/git/utils.py 53.7% <10.71%> (ø)
app/dashboard/views.py 15.23% <100%> (ø) ⬆️
... and 13 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 fadd6e0...c86f3e0. Read the comment docs.

SaptakS
SaptakS previously approved these changes Jul 18, 2018
View reviewed changes
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

LGTM

thelostone-mc
thelostone-mc reviewed Jul 18, 2018
View reviewed changes
app/dashboard/helpers.py
response['title'] = response['title']

keywords = []

Copy link
Member

Choose a reason for hiding this comment

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

🔥

thelostone-mc
thelostone-mc previously approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

🔥 🔥

@mbeacom
Fix isort
f88955e 
Signed-off-by: Mark Beacom <markvbeacom@gmail.com>
@mbeacom mbeacom added this to the July 2018 milestone Jul 18, 2018
thelostone-mc
thelostone-mc previously approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution @mbeacom

@PixelantDesign
Copy link
Contributor

Great! I'll close out 1772

@ghost ghost added the in progress label Jul 18, 2018
@mbeacom mbeacom merged commit 38780fe into master Jul 18, 2018
@ghost ghost removed the in progress label Jul 18, 2018
@mbeacom mbeacom deleted the issue-details-sync-refactor branch July 18, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stickler-ci stickler-ci stickler-ci left review comments

@thelostone-mc thelostone-mc thelostone-mc left review comments

@SaptakS SaptakS SaptakS left review comments

@owocki owocki Awaiting requested review from owocki

Assignees

@mbeacom mbeacom

Labels
backend This needs backend expertise.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mbeacom @PixelantDesign @thelostone-mc @SaptakS @stickler-ci