I attached a patch to realize this feature.
--------------------------------------------------------------------------------
I think this patch is useful because:

* You can clearly understand the original comment
* You can easily go to the original comment by clicking "#note-nnn" link

!31427@2x.png!
--------------------------------------------------------------------------------
LGTM. Setting the target version to 4.1.0.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Committed the patch. Thank you for the nice improvement.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Committed the patch. Thank you for the nice improvement.

Indeed, it's a nice and useful improvement, but I would to review the implementation:

1. @indice = @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice@ generates a lot of unnecessary queries:
<pre><code>
D, [2019-06-02T14:14:13.337124 #12] DEBUG -- : Issue Load (0.2ms) SELECT `issues`.* FROM `issues` WHERE `issues`.`id` = 14 LIMIT 1
D, [2019-06-02T14:14:13.338159 #12] DEBUG -- : Journal Load (0.3ms) SELECT `journals`.* FROM `journals` WHERE `journals`.`journalized_id` = 14 AND `journals`.`journalized_type` = 'Issue' ORDER BY `journals`.`created_on` ASC, `journals`.`id` ASC
D, [2019-06-02T14:14:13.339081 #12] DEBUG -- : JournalDetail Load (0.2ms) SELECT `journal_details`.* FROM `journal_details` WHERE `journal_details`.`journal_id` = 5
D, [2019-06-02T14:14:13.340836 #12] DEBUG -- : User Load (0.9ms) SELECT `users`.* FROM `users` WHERE `users`.`type` IN ('User', 'AnonymousUser') AND `users`.`id` = 2
D, [2019-06-02T14:14:13.346077 #12] DEBUG -- : EmailAddress Load (0.8ms) SELECT `email_addresses`.* FROM `email_addresses` WHERE `email_addresses`.`is_default` = TRUE AND `email_addresses`.`user_id` = 2
D, [2019-06-02T14:14:13.347298 #12] DEBUG -- : CACHE Project Load (0.0ms) SELECT `projects`.* FROM `projects` WHERE `projects`.`id` = 3 LIMIT 1 [["id", 3], ["LIMIT", 1]]
D, [2019-06-02T14:14:13.350107 #12] DEBUG -- : CACHE (0.9ms) SELECT `enabled_modules`.`name` FROM `enabled_modules` WHERE `enabled_modules`.`project_id` = 3
</code></pre>

My proposal is to send the @journal_indice@ as param:
<pre><code class="diff">
diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb
index a0a0352..7a477a0 100644
--- a/app/controllers/journals_controller.rb
+++ b/app/controllers/journals_controller.rb
@@ -66,8 +66,7 @@ class JournalsController < ApplicationController
if @journal
user = @journal.user
text = @journal.notes
- indice = @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice
- @content = +"#{ll(Setting.default_language, :text_user_wrote_in, {:value => user, :link => "#note-#{indice}"})}\n> "
+ @content = +"#{ll(Setting.default_language, :text_user_wrote_in, {:value => user, :link => "#note-#{params[:journal_indice]}"})}\n> "
else
user = @issue.author
text = @issue.description
diff --git a/app/helpers/journals_helper.rb b/app/helpers/journals_helper.rb
index d6182bb..809afb4 100644
--- a/app/helpers/journals_helper.rb
+++ b/app/helpers/journals_helper.rb
@@ -31,7 +31,7 @@ module JournalsHelper
if journal.notes.present?
if options[:reply_links]
links << link_to(l(:button_quote),
- quoted_issue_path(issue, :journal_id => journal),
+ quoted_issue_path(issue, :journal_id => journal, :journal_indice => journal.indice),
:remote => true,
:method => 'post',
:title => l(:button_quote),
</code></pre>

2. I think it's better to move the newly 2 conditions from the views in a single method which can be used in both views because the logic is the same. We can add to that method the @strip.gsub@ as well.
3. This is not related to the current changes, but is there any reason for why we don't replace the @ll(Setting.default_language, :text_user_wrote, @message.author@ with @l(:text_user_wrote, @message.author)@?

--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> 1. @indice = @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice@ generates a lot of unnecessary queries:
> [...]
>
> My proposal is to send the @journal_indice@ as param:
> [...]

Committed the proposed code in r18218. Thanks.

> 2. I think it's better to move the newly 2 conditions from the views in a single method which can be used in both views because the logic is the same. We can add to that method the @strip.gsub@ as well.

I am afraid that adding a method increases complexity because the code in JournalsController and MessagesController are very similar but slightly different.

> 3. This is not related to the current changes, but is there any reason for why we don't replace the @ll(Setting.default_language, :text_user_wrote, @message.author@ with @l(:text_user_wrote, @message.author)@?

I think we should not replace @ll@ with @l@ method. The current implementation uses the default language of Redmine (@Setting.default_language@), but @l@ method uses the current user's default language (@User.current.language@). Probably JPL wanted to use the same language for attribution lines in the same Redmine instance, independent from user's language.

If the code uses the current user's language, you will see many languages of @text_user_wrote@ / @text_user_wrote_in@ in the journal of the same issue.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Marius BALTEANU wrote:
> > 1. @indice = @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice@ generates a lot of unnecessary queries:
> > [...]
> >
> > My proposal is to send the @journal_indice@ as param:
> > [...]
>
> Committed the proposed code in r18218. Thanks.
Thanks!

>
> > 2. I think it's better to move the newly 2 conditions from the views in a single method which can be used in both views because the logic is the same. We can add to that method the @strip.gsub@ as well.
>
> I am afraid that adding a method increases complexity because the code in JournalsController and MessagesController are very similar but slightly different.
Ok, your decision. I see a little bit different, but it is not so important.

> > 3. This is not related to the current changes, but is there any reason for why we don't replace the @ll(Setting.default_language, :text_user_wrote, @message.author@ with @l(:text_user_wrote, @message.author)@?
>
> I think we should not replace @ll@ with @l@ method. The current implementation uses the default language of Redmine (@Setting.default_language@), but @l@ method uses the current user's default language (@User.current.language@). Probably JPL wanted to use the same language for attribution lines in the same Redmine instance, independent from user's language.
>
> If the code uses the current user's language, you will see many languages of @text_user_wrote@ / @text_user_wrote_in@ in the journal of the same issue.

Not really, the user will see the @text_user_wrote@ / @text_user_wrote_in@ only in his language. Should we open a new ticket and assign to Jean-Philippe? I find it more relevant for the user to see the text in his language and not to force instance default language.
--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> > > 3. This is not related to the current changes, but is there any reason for why we don't replace the @ll(Setting.default_language, :text_user_wrote, @message.author@ with @l(:text_user_wrote, @message.author)@?
> >
> > I think we should not replace @ll@ with @l@ method. The current implementation uses the default language of Redmine (@Setting.default_language@), but @l@ method uses the current user's default language (@User.current.language@). Probably JPL wanted to use the same language for attribution lines in the same Redmine instance, independent from user's language.
> >
> > If the code uses the current user's language, you will see many languages of @text_user_wrote@ / @text_user_wrote_in@ in the journal of the same issue.
>
> Not really, the user will see the @text_user_wrote@ / @text_user_wrote_in@ only in his language. Should we open a new ticket and assign to Jean-Philippe? I find it more relevant for the user to see the text in his language and not to force instance default language.

Yes, could you open a new issue in order to close this issue? I am in favor of the current behavior and I don't want to change the behavior without his approval.

I think using the instance default language is better. Consider a team with members of multiple nationalities and they are communicating in English. In this case, the English version of text_user_wrote should be used. But if a member set his language to Japanese, other members who don't understand Japanese will see "John Smith さんは書きました" instead of "John Smith wrote".

Actually, some of my coworkers set his language to Japanese on www.redmine.org. But I think the attribution line in their post on redmine.org should be written in English, not Japanese.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> I think using the instance default language is better. Consider a team with members of multiple nationalities and they are communicating in English. In this case, the English version of text_user_wrote should be used. But if a member set his language to Japanese, other members who don't understand Japanese will see "John Smith さんは書きました" instead of "John Smith wrote".
>
> Actually, some of my coworkers set his language to Japanese on www.redmine.org. But I think the attribution line in their post on redmine.org should be written in English, not Japanese.

Oh, I finally got it and you're perfectly right. Thanks for taking the time to explain to me, I totally missed the part where the translation of "text_user_wrote" will be stored as plain text as part of the note. Sorry for this.

--------------------------------------------------------------------------------
There is a problem that an incomplete link is generated as follows.
<pre>
Redmine Admin wrote in #note-:
> message
</pre>

Reproduction procedure:
* Edit an existing note by clicking the pencil icon
* Quote the edited note

The value of indice is defined in attr_accessor, so it is not maintained after being updated by ajax.
If params[:journal_indice] does not exist, it should be solved by recalculating indice with Issue#visible_journals_with_index.

<pre><code class="diff">
diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb
index 7a477a0b14..3a3248c908 100644
--- a/app/controllers/journals_controller.rb
+++ b/app/controllers/journals_controller.rb
@@ -66,7 +66,11 @@ class JournalsController < ApplicationController
if @journal
user = @journal.user
text = @journal.notes
- @content = +"#{ll(Setting.default_language, :text_user_wrote_in, {:value => user, :link => "#note-#{params[:journal_indice]}"})}\n> "
+ indice = params[:journal_indice]
+ if indice.blank?
+ indice = @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice
+ end
+ @content = +"#{ll(Setting.default_language, :text_user_wrote_in, {:value => user, :link => "#note-#{indice}"})}\n> "
else
user = @issue.author
text = @issue.description

</code></pre>
--------------------------------------------------------------------------------
I attached a patch that added a test to #31427#note-11 's change.
--------------------------------------------------------------------------------
Mizuki, why we don't check the value of @indice@ inside the @render_journal_actions@ method?

Something like that (tested briefly):
<pre><code class="diff">
vagrant@jessie:/vagrant/project/redmine$ git diff
diff --git a/app/helpers/journals_helper.rb b/app/helpers/journals_helper.rb
index 809afb4..391a432 100644
--- a/app/helpers/journals_helper.rb
+++ b/app/helpers/journals_helper.rb
@@ -30,8 +30,9 @@ module JournalsHelper
links = []
if journal.notes.present?
if options[:reply_links]
+ indice = journal.indice || @journal.issue.visible_journals_with_index.find{|j| j.id == @journal.id}.indice
links << link_to(l(:button_quote),
- quoted_issue_path(issue, :journal_id => journal, :journal_indice => journal.indice),
+ quoted_issue_path(issue, :journal_id => journal, :journal_indice => indice),
:remote => true,
:method => 'post',
:title => l(:button_quote),
</code></pre>
--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> Mizuki, why we don't check the value of @indice@ inside the @render_journal_actions@ method?
>
> Something like that (tested briefly):
> [...]

The fix looks cleaner and looks better.
I think that it is good with this correction method.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Should these commits (r18216, r18217 and r18218) be reverted now that the target version was changed to version:"4.2.0"? Or, considering that the fix remained is not so big, I can attach tonight a new patch with my changes and the test made by Mizuki and deliver this in version:"4.1.0".
--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> Should these commits (r18216, r18217 and r18218) be reverted now that the target version was changed to version:"4.2.0"?

I don't think so. The issue reported in #31427#note-11 is small and an edge case. In addition, we can easily fix it as Marius wrote.

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

--------------------------------------------------------------------------------
Here is the patch.

All tests pass: https://gitlab.com/redmine-org/redmine/pipelines/67280161
* Rubocop: https://gitlab.com/redmine-org/redmine/-/jobs/236746558
* Tests: https://gitlab.com/redmine-org/redmine/-/jobs/236746561

--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> Here is the patch.
>
> All tests pass: https://gitlab.com/redmine-org/redmine/pipelines/67280161
> * Rubocop: https://gitlab.com/redmine-org/redmine/-/jobs/236746558
> * Tests: https://gitlab.com/redmine-org/redmine/-/jobs/236746561

I confirmed that the patch solves the problem.
--------------------------------------------------------------------------------
Committed the fix attached in #31427#note-19. Thanks.
--------------------------------------------------------------------------------