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

Fixes #1153 - Adding new fields to issues.db and webhooks #1174

Closed
wants to merge 42 commits into
from

Conversation

Projects
None yet
5 participants
@deepthivenkat
Member

deepthivenkat commented Sep 11, 2016

No description provided.

@deepthivenkat

This comment has been minimized.

Show comment
Hide comment
Member

deepthivenkat commented Sep 11, 2016

Show outdated Hide outdated webcompat/db/__init__.py
def __init__(self, issue_id, summary, url, body):
def __init__(self, issue_id, summary, url, domain, body, status, reported_from, creation_time, last_change_time):

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

line is too long

@karlcow

karlcow Sep 14, 2016

Contributor

line is too long

Show outdated Hide outdated webcompat/db/__init__.py
self.body = body
self.reported_from = reported_from
self.creation_time = datetime.datetime.strptime(creation_time.split('Z')[0],"%Y-%m-%dT%H:%M:%S")
self.last_change_time = datetime.datetime.strptime(last_change_time.split('Z')[0],"%Y-%m-%dT%H:%M:%S")

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

lines are too long

@karlcow

karlcow Sep 14, 2016

Contributor

lines are too long

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

I have the feeling (testing locally) that you just can do:

self.last_change_time = datetime.datetime.strptime(last_change_time,"%Y-%m-%dT%H:%M:%SZ") 

The Z will not be recorded in sqlite.

@karlcow

karlcow Sep 14, 2016

Contributor

I have the feeling (testing locally) that you just can do:

self.last_change_time = datetime.datetime.strptime(last_change_time,"%Y-%m-%dT%H:%M:%SZ") 

The Z will not be recorded in sqlite.

if domain.lower().startswith('www.'):
domain = domain.split('www.')[1]
port_number = re.search(r':\d+$', domain)
if port_number is not None:

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

in Python it is not necessary to this construct. The if will execute only if the value is not None. see for example. so you can directly use the value.

>>> import urlparse
>>> domain = 'http://www.org:5000'
>>> url = 'http://www.org:5000/foobar?blah'
>>> urlparse.urlparse(url)
ParseResult(scheme='http', netloc='www.org:5000', path='/foobar', params='', query='blah', fragment='')
>>> domain = urlparse.urlparse(url).netloc
>>> import re
>>> port_number = re.search(r':\d+$', domain)
>>> port_number
<_sre.SRE_Match object at 0x1097712a0>
>>> url2 = 'http://www.org/foobar?blah'
>>> domain2 = urlparse.urlparse(url2).netloc
>>> port_number2 = re.search(r':\d+$', domain2)
>>> port_number2
>>> if port_number:
...     print 'do something'
... 
do something
>>> if port_number2:
...     print 'do something'
... 
>>> 
@karlcow

karlcow Sep 14, 2016

Contributor

in Python it is not necessary to this construct. The if will execute only if the value is not None. see for example. so you can directly use the value.

>>> import urlparse
>>> domain = 'http://www.org:5000'
>>> url = 'http://www.org:5000/foobar?blah'
>>> urlparse.urlparse(url)
ParseResult(scheme='http', netloc='www.org:5000', path='/foobar', params='', query='blah', fragment='')
>>> domain = urlparse.urlparse(url).netloc
>>> import re
>>> port_number = re.search(r':\d+$', domain)
>>> port_number
<_sre.SRE_Match object at 0x1097712a0>
>>> url2 = 'http://www.org/foobar?blah'
>>> domain2 = urlparse.urlparse(url2).netloc
>>> port_number2 = re.search(r':\d+$', domain2)
>>> port_number2
>>> if port_number:
...     print 'do something'
... 
do something
>>> if port_number2:
...     print 'do something'
... 
>>> 
@@ -147,6 +148,12 @@ def domain_name(url):
# testing if it's an http URL
if url.startswith(SCHEMES):
domain = urlparse.urlparse(url).netloc
if domain.lower().startswith('www.'):
domain = domain.split('www.')[1]

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

It doesn't give us benefit to remove the www., it's a domain on its own. Plus it leads to the potential mistake of cutting off www.TLD example: www.org.

@karlcow

karlcow Sep 14, 2016

Contributor

It doesn't give us benefit to remove the www., it's a domain on its own. Plus it leads to the potential mistake of cutting off www.TLD example: www.org.

domain = domain.split('www.')[1]
port_number = re.search(r':\d+$', domain)
if port_number is not None:
if domain.endswith(port_number.group()):

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

because of the previous if on port_number, this next if will always be true, so it's useless. You are basically testing this:

>>> domain
'www.org:5000'
>>> port_number.group()
':5000'
>>> domain.endswith(port_number.group())
True
>>> 'www.org:5000'.endswith(':5000')
True
@karlcow

karlcow Sep 14, 2016

Contributor

because of the previous if on port_number, this next if will always be true, so it's useless. You are basically testing this:

>>> domain
'www.org:5000'
>>> port_number.group()
':5000'
>>> domain.endswith(port_number.group())
True
>>> 'www.org:5000'.endswith(':5000')
True
port_number = re.search(r':\d+$', domain)
if port_number is not None:
if domain.endswith(port_number.group()):
domain = domain.split(port_number.group())[0]
else:

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

All of that said, I don't think we should normalize www. and :port_number. There are part of the useful information of a domain name and even clearly identify different web properties.

@karlcow

karlcow Sep 14, 2016

Contributor

All of that said, I don't think we should normalize www. and :port_number. There are part of the useful information of a domain name and even clearly identify different web properties.

Show outdated Hide outdated webcompat/webhooks/__init__.py
parse_and_set_label(issue_body, issue_number)
# Setting "Needs Triage" label by default
# to all the new issues raised
set_label('status-needstriage', issue_number)
dump_to_db(issue_title, issue_body, issue_number)
dump_to_db(issue_title, issue_body, issue_number, issue_status, issue_reported_from,
issue_creation_time, issue_last_change_time)

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

line too long

@karlcow

karlcow Sep 14, 2016

Contributor

line too long

Show outdated Hide outdated webcompat/webhooks/helpers.py
@@ -49,7 +51,10 @@ def set_label(label, issue_number):
api_post('labels', payload, issue_number)
def dump_to_db(title, body, issue_number):
def dump_to_db(title, body, issue_number, status, reported_from, creation_time, last_change_time):

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

line too long.
You really need to install a Python linter for your code editor. These are very simple syntactic issues.

@karlcow

karlcow Sep 14, 2016

Contributor

line too long.
You really need to install a Python linter for your code editor. These are very simple syntactic issues.

Show outdated Hide outdated webcompat/webhooks/__init__.py
if 'reported_from' in payload.get('issue'):
issue_reported_from = payload.get('issue')['reported_from']
else:
issue_reported_from = 'null'

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

instead of if/else here. You could set issue_reported_from = 'null' at the beginning after line 37 (aka a default value).
Then just setting the value if it's found in the payload.

@karlcow

karlcow Sep 14, 2016

Contributor

instead of if/else here. You could set issue_reported_from = 'null' at the beginning after line 37 (aka a default value).
Then just setting the value if it's found in the payload.

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Sep 14, 2016

Contributor

ok there is another issue in this pull request. It reuses the labeler webhook.
the labeler webhook is working only on action: opened, we probably want to change this to other kind of events on issues such as 'updated', 'deleted', etc. because in the current context the status will always be the one of the opening time. The last_change_time will never change etc.

Also there is a "security" issue with our current setup. But maybe this should go in private, I'll talk with @miketaylr about it. It is not dramatic, but still.

Contributor

karlcow commented Sep 14, 2016

ok there is another issue in this pull request. It reuses the labeler webhook.
the labeler webhook is working only on action: opened, we probably want to change this to other kind of events on issues such as 'updated', 'deleted', etc. because in the current context the status will always be the one of the opening time. The last_change_time will never change etc.

Also there is a "security" issue with our current setup. But maybe this should go in private, I'll talk with @miketaylr about it. It is not dramatic, but still.

Show outdated Hide outdated webcompat/db/__init__.py
@@ -37,13 +39,23 @@ class WCIssue(IssueBase):
issue_id = Column(String(128), unique=True, primary_key=True)

This comment has been minimized.

@karlcow

karlcow Sep 14, 2016

Contributor

why not constraining this to an Integer type

@karlcow

karlcow Sep 14, 2016

Contributor

why not constraining this to an Integer type

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Sep 14, 2016

Contributor
  • If we really go with normalizing domains, then we need to add tests for it.
  • Currently a lot of errors with the data fail with a Error 500 if the data are not correct, we need probably to make it more robust and catch the errors to log them and/or display a useful error message for the users.
Contributor

karlcow commented Sep 14, 2016

  • If we really go with normalizing domains, then we need to add tests for it.
  • Currently a lot of errors with the data fail with a Error 500 if the data are not correct, we need probably to make it more robust and catch the errors to log them and/or display a useful error message for the users.
@miketaylr

This comment has been minimized.

Show comment
Hide comment
@miketaylr

miketaylr Sep 19, 2016

Member

+1 to @karlcow's review so far. Will take a closer look when those comments are addressed. Thanks!

Member

miketaylr commented Sep 19, 2016

+1 to @karlcow's review so far. Will take a closer look when those comments are addressed. Thanks!

@miketaylr

This comment has been minimized.

Show comment
Hide comment
@miketaylr

miketaylr Sep 27, 2016

Member

I just pushed the security fixes from @karlcow -- please rebase!

Member

miketaylr commented Sep 27, 2016

I just pushed the security fixes from @karlcow -- please rebase!

@deepthivenkat

This comment has been minimized.

Show comment
Hide comment
Member

deepthivenkat commented Sep 27, 2016

Okay!

@miketaylr

This comment has been minimized.

Show comment
Hide comment
@miketaylr

miketaylr Oct 4, 2016

Member

If we really go with normalizing domains, then we need to add tests for it.
Currently a lot of errors with the data fail with a Error 500 if the data are not correct, we need probably to make it more robust and catch the errors to log them and/or display a useful error message for the users.

@karlcow can you clarify this comment? I think @deepthivenkat wasn't 100% sure what it means. Thanks!

Member

miketaylr commented Oct 4, 2016

If we really go with normalizing domains, then we need to add tests for it.
Currently a lot of errors with the data fail with a Error 500 if the data are not correct, we need probably to make it more robust and catch the errors to log them and/or display a useful error message for the users.

@karlcow can you clarify this comment? I think @deepthivenkat wasn't 100% sure what it means. Thanks!

@deepthivenkat

This comment has been minimized.

Show comment
Hide comment
@deepthivenkat

deepthivenkat Oct 4, 2016

Member

Thanks @miketaylr.
@karlcow I wanted to know if by tests if you mean additional validations for the url in the form.py.
I wanted to clarify this before pushing the changes because, these comments will get erased(I am rebasing after mike's security push).

Member

deepthivenkat commented Oct 4, 2016

Thanks @miketaylr.
@karlcow I wanted to know if by tests if you mean additional validations for the url in the form.py.
I wanted to clarify this before pushing the changes because, these comments will get erased(I am rebasing after mike's security push).

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 4, 2016

Contributor

There are two things here:

  • normalizing domains: refer to test ins the test suite. But we decided to not do that. So let's ignore.
  • About Error 500, some parts of the code doesn't really constraint the input in a meaningful way. Like for example, the String(128) instead of Integer. So the game (if you accept it :D ) is basically to challenge the code. Each time there's a chance for the server to do a 500, it means we didn't handle the error gracefully.

BUT first fix the code with the new version then we can chase down the Error 500.

Contributor

karlcow commented Oct 4, 2016

There are two things here:

  • normalizing domains: refer to test ins the test suite. But we decided to not do that. So let's ignore.
  • About Error 500, some parts of the code doesn't really constraint the input in a meaningful way. Like for example, the String(128) instead of Integer. So the game (if you accept it :D ) is basically to challenge the code. Each time there's a chance for the server to do a 500, it means we didn't handle the error gracefully.

BUT first fix the code with the new version then we can chase down the Error 500.

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 19, 2016

Contributor

@deepthivenkat Is there anything I can help with to move this forward? Do not hesitate to say if you feel you are too busy, we can finish it. Hope everything is fine with the new life.

Contributor

karlcow commented Oct 19, 2016

@deepthivenkat Is there anything I can help with to move this forward? Do not hesitate to say if you feel you are too busy, we can finish it. Hope everything is fine with the new life.

miketaylr and others added some commits Oct 4, 2016

Fixing typos/grammar/wording
ln 6 and 36: Pluralized words for consistency. Note that the other headings that don't have 'a' or 'an' before it are all pluralized. eg closing bugs, issues, requests, etc. Questions or Problems also sounds slightly better.

ln 6: pluralized the links

ln 62: duplicate link

ln 127: themselves is gender neutral avoiding the him/herself so is a more succinct way of stating that they shouldn't merge their own pull request. I feel the themself should also be tacked on the end as "merging themselves the pull request" might sound slightly awkward. Similarly stating "or the admin" after "merge the branch" could also be awkward (and beg the question: or the admin what) and stating "the reviewer or the admin" flows better.

ln 143: "that much conventions" is grammatically incorrect, though not technically incorrect "we do not have yet" does not flow as well as "we do not yet have" I also added "When" before "in doubt" since on ln 170 you do use 'when in doubt' so I assume you just forgot "when"

ln 258: changed do to done -- tense agreement

ln 260: undefined reference: clarified what is needed, "installing pip" as what "if you need to" is referring to can be ambiguous

ln 287: changed 'user' to 'username' for consistency since in the next line 'username' is used

ln 376: I think this is what is meant, otherwise this is an odd sentence

ln 391: Windows

ln 403: ambiguous 'that'

ln 403, removed 'do'
Issue #1193. Add a .wc-HomePage class to <main> on home page.
This allows us to hide the form by default on the home page, and show
it by default on the /issues/new page.
Small grammatical change + consistency/clarity fix
ln 2: changed the "Find a bug with a "needsdiagnosis" label or that is otherwise new" to be the same as the corresponding step in "How to Reproduce the Bug" to make that instruction easier to understand, and be consistent in terms of linking to the "Need Diagnosis" tag rather than sending users to a label. 

ln 6;8-9: added the between "if bug" to both be consistent with the other instructions that refer to the bug as "the bug" rather than just "bug" and for grammatical correctness. 

ln 11: It's more correct to say "if there are any errors" than "if there are errors"

[ci skip]
Issue #1193. Show home link for new issue endpoint, rather than add-o…
…n prompts.

The logic being, if you're looking at this page, an add-on probably took you there.
Issue #1193. Update logout selector.
(GitHub changed their markup)
@deepthivenkat

This comment has been minimized.

Show comment
Hide comment
@deepthivenkat

deepthivenkat Oct 19, 2016

Member

@karlcow I have pushed some changes today. I had 3 problems that were holding me from pushing changes:

  1. I wanted to know the difference between 'labeled' or 'unlabeled' actions in payload
  2. I am updating a few fields in issue db for 'edited', 'closed', 'labeled', 'unlabeled' - I wanted to test it in the 'issue.db' file from my local webcompat folder.
    I have a repository in github where the issues that I create in my localhost will be stored. I tried to add webhook to that repository and checked if it stored the fields in my local issue.db file. This attempt was not successful.
  3. In helpers.py, the update command will be called once for every key, value pairs passed into **kwargs. However, I want it to be called once.
  • These are the problems i faced. So now i thought I will push the code and seek your opinion.
Member

deepthivenkat commented Oct 19, 2016

@karlcow I have pushed some changes today. I had 3 problems that were holding me from pushing changes:

  1. I wanted to know the difference between 'labeled' or 'unlabeled' actions in payload
  2. I am updating a few fields in issue db for 'edited', 'closed', 'labeled', 'unlabeled' - I wanted to test it in the 'issue.db' file from my local webcompat folder.
    I have a repository in github where the issues that I create in my localhost will be stored. I tried to add webhook to that repository and checked if it stored the fields in my local issue.db file. This attempt was not successful.
  3. In helpers.py, the update command will be called once for every key, value pairs passed into **kwargs. However, I want it to be called once.
  • These are the problems i faced. So now i thought I will push the code and seek your opinion.
@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 20, 2016

Contributor

@deepthivenkat There are a couple of issues with this last push. There are many commits from @miketaylr . Not only your modifications.

There are files which should not be there such as
c2e88d9#diff-01b9f10c14726ede6655dcc207991901R1

I'm trying to get a better understanding of what has been pushed to the pull request.

Contributor

karlcow commented Oct 20, 2016

@deepthivenkat There are a couple of issues with this last push. There are many commits from @miketaylr . Not only your modifications.

There are files which should not be there such as
c2e88d9#diff-01b9f10c14726ede6655dcc207991901R1

I'm trying to get a better understanding of what has been pushed to the pull request.

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 20, 2016

Contributor

@deepthivenkat

  1. I wanted to know the difference between 'labeled' or 'unlabeled' actions in payload
    https://developer.github.com/webhooks/#events

On issue event gives you answer.

"action": "labeled" and "action": "unlabeled" means that a label has been added or removed to the issue.

Contributor

karlcow commented Oct 20, 2016

@deepthivenkat

  1. I wanted to know the difference between 'labeled' or 'unlabeled' actions in payload
    https://developer.github.com/webhooks/#events

On issue event gives you answer.

"action": "labeled" and "action": "unlabeled" means that a label has been added or removed to the issue.

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 20, 2016

Contributor

@deepthivenkat

  1. I am updating a few fields in issue db for 'edited', 'closed', 'labeled', 'unlabeled' - I wanted to test it in the 'issue.db' file from my local webcompat folder. I have a repository in github where the issues that I create in my localhost will be stored. I tried to add webhook to that repository and checked if it stored the fields in my local issue.db file. This attempt was not successful.

To test webhooks, you can do either:

  • Put a test server online on a personal space and creates a webhook.
  • Impersonate github locally by doing a HTTP POST with a JSON file on your server running on localhost.

See for example https://github.com/miketaylr/web-bugs-issue-tagger#testing-locally

Contributor

karlcow commented Oct 20, 2016

@deepthivenkat

  1. I am updating a few fields in issue db for 'edited', 'closed', 'labeled', 'unlabeled' - I wanted to test it in the 'issue.db' file from my local webcompat folder. I have a repository in github where the issues that I create in my localhost will be stored. I tried to add webhook to that repository and checked if it stored the fields in my local issue.db file. This attempt was not successful.

To test webhooks, you can do either:

  • Put a test server online on a personal space and creates a webhook.
  • Impersonate github locally by doing a HTTP POST with a JSON file on your server running on localhost.

See for example https://github.com/miketaylr/web-bugs-issue-tagger#testing-locally

@karlcow

This comment has been minimized.

Show comment
Hide comment
@karlcow

karlcow Oct 20, 2016

Contributor

@deepthivenkat

  1. In helpers.py, the update command will be called once for every key, value pairs passed into **kwargs. However, I want it to be called once.

I didn't know the answer on the top of my head, but looking at the function:

def update_issue_in_db(issue_number, **kwargs):
    for key, value in kwargs:
        if value:
            update(issue_db).where(issue_db.issue_id == issue_number).values(key=value)

Here you are iterating on key, value and makes the update for each of them. So I checked the documentation of SQLAlchemy for values.

In the documentation they say:

Optional dictionary which specifies the SET conditions of the UPDATE.

  1. You can either pass a dictionary beforehand and uses it in the function call.
  2. Or you iterate other the data in the function and creates a dictionary that you then pass to the update.

Personally I would go with option 1.

So instead of doing

                    update_issue_in_db(int(issue_number),
                                       issue_title=issue_title,
                                       issue_body=issue_body,
                                       issue_last_change_time=updated)

I would pass:

                    update_issue_in_db(int(issue_number),
                                       {'issue_title'=issue_title,
                                       'issue_body'=issue_body,
                                       'issue_last_change_time'=updated})

and

def update_issue_in_db(issue_number, new_values):
    '''Updates the database with new values.

    new_values is a dictionary.'''
    update(issue_db).where(issue_db.issue_id == issue_number).values(new_values)
Contributor

karlcow commented Oct 20, 2016

@deepthivenkat

  1. In helpers.py, the update command will be called once for every key, value pairs passed into **kwargs. However, I want it to be called once.

I didn't know the answer on the top of my head, but looking at the function:

def update_issue_in_db(issue_number, **kwargs):
    for key, value in kwargs:
        if value:
            update(issue_db).where(issue_db.issue_id == issue_number).values(key=value)

Here you are iterating on key, value and makes the update for each of them. So I checked the documentation of SQLAlchemy for values.

In the documentation they say:

Optional dictionary which specifies the SET conditions of the UPDATE.

  1. You can either pass a dictionary beforehand and uses it in the function call.
  2. Or you iterate other the data in the function and creates a dictionary that you then pass to the update.

Personally I would go with option 1.

So instead of doing

                    update_issue_in_db(int(issue_number),
                                       issue_title=issue_title,
                                       issue_body=issue_body,
                                       issue_last_change_time=updated)

I would pass:

                    update_issue_in_db(int(issue_number),
                                       {'issue_title'=issue_title,
                                       'issue_body'=issue_body,
                                       'issue_last_change_time'=updated})

and

def update_issue_in_db(issue_number, new_values):
    '''Updates the database with new values.

    new_values is a dictionary.'''
    update(issue_db).where(issue_db.issue_id == issue_number).values(new_values)
@miketaylr

This comment has been minimized.

Show comment
Hide comment
@miketaylr

miketaylr Dec 23, 2016

Member

Is there a status update on this work @deepthivenkat @karlcow? Do we plan on landing this PR, or should we close it, iterate on a branch, and attempt to land it in a new PR?

Member

miketaylr commented Dec 23, 2016

Is there a status update on this work @deepthivenkat @karlcow? Do we plan on landing this PR, or should we close it, iterate on a branch, and attempt to land it in a new PR?

@deepthivenkat

This comment has been minimized.

Show comment
Hide comment
@deepthivenkat

deepthivenkat Dec 23, 2016

Member

I discussed with Karl on IRC but forgot to update it here.
@miketaylr we should close it and use a new PR.

Member

deepthivenkat commented Dec 23, 2016

I discussed with Karl on IRC but forgot to update it here.
@miketaylr we should close it and use a new PR.

@miketaylr

This comment has been minimized.

Show comment
Hide comment
@miketaylr

miketaylr Dec 23, 2016

Member

OK, let's do that. Thanks!

Member

miketaylr commented Dec 23, 2016

OK, let's do that. Thanks!

@miketaylr miketaylr closed this Dec 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment