Merge pull request #6 from finnlabs/feature/rails3_fix_cukes

app/controllers/cost_types_controller.rb

@@ -3,10 +3,7 @@ class CostTypesController < ApplicationController
 
   # Allow only admins here
   before_filter :require_admin
-  before_filter :find_cost_type, :only => [:set_rate, :toggle_delete]
-  before_filter :find_optional_cost_type, :only => [:edit, :update]
-
-  verify :method => :post, :only => [:set_rate, :toggle_delete], :redirect_to => { :action => :index }
+  before_filter :find_cost_type, :only => [:edit, :update, :set_rate, :toggle_delete]
 
   helper :sort
   include SortHelper
@@ -34,38 +31,18 @@ class CostTypesController < ApplicationController
   end
 
   def edit
-    if !@cost_type
-      @cost_type = CostType.new()
-    end
-
-    @cost_type.attributes = permitted_params.cost_type if params[:cost_type]
-
-    if request.post? && @cost_type.save
-      flash[:notice] = l(:notice_successful_update)
-      redirect_back_or_default(:action => 'index')
-    else
-      @cost_type.rates.build({:valid_from => Date.today}) if @cost_type.rates.empty?
-      render :action => "edit", :layout => !request.xhr?
-    end
-  rescue ActiveRecord::StaleObjectError
-    # Optimistic locking exception
-    flash.now[:error] = l(:notice_locking_conflict)
+    render :action => "edit", :layout => !request.xhr?
   end
 
   def update
     # TODO: method is copied over from edit
     # remove code as appropriate
-    if !@cost_type
-      @cost_type = CostType.new()
-    end
-
-    @cost_type.attributes = permitted_params.cost_type if params[:cost_type]
+    @cost_type.attributes = permitted_params.cost_type
 
     if @cost_type.save
       flash[:notice] = l(:notice_successful_update)
       redirect_back_or_default(:action => 'index')
     else
-      @cost_type.rates.build({:valid_from => Date.today}) if @cost_type.rates.empty?
       render :action => "edit", :layout => !request.xhr?
     end
   rescue ActiveRecord::StaleObjectError
@@ -76,13 +53,19 @@ class CostTypesController < ApplicationController
   def new
     # TODO: method is copied over from edit
     # remove code as appropriate
-    if !@cost_type
-      @cost_type = CostType.new()
-    end
+    @cost_type = CostType.new()
 
-    @cost_type.attributes = permitted_params.cost_type if params[:cost_type]
+    @cost_type.rates.build({:valid_from => Date.today}) if @cost_type.rates.empty?
 
-    if request.post? && @cost_type.save
+    render :action => "edit", :layout => !request.xhr?
+  end
+
+  def create
+    # TODO: method is copied over from edit
+    # remove code as appropriate
+    @cost_type = CostType.new(permitted_params.cost_type)
+
+    if @cost_type.save
       flash[:notice] = l(:notice_successful_update)
       redirect_back_or_default(:action => 'index')
     else
@@ -130,12 +113,6 @@ private
     render_404
   end
 
-  def find_optional_cost_type
-    if !params[:id].blank?
-      @cost_type = CostType.find(params[:id])
-    end
-  end
-
   def default_breadcrumb
     l(:caption_cost_type_plural)
   end

app/views/cost_objects/_labor_budget_item.html.erb

@@ -12,7 +12,7 @@
 -%>
 
 <% unless error_messages.blank? %><tr><td colspan="5"><%= error_messages %></td></tr><% end %>
-<% fields_for prefix, labor_budget_item do |cost_form| %>
+<%= fields_for prefix, labor_budget_item do |cost_form| %>
 <tr class="cost_entry <%= classes %>" id="<%= id_prefix %>">
   <td class="units">
     <label class="hidden-for-sighted" for="<%= id_prefix %>_units"><%= l(:field_hours) %></label>

app/views/cost_objects/_material_budget_item.html.erb

@@ -12,7 +12,7 @@
 -%>
 
 <% unless error_messages.blank? %><tr><td colspan="5"><%= error_messages %></td></tr><% end %>
-<% fields_for prefix, material_budget_item do |cost_form| %>
+<%= fields_for prefix, material_budget_item do |cost_form| %>
 <tr class="cost_entry <%= classes %>" id="<%= id_prefix %>">
   <td class="units">
     <label class="hidden-for-sighted" for="<%= id_prefix %>_units"><%= l(:field_units) %></label>

app/views/cost_types/_rate.html.erb

@@ -13,7 +13,7 @@
 
 
 <% unless error_messages.blank? %><tr><td colspan="3"><%= error_messages %></td></tr><% end %>
-<% fields_for prefix, rate do |rate_form| %>
+<%= fields_for prefix, rate do |rate_form| %>
 <tr class="<%= classes %>" id="<%= id_prefix %>">
   <td>
     <label class="hidden-for-sighted", for="<%= "#{id_prefix}_valid_from" %>"><%= l(:caption_valid_from) %></label>

app/views/cost_types/edit.html.erb

@@ -1,7 +1,7 @@
 <%= render :partial => 'shared/costs_header' %>
 <h2><%= l(:caption_cost_type) %></h2>
 
-<%= labelled_tabular_form_for @cost_type, :url => {:action => 'update'}, :method => :put do |f| %>
+<%= labelled_tabular_form_for @cost_type do |f| %>
   <%= error_messages_for 'cost_type' %>
   <%= back_url_hidden_field_tag %>
 
@@ -13,7 +13,7 @@
   </div>
 
   <h3><%= l :caption_rate_history %></h3>
-  <% javascript_tag do -%>
+  <%= javascript_tag do -%>
     RatesForm = new Subform('<%= escape_javascript(render(:partial => "rate", :object => CostRate.new )) %>',<%= @cost_type.rates.length %>,'rates_body');
   <% end -%>
   <table class="list" style="width:auto">

app/views/cost_types/index.html.erb

@@ -5,28 +5,31 @@
 
 <h2><%= l(:caption_cost_type_plural) %></h2>
 
-<%= form_tag({ :controller => 'cost_types', :action => 'index' }, :id => 'query_form') do %>
-<fieldset id="filters"><legend><%= l(:label_filter_plural) %></legend>
-<p>
-  <label for="fixed_date"><%= l(:label_fixed_date) %></label>
-  <%= text_field_tag :fixed_date, @fixed_date, :size => 10 %><%= calendar_for('fixed_date') %>
-</p>
+<%= form_tag(cost_types_path, { :method => :get, :id => 'query_form' }) do %>
+  <fieldset id="filters">
+    <legend><%= l(:label_filter_plural) %></legend>
+    <p>
+      <label for="fixed_date"><%= l(:label_fixed_date) %></label>
+      <%= text_field_tag :fixed_date, @fixed_date, :size => 10 %><%= calendar_for('fixed_date') %>
+    </p>
 <p>
   <%= check_box_tag :include_deleted, "1", @include_deleted, :autocomplete => "off" %>
   <label for="include_deleted"><%= l(:caption_show_locked) %></label>
 </p>
+</fieldset>
 <p class="buttons">
 <%= link_to_remote l(:button_apply),
                    { :update => "content",
-                     :with => "Form.serialize('query_form')"
+                     :with => "Form.serialize('query_form')",
+                     :method => :get
                    }, :class => 'icon icon-checked' %>
 
 <%= link_to_remote l(:button_clear),
                    { :url => { :clear_filter => true },
+                     :method => :get,
                      :update => "content",
                    }, :class => 'icon icon-reload'  %>
 </p>
-</fieldset>
 <% end %>
 
 <div id="cost_type_flash_notice_outer" style="display:none">

app/views/hourly_rates/_rate.html.erb

@@ -13,7 +13,7 @@
 
 
 <% unless error_messages.blank? %><tr><td colspan="3"><%= error_messages %></td></tr><% end %>
-<% fields_for prefix, rate do |rate_form| %>
+<%= fields_for prefix, rate do |rate_form| %>
 <tr class="<%= classes %>" id="<%= id_prefix %>">
   <td>
     <label class="hidden-for-sighted", for="<%= "#{id_prefix}_valid_from" %>"><%= l(:caption_valid_from) %></label>

app/views/hourly_rates/edit.html.erb

@@ -6,7 +6,7 @@
 <p><strong><%= l(:label_current_default_rate) %>:</strong> <%= number_to_currency(default_rate.rate)%></p>
 <% end %>
 
-<% javascript_tag do -%>
+<%= javascript_tag do -%>
   RatesForm = new Subform('<%= escape_javascript(render(:partial => "rate", :object => HourlyRate.new )) %>',<%= @rates.length %>,'rates_body');
 <% end -%>
 

config/routes.rb

@@ -20,11 +20,11 @@ OpenProject::Application.routes.draw do
     post :preview, :on => :member
   end
 
-  resources :cost_types, :only => [:index, :new, :edit, :update] do
-    # TODO: change to put or even better, replace with update method
-    post :set_rate, :on => :member
+  resources :cost_types, :only => [:index, :new, :edit, :update, :create] do
+    # TODO: check if this can be replaced with update method
+    put :set_rate, :on => :member
     # TODO: change to destroy or even better, replace with destroy method
-    post :toggle_delete, :on => :member
+    put :toggle_delete, :on => :member
   end
 
   # TODO: this is a duplicate from a route defined under project/:project_id, check whether we really want to do that

features/create_budget.feature

@@ -13,10 +13,8 @@ Feature: Creating a Budget
     And I am already logged in as "testuser"
 
     When I go to the overview page of the project called "project1"
-    And I toggle the "Budgets" submenu
-    And I follow "New Budget" within "#main-menu"
-    And I fill in "cost_object_subject" with "budget1"
-    And I press "Create"
+    And I create a budget with the following:
+      | subject | budget1 |
 
     Then I should be on the show page for the budget "budget1"
     And I should see "Successful creation"

features/credit_unit_costs.feature

@@ -17,8 +17,8 @@ Feature: Credit unit costs
   Scenario: Crediting units costs to an issue
     When I am already logged in as "manager"
     And I go to the page of the issue "issue1"
-    And I follow "More functions" within ".action_menu_main"
-    And I follow "Log unit costs" within ".action_menu_main"
+    And I select "Log unit costs" from the action menu
     And I fill in "cost_entry_units" with "100"
+    And I select "cost_type_1" from "Cost type"
     And I press "Save"
     Then I should be on the page of the issue "issue1"

features/step_definitions/cost_object_steps.rb

@@ -0,0 +1,10 @@
+When(/^I create a budget with the following:$/) do |table|
+
+  rows = table.rows_hash
+
+  steps %Q{And I toggle the "Budgets" submenu
+           And I follow "New Budget" within "#main-menu"
+           And I fill in "Subject" with "#{rows['subject']}"}
+
+  click_button(I18n.t(:button_create), :exact => true)
+end

features/step_definitions/cost_steps.rb

@@ -12,10 +12,8 @@ Given /^there is 1 cost type with the following:$/ do |table|
   ct = CostType.generate
   send_table_to_object(ct, table, {
     :cost_rate => Proc.new do |o,v|
-      CostRate.generate.tap do |cr|
-        cr.rate = v
-        cr.cost_type = o
-      end.save!
+      FactoryGirl.create(:cost_rate, :rate => v,
+                                     :cost_type => o)
     end,
     :name => Proc.new do |o,v|
       o.name = v
@@ -78,7 +76,7 @@ end
 Given /^the issue "([^\"]+)" has (\d+) [Cc]ost(?: )?[Ee]ntr(?:ies|y) with the following:$/ do |issue, count, table|
   i = Issue.find(:last, :conditions => ["subject = '#{issue}'"])
   as_admin count do
-    ce = Factory.build(:cost_entry, :spent_on => (table.rows_hash["date"] ? table.rows_hash["date"].to_date : Date.today),
+    ce = FactoryGirl.build(:cost_entry, :spent_on => (table.rows_hash["date"] ? table.rows_hash["date"].to_date : Date.today),
                                     :units => table.rows_hash["units"],
                                     :project => i.project,
                                     :issue => i,
@@ -95,9 +93,13 @@ Given /^there is a standard cost control project named "([^\"]*)"$/ do |name|
   steps %Q{
     Given there is 1 project with the following:
       | Name | #{name} |
+    And the project "#{name}" has the following trackers:
+      | name     |
+      | tracker1 |
     And the project "#{name}" has 1 subproject
     And the project "#{name}" has 1 issue with:
       | subject | #{name}issue |
+    And there is a role "Manager"
     And the role "Manager" may have the following rights:
       | view_own_hourly_rate |
       | view_issues |
@@ -107,8 +109,10 @@ Given /^there is a standard cost control project named "([^\"]*)"$/ do |name|
     And there is a role "Controller"
     And the role "Controller" may have the following rights:
       | View own cost entries |
+    And there is a role "Developer"
     And the role "Developer" may have the following rights:
       | View own cost entries |
+    And there is a role "Reporter"
     And the role "Reporter" may have the following rights:
       | Create issues |
     And there is a role "Supplier"
@@ -162,7 +166,7 @@ Given /^users have times and the cost type "([^\"]*)" logged on the issue "([^\"
 end
 
 Given /^there is a variable cost object with the following:$/ do |table|
-  cost_object = Factory.build(:variable_cost_object)
+  cost_object = FactoryGirl.build(:variable_cost_object)
 
   table_hash = table.rows_hash
 
@@ -171,7 +175,7 @@ Given /^there is a variable cost object with the following:$/ do |table|
                              Time.now
   cost_object.fixed_date = cost_object.created_on.to_date
   cost_object.project = (Project.find_by_identifier(table_hash["project"]) || Project.find_by_name(table_hash ["project"])) if table_hash.has_key? "project"
-  cost_object.author = User.current
+  cost_object.author = User.find_by_login(table_hash["author"]) || cost_object.project.members.first.principal
   cost_object.subject = table_hash["subject"] if table_hash.has_key? "subject"
 
   cost_object.save!

features/view_own_rates.feature

@@ -53,7 +53,6 @@ Feature: Permission View Own hourly and cost rates
       | Labor Costs    |
       | Unit Costs     |
     And I follow "Apply"
-    And I wait for AJAX
 		Then I should see "24.00 EUR"
     And I should see "10.00 EUR"
     And I should see "14.00 EUR"

lib/open_project/costs/engine.rb

@@ -1,10 +1,12 @@
+require_dependency 'open_project/costs/patches/i18n_patch'
+
 module OpenProject::Costs
   class Engine < ::Rails::Engine
     engine_name :openproject_costs
 
     def self.settings
       { :default => { 'costs_currency' => 'EUR',
-                     'costs_currency_format' => '%n %u' },
+                      'costs_currency_format' => '%n %u' },
         :partial => 'settings/openproject_costs' }
     end
 

lib/open_project/costs/patches/i18n_patch.rb

@@ -1,17 +1,30 @@
-module ActionView::Helpers::NumberHelper
-  def number_to_currency_with_l10n(number, options = {})
-    options[:delimiter] = l(:currency_delimiter) unless options[:delimiter]
-    options[:separator] = l(:currency_separator) unless options[:separator]
+module OpenProject::Costs::Patches
+  module NumberHelper
+    def self.included(base) # :nodoc:
+      base.class_eval do
+        include InstanceMethods
 
-    options[:unit] = Setting.plugin_openproject_costs['costs_currency'] unless options[:unit]
-    options[:format] = Setting.plugin_openproject_costs['costs_currency_format'] unless options[:format]
-  
-    # FIXME: patch ruby instead of this code
-    # this circumvents the broken BigDecimal#to_f on Siemens's ruby
-    number = number.to_s if number.is_a? BigDecimal
-  
-    number_to_currency_without_l10n(number, options)
+        alias_method_chain :number_to_currency, :l10n
+      end
+    end
+
+    module InstanceMethods
+      def number_to_currency_with_l10n(number, options = {})
+        options_with_default = { unit: Setting.plugin_openproject_costs['costs_currency'],
+                                 format: Setting.plugin_openproject_costs['costs_currency_format'],
+                                 delimiter: l(:currency_delimiter),
+                                 separator: l(:currency_separator) }.merge(options)
+
+        # FIXME: patch ruby instead of this code
+        # this circumvents the broken BigDecimal#to_f on Siemens's ruby
+        number = number.to_s if number.is_a? BigDecimal
+
+        number_to_currency_without_l10n(number, options_with_default)
+      end
+    end
   end
+end
 
-  alias_method_chain :number_to_currency, :l10n
-end
+unless ActionView::Helpers::NumberHelper.included_modules.include?(OpenProject::Costs::Patches::NumberHelper)
+  ActionView::Helpers::NumberHelper.send(:include, OpenProject::Costs::Patches::NumberHelper)
+end

lib/open_project/costs/patches/permitted_params_patch.rb

@@ -27,8 +27,8 @@ module OpenProject::Costs::Patches::PermittedParamsPatch
                                         :unit,
                                         :unit_plural,
                                         :default,
-                                        :new_rate_attributes,
-                                        :existing_rate_attributes)
+                                        { new_rate_attributes: [:valid_from, :rate] },
+                                        { existing_rate_attributes: [:valid_from, :rate] })
     end
 
     def labor_budget_item

spec/models/permitted_params_spec.rb

@@ -99,15 +99,15 @@ describe PermittedParams do
     end
 
     it "should return new_rate_attributes" do
-      params = ActionController::Parameters.new(:cost_type => { "new_rate_attributes" => "new_rate_attributes_test" } )
+      params = ActionController::Parameters.new(:cost_type => { "new_rate_attributes" => { "0" => { "valid_from" => "2013-05-08", "rate" => "5002" }, "1" => { "valid_from" => "2013-05-10", "rate" => "5004" } } } )
 
-      PermittedParams.new(params, user).cost_type.should == { "new_rate_attributes" => "new_rate_attributes_test" }
+      PermittedParams.new(params, user).cost_type.should == { "new_rate_attributes" => { "0" => { "valid_from" => "2013-05-08", "rate" => "5002" }, "1" => { "valid_from" => "2013-05-10", "rate" => "5004" } } }
     end
 
     it "should return existing_rate_attributes" do
-      params = ActionController::Parameters.new(:cost_type => { "existing_rate_attributes" => "new_rate_attributes_test" } )
+      params = ActionController::Parameters.new(:cost_type => { "existing_rate_attributes" => { "9" => { "valid_from" => "2013-05-05", "rate" => "50.0" } } } )
 
-      PermittedParams.new(params, user).cost_type.should == { "existing_rate_attributes" => "new_rate_attributes_test" }
+      PermittedParams.new(params, user).cost_type.should == { "existing_rate_attributes" => { "9" => { "valid_from" => "2013-05-05", "rate" => "50.0" } } }
     end
 
     it "should not return project_id" do

spec/routing/cost_types_routing_spec.rb

@@ -5,6 +5,9 @@ describe CostTypesController do
     it { get('/cost_types').should route_to(:controller => 'cost_types',
                                             :action => 'index') }
 
+    it { post('/cost_types').should route_to(:controller => 'cost_types',
+                                             :action => 'create') }
+
     it { get('/cost_types/new').should route_to(:controller => 'cost_types',
                                                 :action => 'new') }
 
@@ -16,11 +19,12 @@ describe CostTypesController do
                                               :action => 'update',
                                               :id => '5') }
 
-    it { post('/cost_types/5/set_rate').should route_to(:controller => 'cost_types',
-                                                        :action => 'set_rate',
-                                                        :id => '5') }
-    it { post('/cost_types/5/toggle_delete').should route_to(:controller => 'cost_types',
-                                                             :action => 'toggle_delete',
-                                                             :id => '5') }
+    it { put('/cost_types/5/set_rate').should route_to(:controller => 'cost_types',
+                                                       :action => 'set_rate',
+                                                       :id => '5') }
+
+    it { put('/cost_types/5/toggle_delete').should route_to(:controller => 'cost_types',
+                                                            :action => 'toggle_delete',
+                                                            :id => '5') }
   end
 end