Commit ea20a3dc19a595f36b9c153b6a897485a792b53f
Exists in
web_steps_improvements
and in
10 other branches
Merge branch 'fix-article-comments-ordering' into 'master'
Fix article comments ordering Fix article's comments ordering bug (had wrong logic before correction) and pagination missing bug (js was overwriting pagination after ajax request). References: https://softwarepublico.gov.br/gitlab/softwarepublico/softwarepublico/issues/630 https://softwarepublico.gov.br/gitlab/softwarepublico/softwarepublico/issues/632 See merge request !792
Showing
5 changed files
with
35 additions
and
11 deletions
Show diff stats
app/controllers/public/content_viewer_controller.rb
| @@ -64,11 +64,7 @@ class ContentViewerController < ApplicationController | @@ -64,11 +64,7 @@ class ContentViewerController < ApplicationController | ||
| 64 | process_comments(params) | 64 | process_comments(params) |
| 65 | 65 | ||
| 66 | if request.xhr? and params[:comment_order] | 66 | if request.xhr? and params[:comment_order] |
| 67 | - if @comment_order == 'newest' | ||
| 68 | - @comments = @comments.reverse | ||
| 69 | - end | ||
| 70 | - | ||
| 71 | - return render :partial => 'comment/comment', :collection => @comments | 67 | + return render :partial => 'comment/comments_with_pagination' |
| 72 | end | 68 | end |
| 73 | 69 | ||
| 74 | if params[:slideshow] | 70 | if params[:slideshow] |
| @@ -275,8 +271,12 @@ class ContentViewerController < ApplicationController | @@ -275,8 +271,12 @@ class ContentViewerController < ApplicationController | ||
| 275 | @comments = @page.comments.without_spam | 271 | @comments = @page.comments.without_spam |
| 276 | @comments = @plugins.filter(:unavailable_comments, @comments) | 272 | @comments = @plugins.filter(:unavailable_comments, @comments) |
| 277 | @comments_count = @comments.count | 273 | @comments_count = @comments.count |
| 278 | - @comments = @comments.without_reply.paginate(:per_page => per_page, :page => params[:comment_page] ) | ||
| 279 | @comment_order = params[:comment_order].nil? ? 'oldest' : params[:comment_order] | 274 | @comment_order = params[:comment_order].nil? ? 'oldest' : params[:comment_order] |
| 275 | + @comments = @comments.without_reply | ||
| 276 | + if @comment_order == 'newest' | ||
| 277 | + @comments = @comments.reverse | ||
| 278 | + end | ||
| 279 | + @comments = @comments.paginate(:per_page => per_page, :page => params[:comment_page] ) | ||
| 280 | end | 280 | end |
| 281 | 281 | ||
| 282 | private | 282 | private |
app/views/content_viewer/view_page.html.erb
| @@ -81,7 +81,7 @@ | @@ -81,7 +81,7 @@ | ||
| 81 | <ul class="article-comments-list"> | 81 | <ul class="article-comments-list"> |
| 82 | <% if @comments.present? %> | 82 | <% if @comments.present? %> |
| 83 | <%= render :partial => 'comment/comment', :collection => @comments %> | 83 | <%= render :partial => 'comment/comment', :collection => @comments %> |
| 84 | - <%= pagination_links @comments, :param_name => 'comment_page' %> | 84 | + <%= pagination_links @comments, :param_name => 'comment_page', :params => { :comment_order => @comment_order } %> |
| 85 | <% end %> | 85 | <% end %> |
| 86 | </ul> | 86 | </ul> |
| 87 | 87 |
public/javascripts/comment_order.js
| @@ -10,10 +10,9 @@ function send_order(order, url) { | @@ -10,10 +10,9 @@ function send_order(order, url) { | ||
| 10 | }); | 10 | }); |
| 11 | } | 11 | } |
| 12 | 12 | ||
| 13 | - | ||
| 14 | jQuery(document).ready(function(){ | 13 | jQuery(document).ready(function(){ |
| 15 | jQuery("#comment_order").change(function(){ | 14 | jQuery("#comment_order").change(function(){ |
| 16 | - var url = jQuery("#page_url").val(); | 15 | + var url = window.location.href; |
| 17 | send_order(this.value, url); | 16 | send_order(this.value, url); |
| 18 | }); | 17 | }); |
| 19 | -}); | ||
| 20 | \ No newline at end of file | 18 | \ No newline at end of file |
| 19 | +}); |
test/functional/content_viewer_controller_test.rb
| @@ -328,6 +328,27 @@ class ContentViewerControllerTest < ActionController::TestCase | @@ -328,6 +328,27 @@ class ContentViewerControllerTest < ActionController::TestCase | ||
| 328 | assert_tag :content => /list my comment/ | 328 | assert_tag :content => /list my comment/ |
| 329 | end | 329 | end |
| 330 | 330 | ||
| 331 | + should 'order comments according to comments ordering option' do | ||
| 332 | + article = fast_create(Article, :profile_id => profile.id) | ||
| 333 | + for n in 1..24 | ||
| 334 | + article.comments.create!(:author => profile, :title => "some title #{n}", :body => "some body #{n}") | ||
| 335 | + end | ||
| 336 | + | ||
| 337 | + get 'view_page', :profile => profile.identifier, :page => article.path.split('/') | ||
| 338 | + | ||
| 339 | + for i in 1..12 | ||
| 340 | + assert_tag :tag => 'div', :attributes => { :class => 'comment-details' }, :descendant => { :tag => 'h4', :content => "some title #{i}" } | ||
| 341 | + assert_no_tag :tag => 'div', :attributes => { :class => 'comment-details' }, :descendant => { :tag => 'h4', :content => "some title #{i + 12}" } | ||
| 342 | + end | ||
| 343 | + | ||
| 344 | + xhr :get, :view_page, :profile => profile.identifier, :page => article.path.split('/'), :comment_page => 1, :comment_order => 'newest' | ||
| 345 | + | ||
| 346 | + for i in 1..12 | ||
| 347 | + assert_no_tag :tag => 'div', :attributes => { :class => 'comment-details' }, :descendant => { :tag => 'h4', :content => "some title #{i}" } | ||
| 348 | + assert_tag :tag => 'div', :attributes => { :class => 'comment-details' }, :descendant => { :tag => 'h4', :content => "some title #{i + 12}" } | ||
| 349 | + end | ||
| 350 | + end | ||
| 351 | + | ||
| 331 | should 'redirect to new article path under an old path' do | 352 | should 'redirect to new article path under an old path' do |
| 332 | p = create_user('test_user').person | 353 | p = create_user('test_user').person |
| 333 | a = p.articles.create(:name => 'old-name') | 354 | a = p.articles.create(:name => 'old-name') |
| @@ -1351,7 +1372,7 @@ class ContentViewerControllerTest < ActionController::TestCase | @@ -1351,7 +1372,7 @@ class ContentViewerControllerTest < ActionController::TestCase | ||
| 1351 | assert_equal 15, article.comments.count | 1372 | assert_equal 15, article.comments.count |
| 1352 | 1373 | ||
| 1353 | get 'view_page', :profile => profile.identifier, :page => article.path.split('/') | 1374 | get 'view_page', :profile => profile.identifier, :page => article.path.split('/') |
| 1354 | - assert_tag :tag => 'a', :attributes => { :href => "/#{profile.identifier}/#{article.path}?comment_page=2", :rel => 'next' } | 1375 | + assert_tag :tag => 'a', :attributes => { :href => "/#{profile.identifier}/#{article.path}?comment_order=oldest&comment_page=2", :rel => 'next' } |
| 1355 | end | 1376 | end |
| 1356 | 1377 | ||
| 1357 | should 'not escape acceptable HTML in list of blog posts' do | 1378 | should 'not escape acceptable HTML in list of blog posts' do |