Patch #26030
Like issues and news comments, want to specify the display order of the forum's reply.
Status: | New | Start date: | ||
---|---|---|---|---|
Priority: | Normal | Due date: | ||
Assignee: | - | % Done: | 0% | |
Category: | Forums | |||
Target version: | Candidate for next major release |
Description
This patch also applies the setting order of comments in "My account" to the forum.
Related issues
History
#1
Updated by Go MAEDA over 5 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.
#2
Updated by Minoru Maeda over 5 years ago
- File reply_display_order_with_pre_page.patch
added
Hello, Go MAEDA. Thank you for your advice.
I improved it in order of reply display considering "per_page".
#3
Updated by Toshi MARUYAMA over 5 years ago
- Related to Feature #26033: Respect "Objects per page" option when displaying forum replies added
#4
Updated by Toshi MARUYAMA over 5 years ago
- Related to Patch #11120: Order replies of messages boards based on user preference added
#5
Updated by Go MAEDA over 5 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.
#6
Updated by Popoki Tom (@cat_in_136) over 3 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
- 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
#7
Updated by Popoki Tom (@cat_in_136) over 3 years ago
- File reply_display_order_with_pre_page_2.patch
added
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/
.
#8
Updated by Marius BALTEANU over 3 years ago
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.
#9
Updated by Go MAEDA over 3 years ago
- Target version changed from 4.1.0 to 4.2.0
#10
Updated by Popoki Tom (@cat_in_136) over 3 years ago
- File reply_display_order_with_pre_page_3.patch
added
Opps, Thank you for checking my patch. I've removed the duplicated MessageTest
.
#11
Updated by Popoki Tom (@cat_in_136) about 3 years ago
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.
#12
Updated by Popoki Tom (@cat_in_136) over 2 years ago
- File reply_display_order_with_pre_page_4.patch
added
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.
#13
Updated by Marius BALTEANU almost 2 years ago
- Target version changed from 4.2.0 to 5.0.0
#14
Updated by Marius BALTEANU 11 months ago
- Target version changed from 5.0.0 to Candidate for next major release