From fbd38b3678072dcaaf23ad78f0eec16bc2ca7419 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 10:32:04 -0700 Subject: [PATCH 01/26] Fix tests due to core changes --- app/helpers/contracts_helper.rb | 9 ++++----- .../helper_issues_show_detail_after_setting_hook_test.rb | 6 +++--- .../hooks/view_issues_show_details_bottom_hook_test.rb | 3 +++ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index b50249e..4e49e50 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -1,10 +1,9 @@ module ContractsHelper def setup_nested_deliverable_records(deliverable) - returning(deliverable) do |d| - d.labor_budgets.build if d.labor_budgets.empty? - d.overhead_budgets.build if d.overhead_budgets.empty? - d.fixed_budgets.build if d.fixed_budgets.empty? - end + deliverable.labor_budgets.build if deliverable.labor_budgets.empty? + deliverable.overhead_budgets.build if deliverable.overhead_budgets.empty? + deliverable.fixed_budgets.build if deliverable.fixed_budgets.empty? + deliverable end # Simple helper to show the values of a field on an object in a standard format diff --git a/test/integration/redmine_contracts/hooks/helper_issues_show_detail_after_setting_hook_test.rb b/test/integration/redmine_contracts/hooks/helper_issues_show_detail_after_setting_hook_test.rb index 9debdf2..c96dad8 100644 --- a/test/integration/redmine_contracts/hooks/helper_issues_show_detail_after_setting_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/helper_issues_show_detail_after_setting_hook_test.rb @@ -19,15 +19,15 @@ class RedmineContracts::Hooks::HelperIssuesShowDetailAfterSettingHookTest < Acti # Set first @issue.init_journal(@manager) @issue.deliverable = @deliverable1 - @issue.save! + @issue.save! && @issue.reload # Change @issue.init_journal(@manager) @issue.deliverable = @deliverable2 - @issue.save! + @issue.save! && @issue.reload # Unset @issue.init_journal(@manager) @issue.deliverable = nil - @issue.save! + @issue.save! && @issue.reload login_as('manager', 'existing') diff --git a/test/unit/lib/redmine_contracts/hooks/view_issues_show_details_bottom_hook_test.rb b/test/unit/lib/redmine_contracts/hooks/view_issues_show_details_bottom_hook_test.rb index 0e42dfb..845cccb 100644 --- a/test/unit/lib/redmine_contracts/hooks/view_issues_show_details_bottom_hook_test.rb +++ b/test/unit/lib/redmine_contracts/hooks/view_issues_show_details_bottom_hook_test.rb @@ -19,6 +19,9 @@ class RedmineContracts::Hooks::ViewIssuesShowDetailsBottomTest < ActionControlle @controller ||= ApplicationController.new @controller.class.send(:include, ::Redmine::I18n) @controller.response ||= ActionController::TestResponse.new + def @controller.api_request? + false + end # Hack to support render_on @controller.instance_variable_set('@template', template) @controller.response = response From 9453b52c100fc5c1876f7443539af42beea4618a Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 11:48:35 -0700 Subject: [PATCH 02/26] [#6441] Add a manual Contract#status --- app/helpers/contracts_helper.rb | 12 ++++ app/models/contract.rb | 11 +++- app/views/contracts/_form.html.erb | 1 + app/views/contracts/index.html.erb | 74 ++++++++++++++++++++--- app/views/contracts/show.html.erb | 5 +- db/migrate/018_add_status_to_contracts.rb | 10 +++ test/integration/contracts_edit_test.rb | 3 +- test/integration/contracts_list_test.rb | 28 +++++++-- test/integration/contracts_new_test.rb | 2 + test/unit/contract_test.rb | 9 +++ 10 files changed, 136 insertions(+), 19 deletions(-) create mode 100644 db/migrate/018_add_status_to_contracts.rb diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 4e49e50..9d93b38 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -6,6 +6,18 @@ module ContractsHelper deliverable end + def group_contracts_by_status(contracts) + grouped_contracts = contracts.inject({}) do |grouped, contract| + grouped[contract.status] ||= [] + grouped[contract.status] << contract + grouped + end + grouped_contracts["open"] ||= [] + grouped_contracts["locked"] ||= [] + grouped_contracts["closed"] ||= [] + grouped_contracts + end + # Simple helper to show the values of a field on an object in a standard format # #

diff --git a/app/models/contract.rb b/app/models/contract.rb index 80d8d61..87e5de6 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -17,6 +17,7 @@ class Contract < ActiveRecord::Base validates_presence_of :start_date validates_presence_of :end_date validates_inclusion_of :discount_type, :in => %w($ %), :allow_blank => true, :allow_nil => true + validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true validate :start_and_end_date_are_valid # Accessors @@ -33,15 +34,20 @@ class Contract < ActiveRecord::Base attr_accessible :po_number attr_accessible :client_point_of_contact attr_accessible :details + attr_accessible :status named_scope :by_name, {:order => "#{Contract.table_name}.name ASC"} - [:status, :contract_type, + [:contract_type, :discount_spent, :discount_budget ].each do |mthd| define_method(mthd) { "TODO in later release" } end + def status + read_attribute(:status) || "open" + end + # ------------------------------------------------------------ # Labor Methods # ------------------------------------------------------------ @@ -229,6 +235,7 @@ class Contract < ActiveRecord::Base def after_initialize self.executed = false unless self.executed.present? + self.status = "open" unless self.status.present? end # Are the start_date and end_date valid? @@ -270,5 +277,5 @@ class Contract < ActiveRecord::Base def summarize_associated_values(records, value_method) records.inject(0) {|total, record| total += record.send(value_method)} end - + end diff --git a/app/views/contracts/_form.html.erb b/app/views/contracts/_form.html.erb index 0a8c204..8a762cc 100644 --- a/app/views/contracts/_form.html.erb +++ b/app/views/contracts/_form.html.erb @@ -1,6 +1,7 @@

<% form.inputs :name => l(:text_general_legend) do %> <%= form.input :name, :required => true %> + <%= form.input :status, :required => true, :collection => [["Open","open"],["Locked","locked"],["Closed","closed"]] %> <%= form.input :account_executive, :required => true, :collection => @project.users.sort %>
  • <%= label('contract', 'executed') %> diff --git a/app/views/contracts/index.html.erb b/app/views/contracts/index.html.erb index 42d2734..b4534e8 100644 --- a/app/views/contracts/index.html.erb +++ b/app/views/contracts/index.html.erb @@ -12,10 +12,10 @@
    • - <% if collection.empty? %> + <% if group_contracts_by_status(collection)["open"].empty? %>

    <%= l(:label_no_data) %>

    <% else %> - +
    @@ -26,11 +26,11 @@ - <% collection.each do |contract| %> + <% group_contracts_by_status(collection)["open"].each do |contract| %> <% content_tag_for(:tr, contract, :class => cycle('','odd')) do %> - + @@ -42,11 +42,71 @@ <% end %>
    -

    Inactive Contracts

    +

    Locked Contracts

    -

    <%= l(:label_no_data) %>

    -

    <%= release(5, "Contract Status. Split contracts by active and inactive") %>

    + <% if group_contracts_by_status(collection)["locked"].empty? %> +

    <%= l(:label_no_data) %>

    + <% else %> +
    <%= l(:field_id) %> <%= l(:field_name) %><%= l(:field_end_date) %>
    <%= link_to(h(contract.id), contract_path(@project, contract)) %> <%= link_to(h(contract.name), contract_path(@project, contract)) %><%= release(5, "Contract Status") %><%= h(contract.status) %> <%= release(5, "Contract Type") %> <%= h(format_value_field_for_contracts(contract.total_budget)) %>
    + + + + + + + + + + + <% group_contracts_by_status(collection)["locked"].each do |contract| %> + <% content_tag_for(:tr, contract, :class => cycle('','odd')) do %> + + + + + + + + <% end %> + <% end %> + +
    <%= l(:field_id) %><%= l(:field_name) %><%= l(:field_status) %><%= l(:field_type) %><%= l(:field_account_executive_short) %><%= l(:field_total_budget) %><%= l(:field_end_date) %>
    <%= link_to(h(contract.id), contract_path(@project, contract)) %><%= link_to(h(contract.name), contract_path(@project, contract)) %><%= h(contract.status) %><%= release(5, "Contract Type") %><%= h(format_value_field_for_contracts(contract.total_budget)) %><%= h format_date(contract.end_date) %>
    + <% end %> + +
    +

    Closed Contracts

    +
    + + <% if group_contracts_by_status(collection)["closed"].empty? %> +

    <%= l(:label_no_data) %>

    + <% else %> + + + + + + + + + + + + <% group_contracts_by_status(collection)["closed"].each do |contract| %> + <% content_tag_for(:tr, contract, :class => cycle('','odd')) do %> + + + + + + + + <% end %> + <% end %> + +
    <%= l(:field_id) %><%= l(:field_name) %><%= l(:field_status) %><%= l(:field_type) %><%= l(:field_account_executive_short) %><%= l(:field_total_budget) %><%= l(:field_end_date) %>
    <%= link_to(h(contract.id), contract_path(@project, contract)) %><%= link_to(h(contract.name), contract_path(@project, contract)) %><%= h(contract.status) %><%= release(5, "Contract Type") %><%= h(format_value_field_for_contracts(contract.total_budget)) %><%= h format_date(contract.end_date) %>
    + <% end %> + diff --git a/app/views/contracts/show.html.erb b/app/views/contracts/show.html.erb index ea9830a..5c68f7f 100644 --- a/app/views/contracts/show.html.erb +++ b/app/views/contracts/show.html.erb @@ -15,10 +15,7 @@
    - - <%# show_field(resource, :status, :html_options => {:class => 'contract-status'}) %> - - + <%= show_field(resource, :status, :html_options => {:class => 'contract-status'}) %> <%= show_field(resource, :account_executive, :html_options => {:class => 'contract-account-manager'}) %> <%# show_field(resource, :contract_type, :html_options => {:class => 'contract-type'}) %> diff --git a/db/migrate/018_add_status_to_contracts.rb b/db/migrate/018_add_status_to_contracts.rb new file mode 100644 index 0000000..58d1823 --- /dev/null +++ b/db/migrate/018_add_status_to_contracts.rb @@ -0,0 +1,10 @@ +class AddStatusToContracts < ActiveRecord::Migration + def self.up + add_column :contracts, :status, :string + add_index :contracts, :status + end + + def self.down + remove_column :contracts, :status + end +end diff --git a/test/integration/contracts_edit_test.rb b/test/integration/contracts_edit_test.rb index 0e10ae7..5115067 100644 --- a/test/integration/contracts_edit_test.rb +++ b/test/integration/contracts_edit_test.rb @@ -48,12 +48,13 @@ class ContractsEditTest < ActionController::IntegrationTest end fill_in "Name", :with => 'An updated name' + select "Locked", :from => "Status" click_button "Save Contract" assert_response :success assert_template 'contracts/show' assert_equal "An updated name", @contract.reload.name - + assert_equal "locked", @contract.reload.status end end diff --git a/test/integration/contracts_list_test.rb b/test/integration/contracts_list_test.rb index a80d290..4888b91 100644 --- a/test/integration/contracts_list_test.rb +++ b/test/integration/contracts_list_test.rb @@ -5,8 +5,10 @@ class ContractsListTest < ActionController::IntegrationTest def setup @project = Project.generate!(:identifier => 'main') - @contract = Contract.generate!(:project => @project) - @contract2 = Contract.generate!(:project => @project) + @contract = Contract.generate!(:project => @project, :name => 'Contract1').reload + @contract2 = Contract.generate!(:project => @project, :name => 'Contract2').reload + @contract_locked = Contract.generate!(:project => @project, :status => 'locked', :name => 'LockedContract').reload + @contract_closed = Contract.generate!(:project => @project, :status => 'closed', :name => 'ClosedContract').reload @other_project = Project.generate!(:identifier => 'other') @other_contract = Contract.generate!(:project => @other_project) @@ -44,10 +46,10 @@ class ContractsListTest < ActionController::IntegrationTest visit_contracts_for_project(@project) end - should "list all contracts for the project" do + should "list all contracts for the project grouped by status" do visit_contracts_for_project(@project) - assert_select "table#contracts" do + assert_select "table#contracts.open" do [@contract, @contract2].each do |contract| assert_select "td.id", :text => /#{contract.id}/ assert_select "td.name", :text => /#{contract.name}/ @@ -56,7 +58,23 @@ class ContractsListTest < ActionController::IntegrationTest assert_select "td.total-budget" end end - + + assert_select "table#contracts.locked" do + assert_select "td.id", :text => /#{@contract_locked.id}/ + assert_select "td.name", :text => /#{@contract_locked.name}/ + assert_select "td.account-executive", :text => /#{@contract_locked.account_executive.name}/ + assert_select "td.end-date", :text => /#{format_date(@contract_locked.end_date)}/ + assert_select "td.total-budget" + end + + assert_select "table#contracts.closed" do + assert_select "td.id", :text => /#{@contract_closed.id}/ + assert_select "td.name", :text => /#{@contract_closed.name}/ + assert_select "td.account-executive", :text => /#{@contract_closed.account_executive.name}/ + assert_select "td.end-date", :text => /#{format_date(@contract_closed.end_date)}/ + assert_select "td.total-budget" + end + end should "not list contracts from other projects" do diff --git a/test/integration/contracts_new_test.rb b/test/integration/contracts_new_test.rb index 399d970..6bf55c8 100644 --- a/test/integration/contracts_new_test.rb +++ b/test/integration/contracts_new_test.rb @@ -52,6 +52,7 @@ class ContractsNewTest < ActionController::IntegrationTest fill_in "Start", :with => '2010-01-01' fill_in "End Date", :with => '2010-12-31' select "Net 30", :from => "Payment Terms" + select "Locked", :from => "Status" click_button "Save Contract" @@ -64,6 +65,7 @@ class ContractsNewTest < ActionController::IntegrationTest assert_equal '2010-01-01', @contract.start_date.to_s assert_equal '2010-12-31', @contract.end_date.to_s assert_equal 'Net 30', @contract.payment_term.name + assert_equal "locked", @contract.status end end diff --git a/test/unit/contract_test.rb b/test/unit/contract_test.rb index 1eae9a4..5e0242b 100644 --- a/test/unit/contract_test.rb +++ b/test/unit/contract_test.rb @@ -17,6 +17,9 @@ class ContractTest < ActiveSupport::TestCase should_allow_values_for :discount_type, "$", "%", nil, '' should_not_allow_values_for :discount_type, ["amount", "percent", "bar"] + should_allow_values_for :status, "", nil, 'open', 'locked', 'closed' + should_not_allow_values_for :status, "other", "things", "1" + context "end_date" do should "be after start_date" do @contract = Contract.new(:start_date => Date.today, :end_date => Date.yesterday) @@ -32,6 +35,12 @@ class ContractTest < ActiveSupport::TestCase assert_equal false, @contract.executed end + should "default status to open" do + @contract = Contract.new + + assert_equal "open", @contract.status + end + context "#labor_budget" do should "sum all of the labor budgets of the Deliverables" do contract = Contract.generate! From 5e2e3093045ce7fc3a266c1e00c7738840739dbd Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 12:08:46 -0700 Subject: [PATCH 03/26] [#6441] Add a manual Deliverable#status --- app/models/deliverable.rb | 6 +++++- app/views/contracts/show.html.erb | 2 +- app/views/deliverables/_form.html.erb | 1 + db/migrate/019_add_status_to_deliverables.rb | 10 ++++++++++ test/integration/deliverables_edit_test.rb | 4 ++++ test/integration/deliverables_new_test.rb | 10 ++++++++-- test/unit/deliverable_test.rb | 7 +++++++ 7 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 db/migrate/019_add_status_to_deliverables.rb diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 003805b..fa93bf9 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -19,6 +19,7 @@ class Deliverable < ActiveRecord::Base validates_presence_of :title validates_presence_of :type validates_presence_of :manager + validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true # Accessors include DollarizedAttribute @@ -27,7 +28,10 @@ class Deliverable < ActiveRecord::Base delegate :name, :to => :contract, :prefix => true, :allow_nil => true # Callbacks - + def after_initialize + self.status = "open" unless self.status.present? + end + # Register callbacks here, on new records the class isn't set so class-specific # callbacks don't fire. def after_save diff --git a/app/views/contracts/show.html.erb b/app/views/contracts/show.html.erb index 5c68f7f..aff2cda 100644 --- a/app/views/contracts/show.html.erb +++ b/app/views/contracts/show.html.erb @@ -130,7 +130,7 @@ - + <%= format_budget_for_deliverable(deliverable, deliverable.labor_budget_spent, deliverable.labor_budget_total, :class => 'labor') %> <%= format_budget_for_deliverable(deliverable, deliverable.overhead_spent, deliverable.overhead_budget_total, :class => 'overhead') %> diff --git a/app/views/deliverables/_form.html.erb b/app/views/deliverables/_form.html.erb index 1da5da6..b824160 100644 --- a/app/views/deliverables/_form.html.erb +++ b/app/views/deliverables/_form.html.erb @@ -19,6 +19,7 @@ <%= form.input :type, :as => :hidden, :class => 'type' %> <% end %> + <%= form.input :status, :required => true, :collection => [["Open","open"],["Locked","locked"],["Closed","closed"]] %> <%= form.input :manager, :required => true, :collection => @project.users.sort %> <%= form.input :start_date, :as => :string, :input_html => {:size => 10, :class => 'start-date', :id => 'deliverable_start_date'}, :hint => calendar_for('deliverable_start_date') %> diff --git a/db/migrate/019_add_status_to_deliverables.rb b/db/migrate/019_add_status_to_deliverables.rb new file mode 100644 index 0000000..b4dbb7d --- /dev/null +++ b/db/migrate/019_add_status_to_deliverables.rb @@ -0,0 +1,10 @@ +class AddStatusToDeliverables < ActiveRecord::Migration + def self.up + add_column :deliverables, :status, :string + add_index :deliverables, :status + end + + def self.down + remove_column :deliverables, :status + end +end diff --git a/test/integration/deliverables_edit_test.rb b/test/integration/deliverables_edit_test.rb index 4082190..cde9edc 100644 --- a/test/integration/deliverables_edit_test.rb +++ b/test/integration/deliverables_edit_test.rb @@ -49,6 +49,7 @@ class DeliverablesEditTest < ActionController::IntegrationTest within("#deliverable-details") do fill_in "Title", :with => 'An updated title' + select "Locked", :from => "Status" check "Feature Sign Off" check "Warranty Sign Off" end @@ -61,6 +62,7 @@ class DeliverablesEditTest < ActionController::IntegrationTest assert_equal "FixedDeliverable", @fixed_deliverable.reload.type assert @fixed_deliverable.reload.warranty_sign_off? assert @fixed_deliverable.reload.feature_sign_off? + assert_equal "locked", @fixed_deliverable.reload.status end @@ -78,6 +80,7 @@ class DeliverablesEditTest < ActionController::IntegrationTest within("#deliverable-details") do fill_in "Title", :with => 'An updated title' + select "Locked", :from => "Status" check "Feature Sign Off" check "Warranty Sign Off" end @@ -101,6 +104,7 @@ class DeliverablesEditTest < ActionController::IntegrationTest assert_equal "HourlyDeliverable", @hourly_deliverable.reload.type assert @hourly_deliverable.reload.warranty_sign_off? assert @hourly_deliverable.reload.feature_sign_off? + assert_equal "locked", @hourly_deliverable.reload.status assert_equal 1, @hourly_deliverable.labor_budgets.count @labor_budget = @hourly_deliverable.labor_budgets.first diff --git a/test/integration/deliverables_new_test.rb b/test/integration/deliverables_new_test.rb index a55d5f8..97edac4 100644 --- a/test/integration/deliverables_new_test.rb +++ b/test/integration/deliverables_new_test.rb @@ -74,6 +74,7 @@ class DeliverablesNewTest < ActionController::IntegrationTest within("#deliverable-details") do fill_in "Title", :with => 'A New Deliverable' select "Fixed", :from => "Type" + select "Locked", :from => "Status" select @manager.name, :from => "Manager" fill_in "Start", :with => '2010-01-01' fill_in "End Date", :with => '2010-12-31' @@ -95,6 +96,7 @@ class DeliverablesNewTest < ActionController::IntegrationTest assert_equal '2010-12-31', @deliverable.end_date.to_s assert_equal @manager, @deliverable.manager assert_equal 1000.0, @deliverable.total.to_f + assert_equal "locked", @deliverable.status end should "create a new Hourly deliverable" do @@ -109,6 +111,7 @@ class DeliverablesNewTest < ActionController::IntegrationTest within("#deliverable-details") do fill_in "Title", :with => 'A New Deliverable' select "Hourly", :from => "Type" + select "Locked", :from => "Status" select @manager.name, :from => "Manager" fill_in "Start", :with => '2010-01-01' fill_in "End Date", :with => '2010-12-31' @@ -128,7 +131,8 @@ class DeliverablesNewTest < ActionController::IntegrationTest assert_equal '2010-01-01', @deliverable.start_date.to_s assert_equal '2010-12-31', @deliverable.end_date.to_s assert_equal @manager, @deliverable.manager - + assert_equal "locked", @deliverable.status + end should "create a new Retainer deliverable" do @@ -143,6 +147,7 @@ class DeliverablesNewTest < ActionController::IntegrationTest within("#deliverable-details") do fill_in "Title", :with => 'A New Deliverable' select "Retainer", :from => "Type" + select "Locked", :from => "Status" select @manager.name, :from => "Manager" fill_in "Start", :with => '2010-01-01' fill_in "End Date", :with => '2010-12-31' @@ -171,7 +176,8 @@ class DeliverablesNewTest < ActionController::IntegrationTest assert_equal '2010-01-01', @deliverable.start_date.to_s assert_equal '2010-12-31', @deliverable.end_date.to_s assert_equal @manager, @deliverable.manager - + assert_equal "locked", @deliverable.status + # Budget items, one per month labor_budgets = @deliverable.labor_budgets assert_equal 12, labor_budgets.length diff --git a/test/unit/deliverable_test.rb b/test/unit/deliverable_test.rb index 694adc0..4cd7965 100644 --- a/test/unit/deliverable_test.rb +++ b/test/unit/deliverable_test.rb @@ -12,6 +12,13 @@ class DeliverableTest < ActiveSupport::TestCase should_validate_presence_of :type should_validate_presence_of :manager + should_allow_values_for :status, "", nil, 'open', 'locked', 'closed' + should_not_allow_values_for :status, "other", "things", "1" + + should "default status to open" do + assert_equal "open", Deliverable.new.status + end + context "#total=" do should "strip dollar signs when writing" do d = Deliverable.new From 953587ce467c6b4b93adabd3ee59eec93a359244 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 13:46:28 -0700 Subject: [PATCH 04/26] [#6441] Block logging time on closed contracts or deliverables --- app/models/contract.rb | 16 +++ app/models/deliverable.rb | 16 +++ config/locales/en.yml | 6 + init.rb | 3 + .../patches/time_entry_patch.rb | 36 ++++++ .../patches/time_entry_patch_test.rb | 109 ++++++++++++++++++ 6 files changed, 186 insertions(+) create mode 100644 lib/redmine_contracts/patches/time_entry_patch.rb create mode 100644 test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb diff --git a/app/models/contract.rb b/app/models/contract.rb index 87e5de6..e734b7a 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -48,6 +48,22 @@ class Contract < ActiveRecord::Base read_attribute(:status) || "open" end + def lock! + update_attribute(:status, "locked") + end + + def close! + update_attribute(:status, "closed") + end + + def locked? + self.status == "locked" + end + + def closed? + self.status == "closed" + end + # ------------------------------------------------------------ # Labor Methods # ------------------------------------------------------------ diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index fa93bf9..e52aeb6 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -55,6 +55,22 @@ class Deliverable < ActiveRecord::Base nil end + def lock! + update_attribute(:status, "locked") + end + + def close! + update_attribute(:status, "closed") + end + + def locked? + self.status == "locked" + end + + def closed? + self.status == "closed" + end + def to_s title end diff --git a/config/locales/en.yml b/config/locales/en.yml index 661837d..0001f08 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,4 +1,10 @@ en: + activerecord: + errors: + messages: + cant_to_closed_deliverable: "Can't create a time entry on a closed deliverable" + cant_to_closed_contract: "Can't create a time entry on a closed contract" + field_end_date: End Date field_executed: Executed text_contracts: Contracts diff --git a/init.rb b/init.rb index 19e673c..eb3c616 100644 --- a/init.rb +++ b/init.rb @@ -62,6 +62,9 @@ end require 'dispatcher' Dispatcher.to_prepare :redmine_contracts do + + require_dependency 'time_entry' + TimeEntry.send(:include, RedmineContracts::Patches::TimeEntryPatch) gem 'inherited_resources', :version => '1.0.6' require_dependency 'inherited_resources' require_dependency 'inherited_resources/base' diff --git a/lib/redmine_contracts/patches/time_entry_patch.rb b/lib/redmine_contracts/patches/time_entry_patch.rb new file mode 100644 index 0000000..58f9d69 --- /dev/null +++ b/lib/redmine_contracts/patches/time_entry_patch.rb @@ -0,0 +1,36 @@ +module RedmineContracts + module Patches + module TimeEntryPatch + def self.included(base) + base.extend(ClassMethods) + + base.send(:include, InstanceMethods) + base.class_eval do + unloadable + + validate :validate_deliverable_status + validate :validate_contract_status + + def validate_deliverable_status + if issue.present? && issue.deliverable.present? + errors.add_to_base(:cant_to_closed_deliverable) if issue.deliverable.closed? + end + end + + def validate_contract_status + if issue.present? && issue.deliverable.present? && issue.deliverable.contract.present? + errors.add_to_base(:cant_to_closed_contract) if issue.deliverable.contract.closed? + end + end + + end + end + + module ClassMethods + end + + module InstanceMethods + end + end + end +end diff --git a/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb b/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb new file mode 100644 index 0000000..d844c15 --- /dev/null +++ b/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb @@ -0,0 +1,109 @@ +require File.dirname(__FILE__) + '/../../../../test_helper' + +class RedmineContracts::Patches::TimeEntryTest < ActionController::TestCase + + def setup + @project = Project.generate! + @contract = Contract.generate!(:project => @project, :status => 'open') + @deliverable = FixedDeliverable.generate!(:contract => @contract, :status => 'open').reload + @issue = Issue.generate_for_project!(@project, :deliverable => @deliverable).reload + assert_equal @deliverable, @issue.deliverable + @user = User.generate! + @role = Role.generate! + User.add_to_project(@user, @project, @role) + @activity = TimeEntryActivity.generate! + end + + def create_time_entry + @issue.reload + @time_entry = TimeEntry.create(:issue => @issue, + :project => @project, + :spent_on => Date.today, + :activity => @activity, + :hours => 10, + :user => @user) + end + + def assert_error_about_closed_deliverable(time_entry) + assert_equal "Can't create a time entry on a closed deliverable", time_entry.errors.on_base + end + + def assert_error_about_closed_contract(time_entry) + assert_equal "Can't create a time entry on a closed contract", time_entry.errors.on_base + end + + should "allow logging time to an issue on an open deliverable, open contract" do + assert_difference("TimeEntry.count") { create_time_entry } + end + + should "allow logging time to an issue on a locked deliverable, open contract" do + assert @deliverable.lock! + assert @deliverable.locked? + + assert_difference("TimeEntry.count") { create_time_entry } + end + + should "allow logging time to an issue on an open deliverable, locked contract" do + assert @contract.lock! + assert @contract.locked? + + assert_difference("TimeEntry.count") { create_time_entry } + end + + should "allow logging time to an issue on a locked deliverable, locked contract" do + assert @deliverable.lock! + assert @deliverable.locked? + assert @contract.lock! + assert @contract.locked? + + assert_difference("TimeEntry.count") { create_time_entry } + end + + should "block logging time to an issue on a closed deliverable, open contract" do + assert @deliverable.close! + assert @deliverable.closed? + + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_closed_deliverable(@time_entry) + end + + should "block logging time to an issue on a closed deliverable, locked contract" do + assert @deliverable.close! + assert @deliverable.closed? + assert @contract.lock! + assert @contract.locked? + + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_closed_deliverable(@time_entry) + end + + should "block logging time to an issue on an open deliverable, closed contract" do + assert @contract.close! + assert @contract.closed? + + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_closed_contract(@time_entry) + end + + should "block logging time to an issue on a locked deliverable, closed contract" do + assert @deliverable.lock! + assert @deliverable.locked? + assert @contract.close! + assert @contract.closed? + + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_closed_contract(@time_entry) + end + + should "block logging time to an issue on a closed deliverable, closed contract" do + assert @deliverable.close! + assert @deliverable.closed? + assert @contract.close! + assert @contract.closed? + + assert_no_difference("TimeEntry.count") { create_time_entry } + assert @time_entry.errors.on_base.include?("Can't create a time entry on a closed deliverable") + assert @time_entry.errors.on_base.include?("Can't create a time entry on a closed contract") + end + +end From 07c3c42ca847a37eb1a2725d4b3358ed8c518994 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 14:12:06 -0700 Subject: [PATCH 05/26] [#6441] Refactor logic in view to helpers --- app/helpers/contracts_helper.rb | 10 ++++++++++ app/views/issues/_edit_deliverable.html.erb | 5 +---- init.rb | 3 +++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 9d93b38..961ce05 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -17,6 +17,16 @@ module ContractsHelper grouped_contracts["closed"] ||= [] grouped_contracts end + + def deliverable_options(project) + project.contracts.inject([]) do |data, contract| + data << [contract.name, deliverable_options_for_contract(contract)] + end + end + + def deliverable_options_for_contract(contract) + contract.deliverables.collect {|d| [d.title, d.id]} + end # Simple helper to show the values of a field on an object in a standard format # diff --git a/app/views/issues/_edit_deliverable.html.erb b/app/views/issues/_edit_deliverable.html.erb index 47fd42b..9daab6f 100644 --- a/app/views/issues/_edit_deliverable.html.erb +++ b/app/views/issues/_edit_deliverable.html.erb @@ -1,9 +1,6 @@ <% if project.module_enabled?(:contracts) && User.current.allowed_to?(:assign_deliverable_to_issue, project) %>

    - <% options = project.contracts.inject([]) {|data, contract| - data << [contract.name, contract.deliverables.collect {|d| [d.title, d.id]} ] - } %> - <%= form.select(:deliverable_id, grouped_options_for_select(options, issue.deliverable_id), {:include_blank => true}) %> + <%= form.select(:deliverable_id, grouped_options_for_select(deliverable_options(project), issue.deliverable_id), {:include_blank => true}) %>

    <% end %> diff --git a/init.rb b/init.rb index eb3c616..ea166d9 100644 --- a/init.rb +++ b/init.rb @@ -102,6 +102,9 @@ Dispatcher.to_prepare :redmine_contracts do unless Query.available_columns.collect(&:name).include?(:contract_name) Query.add_available_column(QueryColumn.new(:contract_name, :sortable => "#{Contract.table_name}.name", :groupable => 'contracts.name')) end + + require_dependency 'application_controller' + ApplicationController.send(:helper, :contracts) end require 'redmine_contracts/hooks/view_layouts_base_html_head_hook' From 51f62ca5a31760cc561ef19800c3251e45d97700 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 15:14:22 -0700 Subject: [PATCH 06/26] [#6441] Block assigning issues to a locked or closed Deliverable --- config/locales/en.yml | 6 +- lib/redmine_contracts/patches/issue_patch.rb | 10 +++ .../patches/time_entry_patch.rb | 4 +- .../controller_issues_edit_before_save.rb | 79 ++++++++++++++++++- 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 0001f08..dec6e89 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,8 +2,10 @@ en: activerecord: errors: messages: - cant_to_closed_deliverable: "Can't create a time entry on a closed deliverable" - cant_to_closed_contract: "Can't create a time entry on a closed contract" + cant_create_time_on_closed_deliverable: "Can't create a time entry on a closed deliverable" + cant_create_time_on_closed_contract: "Can't create a time entry on a closed contract" + cant_assign_to_closed_deliverable: "Can't assign issue to a closed deliverable" + cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" field_end_date: End Date field_executed: Executed diff --git a/lib/redmine_contracts/patches/issue_patch.rb b/lib/redmine_contracts/patches/issue_patch.rb index 4df6003..ba9100d 100644 --- a/lib/redmine_contracts/patches/issue_patch.rb +++ b/lib/redmine_contracts/patches/issue_patch.rb @@ -15,6 +15,16 @@ module RedmineContracts def contract_name contract.try(:name) end + + validate :validate_deliverable_status + + def validate_deliverable_status + if deliverable.present? && changes["deliverable_id"].present? + errors.add_to_base(:cant_assign_to_closed_deliverable) if deliverable.closed? + errors.add_to_base(:cant_assign_to_locked_deliverable) if deliverable.locked? + end + end + end end diff --git a/lib/redmine_contracts/patches/time_entry_patch.rb b/lib/redmine_contracts/patches/time_entry_patch.rb index 58f9d69..0a327b2 100644 --- a/lib/redmine_contracts/patches/time_entry_patch.rb +++ b/lib/redmine_contracts/patches/time_entry_patch.rb @@ -13,13 +13,13 @@ module RedmineContracts def validate_deliverable_status if issue.present? && issue.deliverable.present? - errors.add_to_base(:cant_to_closed_deliverable) if issue.deliverable.closed? + errors.add_to_base(:cant_create_time_on_closed_deliverable) if issue.deliverable.closed? end end def validate_contract_status if issue.present? && issue.deliverable.present? && issue.deliverable.contract.present? - errors.add_to_base(:cant_to_closed_contract) if issue.deliverable.contract.closed? + errors.add_to_base(:cant_create_time_on_closed_contract) if issue.deliverable.contract.closed? end end diff --git a/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb b/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb index 4b475b2..6810379 100644 --- a/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb +++ b/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb @@ -24,10 +24,10 @@ class RedmineContracts::Hooks::ControllerIssuesEditBeforeSaveTest < ActionContro context "for a new issue" do setup do visit_project(@project) - click_link "New issue" end should "set the issue's deliverable" do + click_link "New issue" fill_in "Subject", :with => 'Hook test' select @deliverable2.title, :from => "Deliverable" click_button "Create" @@ -38,6 +38,38 @@ class RedmineContracts::Hooks::ControllerIssuesEditBeforeSaveTest < ActionContro end + should "not allow setting a locked Deliverable" do + assert @deliverable2.lock! + click_link "New issue" + + fill_in "Subject", :with => 'Hook test' + select @deliverable2.title, :from => "Deliverable" + assert_no_difference("Issue.count") do + click_button "Create" + + assert_response :success + end + + assert_equal nil, Issue.last.deliverable + + end + + should "not allow setting a closed Deliverable" do + assert @deliverable2.close! + click_link "New issue" + + fill_in "Subject", :with => 'Hook test' + select @deliverable2.title, :from => "Deliverable" + assert_no_difference("Issue.count") do + click_button "Create" + + assert_response :success + end + + assert_equal nil, Issue.last.deliverable + + end + context "with no permission to Assign Deliverable" do should "not allow setting the Deliverable (force HTTP request)" do @role.permissions.delete(:assign_deliverable_to_issue) @@ -69,6 +101,51 @@ class RedmineContracts::Hooks::ControllerIssuesEditBeforeSaveTest < ActionContro end + should "not allow updating to a locked deliverable" do + assert @deliverable2.lock! + select @deliverable2.title, :from => "Deliverable" + click_button "Submit" + + assert_response :success + + @issue.reload + assert_equal nil, @issue.deliverable + + end + + should "not allow updating to a closed deliverable" do + assert @deliverable2.close! + select @deliverable2.title, :from => "Deliverable" + click_button "Submit" + + assert_response :success + + @issue.reload + assert_equal nil, @issue.deliverable + + end + + should "allow updating an issue, even if the deliverable is locked as long as the deliverable isn't changed" do + select @deliverable2.title, :from => "Deliverable" + click_button "Submit" + + assert_response :success + + @issue.reload + assert_equal @deliverable2, @issue.deliverable + + # Now normal update after locking + assert @deliverable2.lock! + fill_in "Subject", :with => 'Change subject' + click_button "Submit" + + assert_response :success + @issue.reload + assert_equal "Change subject", @issue.subject + assert_equal @deliverable2, @issue.deliverable + + end + context "with no permission to Assign Deliverable" do should "not allow setting the Deliverable (force HTTP request)" do @role.permissions.delete(:assign_deliverable_to_issue) From ac24ae08e67a684f54b41dccaf10fe3df89215e5 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Mon, 8 Aug 2011 15:52:13 -0700 Subject: [PATCH 07/26] [#6441] Block editing Locked or Closed Deliverables. --- app/models/deliverable.rb | 27 ++++ app/views/deliverables/_form.html.erb | 6 + config/locales/en.yml | 5 +- test/integration/deliverables_edit_test.rb | 166 ++++++++++++++++++++- 4 files changed, 202 insertions(+), 2 deletions(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index e52aeb6..079bbc6 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -20,6 +20,7 @@ class Deliverable < ActiveRecord::Base validates_presence_of :type validates_presence_of :manager validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true + validate :validate_update_allowed # Accessors include DollarizedAttribute @@ -63,6 +64,10 @@ class Deliverable < ActiveRecord::Base update_attribute(:status, "closed") end + def open? + self.status == "open" + end + def locked? self.status == "locked" end @@ -71,6 +76,28 @@ class Deliverable < ActiveRecord::Base self.status == "closed" end + def editable? + (new_record? || open?) + end + + def validate_update_allowed + unless new_record? + if changes.keys == ["status"] + noop("Allow changes to the status only") + elsif changes["status"].present? && changes["status"].second == "open" + noop("Allow any changes when going to 'open'") + elsif changes["status"].present? + errors.add_to_base(:cant_update_locked_deliverable) if locked? + errors.add_to_base(:cant_update_closed_deliverable) if closed? + end + end + end + + # No operation method, useful to clean up logic with an optional message + # for documentation + def noop(message="") + end + def to_s title end diff --git a/app/views/deliverables/_form.html.erb b/app/views/deliverables/_form.html.erb index b824160..54ddded 100644 --- a/app/views/deliverables/_form.html.erb +++ b/app/views/deliverables/_form.html.erb @@ -2,6 +2,12 @@ <%= javascript_tag("var i18nEndDateEmpty = '#{l(:text_end_date_empty)}'") %> <%= javascript_tag("var i18nChangedPeriodMessage = '#{l(:text_changed_period_message)}'") %> +<% if resource.locked? || resource.closed? %> +
    +

    <%= resource.locked? ? l(:text_deliverable_locked_warning) : l(:text_deliverable_closed_warning) %>

    +
    +<% end %> +
    <% form.inputs :name => l(:text_deliverable_details_legend), :id => 'deliverable-details' do %> <%# Used by jquery to check if this is a new or existing record %> diff --git a/config/locales/en.yml b/config/locales/en.yml index dec6e89..5c260b7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -6,6 +6,8 @@ en: cant_create_time_on_closed_contract: "Can't create a time entry on a closed contract" cant_assign_to_closed_deliverable: "Can't assign issue to a closed deliverable" cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" + cant_update_locked_deliverable: "Can't update a locked deliverable" + cant_update_closed_deliverable: "Can't update a closed deliverable" field_end_date: End Date field_executed: Executed @@ -89,4 +91,5 @@ en: text_error_message_orphaned_time: "There is {{amount}} worth of time clocked to issues that are not assigned to any deliverables." text_error_message_update_orphaned_time: "Please update the orphaned issues." field_estimated: Estimated - + text_deliverable_locked_warning: "This deliverable is locked and cannot be saved without changing it's status to Open." + text_deliverable_closed_warning: "This deliverable is closed and cannot be saved without changing it's status to Open." diff --git a/test/integration/deliverables_edit_test.rb b/test/integration/deliverables_edit_test.rb index cde9edc..55a5c98 100644 --- a/test/integration/deliverables_edit_test.rb +++ b/test/integration/deliverables_edit_test.rb @@ -9,7 +9,7 @@ class DeliverablesEditTest < ActionController::IntegrationTest @manager = User.generate! @role = Role.generate! User.add_to_project(@manager, @project, @role) - @fixed_deliverable = FixedDeliverable.generate!(:contract => @contract, :manager => @manager, :title => 'The Title') + @fixed_deliverable = FixedDeliverable.generate!(:contract => @contract, :manager => @manager, :title => 'The Title', :notes => "", :feature_sign_off => false, :warranty_sign_off => false) @hourly_deliverable = HourlyDeliverable.generate!(:contract => @contract, :manager => @manager, :title => 'An Hourly') @user = User.generate_user_with_permission_to_manage_budget(:project => @project) @@ -475,4 +475,168 @@ class DeliverablesEditTest < ActionController::IntegrationTest assert_equal 3, @retainer_deliverable.fixed_budgets.count assert_equal [600, nil, nil], @retainer_deliverable.fixed_budgets.collect(&:budget) end + + context "locked deliverable" do + setup do + assert @fixed_deliverable.lock! + end + + should "block edits to locked deliverables" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + end + + should "block edits to locked deliverables even when status changes to closed" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + select "Closed", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + assert @fixed_deliverable.reload.locked? + end + + should "be allowed to change the status on a locked deliverables to open" do + assert @fixed_deliverable.lock! + + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Open", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.open? + end + + should "be allowed to change the status on a locked deliverables to closed" do + assert @fixed_deliverable.lock! + + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Closed", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.closed? + end + + end + + context "closed deliverable" do + setup do + assert @fixed_deliverable.close! + end + + should "block edits to closed deliverables" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + end + + should "block edits to closed deliverables even when the status is changed to locked" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + select "Locked", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + assert @fixed_deliverable.reload.closed? + end + + should "be allowed to change the status on a closed deliverables to open" do + assert @fixed_deliverable.close! + + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Open", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.open? + end + + should "be allowed to change the status on a closed deliverables to Locked" do + assert @fixed_deliverable.lock! + + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Locked", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.locked? + end + end + + + end From 0a916fea707afaba7e0df4cd355a7a95717d85e7 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 15:23:08 -0700 Subject: [PATCH 08/26] [#6441] Fix a Deliverable validation error when editing with a status change --- app/models/deliverable.rb | 4 +++- test/integration/deliverables_edit_test.rb | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 079bbc6..867ef7d 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -86,7 +86,9 @@ class Deliverable < ActiveRecord::Base noop("Allow changes to the status only") elsif changes["status"].present? && changes["status"].second == "open" noop("Allow any changes when going to 'open'") - elsif changes["status"].present? + elsif changes["status"].present? && changes["status"].first == "open" + noop("Allow any changes when going from 'open' to another status") + else errors.add_to_base(:cant_update_locked_deliverable) if locked? errors.add_to_base(:cant_update_closed_deliverable) if closed? end diff --git a/test/integration/deliverables_edit_test.rb b/test/integration/deliverables_edit_test.rb index 55a5c98..e783b54 100644 --- a/test/integration/deliverables_edit_test.rb +++ b/test/integration/deliverables_edit_test.rb @@ -618,8 +618,6 @@ class DeliverablesEditTest < ActionController::IntegrationTest end should "be allowed to change the status on a closed deliverables to Locked" do - assert @fixed_deliverable.lock! - visit_contract_page(@contract) click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' assert_response :success From 5c1e254181d146ff0711b1d7c86e37ea907f44cc Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 15:24:58 -0700 Subject: [PATCH 09/26] [#6441] Remove test code that is now in a setup block --- test/integration/deliverables_edit_test.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/integration/deliverables_edit_test.rb b/test/integration/deliverables_edit_test.rb index e783b54..72c71f6 100644 --- a/test/integration/deliverables_edit_test.rb +++ b/test/integration/deliverables_edit_test.rb @@ -518,8 +518,6 @@ class DeliverablesEditTest < ActionController::IntegrationTest end should "be allowed to change the status on a locked deliverables to open" do - assert @fixed_deliverable.lock! - visit_contract_page(@contract) click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' assert_response :success @@ -537,8 +535,6 @@ class DeliverablesEditTest < ActionController::IntegrationTest end should "be allowed to change the status on a locked deliverables to closed" do - assert @fixed_deliverable.lock! - visit_contract_page(@contract) click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' assert_response :success @@ -599,8 +595,6 @@ class DeliverablesEditTest < ActionController::IntegrationTest end should "be allowed to change the status on a closed deliverables to open" do - assert @fixed_deliverable.close! - visit_contract_page(@contract) click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' assert_response :success From 62cdfd35dee3b680744c55d1066124dff2f12ad2 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 15:34:53 -0700 Subject: [PATCH 10/26] [#6441] Refactor: only call validation on updates --- app/models/deliverable.rb | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 867ef7d..ae47687 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -20,7 +20,7 @@ class Deliverable < ActiveRecord::Base validates_presence_of :type validates_presence_of :manager validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true - validate :validate_update_allowed + validate_on_update :validate_status_changes # Accessors include DollarizedAttribute @@ -80,18 +80,16 @@ class Deliverable < ActiveRecord::Base (new_record? || open?) end - def validate_update_allowed - unless new_record? - if changes.keys == ["status"] - noop("Allow changes to the status only") - elsif changes["status"].present? && changes["status"].second == "open" - noop("Allow any changes when going to 'open'") - elsif changes["status"].present? && changes["status"].first == "open" - noop("Allow any changes when going from 'open' to another status") - else - errors.add_to_base(:cant_update_locked_deliverable) if locked? - errors.add_to_base(:cant_update_closed_deliverable) if closed? - end + def validate_status_changes + if changes.keys == ["status"] + noop("Allow changes to the status only") + elsif changes["status"].present? && changes["status"].second == "open" + noop("Allow any changes when going to 'open'") + elsif changes["status"].present? && changes["status"].first == "open" + noop("Allow any changes when going from 'open' to another status") + else + errors.add_to_base(:cant_update_locked_deliverable) if locked? + errors.add_to_base(:cant_update_closed_deliverable) if closed? end end From 2f79493ccad75f4d850771ad82743da15d75e45b Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 15:48:46 -0700 Subject: [PATCH 11/26] [#6441] Block logging time on locked contracts and deliverables --- config/locales/en.yml | 2 ++ .../patches/time_entry_patch.rb | 2 ++ .../patches/time_entry_patch_test.rb | 30 ++++++++++++++----- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 5c260b7..85aaa5e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -4,6 +4,8 @@ en: messages: cant_create_time_on_closed_deliverable: "Can't create a time entry on a closed deliverable" cant_create_time_on_closed_contract: "Can't create a time entry on a closed contract" + cant_create_time_on_locked_deliverable: "Can't create a time entry on a locked deliverable" + cant_create_time_on_locked_contract: "Can't create a time entry on a locked contract" cant_assign_to_closed_deliverable: "Can't assign issue to a closed deliverable" cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" cant_update_locked_deliverable: "Can't update a locked deliverable" diff --git a/lib/redmine_contracts/patches/time_entry_patch.rb b/lib/redmine_contracts/patches/time_entry_patch.rb index 0a327b2..b3ce8a7 100644 --- a/lib/redmine_contracts/patches/time_entry_patch.rb +++ b/lib/redmine_contracts/patches/time_entry_patch.rb @@ -13,12 +13,14 @@ module RedmineContracts def validate_deliverable_status if issue.present? && issue.deliverable.present? + errors.add_to_base(:cant_create_time_on_locked_deliverable) if issue.deliverable.locked? errors.add_to_base(:cant_create_time_on_closed_deliverable) if issue.deliverable.closed? end end def validate_contract_status if issue.present? && issue.deliverable.present? && issue.deliverable.contract.present? + errors.add_to_base(:cant_create_time_on_locked_contract) if issue.deliverable.contract.locked? errors.add_to_base(:cant_create_time_on_closed_contract) if issue.deliverable.contract.closed? end end diff --git a/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb b/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb index d844c15..52c3d57 100644 --- a/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb +++ b/test/unit/lib/redmine_contracts/patches/time_entry_patch_test.rb @@ -24,6 +24,14 @@ class RedmineContracts::Patches::TimeEntryTest < ActionController::TestCase :user => @user) end + def assert_error_about_locked_deliverable(time_entry) + assert_equal "Can't create a time entry on a locked deliverable", time_entry.errors.on_base + end + + def assert_error_about_locked_contract(time_entry) + assert_equal "Can't create a time entry on a locked contract", time_entry.errors.on_base + end + def assert_error_about_closed_deliverable(time_entry) assert_equal "Can't create a time entry on a closed deliverable", time_entry.errors.on_base end @@ -36,27 +44,31 @@ class RedmineContracts::Patches::TimeEntryTest < ActionController::TestCase assert_difference("TimeEntry.count") { create_time_entry } end - should "allow logging time to an issue on a locked deliverable, open contract" do + should "block logging time to an issue on a locked deliverable, open contract" do assert @deliverable.lock! assert @deliverable.locked? - assert_difference("TimeEntry.count") { create_time_entry } + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_locked_deliverable(@time_entry) end - should "allow logging time to an issue on an open deliverable, locked contract" do + should "block logging time to an issue on an open deliverable, locked contract" do assert @contract.lock! assert @contract.locked? - assert_difference("TimeEntry.count") { create_time_entry } + assert_no_difference("TimeEntry.count") { create_time_entry } + assert_error_about_locked_contract(@time_entry) end - should "allow logging time to an issue on a locked deliverable, locked contract" do + should "block logging time to an issue on a locked deliverable, locked contract" do assert @deliverable.lock! assert @deliverable.locked? assert @contract.lock! assert @contract.locked? - assert_difference("TimeEntry.count") { create_time_entry } + assert_no_difference("TimeEntry.count") { create_time_entry } + assert @time_entry.errors.on_base.include?("Can't create a time entry on a locked deliverable") + assert @time_entry.errors.on_base.include?("Can't create a time entry on a locked contract") end should "block logging time to an issue on a closed deliverable, open contract" do @@ -74,7 +86,8 @@ class RedmineContracts::Patches::TimeEntryTest < ActionController::TestCase assert @contract.locked? assert_no_difference("TimeEntry.count") { create_time_entry } - assert_error_about_closed_deliverable(@time_entry) + assert @time_entry.errors.on_base.include?("Can't create a time entry on a closed deliverable") + assert @time_entry.errors.on_base.include?("Can't create a time entry on a locked contract") end should "block logging time to an issue on an open deliverable, closed contract" do @@ -92,7 +105,8 @@ class RedmineContracts::Patches::TimeEntryTest < ActionController::TestCase assert @contract.closed? assert_no_difference("TimeEntry.count") { create_time_entry } - assert_error_about_closed_contract(@time_entry) + assert @time_entry.errors.on_base.include?("Can't create a time entry on a locked deliverable") + assert @time_entry.errors.on_base.include?("Can't create a time entry on a closed contract") end should "block logging time to an issue on a closed deliverable, closed contract" do From 7c23725bbd50e77503c2eb6b8fba9f48258d527d Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 15:59:24 -0700 Subject: [PATCH 12/26] [#6441] Refactor: merge i18n strings by passing in a reason and object --- config/locales/en.yml | 5 +---- lib/redmine_contracts/patches/time_entry_patch.rb | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 85aaa5e..ddaeb5e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,10 +2,7 @@ en: activerecord: errors: messages: - cant_create_time_on_closed_deliverable: "Can't create a time entry on a closed deliverable" - cant_create_time_on_closed_contract: "Can't create a time entry on a closed contract" - cant_create_time_on_locked_deliverable: "Can't create a time entry on a locked deliverable" - cant_create_time_on_locked_contract: "Can't create a time entry on a locked contract" + cant_create_time_on_object: "Can't create a time entry on a %{reason} %{thing}" cant_assign_to_closed_deliverable: "Can't assign issue to a closed deliverable" cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" cant_update_locked_deliverable: "Can't update a locked deliverable" diff --git a/lib/redmine_contracts/patches/time_entry_patch.rb b/lib/redmine_contracts/patches/time_entry_patch.rb index b3ce8a7..cca2541 100644 --- a/lib/redmine_contracts/patches/time_entry_patch.rb +++ b/lib/redmine_contracts/patches/time_entry_patch.rb @@ -13,15 +13,15 @@ module RedmineContracts def validate_deliverable_status if issue.present? && issue.deliverable.present? - errors.add_to_base(:cant_create_time_on_locked_deliverable) if issue.deliverable.locked? - errors.add_to_base(:cant_create_time_on_closed_deliverable) if issue.deliverable.closed? + errors.add_to_base("#{l(:"activerecord.errors.messages.cant_create_time_on_object", :reason => 'locked', :thing => 'deliverable')}") if issue.deliverable.locked? + errors.add_to_base("#{l(:"activerecord.errors.messages.cant_create_time_on_object", :reason => 'closed', :thing => 'deliverable')}") if issue.deliverable.closed? end end def validate_contract_status if issue.present? && issue.deliverable.present? && issue.deliverable.contract.present? - errors.add_to_base(:cant_create_time_on_locked_contract) if issue.deliverable.contract.locked? - errors.add_to_base(:cant_create_time_on_closed_contract) if issue.deliverable.contract.closed? + errors.add_to_base("#{l(:"activerecord.errors.messages.cant_create_time_on_object", :reason => 'locked', :thing => 'contract')}") if issue.deliverable.contract.locked? + errors.add_to_base("#{l(:"activerecord.errors.messages.cant_create_time_on_object", :reason => 'closed', :thing => 'contract')}") if issue.deliverable.contract.closed? end end From f8fe23a68d9e3cf7143fe474b4f318249eddab4b Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Tue, 9 Aug 2011 16:15:04 -0700 Subject: [PATCH 13/26] [#6441] Refactor: extract method in the complex validation --- app/models/deliverable.rb | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index ae47687..dd96315 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -80,17 +80,27 @@ class Deliverable < ActiveRecord::Base (new_record? || open?) end + def valid_status_change? + change_to_status_only? || changing_to_the_open_status? || changing_from_the_open_status? + end + + def change_to_status_only? + ["status"] == changes.keys + end + + def changing_to_the_open_status? + changes["status"].present? && "open" == changes["status"].second + end + + def changing_from_the_open_status? + changes["status"].present? && "open" == changes["status"].first + end + def validate_status_changes - if changes.keys == ["status"] - noop("Allow changes to the status only") - elsif changes["status"].present? && changes["status"].second == "open" - noop("Allow any changes when going to 'open'") - elsif changes["status"].present? && changes["status"].first == "open" - noop("Allow any changes when going from 'open' to another status") - else - errors.add_to_base(:cant_update_locked_deliverable) if locked? - errors.add_to_base(:cant_update_closed_deliverable) if closed? - end + return if valid_status_change? + + errors.add_to_base(:cant_update_locked_deliverable) if locked? + errors.add_to_base(:cant_update_closed_deliverable) if closed? end # No operation method, useful to clean up logic with an optional message From be37e00f1022ae964d3a1a66b4f704847f64366a Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 11:31:54 -0700 Subject: [PATCH 14/26] [#6441] Block editing locked and closed contracts --- app/models/contract.rb | 35 +++++++ app/models/deliverable.rb | 1 + app/views/contracts/_form.html.erb | 6 ++ config/locales/en.yml | 4 + test/integration/contracts_edit_test.rb | 130 +++++++++++++++++++++++- 5 files changed, 174 insertions(+), 2 deletions(-) diff --git a/app/models/contract.rb b/app/models/contract.rb index e734b7a..6ebf8a1 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -19,6 +19,7 @@ class Contract < ActiveRecord::Base validates_inclusion_of :discount_type, :in => %w($ %), :allow_blank => true, :allow_nil => true validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true validate :start_and_end_date_are_valid + validate_on_update :validate_status_changes # Accessors attr_accessible :name @@ -56,6 +57,10 @@ class Contract < ActiveRecord::Base update_attribute(:status, "closed") end + def open? + self.status == "open" + end + def locked? self.status == "locked" end @@ -261,6 +266,30 @@ class Contract < ActiveRecord::Base end end + def valid_status_change? + change_to_status_only? || changing_to_the_open_status? || changing_from_the_open_status? + end + + def change_to_status_only? + ["status"] == changes.keys + end + + def changing_to_the_open_status? + changes["status"].present? && "open" == changes["status"].second + end + + def changing_from_the_open_status? + changes["status"].present? && "open" == changes["status"].first + end + + # TODO: duplicated on Deliverable, refactor after one more duplication + def validate_status_changes + return if valid_status_change? + + errors.add_to_base(:cant_update_locked_contract) if locked? + errors.add_to_base(:cant_update_closed_contract) if closed? + end + # Currency amount of time that is logged to the project or to issues # that are not assigned to a Deliverable def orphaned_time @@ -279,6 +308,12 @@ class Contract < ActiveRecord::Base generator_for :executed => true generator_for(:start_date) { Date.yesterday } generator_for(:end_date) { Date.tomorrow } + generator_for :discount, '' + generator_for :details, '' + generator_for :discount_note, '' + generator_for :client_point_of_contact, '' + generator_for :client_ap_contact_information, '' + generator_for :po_number, '' def self.next_name @last_name ||= 'Contract 0000' diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index dd96315..e6a24ac 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -96,6 +96,7 @@ class Deliverable < ActiveRecord::Base changes["status"].present? && "open" == changes["status"].first end + # TODO: duplicated on Contract, refactor after one more duplication def validate_status_changes return if valid_status_change? diff --git a/app/views/contracts/_form.html.erb b/app/views/contracts/_form.html.erb index 8a762cc..103874a 100644 --- a/app/views/contracts/_form.html.erb +++ b/app/views/contracts/_form.html.erb @@ -1,3 +1,9 @@ +<% if resource.locked? || resource.closed? %> +
    +

    <%= resource.locked? ? l(:text_contract_locked_warning) : l(:text_contract_closed_warning) %>

    +
    +<% end %> +
    <% form.inputs :name => l(:text_general_legend) do %> <%= form.input :name, :required => true %> diff --git a/config/locales/en.yml b/config/locales/en.yml index ddaeb5e..5c5f407 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -7,6 +7,8 @@ en: cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" cant_update_locked_deliverable: "Can't update a locked deliverable" cant_update_closed_deliverable: "Can't update a closed deliverable" + cant_update_locked_contract: "Can't update a locked contract" + cant_update_closed_contract: "Can't update a closed contract" field_end_date: End Date field_executed: Executed @@ -92,3 +94,5 @@ en: field_estimated: Estimated text_deliverable_locked_warning: "This deliverable is locked and cannot be saved without changing it's status to Open." text_deliverable_closed_warning: "This deliverable is closed and cannot be saved without changing it's status to Open." + text_contract_locked_warning: "This contract is locked and cannot be saved without changing it's status to Open." + text_contract_closed_warning: "This contract is closed and cannot be saved without changing it's status to Open." diff --git a/test/integration/contracts_edit_test.rb b/test/integration/contracts_edit_test.rb index 5115067..d302cf4 100644 --- a/test/integration/contracts_edit_test.rb +++ b/test/integration/contracts_edit_test.rb @@ -48,13 +48,139 @@ class ContractsEditTest < ActionController::IntegrationTest end fill_in "Name", :with => 'An updated name' - select "Locked", :from => "Status" click_button "Save Contract" assert_response :success assert_template 'contracts/show' assert_equal "An updated name", @contract.reload.name - assert_equal "locked", @contract.reload.status + end + + context "locked contract" do + setup do + assert @contract.lock! + end + + should "block edits" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + fill_in "Name", :with => 'An updated name' + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/edit' + + assert_not_equal "An updated name", @contract.reload.name + end + + should "block edits even when the status is changed to closed" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + fill_in "Name", :with => 'An updated name' + select "Closed", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/edit' + + assert_not_equal "An updated name", @contract.reload.name + assert @contract.reload.locked? + end + + should "be allowed to change the status from locked to open" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + select "Open", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/show' + + assert @contract.reload.open? + end + + should "be allowed to change the status from locked to closed" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + select "Closed", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/show' + + assert @contract.reload.closed? + end + end + + context "closed contract" do + setup do + assert @contract.close! + end + + should "block edits" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + fill_in "Name", :with => 'An updated name' + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/edit' + + assert_not_equal "An updated name", @contract.reload.name + end + + should "block edits weven when the status is changed to locked" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + fill_in "Name", :with => 'An updated name' + select "Locked", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/edit' + + assert_not_equal "An updated name", @contract.reload.name + assert @contract.reload.closed? + end + + should "be allowed to change the status from closed to open" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + select "Open", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/show' + + assert @contract.reload.open? + end + + should "be allowed to change the status from closed to locked" do + visit_contract_page(@contract) + click_link 'Update' + assert_response :success + + select "Locked", :from => "Status" + click_button 'Save Contract' + + assert_response :success + assert_template 'contracts/show' + + assert @contract.reload.locked? + end end end From 495407f12790b8eeeb9cb856f727c840fb1f1a21 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 11:56:08 -0700 Subject: [PATCH 15/26] [#6441] Block editing deliverables on locked and closed contracts --- app/models/deliverable.rb | 12 ++++ app/views/deliverables/_form.html.erb | 7 +- test/integration/deliverables_edit_test.rb | 80 ++++++++++++++++++++++ 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index e6a24ac..2e785ba 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -21,12 +21,16 @@ class Deliverable < ActiveRecord::Base validates_presence_of :manager validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true validate_on_update :validate_status_changes + validate_on_update :validate_contract_status # Accessors include DollarizedAttribute dollarized_attribute :total delegate :name, :to => :contract, :prefix => true, :allow_nil => true + delegate "open?", :to => :contract, :prefix => true, :allow_nil => true + delegate "closed?", :to => :contract, :prefix => true, :allow_nil => true + delegate "locked?", :to => :contract, :prefix => true, :allow_nil => true # Callbacks def after_initialize @@ -104,6 +108,14 @@ class Deliverable < ActiveRecord::Base errors.add_to_base(:cant_update_closed_deliverable) if closed? end + def validate_contract_status + return if contract_open? + return if change_to_status_only? + + errors.add_to_base(:cant_update_locked_contract) if contract_locked? + errors.add_to_base(:cant_update_closed_contract) if contract_closed? + end + # No operation method, useful to clean up logic with an optional message # for documentation def noop(message="") diff --git a/app/views/deliverables/_form.html.erb b/app/views/deliverables/_form.html.erb index 54ddded..fdd4357 100644 --- a/app/views/deliverables/_form.html.erb +++ b/app/views/deliverables/_form.html.erb @@ -2,9 +2,14 @@ <%= javascript_tag("var i18nEndDateEmpty = '#{l(:text_end_date_empty)}'") %> <%= javascript_tag("var i18nChangedPeriodMessage = '#{l(:text_changed_period_message)}'") %> -<% if resource.locked? || resource.closed? %> +<% if resource.locked? || resource.closed? || resource.contract_locked? || resource.contract_closed? %>
    + <% if resource.contract_locked? || resource.contract_closed? %> +

    <%= resource.contract_locked? ? l(:text_contract_locked_warning) : l(:text_contract_closed_warning) %>

    + <% end %> + <% if resource.locked? || resource.closed? %>

    <%= resource.locked? ? l(:text_deliverable_locked_warning) : l(:text_deliverable_closed_warning) %>

    + <% end %>
    <% end %> diff --git a/test/integration/deliverables_edit_test.rb b/test/integration/deliverables_edit_test.rb index 72c71f6..8d1b569 100644 --- a/test/integration/deliverables_edit_test.rb +++ b/test/integration/deliverables_edit_test.rb @@ -629,6 +629,86 @@ class DeliverablesEditTest < ActionController::IntegrationTest end end + context "a Deliverable on a locked Contract" do + setup do + assert @contract.lock! + end + + should "be blocked from editing" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + end + + should "allow status only changes" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Locked", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.locked? + end + + end + + context "a Deliverable on a closed Contract" do + setup do + assert @contract.close! + end + + should "be blocked from editing" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + fill_in "Title", :with => 'An updated title' + end + + click_button "Save" + + assert_response :success + assert_template 'deliverables/edit' + + assert_not_equal "An updated title", @fixed_deliverable.reload.title + end + + should "allow status only changes" do + visit_contract_page(@contract) + click_link_within "#deliverable_details_#{@fixed_deliverable.id}", 'Edit' + assert_response :success + + within("#deliverable-details") do + select "Locked", :from => "Status" + end + + click_button "Save" + + assert_response :success + assert_template 'contracts/show' + + assert @fixed_deliverable.reload.locked? + end + + end end From f70ade5011e413405df60d1172c48aeb66f2861f Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 12:09:09 -0700 Subject: [PATCH 16/26] [#6441] Block assigning issues to deliverable on a locked or closed contract --- config/locales/en.yml | 2 + lib/redmine_contracts/patches/issue_patch.rb | 8 +++ .../controller_issues_edit_before_save.rb | 56 +++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index 5c5f407..f7e8c3a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -5,6 +5,8 @@ en: cant_create_time_on_object: "Can't create a time entry on a %{reason} %{thing}" cant_assign_to_closed_deliverable: "Can't assign issue to a closed deliverable" cant_assign_to_locked_deliverable: "Can't assign issue to a locked deliverable" + cant_assign_to_closed_contract: "Can't assign issue to a closed contract" + cant_assign_to_locked_contract: "Can't assign issue to a locked contract" cant_update_locked_deliverable: "Can't update a locked deliverable" cant_update_closed_deliverable: "Can't update a closed deliverable" cant_update_locked_contract: "Can't update a locked contract" diff --git a/lib/redmine_contracts/patches/issue_patch.rb b/lib/redmine_contracts/patches/issue_patch.rb index ba9100d..38f4a45 100644 --- a/lib/redmine_contracts/patches/issue_patch.rb +++ b/lib/redmine_contracts/patches/issue_patch.rb @@ -17,6 +17,7 @@ module RedmineContracts end validate :validate_deliverable_status + validate :validate_contract_status def validate_deliverable_status if deliverable.present? && changes["deliverable_id"].present? @@ -25,6 +26,13 @@ module RedmineContracts end end + def validate_contract_status + if deliverable.present? && changes["deliverable_id"].present? + errors.add_to_base(:cant_assign_to_closed_contract) if contract.closed? + errors.add_to_base(:cant_assign_to_locked_contract) if contract.locked? + end + end + end end diff --git a/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb b/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb index 6810379..e865245 100644 --- a/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb +++ b/test/integration/redmine_contracts/hooks/controller_issues_edit_before_save.rb @@ -70,6 +70,38 @@ class RedmineContracts::Hooks::ControllerIssuesEditBeforeSaveTest < ActionContro end + should "not allow setting a Deliverable on a locked Contract" do + assert @contract2.lock! + click_link "New issue" + + fill_in "Subject", :with => 'Hook test' + select @deliverable2.title, :from => "Deliverable" + assert_no_difference("Issue.count") do + click_button "Create" + + assert_response :success + end + + assert_equal nil, Issue.last.deliverable + + end + + should "not allow setting a Deliverable on a closed Contract" do + assert @contract2.close! + click_link "New issue" + + fill_in "Subject", :with => 'Hook test' + select @deliverable2.title, :from => "Deliverable" + assert_no_difference("Issue.count") do + click_button "Create" + + assert_response :success + end + + assert_equal nil, Issue.last.deliverable + + end + context "with no permission to Assign Deliverable" do should "not allow setting the Deliverable (force HTTP request)" do @role.permissions.delete(:assign_deliverable_to_issue) @@ -125,6 +157,30 @@ class RedmineContracts::Hooks::ControllerIssuesEditBeforeSaveTest < ActionContro end + should "not allow updating to a deliverable on a locked contract" do + assert @contract2.lock! + select @deliverable2.title, :from => "Deliverable" + click_button "Submit" + + assert_response :success + + @issue.reload + assert_equal nil, @issue.deliverable + + end + + should "not allow updating to a deliverable on a closed contract" do + assert @contract2.close! + select @deliverable2.title, :from => "Deliverable" + click_button "Submit" + + assert_response :success + + @issue.reload + assert_equal nil, @issue.deliverable + + end + should "allow updating an issue, even if the deliverable is locked as long as the deliverable isn't changed" do select @deliverable2.title, :from => "Deliverable" click_button "Submit" From e019a9435eab06a42faeb51896b000069fe181e8 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 13:10:05 -0700 Subject: [PATCH 17/26] [#6441] Hide and disable unassignable deliverables from the issue * Hide - Closed deliverables * Hide - Closed contract deliverables * Disable - Locked deliverables * Disable - Locked contract deliverables --- app/helpers/contracts_helper.rb | 15 ++++++++ app/models/contract.rb | 15 ++++---- app/models/deliverable.rb | 5 +++ .../issues/_bulk_edit_deliverable.html.erb | 6 +--- app/views/issues/_edit_deliverable.html.erb | 2 +- lib/redmine_contracts/patches/issue_patch.rb | 2 +- ...sues_bulk_edit_details_bottom_hook_test.rb | 36 +++++++++++++++++++ ...ew_issues_form_details_bottom_hook_test.rb | 36 +++++++++++++++++++ 8 files changed, 103 insertions(+), 14 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 961ce05..c45e5e4 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -24,6 +24,21 @@ module ContractsHelper end end + def grouped_deliverable_options_for_select(project, selected_key=nil) + project.contracts.with_status(["open","locked"]).inject([]) do |html, contract| + options = contract.deliverables.with_status(["open","locked"]).collect do |deliverable| + option_attributes = {} + option_attributes[:value] = h(deliverable.id) + option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id + option_attributes[:disabled] = "disabled" if deliverable.locked? || contract.locked? + + content_tag(:option, h(deliverable.title), option_attributes) + end + + html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) + end.join('\n') + end + def deliverable_options_for_contract(contract) contract.deliverables.collect {|d| [d.title, d.id]} end diff --git a/app/models/contract.rb b/app/models/contract.rb index 6ebf8a1..dabad9c 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -38,7 +38,12 @@ class Contract < ActiveRecord::Base attr_accessible :status named_scope :by_name, {:order => "#{Contract.table_name}.name ASC"} - + named_scope :with_status, lambda {|statuses| + { + :conditions => ["#{Contract.table_name}.status IN (?)", statuses] + } + } + [:contract_type, :discount_spent, :discount_budget ].each do |mthd| @@ -304,7 +309,7 @@ class Contract < ActiveRecord::Base end if Rails.env.test? - generator_for :name, :method => :next_name + generator_for :name, :start => "Contract 0000" generator_for :executed => true generator_for(:start_date) { Date.yesterday } generator_for(:end_date) { Date.tomorrow } @@ -314,11 +319,7 @@ class Contract < ActiveRecord::Base generator_for :client_point_of_contact, '' generator_for :client_ap_contact_information, '' generator_for :po_number, '' - - def self.next_name - @last_name ||= 'Contract 0000' - @last_name.succ! - end + generator_for :status, 'open' end diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 2e785ba..459b7fe 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -46,6 +46,11 @@ class Deliverable < ActiveRecord::Base end named_scope :by_title, {:order => "#{Deliverable.table_name}.title ASC"} + named_scope :with_status, lambda {|statuses| + { + :conditions => ["#{Deliverable.table_name}.status IN (?)", statuses] + } + } def short_type '' diff --git a/app/views/issues/_bulk_edit_deliverable.html.erb b/app/views/issues/_bulk_edit_deliverable.html.erb index f4a0e97..26ed093 100644 --- a/app/views/issues/_bulk_edit_deliverable.html.erb +++ b/app/views/issues/_bulk_edit_deliverable.html.erb @@ -1,14 +1,10 @@ <% if project.module_enabled?(:contracts) && User.current.allowed_to?(:assign_deliverable_to_issue, project) %>

    <%= label_tag(:deliverable_id, l(:field_deliverable)) %> - <% options = project.contracts.inject([]) {|data, contract| - data << [contract.name, contract.deliverables.collect {|d| [d.title, d.id]} ] - } %> - <%= select_tag('deliverable_id', content_tag('option', l(:label_no_change_option), :value => '') + content_tag('option', l(:label_none), :value => 'none') + - grouped_options_for_select(options)) %> + grouped_deliverable_options_for_select(project)) %>

    <% end %> diff --git a/app/views/issues/_edit_deliverable.html.erb b/app/views/issues/_edit_deliverable.html.erb index 9daab6f..c07a1a7 100644 --- a/app/views/issues/_edit_deliverable.html.erb +++ b/app/views/issues/_edit_deliverable.html.erb @@ -1,6 +1,6 @@ <% if project.module_enabled?(:contracts) && User.current.allowed_to?(:assign_deliverable_to_issue, project) %>

    - <%= form.select(:deliverable_id, grouped_options_for_select(deliverable_options(project), issue.deliverable_id), {:include_blank => true}) %> + <%= form.select(:deliverable_id, grouped_deliverable_options_for_select(project, issue.deliverable_id), {:include_blank => true}) %>

    <% end %> diff --git a/lib/redmine_contracts/patches/issue_patch.rb b/lib/redmine_contracts/patches/issue_patch.rb index 38f4a45..f1157ad 100644 --- a/lib/redmine_contracts/patches/issue_patch.rb +++ b/lib/redmine_contracts/patches/issue_patch.rb @@ -27,7 +27,7 @@ module RedmineContracts end def validate_contract_status - if deliverable.present? && changes["deliverable_id"].present? + if deliverable.present? && changes["deliverable_id"].present? && contract.present? errors.add_to_base(:cant_assign_to_closed_contract) if contract.closed? errors.add_to_base(:cant_assign_to_locked_contract) if contract.locked? end diff --git a/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb b/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb index fd28f57..3c0d6a8 100644 --- a/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb @@ -11,12 +11,19 @@ class RedmineContracts::Hooks::ViewIssuesBulkEditDetailsBottomHookTest < ActionC @issue3 = Issue.generate_for_project!(@project) @contract1 = Contract.generate!(:project => @project) @contract2 = Contract.generate!(:project => @project) + @locked_contract = Contract.generate!(:project => @project, :status => 'locked') + @closed_contract = Contract.generate!(:project => @project, :status => 'closed') @manager = User.generate!(:login => 'manager', :password => 'existing', :password_confirmation => 'existing') @role = Role.generate!(:permissions => [:view_issues, :edit_issues]) User.add_to_project(@manager, @project, @role) @deliverable1 = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'The Title') @deliverable2 = FixedDeliverable.generate!(:contract => @contract2, :manager => @manager, :title => 'The Title') + @locked_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Locked Deliverable', :status => 'locked') + @closed_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Closed Deliverable', :status => 'closed') + @deliverable1_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 1 on locked contract') + @deliverable2_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 2 on locked contract') + @deliverable_on_closed_contract = FixedDeliverable.generate!(:contract => @closed_contract, :manager => @manager, :title => 'Deliverable on closed contract') @issue.deliverable = @deliverable1 login_as('manager', 'existing') @@ -42,6 +49,35 @@ class RedmineContracts::Hooks::ViewIssuesBulkEditDetailsBottomHookTest < ActionC end end end + + should "disable all locked deliverables" do + assert_select "select#deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@locked_deliverable.title}/ + end + end + + should "disable all deliverables on locked contracts" do + assert_select "select#deliverable_id" do + assert_select "optgroup[label=?]", @locked_contract.name do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1_on_locked_contract.title}/ + assert_select "option[disabled=disabled]", :text => /#{@deliverable2_on_locked_contract.title}/ + end + end + end + + should "not show closed deliverables" do + assert_select "select#deliverable_id" do + assert_select "option", :text => /#{@closed_deliverable.title}/, :count => 0 + end + end + + should "not show deliverables on closed contracts" do + assert_select "select#deliverable_id" do + assert_select "optgroup[label=?]", @closed_contract.name, :count => 0 + assert_select "option", :text => /#{@deliverable_on_closed_contract.title}/, :count => 0 + end + end + end context "with no permission to Assign Deliverable" do diff --git a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb index e91b926..0c9292e 100644 --- a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb @@ -9,12 +9,19 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle @issue = Issue.generate_for_project!(@project) @contract1 = Contract.generate!(:project => @project) @contract2 = Contract.generate!(:project => @project) + @locked_contract = Contract.generate!(:project => @project, :status => 'locked') + @closed_contract = Contract.generate!(:project => @project, :status => 'closed') @manager = User.generate!(:login => 'manager', :password => 'existing', :password_confirmation => 'existing') @role = Role.generate!(:permissions => [:view_issues, :edit_issues]) User.add_to_project(@manager, @project, @role) @deliverable1 = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'The Title') @deliverable2 = FixedDeliverable.generate!(:contract => @contract2, :manager => @manager, :title => 'The Title') + @locked_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Locked Deliverable', :status => 'locked') + @closed_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Closed Deliverable', :status => 'closed') + @deliverable1_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 1 on locked contract') + @deliverable2_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 2 on locked contract') + @deliverable_on_closed_contract = FixedDeliverable.generate!(:contract => @closed_contract, :manager => @manager, :title => 'Deliverable on closed contract') @issue.deliverable = @deliverable1 login_as('manager', 'existing') @@ -39,6 +46,35 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle end end end + + should "disable all locked deliverables" do + assert_select "select#issue_deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@locked_deliverable.title}/ + end + end + + should "disable all deliverables on locked contracts" do + assert_select "select#issue_deliverable_id" do + assert_select "optgroup[label=?]", @locked_contract.name do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1_on_locked_contract.title}/ + assert_select "option[disabled=disabled]", :text => /#{@deliverable2_on_locked_contract.title}/ + end + end + end + + should "not show closed deliverables" do + assert_select "select#issue_deliverable_id" do + assert_select "option", :text => /#{@closed_deliverable.title}/, :count => 0 + end + end + + should "not show deliverables on closed contracts" do + assert_select "select#issue_deliverable_id" do + assert_select "optgroup[label=?]", @closed_contract.name, :count => 0 + assert_select "option", :text => /#{@deliverable_on_closed_contract.title}/, :count => 0 + end + end + end context "with no permission to Assign Deliverable" do From 1b4191dc15be5d4829fbbbe4205c5586a1c87dbd Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:07:44 -0700 Subject: [PATCH 18/26] [#6441] Don't hide or disable the issue's current deliverable --- app/helpers/contracts_helper.rb | 27 ++++++---- app/models/deliverable.rb | 3 +- ...ew_issues_form_details_bottom_hook_test.rb | 50 +++++++++++++++++-- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index c45e5e4..643e451 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -25,17 +25,24 @@ module ContractsHelper end def grouped_deliverable_options_for_select(project, selected_key=nil) - project.contracts.with_status(["open","locked"]).inject([]) do |html, contract| - options = contract.deliverables.with_status(["open","locked"]).collect do |deliverable| - option_attributes = {} - option_attributes[:value] = h(deliverable.id) - option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id - option_attributes[:disabled] = "disabled" if deliverable.locked? || contract.locked? - - content_tag(:option, h(deliverable.title), option_attributes) - end + project.contracts.all(:include => :deliverables).inject([]) do |html, contract| + if contract.closed? && !contract.deliverable_ids.include?(selected_key.to_i) + # skip + else + options = contract.deliverables.collect do |deliverable| + option_attributes = {} + option_attributes[:value] = h(deliverable.id) + option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id + option_attributes[:disabled] = "disabled" if (deliverable.locked? || contract.locked?) && selected_key.to_i != deliverable.id - html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) + next if deliverable.closed? && option_attributes[:selected].blank? # Skip unselected, closed + + content_tag(:option, h(deliverable.title), option_attributes) + end + + html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) + end + html end.join('\n') end diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 459b7fe..af6ce39 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -289,7 +289,8 @@ class Deliverable < ActiveRecord::Base if Rails.env.test? generator_for :title, :method => :next_title - + generator_for :status, 'open' + def self.next_title @last_title ||= 'Deliverable 0000' @last_title.succ! diff --git a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb index 0c9292e..4f54296 100644 --- a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb @@ -15,14 +15,15 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle @manager = User.generate!(:login => 'manager', :password => 'existing', :password_confirmation => 'existing') @role = Role.generate!(:permissions => [:view_issues, :edit_issues]) User.add_to_project(@manager, @project, @role) - @deliverable1 = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'The Title') - @deliverable2 = FixedDeliverable.generate!(:contract => @contract2, :manager => @manager, :title => 'The Title') + @deliverable1 = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Deliverable1') + @deliverable2 = FixedDeliverable.generate!(:contract => @contract2, :manager => @manager, :title => 'Deliverable2') @locked_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Locked Deliverable', :status => 'locked') @closed_deliverable = FixedDeliverable.generate!(:contract => @contract1, :manager => @manager, :title => 'Closed Deliverable', :status => 'closed') @deliverable1_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 1 on locked contract') @deliverable2_on_locked_contract = FixedDeliverable.generate!(:contract => @locked_contract, :manager => @manager, :title => 'Deliverable 2 on locked contract') @deliverable_on_closed_contract = FixedDeliverable.generate!(:contract => @closed_contract, :manager => @manager, :title => 'Deliverable on closed contract') @issue.deliverable = @deliverable1 + assert @issue.save login_as('manager', 'existing') end @@ -74,7 +75,50 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle assert_select "option", :text => /#{@deliverable_on_closed_contract.title}/, :count => 0 end end - + + should "show the assigned deliverable as an option, even if it's locked" do + @deliverable1.lock! + visit_issue_page(@issue) + + assert_select "select#issue_deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1.title}/, :count => 0 # Not disabled + assert_select "option", :text => /#{@deliverable1.title}/, :count => 1 # Present + end + + end + + should "show the assigned deliverable as an option, even if it's closed" do + @deliverable1.close! + visit_issue_page(@issue) + + assert_select "select#issue_deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1.title}/, :count => 0 # Not disabled + assert_select "option", :text => /#{@deliverable1.title}/, :count => 1 # Present + end + + end + + should "show the assigned deliverable as an option, even if it's contract is locked" do + @contract1.lock! + visit_issue_page(@issue) + + assert_select "select#issue_deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1.title}/, :count => 0 # Not disabled + assert_select "option", :text => /#{@deliverable1.title}/, :count => 1 # Present + end + + end + + should "show the assigned deliverable as an option, even if it's contract is closed" do + @contract1.close! + visit_issue_page(@issue) + + assert_select "select#issue_deliverable_id" do + assert_select "option[disabled=disabled]", :text => /#{@deliverable1.title}/, :count => 0 # Not disabled + assert_select "option", :text => /#{@deliverable1.title}/, :count => 1 # Present + end + + end end context "with no permission to Assign Deliverable" do From 6357fd83e4704cd9b4e6924dffe20445235c8096 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:11:45 -0700 Subject: [PATCH 19/26] [#6441] Refactor: extract method --- app/helpers/contracts_helper.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 643e451..bf6b8d4 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -30,14 +30,7 @@ module ContractsHelper # skip else options = contract.deliverables.collect do |deliverable| - option_attributes = {} - option_attributes[:value] = h(deliverable.id) - option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id - option_attributes[:disabled] = "disabled" if (deliverable.locked? || contract.locked?) && selected_key.to_i != deliverable.id - - next if deliverable.closed? && option_attributes[:selected].blank? # Skip unselected, closed - - content_tag(:option, h(deliverable.title), option_attributes) + deliverable_option(deliverable, contract, selected_key) end html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) @@ -46,6 +39,17 @@ module ContractsHelper end.join('\n') end + def deliverable_option(deliverable, contract, selected_key) + option_attributes = {} + option_attributes[:value] = h(deliverable.id) + option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id + option_attributes[:disabled] = "disabled" if (deliverable.locked? || contract.locked?) && selected_key.to_i != deliverable.id + + return "" if deliverable.closed? && option_attributes[:selected].blank? # Skip unselected, closed + + content_tag(:option, h(deliverable.title), option_attributes) + end + def deliverable_options_for_contract(contract) contract.deliverables.collect {|d| [d.title, d.id]} end From 2f77619fefe5520ad6232c39babba8a05f4cfce0 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:13:01 -0700 Subject: [PATCH 20/26] [#6441] Refactor: remove unneeded contract arg --- app/helpers/contracts_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index bf6b8d4..1991b94 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -30,7 +30,7 @@ module ContractsHelper # skip else options = contract.deliverables.collect do |deliverable| - deliverable_option(deliverable, contract, selected_key) + deliverable_option(deliverable, selected_key) end html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) @@ -39,11 +39,11 @@ module ContractsHelper end.join('\n') end - def deliverable_option(deliverable, contract, selected_key) + def deliverable_option(deliverable, selected_key) option_attributes = {} option_attributes[:value] = h(deliverable.id) option_attributes[:selected] = "selected" if selected_key.to_i == deliverable.id - option_attributes[:disabled] = "disabled" if (deliverable.locked? || contract.locked?) && selected_key.to_i != deliverable.id + option_attributes[:disabled] = "disabled" if (deliverable.locked? || deliverable.contract_locked?) && selected_key.to_i != deliverable.id return "" if deliverable.closed? && option_attributes[:selected].blank? # Skip unselected, closed From edc9e84719e2b240d695fe2bc46c60db47a45c49 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:15:26 -0700 Subject: [PATCH 21/26] [#6441] Refactor: extract and move method --- app/helpers/contracts_helper.rb | 2 +- app/models/contract.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 1991b94..41e2796 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -26,7 +26,7 @@ module ContractsHelper def grouped_deliverable_options_for_select(project, selected_key=nil) project.contracts.all(:include => :deliverables).inject([]) do |html, contract| - if contract.closed? && !contract.deliverable_ids.include?(selected_key.to_i) + if contract.closed? && !contract.includes_deliverable_id?(selected_key) # skip else options = contract.deliverables.collect do |deliverable| diff --git a/app/models/contract.rb b/app/models/contract.rb index dabad9c..aa52d8a 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -74,6 +74,10 @@ class Contract < ActiveRecord::Base self.status == "closed" end + def includes_deliverable_id?(deliverable_id) + deliverable_ids.include?(deliverable_id.to_i) + end + # ------------------------------------------------------------ # Labor Methods # ------------------------------------------------------------ From b99f57f51cbe8bcb91417ad3719ef2dc15ae406c Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:16:54 -0700 Subject: [PATCH 22/26] [#6441] Use a string so the flow is clearer --- app/helpers/contracts_helper.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 41e2796..2231d2d 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -25,9 +25,9 @@ module ContractsHelper end def grouped_deliverable_options_for_select(project, selected_key=nil) - project.contracts.all(:include => :deliverables).inject([]) do |html, contract| + project.contracts.all(:include => :deliverables).inject("") do |html, contract| if contract.closed? && !contract.includes_deliverable_id?(selected_key) - # skip + html else options = contract.deliverables.collect do |deliverable| deliverable_option(deliverable, selected_key) @@ -35,8 +35,7 @@ module ContractsHelper html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) end - html - end.join('\n') + end end def deliverable_option(deliverable, selected_key) From e0ecae822041e256309c29ebb4baf726ae06cf15 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:18:37 -0700 Subject: [PATCH 23/26] [#6441] Remove dead code --- app/helpers/contracts_helper.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 2231d2d..4af0e28 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -18,12 +18,6 @@ module ContractsHelper grouped_contracts end - def deliverable_options(project) - project.contracts.inject([]) do |data, contract| - data << [contract.name, deliverable_options_for_contract(contract)] - end - end - def grouped_deliverable_options_for_select(project, selected_key=nil) project.contracts.all(:include => :deliverables).inject("") do |html, contract| if contract.closed? && !contract.includes_deliverable_id?(selected_key) @@ -48,10 +42,6 @@ module ContractsHelper content_tag(:option, h(deliverable.title), option_attributes) end - - def deliverable_options_for_contract(contract) - contract.deliverables.collect {|d| [d.title, d.id]} - end # Simple helper to show the values of a field on an object in a standard format # From 256325a39d7b850b40c927401175718db733102f Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:21:45 -0700 Subject: [PATCH 24/26] [#6441] Refactor: extract method --- app/helpers/contracts_helper.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/helpers/contracts_helper.rb b/app/helpers/contracts_helper.rb index 4af0e28..b511a0a 100644 --- a/app/helpers/contracts_helper.rb +++ b/app/helpers/contracts_helper.rb @@ -23,15 +23,19 @@ module ContractsHelper if contract.closed? && !contract.includes_deliverable_id?(selected_key) html else - options = contract.deliverables.collect do |deliverable| - deliverable_option(deliverable, selected_key) - end - - html << content_tag(:optgroup, options.join("\n"), :label => h(contract.name)) + html << content_tag(:optgroup, + deliverable_options_for_contract(contract, selected_key).join("\n"), + :label => h(contract.name)) end end end + def deliverable_options_for_contract(contract, selected_key) + contract.deliverables.collect do |deliverable| + deliverable_option(deliverable, selected_key) + end + end + def deliverable_option(deliverable, selected_key) option_attributes = {} option_attributes[:value] = h(deliverable.id) From bf1339fc61ac1e005cd7c2a79dec9b391b4f030c Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 15:53:36 -0700 Subject: [PATCH 25/26] [#6441] Block creating deliverables on closed or locked contracts --- app/models/deliverable.rb | 19 ++++++++++++++++--- app/views/contracts/show.html.erb | 2 +- config/locales/en.yml | 2 ++ ...sues_bulk_edit_details_bottom_hook_test.rb | 8 ++++++-- ...ew_issues_form_details_bottom_hook_test.rb | 8 ++++++-- test/unit/deliverable_test.rb | 19 +++++++++++++++++++ 6 files changed, 50 insertions(+), 8 deletions(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index af6ce39..1a7ddef 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -21,7 +21,7 @@ class Deliverable < ActiveRecord::Base validates_presence_of :manager validates_inclusion_of :status, :in => ["open","locked","closed"], :allow_blank => true, :allow_nil => true validate_on_update :validate_status_changes - validate_on_update :validate_contract_status + validate :validate_contract_status # Accessors include DollarizedAttribute @@ -117,8 +117,21 @@ class Deliverable < ActiveRecord::Base return if contract_open? return if change_to_status_only? - errors.add_to_base(:cant_update_locked_contract) if contract_locked? - errors.add_to_base(:cant_update_closed_contract) if contract_closed? + if contract_locked? + if new_record? + errors.add_to_base(:cant_create_deliverable_on_locked_contract) + else + errors.add_to_base(:cant_update_locked_contract) + end + end + + if contract_closed? + if new_record? + errors.add_to_base(:cant_create_deliverable_on_closed_contract) + else + errors.add_to_base(:cant_update_closed_contract) + end + end end # No operation method, useful to clean up logic with an optional message diff --git a/app/views/contracts/show.html.erb b/app/views/contracts/show.html.erb index aff2cda..79e0e81 100644 --- a/app/views/contracts/show.html.erb +++ b/app/views/contracts/show.html.erb @@ -102,7 +102,7 @@
    <%= release(2, "CSV") %> <%= release(5, "View All/Pagination") %> - <%= link_to(l(:button_add_new), new_contract_deliverable_path(@project, resource), :id => 'new-deliverable') %> + <%= link_to(l(:button_add_new), new_contract_deliverable_path(@project, resource), :id => 'new-deliverable') if resource.open? %>
    diff --git a/config/locales/en.yml b/config/locales/en.yml index f7e8c3a..acb3852 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -11,6 +11,8 @@ en: cant_update_closed_deliverable: "Can't update a closed deliverable" cant_update_locked_contract: "Can't update a locked contract" cant_update_closed_contract: "Can't update a closed contract" + cant_create_deliverable_on_locked_contract: "Can't create a deliverable on a locked contract" + cant_create_deliverable_on_closed_contract: "Can't create a deliverable on a closed contract" field_end_date: End Date field_executed: Executed diff --git a/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb b/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb index 3c0d6a8..f33b8df 100644 --- a/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/view_issues_bulk_edit_details_bottom_hook_test.rb @@ -11,8 +11,8 @@ class RedmineContracts::Hooks::ViewIssuesBulkEditDetailsBottomHookTest < ActionC @issue3 = Issue.generate_for_project!(@project) @contract1 = Contract.generate!(:project => @project) @contract2 = Contract.generate!(:project => @project) - @locked_contract = Contract.generate!(:project => @project, :status => 'locked') - @closed_contract = Contract.generate!(:project => @project, :status => 'closed') + @locked_contract = Contract.generate!(:project => @project) + @closed_contract = Contract.generate!(:project => @project) @manager = User.generate!(:login => 'manager', :password => 'existing', :password_confirmation => 'existing') @role = Role.generate!(:permissions => [:view_issues, :edit_issues]) @@ -26,6 +26,10 @@ class RedmineContracts::Hooks::ViewIssuesBulkEditDetailsBottomHookTest < ActionC @deliverable_on_closed_contract = FixedDeliverable.generate!(:contract => @closed_contract, :manager => @manager, :title => 'Deliverable on closed contract') @issue.deliverable = @deliverable1 + # Set contract statuses now that all deliverables are created + assert @locked_contract.lock! + assert @closed_contract.close! + login_as('manager', 'existing') end diff --git a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb index 4f54296..b2995e8 100644 --- a/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb +++ b/test/integration/redmine_contracts/hooks/view_issues_form_details_bottom_hook_test.rb @@ -9,8 +9,8 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle @issue = Issue.generate_for_project!(@project) @contract1 = Contract.generate!(:project => @project) @contract2 = Contract.generate!(:project => @project) - @locked_contract = Contract.generate!(:project => @project, :status => 'locked') - @closed_contract = Contract.generate!(:project => @project, :status => 'closed') + @locked_contract = Contract.generate!(:project => @project) + @closed_contract = Contract.generate!(:project => @project) @manager = User.generate!(:login => 'manager', :password => 'existing', :password_confirmation => 'existing') @role = Role.generate!(:permissions => [:view_issues, :edit_issues]) @@ -25,6 +25,10 @@ class RedmineContracts::Hooks::ViewIssuesFormDetailsBottomTest < ActionControlle @issue.deliverable = @deliverable1 assert @issue.save + # Set contract statuses now that all deliverables are created + assert @locked_contract.lock! + assert @closed_contract.close! + login_as('manager', 'existing') end diff --git a/test/unit/deliverable_test.rb b/test/unit/deliverable_test.rb index 4cd7965..08b2d75 100644 --- a/test/unit/deliverable_test.rb +++ b/test/unit/deliverable_test.rb @@ -42,4 +42,23 @@ class DeliverableTest < ActiveSupport::TestCase end end + context "with a locked contract" do + should "block creating a new deliverable" do + contract = Contract.generate!(:status => "locked") + deliverable = FixedDeliverable.spawn(:contract => contract) + + assert !deliverable.valid? + assert deliverable.errors.on_base.include?("Can't create a deliverable on a locked contract") + end + end + + context "with a closed contract" do + should "block creating a new deliverable" do + contract = Contract.generate!(:status => "closed") + deliverable = FixedDeliverable.spawn(:contract => contract) + + assert !deliverable.valid? + assert deliverable.errors.on_base.include?("Can't create a deliverable on a closed contract") + end + end end From 5e50eb7558502db4d8bd1182f6a262c1bcd9bc0c Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Wed, 10 Aug 2011 16:07:07 -0700 Subject: [PATCH 26/26] [#6441] Block deleting deliverables on closed or locked contracts --- app/models/deliverable.rb | 11 ++++++++++ app/views/deliverables/_details_row.html.erb | 2 +- test/unit/deliverable_test.rb | 22 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/models/deliverable.rb b/app/models/deliverable.rb index 1a7ddef..78a395d 100644 --- a/app/models/deliverable.rb +++ b/app/models/deliverable.rb @@ -33,6 +33,9 @@ class Deliverable < ActiveRecord::Base delegate "locked?", :to => :contract, :prefix => true, :allow_nil => true # Callbacks + before_destroy :block_on_locked_contracts + before_destroy :block_on_closed_contracts + def after_initialize self.status = "open" unless self.status.present? end @@ -139,6 +142,14 @@ class Deliverable < ActiveRecord::Base def noop(message="") end + def block_on_locked_contracts + !contract_locked? + end + + def block_on_closed_contracts + !contract_closed? + end + def to_s title end diff --git a/app/views/deliverables/_details_row.html.erb b/app/views/deliverables/_details_row.html.erb index 10bcc4c..c301f60 100644 --- a/app/views/deliverables/_details_row.html.erb +++ b/app/views/deliverables/_details_row.html.erb @@ -7,7 +7,7 @@
    <%= link_to(l(:button_edit), edit_contract_deliverable_path(@project, contract, deliverable), :class => 'icon icon-edit') %> - <%= link_to(l(:button_delete), contract_deliverable_path(@project, contract, deliverable), :method => :delete, :confirm => l(:text_are_you_sure), :class => 'icon icon-del') %> + <%= link_to(l(:button_delete), contract_deliverable_path(@project, contract, deliverable), :method => :delete, :confirm => l(:text_are_you_sure), :class => 'icon icon-del') if contract.open? %>
    <%= textilizable(deliverable, :notes) %> diff --git a/test/unit/deliverable_test.rb b/test/unit/deliverable_test.rb index 08b2d75..fa32573 100644 --- a/test/unit/deliverable_test.rb +++ b/test/unit/deliverable_test.rb @@ -50,6 +50,17 @@ class DeliverableTest < ActiveSupport::TestCase assert !deliverable.valid? assert deliverable.errors.on_base.include?("Can't create a deliverable on a locked contract") end + + should "block deleting a deliverable" do + contract = Contract.generate! + deliverable = FixedDeliverable.generate!(:contract => contract).reload + assert contract.lock! + + assert_no_difference("Deliverable.count") do + deliverable.destroy + end + + end end context "with a closed contract" do @@ -60,5 +71,16 @@ class DeliverableTest < ActiveSupport::TestCase assert !deliverable.valid? assert deliverable.errors.on_base.include?("Can't create a deliverable on a closed contract") end + + should "block deleting a deliverable" do + contract = Contract.generate! + deliverable = FixedDeliverable.generate!(:contract => contract).reload + assert contract.close! + + assert_no_difference("Deliverable.count") do + deliverable.destroy + end + + end end end
    <%= release(5, "Contract status") %>
    <%= h format_date(deliverable.end_date) %> <%= h deliverable.short_type %> <%= h deliverable.title %><%= release(5, "Deliverable status") %><%= h deliverable.status %> <%= h deliverable.manager.try(:name) %>