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

Fixes #639: Add ability to load search results from URL params for non-logged in users #650

Merged
merged 7 commits into from
Jul 31, 2015

Conversation

miketaylr
Copy link
Member

(Used this easy task as an excuse to get my hands dirty in webcompat code again.)

r? @karlcow @tagawa

@miketaylr
Copy link
Member Author

Aww sad, conflict city.

@miketaylr
Copy link
Member Author

OK, rebased against master.

Ping @karlcow?

@miketaylr
Copy link
Member Author

Note: some of these tests will fail because we renamed labels in the test repo and this branch's tests are expecting the old labels. Safe to ignore... 💣

# Non-authed users should never get here--the request is made to
# GitHub client-side)--but return out of paranoia anyways.
elif params.get('q'):
return

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@karlcow
Copy link
Member

karlcow commented Jul 31, 2015

→ git checkout -b 639/1 upstream/issues/639/1
Branch 639/1 set up to track remote branch issues/639/1 from upstream.
Switched to a new branch '639/1'
(webcompatcom)09:54:54 ~/code/webcompat.com
→ subl .
(webcompatcom)09:55:02 ~/code/webcompat.com
→ python run.py 
 * Running on http://localhost:5000/
 * Restarting with reloader

so far so good no issues.

Opening a clean profile of Firefox. Not logged at all to any services.
Heading to http://localhost:5000/issues
and being redirected to http://localhost:5000/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc

no search form is visible in the page. Normal? Did I miss a step?

Entering Manually the search with "love" and getting a result.
http://localhost:5000/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love

But then I went through the python API, the request on the server log is

127.0.0.1 - - [31/Jul/2015 10:06:26] "GET /issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love HTTP/1.1" 200 -

I wonder if we should forbid this against script kiddies. I guess maybe it's the answer to the return without any statement. We should return an error page.

@miketaylr
Copy link
Member Author

no search form is visible in the page. Normal? Did I miss a step?

No, that's fine. Right now the search form is hidden for non-logged in users. #639 is about loading search results from a URL when you aren't logged in.

But then I went through the python API, the request on the server log is [...]

Can you explain what you mean here, or what steps you took via the API? The API shouldn't be making any requests to GitHub at this point (this is the explicit return part which I'll change to 404).

I wonder if we should forbid this against script kiddies. I guess maybe it's the answer to the return without any statement.

This search API request is made from the client-side. So if people abuse it, GitHub will punish their IP, not us.

@karlcow
Copy link
Member

karlcow commented Jul 31, 2015

If I understood your modifications, the request is made by the JavaScript client to Github directly (no webcompat involved). But if I enter http://localhost:5000/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love I do request webcompat and not github.

And because the form is not here, it's not the JS client doing the request to github.

@karlcow
Copy link
Member

karlcow commented Jul 31, 2015

In the two previous comments I explained what I did litterally.
But I can't really test the modifications… without the search form or maybe from the console?

@miketaylr
Copy link
Member Author

OK, let me try to think this through to make sure I'm thinking of the right thing:

  1. Visit http://localhost:5000/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love

  2. Server hits the /issues route and renders the issue-list.html template:
    https://github.com/webcompat/webcompat.com/blob/issues/639/1/webcompat/views.py#L161

  3. The issue-list.html template downloads some HTML and JS. Some of that JS is issue-list.js which eventually gets to loadIssues: https://github.com/webcompat/webcompat.com/blob/issues/639/1/webcompat/static/js/lib/issue-list.js#L317

  4. The first thing we check for is if there are GET params. Then, if the user is not logged in, and if there is a q search param. For the request in 1, this is true so far. So we manually set the model URL endpoint to GitHub and request the issues:
    https://github.com/webcompat/webcompat.com/blob/issues/639/1/webcompat/static/js/lib/issue-list.js#L328-L335

You can see the XHR request going to GitHub, not webcompat.com.

  1. Once we get data, we render like usual.

One way to verify that we're not actually hitting the webcompat API is to check the XHR logs. We can also swap out the empty return for abort(404) (let me commit that real quick). Re-requesting the URL from 1 shows we don't get any 404s.

@miketaylr
Copy link
Member Author

After I log in, I see the following:

127.0.0.1 - - [30/Jul/2015 21:57:27] "GET /issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love HTTP/1.1" 200 -
127.0.0.1 - - [30/Jul/2015 21:57:27] "GET /api/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=love HTTP/1.1" 200 -

While logged out my server console doesn't show a hit to /api/issues.

@karlcow
Copy link
Member

karlcow commented Jul 31, 2015

I should sleep more. 🙏🙇🏼

@karlcow
Copy link
Member

karlcow commented Jul 31, 2015

with the 404. Everything is fine. :)

karlcow added a commit that referenced this pull request Jul 31, 2015
Fixes #639: Add ability to load search results from URL params for non-logged in users
@karlcow karlcow merged commit b091d18 into master Jul 31, 2015
@miketaylr
Copy link
Member Author

I should sleep more

Me too. Bed time. Thanks for thinking through this with me. 🔨

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.

2 participants