Project

General

Profile

Actions

Patch #26030

open

Like issues and news comments, want to specify the display order of the forum's reply.

Added by Minoru Maeda almost 7 years ago. Updated almost 2 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
Forums
Start date:
Due date:
% Done:

0%

Estimated time:

Description

This patch also applies the setting order of comments in "My account" to the forum.


Files

reply_order.patch (2.82 KB) reply_order.patch Minoru Maeda, 2017-05-24 11:23
reply_display_order_with_pre_page.patch (3.87 KB) reply_display_order_with_pre_page.patch Minoru Maeda, 2017-05-25 08:51
reply_display_order_with_pre_page_2.patch (8.97 KB) reply_display_order_with_pre_page_2.patch Popoki Tom (@cat_in_136), 2019-09-01 12:48
reply_display_order_with_pre_page_3.patch (7.46 KB) reply_display_order_with_pre_page_3.patch Popoki Tom (@cat_in_136), 2019-09-28 14:52
reply_display_order_with_pre_page_4.patch (7.55 KB) reply_display_order_with_pre_page_4.patch Popoki Tom (@cat_in_136), 2020-07-05 09:44

Related issues

Related to Redmine - Feature #26033: Respect "Objects per page" option when displaying forum repliesClosed

Actions
Related to Redmine - Patch #11120: Order replies of messages boards based on user preferenceNewGo MAEDA

Actions
Actions #1

Updated by Go MAEDA almost 7 years ago

Thank you for posting the patch.
But I think it would be better not to assume that the value of REPLIES_PER_PAGE constant is always 25. The test in your patch may fail if #26033 is implemented.

Actions #2

Updated by Minoru Maeda almost 7 years ago

Hello, Go MAEDA. Thank you for your advice.
I improved it in order of reply display considering "per_page".

Actions #3

Updated by Toshi MARUYAMA over 6 years ago

  • Related to Feature #26033: Respect "Objects per page" option when displaying forum replies added
Actions #4

Updated by Toshi MARUYAMA over 6 years ago

  • Related to Patch #11120: Order replies of messages boards based on user preference added
Actions #5

Updated by Go MAEDA over 6 years ago

  • Target version set to 4.1.0

The patch works fine for me. And it also implements #26033.
Setting target version to 3.5.0.

Actions #6

Updated by Popoki Tom (@cat_in_136) almost 5 years ago

This patch is great work but sometimes did not work well on my environment.

my environment:
  • MySQL 5.6.42 (ClearDB)
  • Ruby 2.5.5p157
  • Redmine 4.0.3 + reply_display_order_with_pre_page.patch
When showing the topic where
  • count of the topic children is larger than per_page_option, and
  • some children has more than one attachment files

In which case, r=xx parameter may not be expanded correctly due to incorrect paging.

On my environment, @replies.pluck(:id) generated an array containing duplicate IDs.
I could not identify the root cause, but I found a line includes(:author, :attachments, {:board => :project}) cause this problem.
Therefore, I corrected this problem as follows to avoid this problem:

--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -34,7 +34,6 @@ class MessagesController < ApplicationController
     # Find the page of the requested reply
     replies_order = User.current.wants_comments_in_reverse_order? ? 'DESC' : 'ASC'
     @replies = @topic.children.
-      includes(:author, :attachments, {:board => :project}).
       reorder("#{Message.table_name}.created_on #{replies_order}, #{Message.table_name}.id #{replies_order}")

     if params[:r] && page.nil?
@@ -45,6 +44,7 @@ class MessagesController < ApplicationController
     @reply_count = @replies.count
     @reply_pages = Paginator.new @reply_count, per_page_option, page
     @replies = @replies.
+      includes(:author, :attachments, {:board => :project}).
       limit(@reply_pages.per_page).
       offset(@reply_pages.offset).
       to_a
Actions #7

Updated by Popoki Tom (@cat_in_136) over 4 years ago

I improved Minoru-Maeda-san's patch. My patch includes:

  • This issue #26030 itself;
  • display considering "per_page" (#26033);
  • place "reply" block considering the display order (like #31438);
  • fix regression that I reported in #26030#note-6;
  • fix regression where redirection after deleting a message raises an error; and
  • fix test codes to pass rails test -n /Message.*Test/.
Actions #8

Updated by Marius BĂLTEANU over 4 years ago

Popoki Tom (Popoki Tom (@cat_in_136)) wrote:

I improved Minoru-Maeda-san's patch. My patch includes:

  • This issue #26030 itself;
  • display considering "per_page" (#26033);
  • place "reply" block considering the display order (like #31438);
  • fix regression that I reported in #26030#note-6;
  • fix regression where redirection after deleting a message raises an error; and
  • fix test codes to pass rails test -n /Message.*Test/.

Thanks for updating the patch. Can you check the content of test/integration/messages_test.rb? It seems the class class MessagesTest is duplicated.

Actions #9

Updated by Go MAEDA over 4 years ago

  • Target version changed from 4.1.0 to 4.2.0
Actions #10

Updated by Popoki Tom (@cat_in_136) over 4 years ago

Opps, Thank you for checking my patch. I've removed the duplicated MessageTest.

Actions #11

Updated by Popoki Tom (@cat_in_136) about 4 years ago

Popoki Tom (Popoki Tom (@cat_in_136)) wrote:

Opps, Thank you for checking my patch. I've removed the duplicated MessageTest.

My patch has still a some degrade bug where r=xx parameter may not be expanded correctly. This patch should not be merged.

Actions #12

Updated by Popoki Tom (@cat_in_136) over 3 years ago

4th revision of the patch attached.

I've rewrite MessageController#show to fix degrade bug where `r=xx` parameter may not be expanded correctly.
My internal testing (dogfooding) is on-going on my private redmine instance.

Actions #13

Updated by Marius BĂLTEANU almost 3 years ago

  • Target version changed from 4.2.0 to 5.0.0
Actions #14

Updated by Marius BĂLTEANU almost 2 years ago

  • Target version changed from 5.0.0 to Candidate for next major release
Actions

Also available in: Atom PDF