Skip to content

Commit 6f10b5e

Browse files
author
Mike Taylor
authored
Merge pull request #1147 from deepthivenkat/issues/1058/2
Fix #1058 - Display all comments for an issue (not just 30)
2 parents 559cb60 + 51748e4 commit 6f10b5e

4 files changed

Lines changed: 80 additions & 6 deletions

File tree

tests/test_api_urls.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,20 @@ def test_api_labels_without_auth(self):
7171
self.assertEqual(rv.status_code, 200)
7272
self.assertEqual(rv.content_type, 'application/json')
7373

74+
def test_api_comments_link_header_auth(self):
75+
'''API access to comments greater than 30 returns pagination in Link header of the response.'''
76+
query_string = {'callback': 'foo'}
77+
'''Force a JSONP callback response because it gives us the header properties we want to test.'''
78+
rv = self.app.get('/api/issues/398/comments', query_string=query_string, environ_base=headers)
79+
self.assertTrue = all(x in rv.data for x in ['Link', 'rel', 'next', 'last', 'page'])
80+
self.assertEqual(rv.status_code, 200)
81+
self.assertEqual(rv.content_type, 'application/json')
82+
'''API access to comments less than 30 does not return any link header in the response.'''
83+
rv = self.app.get('/api/issues/4/comments', query_string=query_string, environ_base=headers)
84+
self.assertTrue = not all(x in rv.data for x in ['Link', 'rel', 'next', 'last', 'page'])
85+
self.assertEqual(rv.status_code, 200)
86+
self.assertEqual(rv.content_type, 'application/json')
87+
7488
def test_api_set_labels_without_auth(self):
7589
'''API setting labels without auth returns JSON 403 error code.'''
7690
rv = self.app.post('/api/issues/1/labels',

webcompat/static/js/lib/comments.js

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,16 @@ var issues = issues || {};
77
issues.CommentsCollection = Backbone.Collection.extend({
88
model: issues.Comment,
99
url: function() {
10-
return '/api/issues/' + issueNumber + '/comments?per_page=100';
10+
return '/api/issues/' + issueNumber + '/comments?page=' + this.pageNumber;
11+
},
12+
initialize: function(options) {
13+
this.pageNumber = options.pageNumber;
14+
},
15+
fetchPage: function(options) {
16+
if (options.pageNumber) {
17+
this.pageNumber = options.pageNumber;
18+
}
19+
return this.fetch(options);
1120
}
1221
});
1322

webcompat/static/js/lib/issues.js

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@ issues.MainView = Backbone.View.extend({
371371
$(document.body).addClass('language-html');
372372
var issueNum = {number: issueNumber};
373373
this.issue = new issues.Issue(issueNum);
374-
this.comments = new issues.CommentsCollection([]);
374+
this.comments = new issues.CommentsCollection({pageNumber: 1});
375375
this.initSubViews();
376376
this.fetchModels();
377377
},
@@ -426,17 +426,18 @@ issues.MainView = Backbone.View.extend({
426426

427427
// If there are any comments, go fetch the model data
428428
if (this.issue.get('commentNumber') > 0) {
429-
this.comments.fetch(headersBag).success(_.bind(function() {
429+
this.comments.fetch(headersBag).success(_.bind(function(response) {
430430
this.addExistingComments();
431-
// the add event is fired when a model is added to the collection.
432431
this.comments.bind('add', _.bind(this.addComment, this));
433-
434432
// If there's a #hash pointing to a comment (or elsewhere)
435433
// scrollTo it.
436434
if (location.hash !== '') {
437435
var _id = $(location.hash);
438436
window.scrollTo(0, _id.offset().top);
439437
}
438+
if (response[0].lastPageNumber > 1) {
439+
this.getRemainingComments(++response[0].lastPageNumber);
440+
}
440441
}, this)).error(function() {
441442
var msg = 'There was an error retrieving issue comments. Please reload to try again.';
442443
wcEvents.trigger('flash:error', {message: msg, timeout: 4000});
@@ -453,6 +454,17 @@ issues.MainView = Backbone.View.extend({
453454
}
454455
});
455456
},
457+
458+
getRemainingComments: function(count) {
459+
//The first 30 comments for page 1 has already been loaded.
460+
//If more than 30 comments are there the remaining comments are rendered in sets of 30
461+
//in consecutive pages
462+
463+
_.each(_.range(2, count), function(i) {
464+
this.comments.fetchPage({pageNumber: i, headers: {'Accept': 'application/json'}});
465+
}, this);
466+
},
467+
456468
addComment: function(comment) {
457469
// if there's a nsfw label, add the whatever class.
458470
var view = new issues.CommentView({model: comment});

webcompat/static/js/lib/models/comment.js

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ issues.Comment = Backbone.Model.extend({
1616
url: function() {
1717
return '/api/issues/' + issueNumber + '/comments';
1818
},
19-
parse: function(response) {
19+
parse: function(response, jqXHR) {
2020
this.set({
2121
avatarUrl: response.user.avatar_url,
2222
body: md.render(response.body),
@@ -25,5 +25,44 @@ issues.Comment = Backbone.Model.extend({
2525
createdAt: moment(response.created_at).fromNow(),
2626
rawBody: response.body
2727
});
28+
var linkHeader = jqXHR.xhr.getResponseHeader('Link');
29+
if (linkHeader !== null && !!this.parseHeader(linkHeader).last) {
30+
response.lastPageNumber = this.parseHeader(linkHeader).last.split('\?page\=')[1];
31+
}
32+
else {
33+
response.lastPageNumber = '1';
34+
}
35+
},
36+
parseHeader: function(linkHeader) {
37+
//TODO: Abstract 'parseHeader' method from comment.js in to a mixin
38+
//See Issue #1118
39+
/* Returns an object like so:
40+
{
41+
next: "comments?page=3",
42+
last: "comments?page=4",
43+
first: "comments?page=1",
44+
prev: "comments?page=1"
45+
} */
46+
var result = {};
47+
var entries = linkHeader.split(',');
48+
var relsRegExp = /\brel="?([^"]+)"?\s*;?/;
49+
var keysRegExp = /(\b[0-9a-z\.-]+\b)/g;
50+
var sourceRegExp = /^<(.*)>/;
51+
52+
for (var i = 0; i < entries.length; i++) {
53+
var entry = entries[i].trim();
54+
var rels = relsRegExp.exec(entry);
55+
if (rels) {
56+
var keys = rels[1].match(keysRegExp);
57+
var source = sourceRegExp.exec(entry)[1];
58+
var k;
59+
var kLength = keys.length;
60+
for (k = 0; k < kLength; k += 1) {
61+
result[keys[k]] = source;
62+
}
63+
}
64+
}
65+
66+
return result;
2867
}
2968
});

0 commit comments

Comments
 (0)