Skip to content

Fixes #399 and #516 - Keep issues model and URL params in sync#542

Merged
miketaylr merged 8 commits into
masterfrom
issues/399/2
Jan 29, 2015
Merged

Fixes #399 and #516 - Keep issues model and URL params in sync#542
miketaylr merged 8 commits into
masterfrom
issues/399/2

Conversation

@miketaylr

Copy link
Copy Markdown
Member

This allows you to bookmark whatever you're looking at on the issues page, and get back to that from the bookmark (or hyperlink or whatever).

Not ready for review yet, still working on tests.

@miketaylr

Copy link
Copy Markdown
Member Author

This might fix #535 as well.

@miketaylr

Copy link
Copy Markdown
Member Author

r? @magsout

Admittedly hard to review, it's a lot of changes. I deployed to staging if you want to test changing some of the dropdowns and loading those URLs in new tabs or something. ☕

@karlcow

karlcow commented Jan 22, 2015

Copy link
Copy Markdown
Member

Redirection to Github

When on http://staging.webcompat.com/issues?page=1&per_page=50&state=open&needsdiagnosis=1

type g and the user is redirected to https://github.com/miketaylr/nobody-look-at-this/issues instead of
https://github.com/miketaylr/nobody-look-at-this/issues?q=is%3Aopen+is%3Aissue+label%3Aneedsdiagnosis

URL Scheme

In the URL scheme, any reasons why you chose needsdiagnosis=1 instead of label=needsdiagnosis or stage=needsdiagnosis (just curious).

HTTP Caching.

Maybe this is due to our staging configuration, ignore if it's the case.
We may have caching issues.

First, Go to http://staging.webcompat.com/issues (not logged). It downloads

  • /issues (html)
  • and /issues?page=1&per_page=50&state=open (json). Response contains Etag: W/"d2be4d86ab1513e65d2b18b42faed8ca"
  • And http://staging.webcompat.com/issues?page=1&per_page=50&state=open is pushed to the URL bar.

Second, Reload button.

  • 200 on /issues?page=1&per_page=50&state=open (html) with Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 It didn't send any If-None-Match, I guess normal given the change of URI. The Response has no Etag.
  • 200 on /issues?page=1&per_page=50&state=open (json) with Accept: application/json. Request with If-None-Match: W/"d2be4d86ab1513e65d2b18b42faed8ca" and Cache-Control: max-age=0. The response has Etag: W/"d2be4d86ab1513e65d2b18b42faed8ca".

Here we could imagine the first one is 200 and the second one is 304. The 1st one has a new URI. The second one has nothing changed.

Third, Reload button.

Same results than the previous reload. Both are again 200 when there the resources and representation are entirely identical. Still no etag for the HTML. And the same Etag for the JSON.

It's surprising and I wonder if it's a bug from Firefox.

@karlcow

karlcow commented Jan 22, 2015

Copy link
Copy Markdown
Member

Navigation with next/prev

Look at this sequence:

webcompat navigation sequence

  1. Click on contactready.

    URLbar == http://staging.webcompat.com/issues?page=1&per_page=50&state=open&contactready=1

    JSON_Request == http://staging.webcompat.com/api/issues/search/contactready?page=1&per_page=50&state=open

  2. Click on 25.

    URLbar == http://staging.webcompat.com/issues?page=1&per_page=25&state=open&contactready=1

    JSON_Request == http://staging.webcompat.com/api/issues/search/contactready?page=1&per_page=25&state=open&contactready=1

  3. Click on next.

    URLbar == http://staging.webcompat.com/issues?page=2&per_page=25&state=open&contactready=1.

    JSON_Request == http://staging.webcompat.com/api/issues/search?q=label%3Acontactready+repo%3Amiketaylr%2Fnobody-look-at-this+state%3Aopen&per_page=25&page=2&contactready=1

  4. Click on previous

    URLbar == http://staging.webcompat.com/issues?page=1&per_page=25&state=open&contactready=1

    JSONRequest == http://staging.webcompat.com/api/issues/search?q=label%3Acontactready+repo%3Amiketaylr%2Fnobody-look-at-this+state%3Aopen+repo%3Amiketaylr%2Fnobody-look-at-this&per_page=25&page=1&contactready=1

The URLbar scheme is correct. No issue with that, but we do funky requests which are not consistent. Maybe because of using the Link headers. We might need to fix that too. So we get the proper link and be consistent so a JS UA can navigate without requesting changing routes.

Link: </api/issues/search?q=label%3Acontactready+repo%3Amiketaylr%2Fnobody-look-at-this+state%3Aopen&per_page=25&page=2&contactready=1>; rel="next", </api/issues/search?q=label%3Acontactready+repo%3Amiketaylr%2Fnobody-look-at-this+state%3Aopen&per_page=25&page=3&contactready=1>; rel="last"

@miketaylr

Copy link
Copy Markdown
Member Author

Redirection to GitHub

Good catch. Let's fix this in a separate issue after we land this feature. We'll have to do a bit of "translating" on the client side to get it right.

In the URL scheme, any reasons why you chose needsdiagnosis=1 instead of label=needsdiagnosis or stage=needsdiagnosis (just curious).

No particular reason. I sort of like stage though. If we're going to change it, I guess we potentially break bookmarks (i.e., the https://webcompat.com/issues?new=1 links on the homepage). But I'm OK with that. 😈

HTTP Caching

This seems like a good optimization for the future. Let's file a bug when this lands.

Navigation with next/prev
No issue with that, but we do funky requests which are not consistent. Maybe because of using the Link headers.

Yes, exactly. GitHub is sending us URIs with params that it understands and we just transparently proxy those back.

Again, this seems like a good "fix me later" issue--as it doesn't affect the user functionality. The double repo%3Amiketaylr%2Fnobody-look-at-this in the JSON request of 4 looks weird though. I'll investigate.

So we get the proper link and be consistent so a JS UA can navigate without requesting changing routes.

I think what you mean is 1 and 2 use the issues/search/contactready route, but 3 and 4 just use /search, is that right? If you look at issues/search/contactready, it's just a alias for issues/search with the GitHub parameters set. If anything, I would probably prefer removing the https://github.com/webcompat/webcompat.com/blob/master/webcompat/api/endpoints.py#L180 endpoint and just using /issues/search. That seems preferable to re-writing GitHub Link URIs more than we already are.

@miketaylr

Copy link
Copy Markdown
Member Author

OK, changed the filters to be a stage param and added tests for that.

@karlcow

karlcow commented Jan 22, 2015

Copy link
Copy Markdown
Member

🍰

@karlcow

karlcow commented Jan 22, 2015

Copy link
Copy Markdown
Member

For the legacy URI stuff.

I guess we potentially break bookmarks (i.e., the https://webcompat.com/issues?new=1 links on the homepage). But I'm OK with that. 😈

I put a question on stackoverflow which is slightly generic to see if I'm not missing anything about redirect in flask.

@miketaylr

Copy link
Copy Markdown
Member Author

Sweet, I'll keep my eye on that stackoverflow post.

If anyone else wants a chance to test or review, now is the time. ^_^

@miketaylr

Copy link
Copy Markdown
Member Author

http://butt.holdings/

miketaylr pushed a commit that referenced this pull request Jan 29, 2015
Fixes #399 and #516 - Keep issues model and URL params in sync
@miketaylr miketaylr merged commit 79109d8 into master Jan 29, 2015
@miketaylr miketaylr deleted the issues/399/2 branch January 29, 2015 16:22
@karlcow

karlcow commented Jan 30, 2015

Copy link
Copy Markdown
Member

Congratulations MIke. I will do a series of tests either today or probably next week.

@magsout

magsout commented Jan 30, 2015

Copy link
Copy Markdown
Member

@miketaylr Sorry for not review this PR

@miketaylr

Copy link
Copy Markdown
Member Author

No worries @magsout. Just be sure to find the (hidden) bugs as you find them. 😄

@miketaylr miketaylr restored the issues/399/2 branch July 20, 2015 19:04
@miketaylr miketaylr deleted the issues/399/2 branch July 21, 2015 04:07
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.

3 participants