プロジェクト

全般

プロフィール

Vote #78497

未完了

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

Admin Redmine さんがほぼ2年前に追加. ほぼ2年前に更新.

ステータス:
New
優先度:
通常
担当者:
-
カテゴリ:
Forums_5
開始日:
2022/05/09
期日:
進捗率:

0%

予定工数:
category_id:
5
version_id:
32
issue_org_id:
26030
author_id:
137171
assigned_to_id:
0
comments:
14
status_id:
1
tracker_id:
3
plus1:
0
affected_version:
closed_on:
affected_version_id:
ステータス-->[New]

説明

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


journals

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.
--------------------------------------------------------------------------------
Hello, Go MAEDA. Thank you for your advice.
I improved it in order of reply display considering "per_page".
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
The patch works fine for me. And it also implements #26033.
Setting target version to 3.5.0.
--------------------------------------------------------------------------------
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:

<pre><code class="diff">
--- 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
</code></pre>

--------------------------------------------------------------------------------
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/@.
--------------------------------------------------------------------------------
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.

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

--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------


related_issues

relates,Closed,26033,Respect "Objects per page" option when displaying forum replies
relates,New,11120,Order replies of messages boards based on user preference

Admin Redmine さんがほぼ2年前に更新

  • カテゴリForums_5 にセット
  • 対象バージョンCandidate for next major release_32 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0