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)
?