From e045306a5cea6eff1d07004227d38caf06a3394c Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 13:41:53 -0800 Subject: [PATCH 1/6] [#808] Add routing for journal diffs --- config/routes.rb | 3 ++- test/integration/routing_test.rb | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index f012ce2a..94094df6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -95,7 +95,8 @@ ActionController::Routing::Routes.draw do |map| map.bulk_update_issue 'issues/bulk_edit', :controller => 'issues', :action => 'bulk_update', :conditions => { :method => :post } map.quoted_issue '/issues/:id/quoted', :controller => 'journals', :action => 'new', :id => /\d+/, :conditions => { :method => :post } map.connect '/issues/:id/destroy', :controller => 'issues', :action => 'destroy', :conditions => { :method => :post } # legacy - + map.journal_diff '/journals/:id/diff/:field', :controller => 'journals', :action => 'diff', :conditions => { :method => :get } + map.resource :gantt, :path_prefix => '/issues', :controller => 'gantts', :only => [:show, :update] map.resource :gantt, :path_prefix => '/projects/:project_id/issues', :controller => 'gantts', :only => [:show, :update] map.resource :calendar, :path_prefix => '/issues', :controller => 'calendars', :only => [:show, :update] diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index dc9f0532..abdcc58e 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -109,6 +109,10 @@ class RoutingTest < ActionController::IntegrationTest should_route :post, "/issues/bulk_edit", :controller => 'issues', :action => 'bulk_update' end + context "journals" do + should_route :get, "/journals/100/diff/description", :controller => 'journals', :action => 'diff', :id => '100', :field => 'description' + end + context "issue categories" do should_route :get, "/projects/test/issue_categories/new", :controller => 'issue_categories', :action => 'new', :project_id => 'test' From 5ad97a4ea367ef5ebad309e3468dcae7e47e56fb Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 14:16:38 -0800 Subject: [PATCH 2/6] [#808] Add JournalsController#diff to diff a single field --- app/controllers/journals_controller.rb | 22 +++++++ app/views/journals/diff.html.erb | 9 +++ test/functional/journals_controller_test.rb | 64 +++++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 app/views/journals/diff.html.erb diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb index bf217aa6..4059100d 100644 --- a/app/controllers/journals_controller.rb +++ b/app/controllers/journals_controller.rb @@ -12,6 +12,8 @@ # See doc/COPYRIGHT.rdoc for more details. #++ +require 'diff' + class JournalsController < ApplicationController before_filter :find_journal, :only => [:edit, :diff] before_filter :find_issue, :only => [:new] @@ -84,6 +86,21 @@ class JournalsController < ApplicationController end end + def diff + if valid_field?(params[:field]) + from = @journal.changes[params[:field]][0] + to = @journal.changes[params[:field]][1] + + @diff = Redmine::Helpers::Diff.new(to, from) + @issue = @journal.journaled + respond_to do |format| + format.html { } + end + else + render_404 + end + end + private def find_journal @@ -100,4 +117,9 @@ class JournalsController < ApplicationController rescue ActiveRecord::RecordNotFound render_404 end + + # Is this a valid field for diff'ing? + def valid_field?(field) + field.to_s.strip == "description" + end end diff --git a/app/views/journals/diff.html.erb b/app/views/journals/diff.html.erb new file mode 100644 index 00000000..b1783494 --- /dev/null +++ b/app/views/journals/diff.html.erb @@ -0,0 +1,9 @@ +

<%= authoring @journal.created_at, @journal.user, :label => :label_updated_time_by %>

+ +
+<%= simple_format_without_paragraph @diff.to_html %> +
+ +

<%= link_to(l(:button_back), issue_path(@issue)) %>

+ +<% html_title "#{@issue.tracker.name} ##{@issue.id}: #{@issue.subject}" %> diff --git a/test/functional/journals_controller_test.rb b/test/functional/journals_controller_test.rb index 2646b347..ac32744b 100644 --- a/test/functional/journals_controller_test.rb +++ b/test/functional/journals_controller_test.rb @@ -81,4 +81,68 @@ class JournalsControllerTest < ActionController::TestCase assert_select_rjs :show, "update" end + context "#diff" do + setup do + @request.session[:user_id] = 1 + @issue = Issue.find(6) + @previous_description = @issue.description + @new_description = "New description" + + assert_difference("Journal.count") do + @issue.description = @new_description + assert @issue.save + end + @last_journal = @issue.last_journal + end + + context "without a valid journal" do + should "return a 404" do + get :diff, :id => '0' + assert_response :not_found + end + end + + + context "with no field parameter" do + should "return a 404" do + get :diff, :id => @last_journal.id + assert_response :not_found + end + end + + context "for an invalid field" do + should "return a 404" do + get :diff, :id => @last_journal.id, :field => 'id' + assert_response :not_found + end + end + + context "without permission to view_issues" do + should "return a 403" do + @request.session[:user_id] = 7 + get :diff, :id => @last_journal.id, :field => 'description' + + assert_response :forbidden + end + + end + + context "with permission to view_issues" do + setup do + get :diff, :id => @last_journal.id, :field => 'description' + end + + should "create a diff" do + assert_not_nil assigns(:diff) + assert assigns(:diff).is_a?(Redmine::Helpers::Diff) + end + + should "render an inline diff" do + assert_select "#content .text-diff" + end + + end + + end + end From 08454ab7fa82e79d1b80063657f576713701fd4d Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 15:15:56 -0800 Subject: [PATCH 3/6] [#808] Truncate and show a link to the full journal diff in the issue history --- test/integration/journals_test.rb | 47 +++++++++++++++++++ test/integration_test_helpers.rb | 2 +- .../lib/journal_formatter.rb | 14 ++++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 test/integration/journals_test.rb diff --git a/test/integration/journals_test.rb b/test/integration/journals_test.rb new file mode 100644 index 00000000..3bd067e8 --- /dev/null +++ b/test/integration/journals_test.rb @@ -0,0 +1,47 @@ +#-- encoding: UTF-8 +#-- copyright +# ChiliProject is a project management system. +# +# Copyright (C) 2010-2011 the ChiliProject Team +# +# 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. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ +require File.expand_path('../../test_helper', __FILE__) +require 'capybara/rails' + +class JournalsTest < ActionController::IntegrationTest + fixtures :all + + include IntegrationTestHelpers::CapybaraHelpers + include Capybara::DSL + + test "showing issue description changes as a diff" do + # Description change + @issue = Issue.find(1) + @issue.recreate_initial_journal! + @issue.reload + assert_difference("Journal.count") do + @issue.journal_user = User.find_by_login('jsmith') + @issue.description = "A new description" + assert @issue.save + end + + log_user('jsmith', 'jsmith') + + # Issue page + visit_issue_page(@issue) + assert has_selector?("#history .journal-attributes li i", :text => 'A new description') + within("#history .journal-attributes li") do + find_link("More").click + end + + # Diff page + assert_response :success + assert has_selector?("#content .text-diff", :text => /A new description/) + end +end diff --git a/test/integration_test_helpers.rb b/test/integration_test_helpers.rb index 9e20efef..2e9b0c2d 100644 --- a/test/integration_test_helpers.rb +++ b/test/integration_test_helpers.rb @@ -51,7 +51,7 @@ module IntegrationTestHelpers visit "/login" fill_in 'Login', :with => user fill_in 'Password', :with => password - click_button 'login' + click_button 'Login' assert_response :success assert User.current.logged? end diff --git a/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb b/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb index 44c6bf36..dddf4d8c 100644 --- a/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb +++ b/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb @@ -28,6 +28,7 @@ module JournalFormatter include CustomFieldsHelper include ActionView::Helpers::TagHelper include ActionView::Helpers::UrlHelper + include ActionView::Helpers::TextHelper include ActionController::UrlWriter extend Redmine::I18n @@ -122,6 +123,16 @@ module JournalFormatter [label, old_value, value] end + # Formats a detail to be used with a Journal diff + # + # Truncates the content. Adds a link to view a diff. + def format_html_diff_detail(key, label, old_value, value) + link = link_to(l(:label_more), {:controller => 'journals', :action => 'diff', :id => id, :field => key.to_s}) + old_value = truncate(old_value, :length => 80) + value = truncate(value, :length => 80) + " " + link + [old_value, value] + end + def property(detail) key = prop_key(detail) if key.start_with? "custom_values" @@ -186,6 +197,9 @@ module JournalFormatter unless no_html label, old_value, value = *format_html_detail(label, old_value, value) value = format_html_attachment_detail(key.sub("attachments", ""), value) if attachment_detail + if property(detail) == :attribute && key == "description" + old_value, value = *format_html_diff_detail(key, label, old_value, value) + end end unless value.blank? From aafec2c50fce0f0d491b336c6f84b8da75e225a5 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 16:29:01 -0800 Subject: [PATCH 4/6] Add some nice defaults for jQuery ajax --- public/javascripts/application.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/public/javascripts/application.js b/public/javascripts/application.js index c5c1bd7e..98bbca12 100644 --- a/public/javascripts/application.js +++ b/public/javascripts/application.js @@ -465,6 +465,15 @@ jQuery.viewportHeight = function() { document.body.clientHeight; }; +// Automatically use format.js for jQuery Ajax +jQuery.ajaxSetup({ + 'beforeSend': function(xhr) {xhr.setRequestHeader("Accept", "text/javascript")} +}) + +// Show/hide the ajax indicators +jQuery("#ajax-indicator").ajaxStart(function(){ jQuery(this).show().css('z-index', '9999'); }); +jQuery("#ajax-indicator").ajaxStop(function(){ jQuery(this).hide(); }); + /* TODO: integrate with existing code and/or refactor */ jQuery(document).ready(function($) { From 4acee9e9896f3d780647722956f3cb53a0714231 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 16:29:39 -0800 Subject: [PATCH 5/6] Add a reusable dialog-window for lightbox style popups --- app/views/layouts/base.rhtml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/layouts/base.rhtml b/app/views/layouts/base.rhtml index 2aeaf172..08819a3c 100644 --- a/app/views/layouts/base.rhtml +++ b/app/views/layouts/base.rhtml @@ -145,6 +145,7 @@ + <%= call_hook :view_layouts_base_body_bottom %> From 38d0d530b00215e219e9e72ffb390961a5b5faa0 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 28 Dec 2011 16:31:34 -0800 Subject: [PATCH 6/6] [#808] Show issue description diffs in the lightbox popup --- app/controllers/journals_controller.rb | 1 + public/javascripts/application.js | 27 +++++++++++++++++++ .../lib/journal_formatter.rb | 2 +- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb index 4059100d..9806481b 100644 --- a/app/controllers/journals_controller.rb +++ b/app/controllers/journals_controller.rb @@ -95,6 +95,7 @@ class JournalsController < ApplicationController @issue = @journal.journaled respond_to do |format| format.html { } + format.js { render :layout => false } end else render_404 diff --git a/public/javascripts/application.js b/public/javascripts/application.js index 98bbca12..74e24b2e 100644 --- a/public/javascripts/application.js +++ b/public/javascripts/application.js @@ -617,4 +617,31 @@ jQuery(document).ready(function($) { $('#nav-login-content').click(function(event){ event.stopPropagation(); }); + + $('.lightbox-ajax').click(function(event) { + $('#dialog-window'). + html(''). + load(this.href, function() { + // Set width to the content width + var width = $('#content').width(); + $(this).dialog("option", "width", width).dialog('open'); + }); + + event.preventDefault(); + return false; + + }); + + // Configures the default dialog window + function setUpDialogWindow() { + $('#dialog-window'). + dialog({ + autoOpen: false, + minWidth: 400, + width: 800 + }); + + } + + setUpDialogWindow(); }); diff --git a/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb b/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb index dddf4d8c..9fd7d644 100644 --- a/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb +++ b/vendor/plugins/acts_as_journalized/lib/journal_formatter.rb @@ -127,7 +127,7 @@ module JournalFormatter # # Truncates the content. Adds a link to view a diff. def format_html_diff_detail(key, label, old_value, value) - link = link_to(l(:label_more), {:controller => 'journals', :action => 'diff', :id => id, :field => key.to_s}) + link = link_to(l(:label_more), {:controller => 'journals', :action => 'diff', :id => id, :field => key.to_s}, :class => 'lightbox-ajax') old_value = truncate(old_value, :length => 80) value = truncate(value, :length => 80) + " " + link [old_value, value]