Merge pull request #446 from /issues/420/2

Fixes #420 - Follow Link header links for pagination traversal.

Author: Mike Taylor

tests/test_helpers.py

@@ -0,0 +1,55 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+'''Tests for helper methods in webcompat/helpers.py.'''
+
+import os.path
+import sys
+import unittest
+
+# Add webcompat module to import path
+sys.path.append(os.path.realpath(os.pardir))
+import webcompat
+
+from webcompat.helpers import rewrite_links
+from webcompat.helpers import sanitize_link
+from webcompat.helpers import rewrite_and_sanitize_link
+
+ACCESS_TOKEN_LINK = '<https://api.github.com/repositories/17839063/issues?per_page=50&page=3&access_token=12345>; rel="next", <https://api.github.com/repositories/17839063/issues?access_token=12345&per_page=50&page=4>; rel="last", <https://api.github.com/repositories/17839063/issues?per_page=50&access_token=12345&page=1>; rel="first", <https://api.github.com/repositories/17839063/issues?per_page=50&page=1&access_token=12345>; rel="prev"'
+GITHUB_ISSUES_LINK_HEADER = '<https://api.github.com/repositories/17839063/issues?per_page=50&page=3>; rel="next", <https://api.github.com/repositories/17839063/issues?per_page=50&page=4>; rel="last", <https://api.github.com/repositories/17839063/issues?per_page=50&page=1>; rel="first", <https://api.github.com/repositories/17839063/issues?per_page=50&page=1>; rel="prev"'
+GITHUB_SEARCH_LINK_HEADER = '<https://api.github.com/search/issues?q=taco&page=2>; rel="next", <https://api.github.com/search/issues?q=taco&page=26>; rel="last"'
+REWRITTEN_ISSUES_LINK_HEADER = '</api/issues?per_page=50&page=3>; rel="next", </api/issues?per_page=50&page=4>; rel="last", </api/issues?per_page=50&page=1>; rel="first", </api/issues?per_page=50&page=1>; rel="prev"'
+REWRITTEN_SEARCH_LINK_HEADER = '</api/issues/search?q=taco&page=2>; rel="next", </api/issues/search?q=taco&page=26>; rel="last"'
+
+
+class TestHelpers(unittest.TestCase):
+    def setUp(self):
+        webcompat.app.config['TESTING'] = True
+        self.app = webcompat.app.test_client()
+
+    def tearDown(self):
+        pass
+
+    def test_rewrite_link(self):
+        '''Test we're correctly rewriting the passed in link.'''
+        self.assertEqual(rewrite_links(GITHUB_ISSUES_LINK_HEADER),
+                         REWRITTEN_ISSUES_LINK_HEADER)
+        self.assertEqual(rewrite_links(GITHUB_SEARCH_LINK_HEADER),
+                         REWRITTEN_SEARCH_LINK_HEADER)
+
+    def test_sanitize_link(self):
+        '''Test that we're removing access_token parameters.'''
+        self.assertNotIn('access_token=', sanitize_link(ACCESS_TOKEN_LINK))
+
+    def test_rewrite_and_sanitize_link(self):
+        self.assertNotIn('access_token=',
+                         rewrite_and_sanitize_link(ACCESS_TOKEN_LINK))
+        self.assertEqual(rewrite_and_sanitize_link(ACCESS_TOKEN_LINK),
+                         REWRITTEN_ISSUES_LINK_HEADER)
+
+
+if __name__ == '__main__':
+    unittest.main()

webcompat/api/endpoints.py

@@ -8,6 +8,7 @@
 back to GitHub'''
 
 import json
+import re
 
 from flask import abort
 from flask import Blueprint
@@ -35,7 +36,7 @@
 
 
 def get_username():
-  return session.get('username', 'proxy-user')
+    return session.get('username', 'proxy-user')
 
 
 @api.route('/issues/<int:number>')
@@ -107,13 +108,13 @@ def get_issue_category(issue_category):
     params = request.args.copy()
 
     if issue_category in category_list:
-        params.update({'labels': issue_category})
+        params['labels'] = issue_category
         if g.user:
             issues = github.raw_request('GET', issues_path, params=params)
         else:
             issues = proxy_request('get', params=params)
     elif issue_category == 'closed':
-        params.update({'state': 'closed'})
+        params['state'] = 'closed'
         if g.user:
             issues = github.raw_request('GET', issues_path, params=params)
         else:
@@ -171,26 +172,28 @@ def get_search_results(query_string=None):
                                      headers=request_headers)
     else:
         results = proxy_request('get', params=params, uri=search_uri)
-    # The issues are returned in the items property of the response JSON,
-    # so throw everything else away.
-    json_response = json.loads(results.content)
-    if 'items' in json_response:
-        result = json.dumps(json_response['items'])
-    else:
-        result = results.content
-    return (result, results.status_code, get_headers(results))
+    return (results.content, results.status_code, get_headers(results))
+
 
+@api.route('/issues/search/<issue_category>')
+def get_category_from_search(issue_category):
+    '''XHR endpoint to get issues categories from GitHub's Search API.
 
-@api.route('/issues/search/untriaged')
-def get_untriaged_from_search():
-    '''XHR endpoint to get "untriaged" issues from GitHub's Search API.
+    There is some overlap between /issues/category/<issue_category> as used on
+    the home page - but this endpoint returns paginated results.
 
-    There is some overlap between /issues/category/untriaged as used on the
-    home page - but this endpoint returns paginated results paginated.
-    TODO: Unify that at some point.
+    Note: until GitHub fixes a bug where requesting issues filtered by labels
+    doesn't return pagination via Link, we get those results from this endpoint.
+    Once it's fixed, we can get "contactready", "needsdiagnosis" and "sitewait"
+    issues from /issues/category/<issue_category>.
     '''
-    query_string = ('state:open -label:contactready '
-                    '-label:sitewait -label:needsdiagnosis')
+    category_list = ['contactready', 'needsdiagnosis', 'sitewait']
+
+    if issue_category in category_list:
+        query_string = 'label:{0}'.format(issue_category)
+    elif issue_category == 'untriaged':
+        query_string = ('state:open -label:contactready '
+                        '-label:sitewait -label:needsdiagnosis')
     return get_search_results(query_string)
 
 

webcompat/helpers.py

@@ -7,6 +7,7 @@
 import datetime
 import math
 import urlparse
+import re
 
 from babel.dates import format_timedelta
 from flask import session
@@ -105,7 +106,7 @@ def get_headers(response):
                'content-type': JSON_MIME}
 
     if response.headers.get('link'):
-        headers['link'] = sanitize_link(response.headers.get('link'))
+        headers['link'] = rewrite_and_sanitize_link(response.headers.get('link'))
     return headers
 
 
@@ -138,13 +139,37 @@ def get_referer(request):
         return None
 
 
+def rewrite_links(link_header):
+    '''Rewrites Link header Github API endpoints to our own.
+
+    <https://api.github.com/repositories/17839063/issues?per_page=50&page=2>; rel="next",
+    <https://api.github.com/repositories/17839063/issues?per_page=50&page=4>; rel="last"
+
+    is transformed into
+
+    </api/issues?per_page=50&page=2>; rel="next",
+    </api/issues?per_page=50&page=4>; rel="last" etc.
+    '''
+    links = link_header.split(',')
+    new_links = []
+    for link in links:
+        api_path, endpoint_path = link.rsplit('/', 1)
+        if api_path.strip().startswith('<https://api.github.com/repositories'):
+            new_links.append(endpoint_path.replace('issues?', '</api/issues?'))
+        if api_path.strip().startswith('<https://api.github.com/search'):
+            new_links.append(endpoint_path.replace('issues?', '</api/issues/search?'))
+    return ', '.join(new_links)
+
+
 def sanitize_link(link_header):
-    '''Remove any oauth tokens from the Link header that GitHub gives to us.'''
+    '''Remove any oauth tokens from the Link header that GitHub gives to us,
+    and return a rewritten Link header (see rewrite_links)'''
     links_list = link_header.split(',')
     clean_links_list = []
     for link in links_list:
         uri_info, rel_info = link.split(';')
         uri_info = uri_info.strip()
+        rel_info = rel_info.strip()
         uri = uri_info[1:-1]
         uri_group = urlparse.urlparse(uri)
         parameters = uri_group.query.split('&')
@@ -155,3 +180,7 @@ def sanitize_link(link_header):
         clean_links_list.append('<{0}>; {1}'.format(
             urlparse.urlunparse(clean_uri), rel_info))
     return ', '.join(clean_links_list)
+
+
+def rewrite_and_sanitize_link(link_header):
+    return rewrite_links(sanitize_link(link_header))

webcompat/static/js/lib/issue-list.js

@@ -270,6 +270,7 @@ issueList.IssueView = Backbone.View.extend({
   initialize: function() {
     this.issues = new issueList.IssueCollection();
     // check to see if we should pre-filter results
+    // otherwise load default (unfiltered "all")
     this.loadIssues();
 
     // set up event listeners.
@@ -331,10 +332,10 @@ issueList.IssueView = Backbone.View.extend({
     var nextButton = $('.js-pagination-next');
     var prevButton = $('.js-pagination-previous');
     var isLastPage = _.bind(function() {
-      return this.issues.getNextPageNumber() == null;
+      return this.issues.getNextPage() == null;
     }, this);
     var isFirstPage = _.bind(function() {
-      return this.issues.getPreviousPageNumber() == null;
+      return this.issues.getPrevPage() == null;
     }, this);
     var isSinglePage = isLastPage() && isFirstPage();
 
@@ -365,61 +366,79 @@ issueList.IssueView = Backbone.View.extend({
     e.preventDefault();
   },
   requestNextPage: function() {
-    if (this.issues.getNextPageNumber()) {
-      // chop off the last character, which is the page number
-      // TODO: this feels gross. ideally we should get the URL from the
-      // link header and send that to an API endpoint which then requests
-      // it from GitHub.
-      this.updateModelParams("page", this.issues.getNextPageNumber());
+    var nextPage;
+    if (nextPage = this.issues.getNextPage()) {
+      this.issues.url = nextPage;
+      this.fetchAndRenderIssues();
     }
   },
   requestPreviousPage: function() {
-    if (this.issues.getPreviousPageNumber()) {
-      this.updateModelParams("page", this.issues.getPreviousPageNumber());
+    var prevPage;
+    if (prevPage = this.issues.getPrevPage()) {
+      this.issues.url =  prevPage;
+      this.fetchAndRenderIssues();
     }
   },
   updateIssues: function(category) {
     // depending on what category was clicked (or if a search came in),
     // update the collection instance url property and fetch the issues.
-    var labelCategories = ['closed', 'contactready', 'needsdiagnosis', 'sitewait'];
 
-    //TODO(miket): make generic getModelParams method which can get the latest state
+    // note: until GitHub fixes a bug where requesting issues filtered by labels
+    // doesn't return pagination via Link, we get those results via the Search API.
+    var searchCategories = ['untriaged', 'contactready', 'needsdiagnosis', 'sitewait'];
+
+    // TODO(miket): make generic getModelParams method which can get the latest state
     // merge param objects and serialize
-    var paramsBag = $.extend({page: 1}, this.getPageLimit());
+    var paramsBag = $.extend({page: 1, per_page: 50}, this.getPageLimit());
     var params = $.param(paramsBag);
 
     // note: if query is the empty string, it will load all issues from the
     // '/api/issues' endpoint (which I think we want).
     if (category && category.query) {
       params = $.param($.extend(paramsBag, {q: category.query}));
       this.issues.url = '/api/issues/search?' + params;
-    } else if (_.contains(labelCategories, category)) {
+    } else if (_.contains(searchCategories, category)) {
+      this.issues.url = '/api/issues/search/' + category + '?' + params;
+    } else if (category === "closed") {
       this.issues.url = '/api/issues/category/' + category + '?' + params;
-    } else if (category === "untriaged") {
-      this.issues.url = '/api/issues/search/untriaged?' + params;
     } else {
       this.issues.url = '/api/issues?' + params;
     }
     this.fetchAndRenderIssues();
   },
   updateModelParams: function(paramKey, paramValue) {
-    var modelUrl = this.issues.url.split('?');
-    var modelPath = modelUrl[0];
-    var modelParams = modelUrl[1];
+    var decomposeUrl = function(url) {
+      var _url = url.split('?');
+      return {path: _url[0], params: _url[1]};
+    };
+    var linkUrl;
+    var newParams;
+    var modelUrl = decomposeUrl(this.issues.url);
+    var parsedModelParams = $.deparam(modelUrl.params);
 
     var updateParams = {};
     updateParams[paramKey] = paramValue;
 
+    // do we have a ?link param in the model URL from traversing pagination?
+    if (parsedModelParams.hasOwnProperty('link')) {
+      // if so, decompose link param url, merge updated params, and recompose
+      linkUrl = decomposeUrl(parsedModelParams.link);
+      newParams = $.extend($.deparam(linkUrl.params), updateParams);
+      this.issues.url = modelUrl.path + '?link=' + encodeURIComponent(linkUrl.path + '?' + $.param(newParams));
+      this.fetchAndRenderIssues();
+      return;
+    }
+
     // merge old params with passed in param data
     // $.extend will update existing object keys, and add new ones
-    var newParams = $.extend($.deparam(modelParams), updateParams);
+    newParams = $.extend($.deparam(modelUrl.params), updateParams);
 
     if (paramKey === 'per_page') {
       this._pageLimit = paramValue;
     }
 
     // construct new model URL and re-request issues
-    this.issues.url = modelPath + '?' + $.param(newParams);
+    this.issues.url = modelUrl.path + '?' + $.param(newParams);
     this.fetchAndRenderIssues();
   }
 });

webcompat/static/js/lib/models/issue.js

@@ -115,7 +115,13 @@ issueList.IssueCollection = Backbone.Collection.extend({
     } else {
       this.linkHeader = null;
     }
-    return response;
+    if (response.hasOwnProperty('items')) {
+      // Search API results return an Object with the
+      // issues array in the items key
+      return response.items;
+    } else {
+      return response;
+    }
   },
   parseHeader: function(linkHeader) {
     /* Returns an object like so:
@@ -146,28 +152,18 @@ issueList.IssueCollection = Backbone.Collection.extend({
 
     return result;
   },
-  getPageFromRel: function(relation) {
-    // GitHub will only send us a Link header if pagination is possible.
-    // if we return early with null, we'll know that next and prev pagination
-    // should be disabled.
-    if (this.linkHeader == null) {
+  getNextPage: function() {
+    if (this.linkHeader && this.linkHeader.hasOwnProperty('next')) {
+      return this.linkHeader.next;
+    } else {
       return null;
     }
-    var page;
-    // we only return the page number
-    var re = new RegExp('[&?]page=(\\d)');
-
-    if (page = (this.linkHeader.hasOwnProperty(relation) &&
-                this.linkHeader[relation].match(re))) {
-      return page[1];
+  },
+  getPrevPage: function() {
+    if (this.linkHeader && this.linkHeader.hasOwnProperty('prev')) {
+      return this.linkHeader.prev;
     } else {
       return null;
     }
   },
-  getNextPageNumber: function() {
-    return this.getPageFromRel('next');
-  },
-  getPreviousPageNumber: function() {
-    return this.getPageFromRel('prev');
-  }
 });