Project

General

Profile

Actions
Copy link

Feature #31427

closed

Insert a link to the source to the attribution line when quoting a note or a message

Added by Mizuki ISHIKAWA over 5 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Go MAEDA
Category:
Issues
Target version:
4.1.0
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed

Description

Since pre-tags are omitted when quote, I often go to the quote source.
I think that it is useful to link where you quoted from "John Smith wrote in #note-1:".

John Smith wrote in #note-1:

comment

The same is true when you quote a forum message.

Files

Download all 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

Related to Redmine - Defect #39877: Invalid reference in notes reply after deleting some journal-notesNew

Actions
Actions
Copy link
 #2

Updated by Go MAEDA over 5 years ago

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

Actions
Copy link
 #3

Updated by Go MAEDA over 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.

Actions
Copy link
 #4

Updated by Go MAEDA over 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
Actions
Copy link
 #5

Updated by Go MAEDA over 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.

Actions
Copy link
 #6

Updated by Marius BĂLTEANU over 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)?

Actions
Copy link
 #7

Updated by Go MAEDA over 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 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.

Actions
Copy link
 #8

Updated by Marius BĂLTEANU over 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 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.

Actions
Copy link
 #9

Updated by Go MAEDA over 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 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.

Actions
Copy link
 #10

Updated by Marius BĂLTEANU over 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.

Actions
Copy link
 #11

Updated by Mizuki ISHIKAWA over 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-:
> message

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.

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
Actions
Copy link
 #12

Updated by Mizuki ISHIKAWA over 5 years ago

I attached a patch that added a test to #31427#note-11 's change.

Actions
Copy link
 #13

Updated by Marius BĂLTEANU over 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),

Actions
Copy link
 #14

Updated by Mizuki ISHIKAWA over 5 years ago

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.

Actions
Copy link
 #15

Updated by Jean-Philippe Lang over 5 years ago

  • Target version changed from 4.1.0 to 4.2.0
Actions
Copy link
 #16

Updated by Marius BĂLTEANU over 5 years ago

Should these commits (r18216, r18217 and r18218) be reverted now that the target version was changed to 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 4.1.0.

Actions
Copy link
 #17

Updated by Go MAEDA over 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.

Actions
Copy link
 #18

Updated by Jean-Philippe Lang over 5 years ago

  • Target version changed from 4.2.0 to 4.1.0
Actions
Copy link
 #20

Updated by Mizuki ISHIKAWA over 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.

Actions
Copy link
 #21

Updated by Go MAEDA over 5 years ago

  • Status changed from Reopened to Closed

Committed the fix attached in #31427#note-19. Thanks.

Actions
Copy link
 #22

Updated by Mischa The Evil about 1 year ago

  • Related to Defect #39877: Invalid reference in notes reply after deleting some journal-notes added
Actions
Copy link

Also available in: Atom PDF