Use regular edit/update actions and named routes for JournalsController.

git-svn-id: http://svn.redmine.org/redmine/trunk@15074 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/journals_controller.rb

@@ -16,10 +16,10 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 class JournalsController < ApplicationController
-  before_filter :find_journal, :only => [:edit, :diff]
+  before_filter :find_journal, :only => [:edit, :update, :diff]
   before_filter :find_issue, :only => [:new]
   before_filter :find_optional_project, :only => [:index]
-  before_filter :authorize, :only => [:new, :edit, :diff]
+  before_filter :authorize, :only => [:new, :edit, :update, :diff]
   accept_rss_auth :index
   menu_item :issues
 
@@ -82,19 +82,20 @@ class JournalsController < ApplicationController
 
   def edit
     (render_403; return false) unless @journal.editable_by?(User.current)
-    if request.post?
+    respond_to do |format|
-      @journal.update_attributes(:notes => params[:notes]) if params[:notes]
+      # TODO: implement non-JS journal update
-      @journal.destroy if @journal.details.empty? && @journal.notes.blank?
+      format.js
-      call_hook(:controller_journals_edit_post, { :journal => @journal, :params => params})
+    end
-      respond_to do |format|
+  end
-        format.html { redirect_to issue_path(@journal.journalized) }
+
-        format.js { render :action => 'update' }
+  def update
-      end
+    (render_403; return false) unless @journal.editable_by?(User.current)
-    else
+    @journal.update_attributes(:notes => params[:notes]) if params[:notes]
-      respond_to do |format|
+    @journal.destroy if @journal.details.empty? && @journal.notes.blank?
-        # TODO: implement non-JS journal update
+    call_hook(:controller_journals_edit_post, { :journal => @journal, :params => params})
-        format.js
+    respond_to do |format|
-      end
+      format.html { redirect_to issue_path(@journal.journalized) }
+      format.js
     end
   end
 

app/helpers/issues_helper.rb

@@ -456,8 +456,7 @@ module IssuesHelper
       s = l(:text_journal_changed_no_detail, :label => label)
       unless no_html
         diff_link = link_to 'diff',
-          {:controller => 'journals', :action => 'diff', :id => detail.journal_id,
+          diff_journal_url(detail.journal_id, :detail_id => detail.id, :only_path => options[:only_path]),
-           :detail_id => detail.id, :only_path => options[:only_path]},
           :title => l(:label_view_diff)
         s << " (#{ diff_link })"
       end

app/helpers/journals_helper.rb

@@ -31,23 +31,23 @@ module JournalsHelper
     links = []
     if !journal.notes.blank?
       links << link_to('',
-                       {:controller => 'journals', :action => 'new', :id => issue, :journal_id => journal},
+                       quoted_issue_path(issue, :journal_id => journal),
                        :remote => true,
                        :method => 'post',
                        :title => l(:button_quote),
                        :class => 'icon-only icon-comment'
                       ) if options[:reply_links]
       links << link_to('',
-                       {:controller => 'journals', :action => 'edit', :id => journal},
+                       edit_journal_path(journal),
                        :remote => true,
                        :method => 'get',
                        :title => l(:button_edit),
                        :class => 'icon-only icon-edit'
                       ) if editable
       links << link_to('',
-                       {:controller => 'journals', :action => 'edit', :id => journal, :notes => ""},
+                       journal_path(journal, :notes => ""),
                        :remote => true,
-                       :method => :post, :data => {:confirm => l(:text_are_you_sure)}, 
+                       :method => 'put', :data => {:confirm => l(:text_are_you_sure)}, 
                        :title => l(:button_delete),
                        :class => 'icon-only icon-del'
                       ) if editable

app/views/journals/_notes_form.html.erb

@@ -1,5 +1,6 @@
-<%= form_tag({:controller => 'journals', :action => 'edit', :id => @journal},
+<%= form_tag(journal_path(@journal),
              :remote => true,
+             :method => 'put',
              :id => "journal-#{@journal.id}-form") do %>
     <%= label_tag "notes", l(:description_notes), :class => "hidden-for-sighted" %>
     <%= text_area_tag :notes, @journal.notes,

config/routes.rb

@@ -49,8 +49,11 @@ Rails.application.routes.draw do
   match '/issues/changes', :to => 'journals#index', :as => 'issue_changes', :via => :get
   match '/issues/:id/quoted', :to => 'journals#new', :id => /\d+/, :via => :post, :as => 'quoted_issue'
 
-  match '/journals/diff/:id', :to => 'journals#diff', :id => /\d+/, :via => :get
+  resources :journals, :only => [:edit, :update] do
-  match '/journals/edit/:id', :to => 'journals#edit', :id => /\d+/, :via => [:get, :post]
+    member do
+      get 'diff'
+    end
+  end
 
   get '/projects/:project_id/issues/gantt', :to => 'gantts#show', :as => 'project_gantt'
   get '/issues/gantt', :to => 'gantts#show'

lib/redmine.rb

@@ -105,8 +105,8 @@ Redmine::AccessControl.map do |map|
     map.permission :set_issues_private, {}
     map.permission :set_own_issues_private, {}, :require => :loggedin
     map.permission :add_issue_notes, {:issues => [:edit, :update], :journals => [:new], :attachments => :upload}
-    map.permission :edit_issue_notes, {:journals => :edit}, :require => :loggedin
+    map.permission :edit_issue_notes, {:journals => [:edit, :update]}, :require => :loggedin
-    map.permission :edit_own_issue_notes, {:journals => :edit}, :require => :loggedin
+    map.permission :edit_own_issue_notes, {:journals => [:edit, :update]}, :require => :loggedin
     map.permission :view_private_notes, {}, :read => true, :require => :member
     map.permission :set_notes_private, {}, :require => :member
     map.permission :delete_issues, {:issues => :destroy}, :require => :member

test/functional/journals_controller_test.rb

@@ -199,7 +199,7 @@ class JournalsControllerTest < ActionController::TestCase
 
   def test_update_xhr
     @request.session[:user_id] = 1
-    xhr :post, :edit, :id => 2, :notes => 'Updated notes'
+    xhr :post, :update, :id => 2, :notes => 'Updated notes'
     assert_response :success
     assert_template 'update'
     assert_equal 'text/javascript', response.content_type
@@ -210,7 +210,7 @@ class JournalsControllerTest < ActionController::TestCase
   def test_update_xhr_with_empty_notes_should_delete_the_journal
     @request.session[:user_id] = 1
     assert_difference 'Journal.count', -1 do
-      xhr :post, :edit, :id => 2, :notes => ''
+      xhr :post, :update, :id => 2, :notes => ''
       assert_response :success
       assert_template 'update'
       assert_equal 'text/javascript', response.content_type

test/integration/routing/journals_test.rb

@@ -21,9 +21,9 @@ class RoutingJournalsTest < Redmine::RoutingTest
   def test_journals
     should_route 'POST /issues/1/quoted' => 'journals#new', :id => '1'
     should_route 'GET /issues/changes' => 'journals#index'
-    should_route 'GET /journals/diff/1' => 'journals#diff', :id => '1'
+    should_route 'GET /journals/1/diff' => 'journals#diff', :id => '1'
 
-    should_route 'GET /journals/edit/1' => 'journals#edit', :id => '1'
+    should_route 'GET /journals/1/edit' => 'journals#edit', :id => '1'
-    should_route 'POST /journals/edit/1' => 'journals#edit', :id => '1'
+    should_route 'PUT /journals/1' => 'journals#update', :id => '1'
   end
 end

test/unit/mailer_test.rb

@@ -68,7 +68,7 @@ class MailerTest < ActiveSupport::TestCase
                     # should be https://mydomain.foo/journals/diff/3?detail_id=4
                     # but the Rails 4.2 DOM assertion doesn't handle the ? in the
                     # attribute value
-                    'https://mydomain.foo/journals/diff/3',
+                    'https://mydomain.foo/journals/3/diff',
                     'View differences',
                     :text => 'diff'
       # link to an attachment
@@ -109,7 +109,7 @@ class MailerTest < ActiveSupport::TestCase
                     # should be http://mydomain.foo/rdm/journals/diff/3?detail_id=4
                     # but the Rails 4.2 DOM assertion doesn't handle the ? in the
                     # attribute value
-                    'http://mydomain.foo/rdm/journals/diff/3',
+                    'http://mydomain.foo/rdm/journals/3/diff',
                     'View differences',
                     :text => 'diff'
       # link to an attachment
@@ -179,7 +179,7 @@ class MailerTest < ActiveSupport::TestCase
                     # should be http://mydomain.foo/rdm/journals/diff/3?detail_id=4
                     # but the Rails 4.2 DOM assertion doesn't handle the ? in the
                     # attribute value
-                    'http://mydomain.foo/rdm/journals/diff/3',
+                    'http://mydomain.foo/rdm/journals/3/diff',
                     'View differences',
                     :text => 'diff'
       # link to an attachment