Issue 30011569: Issue 7290 - Fix signature production in sitekey_frame.py (Closed)
|
Created:
Feb. 19, 2019, 10:05 p.m. by Vasily Kuznetsov Modified:
Feb. 21, 2019, 1:46 p.m. Reviewers:
kzar CC:
Tudor Avram Base URL:
https://hg.adblockplus.org/sitescripts Visibility:
Public. |
DescriptionIssue 7290 - Fix signature production in sitekey_frame.py
Repository: https://hg.adblockplus.org/sitescripts
Base revision: 9070626bba97
Patch Set 1 : Initial patch #
Total comments: 5
Patch Set 2 : Remove unrelated improvements #Patch Set 3 : Remove the test for the fix #
MessagesTotal messages: 7
Hi Dave, Here's a fix for https://issues.adblockplus.org/ticket/7290 based on the fix I applied on the server to fix it. Here I also updated the request_path() function, which was the reason for the issue and added a test for the web handler. Let me know what you think. Cheers, Vasily https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/testpag... File sitescripts/testpages/web/sitekey_frame.py (right): https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/testpag... sitescripts/testpages/web/sitekey_frame.py:48: 'http_path': http_path, I pass these variables to the template as well so that they can be used there in case we decide to make debugging of this easier for the future. https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py File sitescripts/web.py (right): https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py#... sitescripts/web.py:43: def request_path(environ, include_query=True): This function is not used anywhere else and its logic seems to be causing the issue so I decided to directly modify it. https://codereview.adblockplus.org/30011569/diff/30011574/tox.ini File tox.ini (right): https://codereview.adblockplus.org/30011569/diff/30011574/tox.ini#newcode67 tox.ini:67: sitescripts/testpages/test/test_sitekey_frame.py : E402 Unfortunately in the tests it's sometimes necessary to run code before some of the imports.
https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py File sitescripts/web.py (right): https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py#... sitescripts/web.py:43: def request_path(environ, include_query=True): On 2019/02/19 22:28:53, Vasily Kuznetsov wrote: > This function is not used anywhere else and its logic seems to be causing the > issue so I decided to directly modify it. IMO The change for #7290 should just contain this change, if it's the actual fix. Would you mind splitting the other changes out in a Noissue to tidy the testpages code up? (I'd be happy to review that too if it helps.)
On 2019/02/20 08:12:08, kzar wrote: > https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py > File sitescripts/web.py (right): > > https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py#... > sitescripts/web.py:43: def request_path(environ, include_query=True): > On 2019/02/19 22:28:53, Vasily Kuznetsov wrote: > > This function is not used anywhere else and its logic seems to be causing the > > issue so I decided to directly modify it. > > IMO The change for #7290 should just contain this change, if it's the actual > fix. Yeah, makes sense. How about this?
https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py File sitescripts/web.py (right): https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py#... sitescripts/web.py:43: def request_path(environ, include_query=True): > On 2019/02/20 08:12:07, kzar wrote: > > On 2019/02/19 22:28:53, Vasily Kuznetsov wrote: > > > This function is not used anywhere else and its logic seems to be causing the > > > issue so I decided to directly modify it. > > > > IMO The change for #7290 should just contain this change, if it's the actual > > fix. > > > > Would you mind splitting the other changes out in a Noissue to tidy the > > testpages code up? (I'd be happy to review that too if it helps.) > Yeah, makes sense. How about this? But there's still a bunch of other changes in the latest patch set too? All your changes (from both patch sets) LGTM, I just ask you do push them as two commits. The first containing this change (`+` to `or`) for 7290. The second with all the other changes as a no/separate issue.
On 2019/02/21 08:12:25, kzar wrote: > https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py > File sitescripts/web.py (right): > > https://codereview.adblockplus.org/30011569/diff/30011574/sitescripts/web.py#... > sitescripts/web.py:43: def request_path(environ, include_query=True): > > On 2019/02/20 08:12:07, kzar wrote: > > > On 2019/02/19 22:28:53, Vasily Kuznetsov wrote: > > > > This function is not used anywhere else and its logic seems to be causing > the > > > > issue so I decided to directly modify it. > > > > > > IMO The change for #7290 should just contain this change, if it's the actual > > > fix. > > > > > > Would you mind splitting the other changes out in a Noissue to tidy the > > > testpages code up? (I'd be happy to review that too if it helps.) > > > Yeah, makes sense. How about this? > > But there's still a bunch of other changes in the latest patch set too? The second patch set just has the fix for #7290 and a test for it which I have added to make sure it works and does what we want. > All your changes (from both patch sets) LGTM, I just ask you do push them as two > commits. The first containing this change (`+` to `or`) for 7290. The second > with all the other changes as a no/separate issue. Do I get it right that you prefer to move the test from "Issue 7290" commit to a separate Noissue commit that will contain the other changes from PS1? I will make a PS3 just so we're clear what's landing in "Issue 7290". Then I will land the rest of PS1 in a separate Noissue. Would this work? Cheers, Vasily
On 2019/02/21 12:16:37, Vasily Kuznetsov wrote: > Do I get it right that you prefer to move the test from "Issue 7290" commit to a > separate Noissue commit that will contain the other changes from PS1? I will > make a PS3 just so we're clear what's landing in "Issue 7290". Then I will land > the rest of PS1 in a separate Noissue. Would this work? Yea, that sounds good to me. PS3 LGTM for the 7290 change, everything else from PS1 LGTM for a separate "tidy up" commit.
On 2019/02/21 12:30:18, kzar wrote: > On 2019/02/21 12:16:37, Vasily Kuznetsov wrote: > > Do I get it right that you prefer to move the test from "Issue 7290" commit to > a > > separate Noissue commit that will contain the other changes from PS1? I will > > make a PS3 just so we're clear what's landing in "Issue 7290". Then I will > land > > the rest of PS1 in a separate Noissue. Would this work? > > Yea, that sounds good to me. PS3 LGTM for the 7290 change, everything else from > PS1 LGTM for a separate "tidy up" commit. Thanks, I will land them this way. 
|
||||||||||||||||||||||||||||||||||||||||||||||
