Feature #31427
closedInsert a link to the source to the attribution line when quoting a note or a message
Added by Mizuki ISHIKAWA about 5 years ago. Updated about 5 years ago.
0%
Description
Files
0002-Change-the-quote-content.patch (4.88 KB) 0002-Change-the-quote-content.patch | Mizuki ISHIKAWA, 2019-05-23 04:37 | ||
0001-Add-text_user_wrote_in.patch (40.9 KB) 0001-Add-text_user_wrote_in.patch | Mizuki ISHIKAWA, 2019-05-23 04:37 | ||
31427@2x.png (8.95 KB) 31427@2x.png | Go MAEDA, 2019-05-23 15:09 | ||
note-11-diff-and-test.patch (2.02 KB) note-11-diff-and-test.patch | Mizuki ISHIKAWA, 2019-06-19 11:14 | ||
0001-Fix-missing-journal-indice-when-updating-a-journal.patch (1.83 KB) 0001-Fix-missing-journal-indice-when-updating-a-journal.patch | Marius BĂLTEANU, 2019-06-20 23:34 |
Related issues
Updated by Mizuki ISHIKAWA about 5 years ago
- File 0001-Add-text_user_wrote_in.patch 0001-Add-text_user_wrote_in.patch added
- File 0002-Change-the-quote-content.patch 0002-Change-the-quote-content.patch added
I attached a patch to realize this feature.
Updated by Go MAEDA about 5 years ago
- File 31427@2x.png 31427@2x.png added
- Target version set to Candidate for next major release
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
Updated by Go MAEDA about 5 years ago
- Target version changed from Candidate for next major release to 4.1.0
LGTM. Setting the target version to 4.1.0.
Updated by Go MAEDA about 5 years ago
- Subject changed from Add a link to the source when you quote to Add a link to the source to the attribution line when quoting a note or a message
Updated by Go MAEDA about 5 years ago
- Subject changed from Add a link to the source to the attribution line when quoting a note or a message to Insert a link to the source to the attribution line when quoting a note or a message
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the patch. Thank you for the nice improvement.
Updated by Marius BĂLTEANU about 5 years ago
- Status changed from Closed to Reopened
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:
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
My proposal is to send the journal_indice
as param:
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),
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)
?
Updated by Go MAEDA about 5 years ago
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
withl(: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.
Updated by Marius BĂLTEANU about 5 years ago
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
withl(:text_user_wrote, @message.author)
?I think we should not replace
ll
withl
method. The current implementation uses the default language of Redmine (Setting.default_language
), butl
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.
Updated by Go MAEDA about 5 years ago
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
withl(:text_user_wrote, @message.author)
?I think we should not replace
ll
withl
method. The current implementation uses the default language of Redmine (Setting.default_language
), butl
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.
Updated by Marius BĂLTEANU about 5 years ago
- Status changed from Reopened to Closed
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.
Updated by Mizuki ISHIKAWA about 5 years ago
- Status changed from Closed to Reopened
There is a problem that an incomplete link is generated as follows.
Redmine Admin wrote in #note-: > messageReproduction 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.
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
Updated by Mizuki ISHIKAWA about 5 years ago
I attached a patch that added a test to #31427#note-11 's change.
Updated by Marius BĂLTEANU about 5 years ago
Mizuki, why we don't check the value of indice
inside the render_journal_actions
method?
Something like that (tested briefly):
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),
Updated by Mizuki ISHIKAWA about 5 years ago
Marius BALTEANU wrote:
Mizuki, why we don't check the value of
indice
inside therender_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.
Updated by Jean-Philippe Lang about 5 years ago
- Target version changed from 4.1.0 to 4.2.0
Updated by Marius BĂLTEANU about 5 years ago
Updated by Go MAEDA about 5 years ago
Marius BALTEANU wrote:
Should these commits (r18216, r18217 and r18218) be reverted now that the target version was changed to 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.
Updated by Jean-Philippe Lang about 5 years ago
- Target version changed from 4.2.0 to 4.1.0
Updated by Marius BĂLTEANU about 5 years ago
- File 0001-Fix-missing-journal-indice-when-updating-a-journal.patch 0001-Fix-missing-journal-indice-when-updating-a-journal.patch added
Here is the patch.
All tests pass: https://gitlab.com/redmine-org/redmine/pipelines/67280161
Updated by Mizuki ISHIKAWA about 5 years ago
Marius BALTEANU wrote:
Here is the patch.
All tests pass: https://gitlab.com/redmine-org/redmine/pipelines/67280161
I confirmed that the patch solves the problem.
Updated by Go MAEDA about 5 years ago
- Status changed from Reopened to Closed
Committed the fix attached in #31427#note-19. Thanks.
Updated by Mischa The Evil 8 months ago
- Related to Defect #39877: Invalid reference in notes reply after deleting some journal-notes added