From 13c0404d79f5f513c229888147c32a3d898a3e3d Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Tue, 6 Feb 2018 12:11:45 +0800
Subject: [PATCH 01/17] removes uses of `raw`

---
 app/views/code_review/index.html.erb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/views/code_review/index.html.erb b/app/views/code_review/index.html.erb
index c82a9c0..4c0a2e6 100644
--- a/app/views/code_review/index.html.erb
+++ b/app/views/code_review/index.html.erb
@@ -19,7 +19,7 @@
 
 <script>
 function change_option(flag) {
-  var url = "<%= raw url_for(:controller => 'code_review', :action=>'index', :id => @project) %>";
+  var url = "<%= j url_for(:controller => 'code_review', :action=>'index', :id => @project) %>";
   $('#content').load(url, {'show_closed': flag});
 }
 </script>
@@ -80,13 +80,13 @@ function change_option(flag) {
                 codepath = review_path[0, 15] + '...' + review_path[review_path.length - 35, 35]
               end
               -%>
-              <%= link_to(raw(codepath), {:controller => 'code_review', :action => 'show', :id => @project, :review_id => review.id}, :title => review_path) -%>
+              <%= link_to(codepath, {:controller => 'code_review', :action => 'show', :id => @project, :review_id => review.id}, :title => review_path) -%>
            </td>
-            <td><%=h review.line %></td>
-            <td><%=h review.revision %></td>
-            <td><%=h review.committer %></td>
-            <td><%=h review.user.name %></td>
-            <td><%=h format_time(review.created_at) %></td>
+            <td><%= review.line %></td>
+            <td><%= review.revision %></td>
+            <td><%= review.committer %></td>
+            <td><%= review.user.name %></td>
+            <td><%= format_time(review.created_at) %></td>
           </tr>
         <% end %>
       </tbody>

From 1031d88673888e1aff9e4c3b28db76817c3f45b1 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Fri, 9 Mar 2018 19:21:08 +0800
Subject: [PATCH 02/17] removes unnecessary html escaping, it's always done
 automatically by Rails

---
 .../_change_revision_view.html.erb            |  2 +-
 app/views/code_review/_new_form.html.erb      | 16 ++++++------
 app/views/code_review/_show.html.erb          | 10 +++----
 app/views/code_review/_show_error.html.erb    | 10 +++----
 app/views/code_review/index.html.erb          | 10 +++----
 .../code_review_settings/_filters.html.erb    | 12 ++++-----
 app/views/code_review_settings/_show.html.erb | 26 +++++++++----------
 7 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/app/views/code_review/_change_revision_view.html.erb b/app/views/code_review/_change_revision_view.html.erb
index 67eb805..29af964 100644
--- a/app/views/code_review/_change_revision_view.html.erb
+++ b/app/views/code_review/_change_revision_view.html.erb
@@ -24,7 +24,7 @@ if @changeset
 %>
 <%- if User.current.allowed_to?(:assign_code_review, @project) -%>
   <div id="code_review_assignments">
-    <%=h l(:review_assignments)%>
+    <%= l(:review_assignments)%>
     <% @changeset.code_review_assignments.each do |assignment|
       issue = assignment.issue %>
       <%= link_to("##{issue.id} ", {:controller => 'issues', :action => 'show', :id => issue.id},
diff --git a/app/views/code_review/_new_form.html.erb b/app/views/code_review/_new_form.html.erb
index a560d17..23d29a5 100644
--- a/app/views/code_review/_new_form.html.erb
+++ b/app/views/code_review/_new_form.html.erb
@@ -38,13 +38,13 @@
     <%= hidden_field_tag(:repository_id, @repository_id) %>
     <%= hidden_field_tag(:diff_all, @review.diff_all) %>
     <p>
-      <label><b><%=h l(:field_subject)%>:</b></label>
+      <label><b><%= l(:field_subject)%>:</b></label>
       <%= f.text_field :subject, :size => 70, :required => true %>
     </p>
 
     <% if @setting.tracker_in_review_dialog %>
       <p>
-        <label><b><%=h l(:label_tracker) %>:</b></label>
+        <label><b><%= l(:label_tracker) %>:</b></label>
         <%= select :issue, :tracker_id, @project.trackers.collect {|t| [t.name, t.id]}, :required => true %>
         <script type="text/javascript">
 
@@ -59,11 +59,11 @@
       </p>
     <% end %>
     <p>
-      <label><b><%=h l(:field_parent_issue)%>:</b></label>
+      <label><b><%= l(:field_parent_issue)%>:</b></label>
       <%= f.text_field :parent_id, :size => 10 %>
       <% if @parent_candidate %>
         <%= raw l(:label_parent_suggestion, {:issue_id => link_to_issue(@parent_candidate)}) %>
-        <input type="button" value="<%=h l(:general_text_Yes)%>" onclick="$('#review_parent_id').val(<%= @parent_candidate.id %>)"/>
+        <input type="button" value="<%= l(:general_text_Yes)%>" onclick="$('#review_parent_id').val(<%= @parent_candidate.id %>)"/>
       <% end %>
     </p>
     <% @issue.custom_field_values.each do |value| %>
@@ -79,18 +79,18 @@
 
     </p>
     <p>
-      <label><b><%=h l(:field_assigned_to) %>:</b></label>
+      <label><b><%= l(:field_assigned_to) %>:</b></label>
       <%= select :issue, :assigned_to_id, (@issue.assignable_users.collect {|m| [m.name, m.id]}), :include_blank => true %>
     </p>
     <% unless @project.issue_categories.empty? %>
       <p>
-        <label><b><%=h l(:field_category) %>:</b></label>
+        <label><b><%= l(:field_category) %>:</b></label>
         <%= select :issue, :category_id, (@project.issue_categories.collect {|c| [c.name, c.id]}), :include_blank => true %>
       </p>
     <% end %>
     <% unless @issue.assignable_versions.empty? %>
       <p>
-        <label><b><%=h l(:field_fixed_version) %>:</b></label>
+        <label><b><%= l(:field_fixed_version) %>:</b></label>
         <%= select :issue, :fixed_version_id, (@issue.assignable_versions.collect {|v| [v.name, v.id]}), :include_blank => true, :selected => @default_version_id %>
       </p>
     <% end %>
@@ -109,7 +109,7 @@
       %>
       <%= button_tag l(:button_apply), type: 'button', onclick: "$('#review-form').load('#{submit_url}', $('#review_form').serialize2json())" %>
 
-      <input type="button" value="<%=h l(:button_cancel) %> " onclick="javascript:hideForm();"/>
+      <input type="button" value="<%= l(:button_cancel) %> " onclick="javascript:hideForm();"/>
       <%= preview_link({ :controller => 'code_review', :action => 'preview', :id => @project}, 'review_form') %>
     </p>
     <div id="preview" class="wiki"></div>
diff --git a/app/views/code_review/_show.html.erb b/app/views/code_review/_show.html.erb
index 24a1edd..9897e01 100644
--- a/app/views/code_review/_show.html.erb
+++ b/app/views/code_review/_show.html.erb
@@ -61,7 +61,7 @@
         <%= link_to h(@issue.tracker.name) + ' #' +@issue.id.to_s + ' (' + h(@review.issue.status) + ')',
           :controller => 'issues', :action => 'show', :id => @review.issue.id %>:
         <br />
-        <%=h @review.subject %>
+        <%= @review.subject %>
       </h2>
 
       <!-- author and creation time -->
@@ -94,12 +94,12 @@
           <%= hidden_field :issue, :lock_version%>
           <%= hidden_field_tag :review_id, @review.id %>
           <p>
-            <label><b><%=h l(:field_subject)%>:</b></label>
+            <label><b><%= l(:field_subject)%>:</b></label>
             <%= f.text_field :subject, :size => 70, :value => @review.subject%>
           </p>
           <% if @allowed_statuses and @allowed_statuses.any? %>
             <p>
-              <label><b><%=h l(:field_status)%>:</b></label>
+              <label><b><%= l(:field_status)%>:</b></label>
               <%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), :required => true %>
             </p>
           <% end %>
@@ -116,7 +116,7 @@
             submit_url = url_for(:controller => 'code_review', :action => 'update', :id => @project)
             -%>
             <%= button_tag l(:button_apply), type: 'button', onclick: "$('#show_review_#{@review.id}').load('#{submit_url}', $('##{review_form_id}').serialize())" %>
-            <input type="button" value="<%=h l(:button_cancel) %>" onclick='$("#review_form_<%= @review.id %>").hide();' />
+            <input type="button" value="<%= l(:button_cancel) %>" onclick='$("#review_form_<%= @review.id %>").hide();' />
             <%= link_to_function l(:label_preview), "$('#preview_#{@review.id}').load('#{url_for(:controller => 'code_review', :action => 'preview', :id => @project)}', $('##{review_form_id}').serialize())" %>
           </p>
           <div id="preview_<%= @review.id%>" class="wiki"></div>
@@ -158,7 +158,7 @@
           <%= hidden_field :issue,  :lock_version%>
           <% if @allowed_statuses and @allowed_statuses.any? %>
             <p>
-              <label><%=h l(:field_status) %>:</label>
+              <label><%= l(:field_status) %>:</label>
               <%  # select :review, :status, {l(:label_review_closed) => CodeReview::STATUS_CLOSED, l(:label_review_open) => CodeReview::STATUS_OPEN} %>
               <%= select :review, :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), :required => true %>
             </p>
diff --git a/app/views/code_review/_show_error.html.erb b/app/views/code_review/_show_error.html.erb
index 7b9c2db..774f2a3 100644
--- a/app/views/code_review/_show_error.html.erb
+++ b/app/views/code_review/_show_error.html.erb
@@ -3,14 +3,14 @@
 
   <p>change not found.</p>
   
-  @path = '<%=h @path %>'<br/>
-  @changeset = <%=h @changeset %><br/>
-  @rev = <%=h @rev %><br/>
+  @path = '<%= @path %>'<br/>
+  @changeset = <%= @changeset %><br/>
+  @rev = <%= @rev %><br/>
   <% if @changeset %>
     <ul>
     <% for change in @changeset.changes do%>
       <li>
-      '<%=h change.path%>'
+      '<%= change.path%>'
       </li>
     <% end %>
     </ul>
@@ -21,4 +21,4 @@
   var innerhtml = $('content').innerHTML;
   $('content').innerHTML = $('error_message_div').innerHTML + innerhtml;
   $('error_message_div').innerHTML = "";
-</script>
\ No newline at end of file
+</script>
diff --git a/app/views/code_review/index.html.erb b/app/views/code_review/index.html.erb
index 4c0a2e6..92537b5 100644
--- a/app/views/code_review/index.html.erb
+++ b/app/views/code_review/index.html.erb
@@ -25,13 +25,13 @@ function change_option(flag) {
 </script>
 <div id="code_review_list">
   <h2>
-    <%=h l(:code_reviews) %>
+    <%= l(:code_reviews) %>
   </h2>
 
   <% if @all_review_count > 0 %>
     <p>
       <%= form_tag({:controller => 'code_review', :action=>'index', :id => @project}, :id => 'optionform') do %>
-        <%= check_box_tag 'show_closed', 'true', @show_closed, :onchange => "change_option($('#show_closed').is(':checked'));"%> <%=h l(:label_show_closed_reviews) %>
+        <%= check_box_tag 'show_closed', 'true', @show_closed, :onchange => "change_option($('#show_closed').is(':checked'));"%> <%= l(:label_show_closed_reviews) %>
       <% end %>
       <%# observe_field 'show_closed', :with => 'show_closed', :update => 'content' %>
     </p>
@@ -50,7 +50,7 @@ function change_option(flag) {
           <%= sort_header_tag "#{Issue.table_name}.subject", :caption => l(:field_subject)%>
           <%= sort_header_tag 'path', :caption => l(:label_code_path)%>
           <th>
-            <%=h l(:label_code_line)%>
+            <%= l(:label_code_line)%>
           </th>
           <%= sort_header_tag "#{Changeset.table_name}.revision", :caption => l(:label_revision)%>
           <%= sort_header_tag "#{Changeset.table_name}.committer", :caption => l(:label_code_author)%>
@@ -67,10 +67,10 @@ function change_option(flag) {
             </td>
 
             <td class="status">
-              <%=h  review.issue.status %>
+              <%=  review.issue.status %>
             </td>
             <td class="subject">
-              <%=h  review.issue.subject %>
+              <%=  review.issue.subject %>
             </td>
             <td class="path">
               <%
diff --git a/app/views/code_review_settings/_filters.html.erb b/app/views/code_review_settings/_filters.html.erb
index a5ebbf0..54d4741 100644
--- a/app/views/code_review_settings/_filters.html.erb
+++ b/app/views/code_review_settings/_filters.html.erb
@@ -24,8 +24,8 @@
 </script>
 
 <p>
-  <label><%=h l(:label_auto_assignment_filters)%></label>
-  <%= check_box_tag 'auto_assign[filter_enabled]', true, @auto_assign.filter_enabled?, :onchange => 'setAutoAssignSettingFiltersEnable();' %><%=h l(:button_activate)%>
+  <label><%= l(:label_auto_assignment_filters)%></label>
+  <%= check_box_tag 'auto_assign[filter_enabled]', true, @auto_assign.filter_enabled?, :onchange => 'setAutoAssignSettingFiltersEnable();' %><%= l(:button_activate)%>
 
 </p>
 <div id="auto_assignment_filter_form" style="display: <%=  @auto_assign.filter_enabled? ? 'true' : 'none'%>;">
@@ -37,9 +37,9 @@
       <thead>
         <tr>
           <th style="width: 1em; text-align: right;">#</th>
-          <th style="width: 8em;"><%=h l(:auto_assign_filter_assign)%>/<%=h l(:auto_assign_filter_drop) %></th>
-          <th><%=h l(:auto_assign_filter_expression)%> </th>
-          <th><%=h l(:button_sort) %> </th>
+          <th style="width: 8em;"><%= l(:auto_assign_filter_assign)%>/<%= l(:auto_assign_filter_drop) %></th>
+          <th><%= l(:auto_assign_filter_expression)%> </th>
+          <th><%= l(:button_sort) %> </th>
           <th></th>
         </tr>
       </thead>
@@ -119,7 +119,7 @@
 
   </div>
   <p>
-    <label><%=h l(:label_default)%></label>
+    <label><%= l(:label_default)%></label>
     <%= select(:auto_assign, :accept_for_default,  [[l(:auto_assign_filter_assign), true], [l(:auto_assign_filter_drop), false]]) %>
   </p>
 
diff --git a/app/views/code_review_settings/_show.html.erb b/app/views/code_review_settings/_show.html.erb
index 74454d4..c205121 100644
--- a/app/views/code_review_settings/_show.html.erb
+++ b/app/views/code_review_settings/_show.html.erb
@@ -91,10 +91,10 @@
       <%= f.hidden_field :lock_version %>
       <p><%= f.check_box :tracker_in_review_dialog %></p>
 
-      <b><%=h l(:select_tracker_for_code_reviews)%>:</b>
+      <b><%= l(:select_tracker_for_code_reviews)%>:</b>
       <p><%= f.select :tracker_id, @project.trackers.collect {|t| [t.name, t.id]}, :required => true %></p>
 
-      <b><%=h l(:select_tracker_for_code_review_assignment)%>:</b>
+      <b><%= l(:select_tracker_for_code_review_assignment)%>:</b>
       <p><%= f.select :assignment_tracker_id, @project.trackers.collect {|t| [t.name, t.id]}, :required => true %></p>
 
       <p>
@@ -102,15 +102,15 @@
       </p>
 
       <p>
-        <label><%=h l(:label_if_revision_has_issues)%>:</label>
+        <label><%= l(:label_if_revision_has_issues)%>:</label>
         <%= f.radio_button(:auto_relation, CodeReviewProjectSetting::AUTORELATION_TYPE_RELATES) %>
-        <%=h l(:label_review_issue_relates) %>
+        <%= l(:label_review_issue_relates) %>
 
         <%= f.radio_button(:auto_relation, CodeReviewProjectSetting::AUTORELATION_TYPE_BLOCKS) %>
-        <%=h l(:label_review_issue_blocks) %>
+        <%= l(:label_review_issue_blocks) %>
 
         <%= f.radio_button(:auto_relation, CodeReviewProjectSetting::AUTORELATION_TYPE_NONE) %>
-        <%=h l(:label_review_issue_do_nothing) %>
+        <%= l(:label_review_issue_do_nothing) %>
 
       </p>
       <hr/>
@@ -120,29 +120,29 @@
         @auto_assign = @code_review_setting.auto_assign_settings
         @auto_assign.subject = l(:code_review_requrest) if @auto_assign.subject.blank?
       -%>
-        <label><%=h l(:label_auto_assign_settings)%></label>
-        <%= check_box_tag "auto_assign[enabled]", true, @auto_assign.enabled?, :onchange => 'setAutoAssignSettingFormEnable();'%><%=h l(:button_activate)%>
+        <label><%= l(:label_auto_assign_settings)%></label>
+        <%= check_box_tag "auto_assign[enabled]", true, @auto_assign.enabled?, :onchange => 'setAutoAssignSettingFormEnable();'%><%= l(:button_activate)%>
       </p>
       <div id="cr_auto_assign_settings" style="display: <%=  @auto_assign.enabled? ? 'true' : 'none'%>;">
         <p>
-          <label><%=h l(:field_author)%></label>
+          <label><%= l(:field_author)%></label>
           <%= select :auto_assign, :author_id,
             (@project.users.collect {|user|
               [user.name, user.id]
             }), :selected => @auto_assign.author_id, :required => true %>
         </p>
         <p>
-          <label><%=h l(:label_candidates_of_reviewer)%></label>
+          <label><%= l(:label_candidates_of_reviewer)%></label>
           <% @project.users.each do |user| %>
-            <label class="floating"><%= check_box_tag 'auto_assign[assignable_list][]', user.id, @auto_assign.assignable?(user) %> <%=h user %></label>
+            <label class="floating"><%= check_box_tag 'auto_assign[assignable_list][]', user.id, @auto_assign.assignable?(user) %> <%= user %></label>
           <% end %>
         </p>
         <p>
-          <label><%=h l(:field_subject)%></label>
+          <label><%= l(:field_subject)%></label>
           <%= text_field(:auto_assign, :subject, :size => 70) %>
         </p>
         <p>
-          <label><%=h l(:field_description) %></label>
+          <label><%= l(:field_description) %></label>
           <%= text_area :auto_assign, :description,
                    :cols => 30,
                    :rows => 12,

From e5a5b009b270b8d3bc747ef4a9573df5b7564ed3 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Fri, 9 Mar 2018 19:29:25 +0800
Subject: [PATCH 03/17] remove unnecessary usage of raw

---
 app/views/code_review/_change_repository_view.html.erb | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/app/views/code_review/_change_repository_view.html.erb b/app/views/code_review/_change_repository_view.html.erb
index 6c7932b..df126df 100644
--- a/app/views/code_review/_change_repository_view.html.erb
+++ b/app/views/code_review/_change_repository_view.html.erb
@@ -19,11 +19,7 @@
 <%
 
 if @changesets
-  changeset_ids = ''
-  @changesets.each { |changeset|
-    changeset_ids << changeset.revision
-    changeset_ids << ','
-  }
+  changeset_ids = safe_join @changesets.map(&:revision), ','
 
   repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
   url = url_for :controller => 'code_review', :action => 'update_revisions_view', :id => project, :repository_id => repository_id
@@ -34,8 +30,8 @@ if @changesets
 
   <script type="text/javascript">
     $(document).ready(function(){
-      $('#code_review_revisions').load('<%=url%>', {changeset_ids: '<%=raw changeset_ids%>'});
+      $('#code_review_revisions').load('<%=url%>', {changeset_ids: '<%= j changeset_ids%>'});
     });
   </script>
 
-<% end %>
\ No newline at end of file
+<% end %>

From a51fa1523a019db9456ca9a1808ca2acc9d642ef Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Fri, 9 Mar 2018 19:32:57 +0800
Subject: [PATCH 04/17] extract helper method

---
 app/helpers/code_review_helper.rb             | 24 +++++++++++++++++++
 .../code_review/_update_revisions.html.erb    | 20 ++--------------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/app/helpers/code_review_helper.rb b/app/helpers/code_review_helper.rb
index 2086c33..4417f25 100644
--- a/app/helpers/code_review_helper.rb
+++ b/app/helpers/code_review_helper.rb
@@ -34,4 +34,28 @@ def show_assignments(assignments, project, options = {})
 
     html
   end
+
+  def progress_for_changeset(changeset)
+    if changeset.review_count > 0
+      progress = '<span style="white-space: nowrap">' + progress_bar([changeset.closed_review_pourcent, changeset.completed_review_pourcent],
+        :width => '60px',
+        :legend => "#{changeset.closed_review_count}/#{changeset.review_count} #{l(:label_closed_issues)}") + '</span>'
+    elsif changeset.assignment_count > 0
+      if (changeset.open_assignment_count > 0)
+        progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_assigned) + '</p>'
+      else
+        progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_reviewed) + '</p>'
+      end
+
+    else
+      progress = '<p class="progress-info" style="white-space: nowrap;"><span  style="white-space: nowrap;">' +
+        l(:lable_no_code_reviews) +
+        '</span>:<span  style="white-space: nowrap;">' +
+        link_to(l(:label_assign_review), {:controller => 'code_review',
+          :action => 'assign', :id=>@project,
+          :rev => changeset.revision,
+          :changeset_id => changeset.id}) +
+          '</span></p>' if User.current.allowed_to?(:assign_code_review, @project)
+    end
+  end
 end
diff --git a/app/views/code_review/_update_revisions.html.erb b/app/views/code_review/_update_revisions.html.erb
index cdaba30..910778a 100644
--- a/app/views/code_review/_update_revisions.html.erb
+++ b/app/views/code_review/_update_revisions.html.erb
@@ -20,26 +20,10 @@
 <script type="text/javascript">
 <% @changesets.each do |changeset| %>
   <%
-  if changeset.review_count > 0
-    progress = '<span style="white-space: nowrap">' + progress_bar([changeset.closed_review_pourcent, changeset.completed_review_pourcent],
-      :width => '60px',
-      :legend => "#{changeset.closed_review_count}/#{changeset.review_count} #{l(:label_closed_issues)}") + '</span>'
-  elsif changeset.assignment_count > 0
-    if (changeset.open_assignment_count > 0)
-      progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_assigned) + '</p>'
-    else
-      progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_reviewed) + '</p>'
-    end
-    
-  else
-    progress = '<p class="progress-info" style="white-space: nowrap;"><span  style="white-space: nowrap;">' + l(:lable_no_code_reviews) + '</span>:<span  style="white-space: nowrap;">' + link_to(l(:label_assign_review), {:controller => 'code_review',
-        :action => 'assign', :id=>@project, 
-        :rev => changeset.revision, 
-        :changeset_id => changeset.id}) + '</span></p>' if User.current.allowed_to?(:assign_code_review, @project)
-  end
+      progress = progress_for_changeset changeset
 %>
 
-    var count = new ReviewCount(<%=  changeset.review_count %>, <%=  changeset.open_review_count %>, '<%= raw  progress %>');
+    var count = new ReviewCount(<%=  changeset.review_count %>, <%=  changeset.open_review_count %>, '<%= raw progress %>');
 
     review_counts['revision_<%=changeset.identifier %>'] = count;
 <% end %>

From 9f1491d330b466ea7a1a7d0c5b73b49deac06ac3 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 10:01:26 +0800
Subject: [PATCH 05/17] refactor progress_for_changeset helper to use tag
 helpers

- also removes the raw call which is now unnecessary and replaces it with java
  script escaping
---
 app/helpers/code_review_helper.rb             | 34 ++++++++++++-------
 .../code_review/_update_revisions.html.erb    |  2 +-
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/app/helpers/code_review_helper.rb b/app/helpers/code_review_helper.rb
index 4417f25..34a2c24 100644
--- a/app/helpers/code_review_helper.rb
+++ b/app/helpers/code_review_helper.rb
@@ -36,26 +36,36 @@ def show_assignments(assignments, project, options = {})
   end
 
   def progress_for_changeset(changeset)
-    if changeset.review_count > 0
-      progress = '<span style="white-space: nowrap">' + progress_bar([changeset.closed_review_pourcent, changeset.completed_review_pourcent],
-        :width => '60px',
-        :legend => "#{changeset.closed_review_count}/#{changeset.review_count} #{l(:label_closed_issues)}") + '</span>'
+    progress = if changeset.review_count > 0
+      content_tag(:span, style: "white-space: nowrap") do
+        progress_bar(
+          [changeset.closed_review_pourcent, changeset.completed_review_pourcent],
+          :width => '60px',
+          :legend => "#{changeset.closed_review_count}/#{changeset.review_count} #{l(:label_closed_issues)}"
+        )
+      end
+
     elsif changeset.assignment_count > 0
       if (changeset.open_assignment_count > 0)
-        progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_assigned) + '</p>'
+        l(:code_review_assigned)
       else
-        progress = '<p class="progress-info" style="white-space: nowrap">' + l(:code_review_reviewed) + '</p>'
+        l(:code_review_reviewed)
       end
 
-    else
-      progress = '<p class="progress-info" style="white-space: nowrap;"><span  style="white-space: nowrap;">' +
-        l(:lable_no_code_reviews) +
-        '</span>:<span  style="white-space: nowrap;">' +
+    elsif User.current.allowed_to?(:assign_code_review, @project)
+      content_tag(:span, style: "white-space: nowrap") do
+        l(:lable_no_code_reviews)
+      end + ':'.html_safe +
+      content_tag(:span, style: "white-space: nowrap") do
         link_to(l(:label_assign_review), {:controller => 'code_review',
           :action => 'assign', :id=>@project,
           :rev => changeset.revision,
-          :changeset_id => changeset.id}) +
-          '</span></p>' if User.current.allowed_to?(:assign_code_review, @project)
+          :changeset_id => changeset.id})
+      end
     end
+
+    content_tag(:p, class: "progress-info", style: "white-space: nowrap;") do
+      progress
+    end if progress
   end
 end
diff --git a/app/views/code_review/_update_revisions.html.erb b/app/views/code_review/_update_revisions.html.erb
index 910778a..2b3ba73 100644
--- a/app/views/code_review/_update_revisions.html.erb
+++ b/app/views/code_review/_update_revisions.html.erb
@@ -23,7 +23,7 @@
       progress = progress_for_changeset changeset
 %>
 
-    var count = new ReviewCount(<%=  changeset.review_count %>, <%=  changeset.open_review_count %>, '<%= raw progress %>');
+    var count = new ReviewCount(<%=  changeset.review_count %>, <%=  changeset.open_review_count %>, '<%= j progress %>');
 
     review_counts['revision_<%=changeset.identifier %>'] = count;
 <% end %>

From 75d5fa6c5e8004a05a78c2a06047e974326d61b3 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 10:21:46 +0800
Subject: [PATCH 06/17] refactor show_assignments helper and update_diff_view

- removes another use of raw
- also introduces javascript escaping (j method) which is better than manually
  gsub'ing '-chars
---
 app/helpers/code_review_helper.rb             | 28 +++++++-----
 .../code_review/_update_diff_view.html.erb    | 44 ++++++++-----------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/app/helpers/code_review_helper.rb b/app/helpers/code_review_helper.rb
index 34a2c24..b564618 100644
--- a/app/helpers/code_review_helper.rb
+++ b/app/helpers/code_review_helper.rb
@@ -17,22 +17,28 @@
 module CodeReviewHelper
   unloadable
   def show_assignments(assignments, project, options = {})
-    html = "#{l(:review_assignments)}:"
-    assignments.each do |assignment|
-      issue = assignment.issue
-      html << link_to("##{issue.id} ", {:controller => 'issues', :action => 'show', :id => issue.id},
-        :class => issue.css_classes, :title => "#{issue}(#{issue.status})")
-    end if assignments
 
-    link = link_to(l(:button_add), {:controller => 'code_review',
+    links = if assignments
+      assignments.map do |assignment|
+        issue = assignment.issue
+        link_to("##{issue.id} ", {:controller => 'issues', :action => 'show', :id => issue.id},
+          :class => issue.css_classes, :title => "#{issue}(#{issue.status})")
+      end
+    else
+      []
+    end
+
+    links << link_to(
+      l(:button_add),
+      { :controller => 'code_review',
         :action => 'assign', :id=>project, :action_type => options[:action_type],
         :rev => options[:rev], :rev_to => options[:rev_to], :path => options[:path],
         :change_id => options[:change_id], :attachment_id => options[:attachment_id],
-        :changeset_id => options[:changeset_id]}, :class => 'icon icon-add')
-
-    html << link if link
+        :changeset_id => options[:changeset_id]},
+      :class => 'icon icon-add'
+    )
 
-    html
+    safe_join links
   end
 
   def progress_for_changeset(changeset)
diff --git a/app/views/code_review/_update_diff_view.html.erb b/app/views/code_review/_update_diff_view.html.erb
index 55993cb..2e1e8c9 100644
--- a/app/views/code_review/_update_diff_view.html.erb
+++ b/app/views/code_review/_update_diff_view.html.erb
@@ -36,44 +36,38 @@
   assignments = @change.code_review_assignments if @change
   attachment_id = @attachment.id if @attachment
   assignments = @attachment.code_review_assignments if @attachment
-  escaped_path = @path.gsub("'"){"\\'"} if @path
   -%>
 
-  <%- if User.current.allowed_to?(:assign_code_review, @project) -%>
-  <%= raw show_assignments assignments, @project, {:action_type => @action_type, :rev => @rev, :rev_to => @rev_to, :path => @path, :change_id => change_id, :attachment_id => attachment_id} -%>
-  <%- end -%>
+  <% if User.current.allowed_to?(:assign_code_review, @project) %>
+    <%= l :review_assignments %>:
+    <%= show_assignments assignments, @project, {:action_type => @action_type, :rev => @rev, :rev_to => @rev_to, :path => @path, :change_id => change_id, :attachment_id => attachment_id} %>
+  <% end %>
 </div>
 
 <script type="text/javascript">
-  repository_id = "<%= @repository_id %>";
-  add_form_title = "<%= l(:label_add_review) %>";
-  review_dialog_title = "<%= l(:code_review) %>";
-  var addReviewUrl = '<%= url_for :controller => 'code_review', :action => 'new', :id => @project %>';
-  var is_readonly = true;
-  <% if authorize_for('code_review', 'new') -%>
-    is_readonly = false;
-  <% end %>
-  var is_diff = false;
-  <% if @action_type == 'diff' or (@attachment and @attachment.is_diff?) -%>
-    is_diff = true;
-  <% end %>
-  action_type = '<%= @action_type %>';
-  rev = '<%= @rev %>';
-  rev_to = '<%= @rev_to %>';
-  path = '<%= escaped_path -%>';
+  var repository_id = "<%= j @repository_id %>";
+  var add_form_title = "<%= l(:label_add_review) %>";
+  var review_dialog_title = "<%= l(:code_review) %>";
+  var addReviewUrl = '<%= j url_for :controller => 'code_review', :action => 'new', :id => @project %>';
+  var is_readonly = <%= !authorize_for('code_review', 'new') %>;
+  var is_diff = <%= @action_type == 'diff' or (@attachment and @attachment.is_diff?) %>;
+  var action_type = '<%= j @action_type %>';
+  var rev = '<%= j @rev %>';
+  var rev_to = '<%= j @rev_to %>';
+  var path = '<%= j @path -%>';
 
-  setAddReviewButton(addReviewUrl, '<%= @change.id if @change %>', '<%= image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= @attachment_id%>' );
+  setAddReviewButton(addReviewUrl, '<%= change_id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= j @attachment_id %>' );
 
-  showReviewUrl = '<%= url_for :controller => 'code_review', :action => 'show', :id=>@project %>';
-  showReviewImageTag = '<%= image_tag('review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';
-  showClosedReviewImageTag = '<%= image_tag('closed_review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';
+  var showReviewUrl = '<%= j url_for :controller => 'code_review', :action => 'show', :id=>@project %>';
+  var showReviewImageTag = '<%= j image_tag('review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';
+  var showClosedReviewImageTag = '<%= j image_tag('closed_review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';
 
   <% for review in @reviews do %>
     setShowReviewButton(<%= review.line %>, <%= review.id %>, <%= review.is_closed? %>, <%= review.file_count %> );
   <% end %>
 
   <% if @show_review_id -%>
-    popupReview(<%= @show_review_id %>);
+    popupReview(<%= @show_review_id.to_i %>);
   <% end -%>
   $('#content table.filecontent:first').before($('#code-review-assign-link'));
 </script>

From 29841bf6227f84ec723d8262c9410e38f348bab1 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 10:52:15 +0800
Subject: [PATCH 07/17] replaces raw with sanitize

- which is totally sufficient in this case
---
 app/views/code_review/_new_form.html.erb | 2 +-
 config/locales/en.yml                    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/views/code_review/_new_form.html.erb b/app/views/code_review/_new_form.html.erb
index 23d29a5..854b8b7 100644
--- a/app/views/code_review/_new_form.html.erb
+++ b/app/views/code_review/_new_form.html.erb
@@ -62,7 +62,7 @@
       <label><b><%= l(:field_parent_issue)%>:</b></label>
       <%= f.text_field :parent_id, :size => 10 %>
       <% if @parent_candidate %>
-        <%= raw l(:label_parent_suggestion, {:issue_id => link_to_issue(@parent_candidate)}) %>
+        <%= sanitize l(:label_parent_suggestion, {:issue_id => link_to_issue(@parent_candidate)}) %>
         <input type="button" value="<%= l(:general_text_Yes)%>" onclick="$('#review_parent_id').val(<%= @parent_candidate.id %>)"/>
       <% end %>
     </p>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index ec82968..3e692e7 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -58,7 +58,7 @@ en:
   auto_assign_filter_assign: Assign
   auto_assign_filter_drop: Drop
 
-  label_parent_suggestion: Maybe "%{issue_id}" ?
+  label_parent_suggestion: "Maybe %{issue_id} ?"
   
    
   label_assign_review: "Assign"

From 7d3178ab1e199685147636099048e5afbf15c045 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 10:55:45 +0800
Subject: [PATCH 08/17] adds url argument to popupReview

---
 app/views/code_review/_update_diff_view.html.erb |  2 +-
 assets/javascripts/code_review.js                | 10 +++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/app/views/code_review/_update_diff_view.html.erb b/app/views/code_review/_update_diff_view.html.erb
index 2e1e8c9..0e9d9af 100644
--- a/app/views/code_review/_update_diff_view.html.erb
+++ b/app/views/code_review/_update_diff_view.html.erb
@@ -67,7 +67,7 @@
   <% end %>
 
   <% if @show_review_id -%>
-    popupReview(<%= @show_review_id.to_i %>);
+    popupReview(<%= @show_review_id.to_i %>, showReviewUrl);
   <% end -%>
   $('#content table.filecontent:first').before($('#code-review-assign-link'));
 </script>
diff --git a/assets/javascripts/code_review.js b/assets/javascripts/code_review.js
index f2eef97..b9f6e59 100644
--- a/assets/javascripts/code_review.js
+++ b/assets/javascripts/code_review.js
@@ -206,16 +206,12 @@ function setShowReviewButton(line, review_id, is_closed, file_count) {
     });
 }
 
-function popupReview(review_id) {
+function popupReview(review_id, url) {
     var span   = $('#review_' + review_id); // span element of view review button
     var pos = span.offset();
     $('html,body').animate({ scrollTop: pos.top },
         {duration: 'fast',
-        complete: function(){showReview(showReviewUrl, review_id, pos.left + 10 + 5, pos.top)}});
-    // position and show popup dialog
-    // create popup dialog
-    //var win = showReview(showReviewUrl, review_id, pos.left + 10 + 5, pos.top);
-//    win.toFront();
+        complete: function(){showReview(url, review_id, pos.left + 10 + 5, pos.top)}});
 }
 
 function showReview(url, review_id, x, y) {
@@ -351,4 +347,4 @@ $.fn.serialize2json = function()
        }
    });
    return o;
-};
\ No newline at end of file
+};

From e81074a40c19c0f1097b1a14cc3389f7ad7dc135 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 11:28:54 +0800
Subject: [PATCH 09/17] use j helper instead of manual gsub

---
 .../code_review/_change_revision_view.html.erb      | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/app/views/code_review/_change_revision_view.html.erb b/app/views/code_review/_change_revision_view.html.erb
index 29af964..97a5f7f 100644
--- a/app/views/code_review/_change_revision_view.html.erb
+++ b/app/views/code_review/_change_revision_view.html.erb
@@ -40,7 +40,7 @@ if @changeset
 
   <script type="text/javascript">
     $('#changes-legend').after($('#code_review_assignments'));
-    urlprefix = '<%=urlprefix%>';
+    urlprefix = '<%= j urlprefix%>';
   <% @changeset.changes.each{|change| %>
     var reviewlist = [];
     <%
@@ -51,7 +51,7 @@ if @changeset
         :controller => 'code_review', :action => 'show', :id => project, :review_id => review.id, :repository_id => repository_id)
     %>
       var review = new CodeReview(<%=review.id%>);
-      review.url = '<%=url%>';
+      review.url = '<%= j url%>';
       <% if review.is_closed? %>
         review.is_closed = true;
       <% end %>
@@ -60,13 +60,10 @@ if @changeset
       cnt += 1
 
     }
-    relative_path = change.path || ""
-    if relative_path[0] != ?/
-      relative_path = '/' + relative_path
-    end
-    escaped_relative_path = relative_path.gsub("'"){"\\'"}
+    path = change.path || ""
+    abs_path = path.start_with?(?/) ? path : "/#{path}"
   %>
-    code_reviews_map['<%=escaped_relative_path-%>'] = reviewlist;
+    code_reviews_map['<%= j abs_path -%>'] = reviewlist;
   <%
   }
 %>

From 9fa3df2ae68ba98a41d256409ca1036b6d8d74db Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 11:28:24 +0800
Subject: [PATCH 10/17] use the show_assignments helper in change_revision_view
 as well

---
 app/helpers/code_review_helper.rb             |  8 ++-----
 .../_change_revision_view.html.erb            | 24 +++++++------------
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/app/helpers/code_review_helper.rb b/app/helpers/code_review_helper.rb
index b564618..cef536a 100644
--- a/app/helpers/code_review_helper.rb
+++ b/app/helpers/code_review_helper.rb
@@ -30,12 +30,8 @@ def show_assignments(assignments, project, options = {})
 
     links << link_to(
       l(:button_add),
-      { :controller => 'code_review',
-        :action => 'assign', :id=>project, :action_type => options[:action_type],
-        :rev => options[:rev], :rev_to => options[:rev_to], :path => options[:path],
-        :change_id => options[:change_id], :attachment_id => options[:attachment_id],
-        :changeset_id => options[:changeset_id]},
-      :class => 'icon icon-add'
+      options.merge(controller: 'code_review', action: 'assign', id: project),
+      class: 'icon icon-add'
     )
 
     safe_join links
diff --git a/app/views/code_review/_change_revision_view.html.erb b/app/views/code_review/_change_revision_view.html.erb
index 97a5f7f..51deeae 100644
--- a/app/views/code_review/_change_revision_view.html.erb
+++ b/app/views/code_review/_change_revision_view.html.erb
@@ -18,25 +18,19 @@
 -%>
 <%
 repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
+
 if @changeset
   urlprefix = url_for(:controller => 'repositories', :action => 'revisions', :id => project, :repository_id => repository_id) +
     '/' + @changeset.identifier + '/entry'
 %>
-<%- if User.current.allowed_to?(:assign_code_review, @project) -%>
-  <div id="code_review_assignments">
-    <%= l(:review_assignments)%>
-    <% @changeset.code_review_assignments.each do |assignment|
-      issue = assignment.issue %>
-      <%= link_to("##{issue.id} ", {:controller => 'issues', :action => 'show', :id => issue.id},
-        :class => issue.css_classes, :title => "#{issue}(#{issue.status})") %>
-    <% end if @changeset.code_review_assignments %>
-
-    <%= link_to(l(:button_add), {:controller => 'code_review',
-        :action => 'assign', :id=>project, 
-        :rev => @changeset.revision, 
-        :changeset_id => @changeset.id, :repository_id => repository_id}, :class => 'icon icon-add') %>
-  </div>
-<%- end -%>
+  <% if User.current.allowed_to?(:assign_code_review, @project) %>
+    <div id="code_review_assignments">
+      <%= l :review_assignments %>:
+      <%= show_assignments @changeset.code_review_assignments, @project,
+      { rev: @changeset.revision, changeset_id: @changeset.id,
+        repository_id: repository_id } %>
+    </div>
+  <% end %>
 
   <script type="text/javascript">
     $('#changes-legend').after($('#code_review_assignments'));

From 4de5dda1615f645a1d780c69f1e2d6d21bbdae8a Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Fri, 9 Mar 2018 20:08:44 +0800
Subject: [PATCH 11/17] remove unnecessary controller and requrest (sic)
 context values

---
 app/views/code_review/_body_bottom.html.erb              | 2 +-
 app/views/code_review/_change_attachement_view.html.erb  | 5 ++---
 .../code_review/_change_entry_norevision_view.html.erb   | 9 ++++-----
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/app/views/code_review/_body_bottom.html.erb b/app/views/code_review/_body_bottom.html.erb
index ba798ee..368c780 100644
--- a/app/views/code_review/_body_bottom.html.erb
+++ b/app/views/code_review/_body_bottom.html.erb
@@ -30,7 +30,7 @@ if project and controller and project.module_enabled?(:code_review)
   is_target = false unless action_name
   is_target = false unless (controller.class.name == 'RepositoriesController' or controller.class.name == 'AttachmentsController')
   if (is_target == true)
-    context = {:project => project, :controller => controller, :requrest => request}
+    context = {project: project}
   %>
     <% if (controller.class.name == 'AttachmentsController' && action_name == "show") %>
       <%= render :partial => 'code_review/change_attachement_view', :locals => context %>
diff --git a/app/views/code_review/_change_attachement_view.html.erb b/app/views/code_review/_change_attachement_view.html.erb
index afaed4c..6abaa4c 100644
--- a/app/views/code_review/_change_attachement_view.html.erb
+++ b/app/views/code_review/_change_attachement_view.html.erb
@@ -17,8 +17,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 -%>
 <%
-parameters = request.parameters
-id = parameters[:id].to_i
+id = params[:id].to_i
 attachment = Attachment.find(id)
 return '' unless attachment.is_text? or attachment.is_diff?
 review_id = parameters[:review_id] unless parameters[:review_id].blank?
@@ -32,4 +31,4 @@ url = url_for :controller => 'code_review', :action => 'update_attachment_view',
   $(document).ready(function(){
     $('#code_review').load('<%=url%>', {'attachment_id': '<%=id%>', 'review_id': '<%=review_id%>'});
   });
-</script>
\ No newline at end of file
+</script>
diff --git a/app/views/code_review/_change_entry_norevision_view.html.erb b/app/views/code_review/_change_entry_norevision_view.html.erb
index 096a1a4..97b1d92 100644
--- a/app/views/code_review/_change_entry_norevision_view.html.erb
+++ b/app/views/code_review/_change_entry_norevision_view.html.erb
@@ -17,11 +17,10 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 -%>
 <%
-parameters = request.parameters
-path = parameters['path']
-rev = parameters['rev']
+path = params[:path]
+rev = params[:rev]
 repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
-unless path.blank? or path.empty?
+unless path.blank?
   changesets = @repository.latest_changesets(path, rev, Setting.repository_log_display_limit.to_i)
   change = changesets[0]
 
@@ -36,4 +35,4 @@ unless path.blank? or path.empty?
     </script>
   <% end %>
 
-<% end %>
\ No newline at end of file
+<% end %>

From 157aed8b336d2b57f061010385a5fb2745c93a94 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 12:34:08 +0800
Subject: [PATCH 12/17] javascript escaping, parameters / params fix

---
 app/views/code_review/_body_bottom.html.erb   | 21 +++++++++----------
 .../_change_attachement_view.html.erb         | 14 +++++++------
 .../code_review/_update_diff_view.html.erb    |  2 +-
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/app/views/code_review/_body_bottom.html.erb b/app/views/code_review/_body_bottom.html.erb
index 368c780..7b0bb3b 100644
--- a/app/views/code_review/_body_bottom.html.erb
+++ b/app/views/code_review/_body_bottom.html.erb
@@ -47,11 +47,10 @@ if project and controller and project.module_enabled?(:code_review)
         <% unless changeset %>
           <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
         <% else
-          parameters = request.parameters
-          rev_to = parameters['rev_to'] unless parameters['rev_to'].blank?
-          review_id = parameters['review_id']
-          rev = parameters['rev']
-          path = parameters['path'].blank? ? '.' : parameters['path']
+          rev_to = params['rev_to'] unless params['rev_to'].blank?
+          review_id = params['review_id']
+          rev = params['rev']
+          path = params['path'].blank? ? '.' : params['path']
 
 
             repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
@@ -61,12 +60,12 @@ if project and controller and project.module_enabled?(:code_review)
             </div>
             <script type="text/javascript">
                 $(document).ready(function(){
-                    $('#code_review').load('<%=url%>', {
-                        rev: '<%=rev%>',
-                        path:'<%=path%>',
-                        review_id: '<%=review_id%>',
-                        action_type:'<%=action_name%>',
-                        rev_to: '<%=rev_to%>'});
+                    $('#code_review').load('<%= j url %>', {
+                        rev: '<%= j rev %>',
+                        path:'<%= j path %>',
+                        review_id: '<%= j review_id %>',
+                        action_type:'<%= j action_name %>',
+                        rev_to: '<%= j rev_to %>'});
                 });
             </script>
           <% end %>
diff --git a/app/views/code_review/_change_attachement_view.html.erb b/app/views/code_review/_change_attachement_view.html.erb
index 6abaa4c..3abe30b 100644
--- a/app/views/code_review/_change_attachement_view.html.erb
+++ b/app/views/code_review/_change_attachement_view.html.erb
@@ -17,18 +17,20 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 -%>
 <%
-id = params[:id].to_i
-attachment = Attachment.find(id)
-return '' unless attachment.is_text? or attachment.is_diff?
-review_id = parameters[:review_id] unless parameters[:review_id].blank?
+attachment = Attachment.find(params[:id])
 url = url_for :controller => 'code_review', :action => 'update_attachment_view', :id => project
--%>
+%>
+
+<% if attachment.is_text? or attachment.is_diff? %>
+
 <div id="code_review">
   <div id="review_comment"/>
 </div>
 
 <script type="text/javascript">
   $(document).ready(function(){
-    $('#code_review').load('<%=url%>', {'attachment_id': '<%=id%>', 'review_id': '<%=review_id%>'});
+    $('#code_review').load('<%= j url %>', {'attachment_id': '<%= attachment.id %>', 'review_id': '<%= j params[:review_id].to_s %>'});
   });
 </script>
+
+<% end %>
diff --git a/app/views/code_review/_update_diff_view.html.erb b/app/views/code_review/_update_diff_view.html.erb
index 0e9d9af..6a4d8a8 100644
--- a/app/views/code_review/_update_diff_view.html.erb
+++ b/app/views/code_review/_update_diff_view.html.erb
@@ -56,7 +56,7 @@
   var rev_to = '<%= j @rev_to %>';
   var path = '<%= j @path -%>';
 
-  setAddReviewButton(addReviewUrl, '<%= change_id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= j @attachment_id %>' );
+  setAddReviewButton(addReviewUrl, '<%= change_id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= attachment_id %>' );
 
   var showReviewUrl = '<%= j url_for :controller => 'code_review', :action => 'show', :id=>@project %>';
   var showReviewImageTag = '<%= j image_tag('review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';

From 0459edfedb9c666cda1a4b1bdd488c68a439e761 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 13:05:17 +0800
Subject: [PATCH 13/17] extract partial for diff view changing

---
 app/views/code_review/_body_bottom.html.erb   | 33 +++----------------
 .../code_review/_change_diff_view.html.erb    | 20 +++++++++++
 2 files changed, 25 insertions(+), 28 deletions(-)
 create mode 100644 app/views/code_review/_change_diff_view.html.erb

diff --git a/app/views/code_review/_body_bottom.html.erb b/app/views/code_review/_body_bottom.html.erb
index 7b0bb3b..f99a453 100644
--- a/app/views/code_review/_body_bottom.html.erb
+++ b/app/views/code_review/_body_bottom.html.erb
@@ -39,36 +39,13 @@ if project and controller and project.module_enabled?(:code_review)
     <% elsif (action_name == 'revision') %>
       <%= render :partial => 'code_review/change_revision_view', :locals => context %>
     <% elsif (action_name == 'diff' or action_name == 'entry' or action_name == 'annotate')%>
-      <%if (controller.params[:rev].blank? or controller.params[:rev] == 'master')%>
+      <%if (params[:rev].blank? or params[:rev] == 'master')%>
         <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
-      <% else
-        changeset = @repository.find_changeset_by_name(controller.params[:rev])
-      %>
-        <% unless changeset %>
-          <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
-        <% else
-          rev_to = params['rev_to'] unless params['rev_to'].blank?
-          review_id = params['review_id']
-          rev = params['rev']
-          path = params['path'].blank? ? '.' : params['path']
+      <% elsif @repository.find_changeset_by_name(params[:rev]) %>
+        <%= render partial: 'code_review/change_diff_view', locals: context %>
 
-
-            repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
-            url = url_for :controller => 'code_review', :action => 'update_diff_view', :id => project, :repository_id => repository_id
-          %>
-            <div id="code_review">
-            </div>
-            <script type="text/javascript">
-                $(document).ready(function(){
-                    $('#code_review').load('<%= j url %>', {
-                        rev: '<%= j rev %>',
-                        path:'<%= j path %>',
-                        review_id: '<%= j review_id %>',
-                        action_type:'<%= j action_name %>',
-                        rev_to: '<%= j rev_to %>'});
-                });
-            </script>
-          <% end %>
+      <% else %>
+        <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
 
       <% end %>
     <% end %>
diff --git a/app/views/code_review/_change_diff_view.html.erb b/app/views/code_review/_change_diff_view.html.erb
new file mode 100644
index 0000000..ef5fa07
--- /dev/null
+++ b/app/views/code_review/_change_diff_view.html.erb
@@ -0,0 +1,20 @@
+<%
+  rev_to = params['rev_to']
+  review_id = params['review_id']
+  rev = params['rev']
+  path = params['path'].blank? ? '.' : Array(params['path']).join('/')
+
+  url = url_for controller: 'code_review', action: 'update_diff_view', id: project, repository_id: @repository.identifier_param
+%>
+<div id="code_review"></div>
+
+<%= javascript_tag do %>
+  $(document).ready(function(){
+    $('#code_review').load('<%= j url %>', {
+      rev: '<%= j rev %>',
+      path:'<%= j path %>',
+      review_id: '<%= j review_id %>',
+      action_type:'<%= j action_name %>',
+      rev_to: '<%= j rev_to %>'});
+  });
+<% end %>

From 216a176d35db459c4a3ddabd72906aff9f0d8492 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 13:43:10 +0800
Subject: [PATCH 14/17] moves the big body_bottom conditional to ruby code.

---
 app/models/code_review_project_setting.rb     |  7 ++-
 app/views/code_review/_body_bottom.html.erb   | 58 -------------------
 .../code_review/_change_diff_view.html.erb    |  7 +++
 lib/code_review_application_hooks.rb          | 43 +++++++++++++-
 4 files changed, 54 insertions(+), 61 deletions(-)
 delete mode 100644 app/views/code_review/_body_bottom.html.erb

diff --git a/app/models/code_review_project_setting.rb b/app/models/code_review_project_setting.rb
index f1a8d28..703b533 100644
--- a/app/models/code_review_project_setting.rb
+++ b/app/models/code_review_project_setting.rb
@@ -35,9 +35,12 @@ class CodeReviewProjectSetting < ActiveRecord::Base
   AUTORELATION_TYPE_RELATES = 1
   AUTORELATION_TYPE_BLOCKS = 2
 
+  def self.find_for(project)
+    where(project_id: project.id).first
+  end
+
   def self.find_or_create(project)
-    setting = CodeReviewProjectSetting.find_by_project_id(project.id)
-    unless setting
+    unless setting = find_for(project)
       setting = CodeReviewProjectSetting.new
       setting.project_id = project.id
       return setting if project.trackers.length == 0
diff --git a/app/views/code_review/_body_bottom.html.erb b/app/views/code_review/_body_bottom.html.erb
deleted file mode 100644
index f99a453..0000000
--- a/app/views/code_review/_body_bottom.html.erb
+++ /dev/null
@@ -1,58 +0,0 @@
-<%
-# Code Review plugin for Redmine
-# Copyright (C) 2010-2014  Haruyuki Iida
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
--%>
-
-<%
-is_target = false
-
-if project and controller and project.module_enabled?(:code_review)
-  is_target = true
-  is_target = false unless User.current.allowed_to?({:controller => 'code_review', :action => 'update_diff_view'}, project)
-  setting = CodeReviewProjectSetting.where(:project_id => project.id) .first
-  is_target = false unless setting
-  is_target = false if(setting && setting.tracker_id == nil)
-  action_name = controller.action_name
-  is_target = false unless action_name
-  is_target = false unless (controller.class.name == 'RepositoriesController' or controller.class.name == 'AttachmentsController')
-  if (is_target == true)
-    context = {project: project}
-  %>
-    <% if (controller.class.name == 'AttachmentsController' && action_name == "show") %>
-      <%= render :partial => 'code_review/change_attachement_view', :locals => context %>
-    <% elsif (action_name == 'show' or action_name == 'revisions') %>
-      <%= render :partial => 'code_review/change_repository_view', :locals => context %>
-    <% elsif (action_name == 'revision') %>
-      <%= render :partial => 'code_review/change_revision_view', :locals => context %>
-    <% elsif (action_name == 'diff' or action_name == 'entry' or action_name == 'annotate')%>
-      <%if (params[:rev].blank? or params[:rev] == 'master')%>
-        <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
-      <% elsif @repository.find_changeset_by_name(params[:rev]) %>
-        <%= render partial: 'code_review/change_diff_view', locals: context %>
-
-      <% else %>
-        <%= render :partial => 'code_review/change_entry_norevision_view', :locals => context %>
-
-      <% end %>
-    <% end %>
-  <%
-  end
-end
--%>
-
-
-
diff --git a/app/views/code_review/_change_diff_view.html.erb b/app/views/code_review/_change_diff_view.html.erb
index ef5fa07..9af9809 100644
--- a/app/views/code_review/_change_diff_view.html.erb
+++ b/app/views/code_review/_change_diff_view.html.erb
@@ -1,3 +1,5 @@
+<% if @repository.find_changeset_by_name(params[:rev]) %>
+
 <%
   rev_to = params['rev_to']
   review_id = params['review_id']
@@ -6,6 +8,7 @@
 
   url = url_for controller: 'code_review', action: 'update_diff_view', id: project, repository_id: @repository.identifier_param
 %>
+
 <div id="code_review"></div>
 
 <%= javascript_tag do %>
@@ -18,3 +21,7 @@
       rev_to: '<%= j rev_to %>'});
   });
 <% end %>
+
+<% else %>
+  <%= render partial: 'code_review/change_entry_norevision_view', locals: { project: project } %>
+<% end %>
diff --git a/lib/code_review_application_hooks.rb b/lib/code_review_application_hooks.rb
index 5e7b0a4..c3a6316 100644
--- a/lib/code_review_application_hooks.rb
+++ b/lib/code_review_application_hooks.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # Code Review plugin for Redmine
 # Copyright (C) 2009-2010  Haruyuki Iida
 #rev
@@ -15,8 +17,47 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
-require 'redmine/utils'
 class CodeReviewApplicationHooks < Redmine::Hook::ViewListener
   render_on :view_layouts_base_html_head, :partial => 'code_review/html_header'
   render_on :view_layouts_base_body_bottom, :partial => 'code_review/body_bottom'
+
+  def view_layouts_base_body_bottom(context)
+    if project = context[:project] and
+      controller = context[:controller] and
+        (controller.is_a?(RepositoriesController) or
+         controller.is_a?(AttachmentsController)) and
+      project.module_enabled?(:code_review) and
+      User.current.allowed_to?(:view_code_review, project) and
+      setting = CodeReviewProjectSetting.find_for(project) and
+      setting.tracker.present?
+
+      params = context[:request].parameters
+
+      partial = case controller
+      when AttachmentsController
+        if params[:action] == "show"
+          'code_review/change_attachement_view'
+        end
+      when RepositoriesController
+        case params[:action]
+        when 'show', 'revisions'
+          'code_review/change_repository_view'
+        when 'revision'
+          'code_review/change_revision_view'
+        when 'diff', 'entry', 'annotate'
+          if params[:rev].blank? or params[:rev] == 'master'
+            'code_review/change_entry_norevision_view'
+          else
+            'code_review/change_diff_view'
+          end
+        end
+      end
+
+      if partial
+        controller.send :render_to_string, { partial: partial,
+                                             locals: { project: project } }
+      end
+    end
+
+  end
 end

From a030dd204ed2724c25255d34bdb88fde54c7986d Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Fri, 9 Mar 2018 20:10:30 +0800
Subject: [PATCH 15/17] if only interested in the first changeset, we only need
 to ask for 1

---
 app/views/code_review/_change_entry_norevision_view.html.erb | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/views/code_review/_change_entry_norevision_view.html.erb b/app/views/code_review/_change_entry_norevision_view.html.erb
index 97b1d92..09d79b4 100644
--- a/app/views/code_review/_change_entry_norevision_view.html.erb
+++ b/app/views/code_review/_change_entry_norevision_view.html.erb
@@ -21,10 +21,7 @@ path = params[:path]
 rev = params[:rev]
 repository_id = @repository.identifier_param if @repository.respond_to?("identifier_param")
 unless path.blank?
-  changesets = @repository.latest_changesets(path, rev, Setting.repository_log_display_limit.to_i)
-  change = changesets[0]
-
-  if change
+  if change = @repository.latest_changesets(path, rev, 1).first
     link = link_to(l(:label_add_review), {:controller => 'code_review',
         :action => 'forward_to_revision', :id => project, :path => path, :rev => rev, :repository_id => repository_id}, 
       :class => 'icon icon-edit')

From f387740551f63cb71477a444fb8cd4c6d2fff7c5 Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 16:17:42 +0800
Subject: [PATCH 16/17] readability, reduce number of variables

---
 app/views/code_review/_update_diff_view.html.erb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/views/code_review/_update_diff_view.html.erb b/app/views/code_review/_update_diff_view.html.erb
index 6a4d8a8..7aec273 100644
--- a/app/views/code_review/_update_diff_view.html.erb
+++ b/app/views/code_review/_update_diff_view.html.erb
@@ -31,16 +31,16 @@
 
 <div id="code-review-assign-link" style="text-align: right;">
   <%
-  change_id = ''
-  change_id = @change.id if @change
-  assignments = @change.code_review_assignments if @change
-  attachment_id = @attachment.id if @attachment
-  assignments = @attachment.code_review_assignments if @attachment
+   assignments = if @change
+     @change.code_review_assignments
+   elsif @attachment
+     @attachment.code_review_assignments
+   end
   -%>
 
   <% if User.current.allowed_to?(:assign_code_review, @project) %>
     <%= l :review_assignments %>:
-    <%= show_assignments assignments, @project, {:action_type => @action_type, :rev => @rev, :rev_to => @rev_to, :path => @path, :change_id => change_id, :attachment_id => attachment_id} %>
+    <%= show_assignments assignments, @project, {:action_type => @action_type, :rev => @rev, :rev_to => @rev_to, :path => @path, :change_id => @change.try(:id), :attachment_id => @attachment.try(:id)} %>
   <% end %>
 </div>
 
@@ -56,7 +56,7 @@
   var rev_to = '<%= j @rev_to %>';
   var path = '<%= j @path -%>';
 
-  setAddReviewButton(addReviewUrl, '<%= change_id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= attachment_id %>' );
+  setAddReviewButton(addReviewUrl, '<%= change.try :id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= @attachment.try :id %>' );
 
   var showReviewUrl = '<%= j url_for :controller => 'code_review', :action => 'show', :id=>@project %>';
   var showReviewImageTag = '<%= j image_tag('review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';

From 40143ef6ae6456fab65cfe80a1173f3594a608ce Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 12 Mar 2018 16:33:05 +0800
Subject: [PATCH 17/17] bugfixes

---
 app/views/code_review/_update_diff_view.html.erb | 2 +-
 init.rb                                          | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/views/code_review/_update_diff_view.html.erb b/app/views/code_review/_update_diff_view.html.erb
index 7aec273..5e3b330 100644
--- a/app/views/code_review/_update_diff_view.html.erb
+++ b/app/views/code_review/_update_diff_view.html.erb
@@ -56,7 +56,7 @@
   var rev_to = '<%= j @rev_to %>';
   var path = '<%= j @path -%>';
 
-  setAddReviewButton(addReviewUrl, '<%= change.try :id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= @attachment.try :id %>' );
+  setAddReviewButton(addReviewUrl, '<%= @change.try :id %>', '<%= j image_tag('edit.png', :alt => l(:label_add_review), :title => l(:label_add_review)) %>', is_readonly, is_diff, '<%= @attachment.try :id %>' );
 
   var showReviewUrl = '<%= j url_for :controller => 'code_review', :action => 'show', :id=>@project %>';
   var showReviewImageTag = '<%= j image_tag('review.png', :plugin => 'redmine_code_review', :alt => l(:label_show_review), :title => l(:label_show_review)) %>';
diff --git a/init.rb b/init.rb
index 1d35765..08bad93 100644
--- a/init.rb
+++ b/init.rb
@@ -49,6 +49,7 @@
     Attachment.send(:include, CodeReviewAttachmentPatch)
   end
 
+  RepositoriesController.send :helper, :code_review
 end
 
 Redmine::Plugin.register :redmine_code_review do