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

[WIP] brainstorming for /vote fast + prototype [WILL NOT BE UN-WIP-ED] #493

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions cron/poll_read_issue_comments.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,31 @@ def get_seconds_remaining(api, comment_id):
return seconds_remaining


def insert_or_update_issue(api, issue_id, number):
# get more info on the issue
gh_issue = gh.issue.open_issue(api, settings.URN, number)

# get user from db
user, _ = User.get_or_create(user_id=gh_issue["user"]["id"],
defaults={"login": gh_issue["user"]["login"]})

# db insert
issue, _ = Issue.get_or_create(issue_id=issue_id,
number=number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know what the difference between these is? Can I drop one of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend using the defaults param here for anything not in the primary key. We were having issues using this syntax before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better?

user=user,
created_at=gh_issue["created_at"],
expedited=False,
is_pr="pull_request" in gh_issue)

return issue


def insert_or_update(api, cmd_obj):
# Find the comment, or create it if it doesn't exit
comment_id = cmd_obj["global_comment_id"]
user, _ = User.get_or_create(user_id=cmd_obj["user"]["id"],
defaults={"login": cmd_obj["user"]["login"]})
issue, _ = Issue.get_or_create(issue_id=cmd_obj["issue_id"],
user=user, # TODO: I don't think this is the right one
created_at=cmd_obj["created_at"], # TODO: I don't think this is the right one
expedited=False)

issue = insert_or_update_issue(api, cmd_obj["issue_id"], cmd_obj["number"])
Copy link
Contributor

Choose a reason for hiding this comment

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

where are you getting cmd_obj["number"] from? I don't think this exists

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 think it ought to be just returned from github, right: https://developer.github.com/v3/issues/#list-issues-for-a-repository?

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Read comments.py / get_all_issue_comments. It doesn't return the entire structure. You might need to change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. Fixed

comment, _ = Comment.get_or_create(comment_id=comment_id,
defaults={
"user": user, "text": cmd_obj["comment_text"],
Expand Down Expand Up @@ -170,6 +185,10 @@ def handle_vote_command(api, command, cmdmeta, votes):
issue_poster = cmdmeta.issue.user
issue_created_at = cmdmeta.issue.created_at

# This must be a PR
if not cmdmeta.issue.is_pr:
return

# The post should not have been updated
if updated_at != created_at:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe leave some kind of response indicating why the command didn't run? Makes debugging easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... yes, I see your point... but would this be prone to inducing spam?

return
Expand Down
2 changes: 2 additions & 0 deletions lib/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ class Comment(BaseModel):

class Issue(BaseModel):
issue_id = pw.IntegerField(primary_key=True)
number = pw.IntegerField()
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR number as oppose to PR id? The thing is when github returns a PR, it returns a value called "id" which is not the PR number we see on github, and it returns a "number" which is the number we see on github. TBH, I am really confused about which to use where. I think we want to use issue/PR numbers, rather than ids, but the db models appear to use the ids. Any help/auditing on this point would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ids are used when making api calls. I think that's why we store them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... interesting. Most of the API I saw for issues and PRs take the issue/PR number, though?

created_at = pw.DateField()
user = pw.ForeignKeyField(User, related_name='poster')
is_pr = pw.BooleanField()
expedited = pw.BooleanField()


Expand Down