From b4b404888bd23c2f4a20ef88cb968916b26963e0 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Wed, 15 Aug 2018 12:25:19 +0300 Subject: [PATCH 1/5] Disallow deposits that are lower than 0.01 EUR --- .../registrar/deposits_controller.rb | 6 +- app/models/deposit.rb | 16 +++-- spec/models/deposit_spec.rb | 39 ------------ test/models/deposit_test.rb | 59 +++++++++++++++++++ .../registrar_area/invoices/new_test.rb | 10 +--- 5 files changed, 76 insertions(+), 54 deletions(-) delete mode 100644 spec/models/deposit_spec.rb create mode 100644 test/models/deposit_test.rb diff --git a/app/controllers/registrar/deposits_controller.rb b/app/controllers/registrar/deposits_controller.rb index ec6d13977..818e38c6d 100644 --- a/app/controllers/registrar/deposits_controller.rb +++ b/app/controllers/registrar/deposits_controller.rb @@ -10,12 +10,12 @@ class Registrar @deposit = Deposit.new(deposit_params.merge(registrar: current_user.registrar)) @invoice = @deposit.issue_prepayment_invoice - if @invoice&.persisted? + if @invoice flash[:notice] = t(:please_pay_the_following_invoice) redirect_to [:registrar, @invoice] else - flash.now[:alert] = t(:failed_to_create_record) - render 'new' + flash[:alert] = @deposit.errors.full_messages.join(', ') + redirect_to new_registrar_deposit_path end end diff --git a/app/models/deposit.rb b/app/models/deposit.rb index 23045196a..589856437 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -4,13 +4,17 @@ class Deposit extend ActiveModel::Naming include DisableHtml5Validation - attr_accessor :amount, :description, :registrar, :registrar_id + attr_accessor :description, :registrar, :registrar_id + attr_writer :amount validates :amount, :registrar, presence: true validate :validate_amount + def validate_amount - return if BigDecimal.new(amount) >= Setting.minimum_deposit - errors.add(:amount, I18n.t(:is_too_small_minimum_deposit_is, amount: Setting.minimum_deposit, currency: 'EUR')) + minimum_deposit = [0.01, Setting.minimum_deposit].max + return if amount >= minimum_deposit + errors.add(:amount, I18n.t(:is_too_small_minimum_deposit_is, amount: minimum_deposit, + currency: 'EUR')) end def initialize(attributes = {}) @@ -24,10 +28,12 @@ class Deposit end def amount - BigDecimal.new(@amount.to_s.sub(/,/, '.')) + return BigDecimal.new('0.0') if @amount.blank? + BigDecimal.new(@amount, 10) end def issue_prepayment_invoice - valid? && registrar.issue_prepayment_invoice(amount, description) + return unless valid? + registrar.issue_prepayment_invoice(amount, description) end end diff --git a/spec/models/deposit_spec.rb b/spec/models/deposit_spec.rb deleted file mode 100644 index ff77dbd98..000000000 --- a/spec/models/deposit_spec.rb +++ /dev/null @@ -1,39 +0,0 @@ -require 'rails_helper' - -describe Deposit do - context 'with invalid attribute' do - before :all do - @deposit = Deposit.new - end - - it 'should not be valid' do - @deposit.valid? - @deposit.errors.full_messages.should match_array([ - "Registrar is missing" - ]) - end - - it 'should have 0 amount' do - @deposit.amount.should == 0 - end - - it 'should not be presisted' do - @deposit.persisted?.should == false - end - - it 'should replace comma with point for 0' do - @deposit.amount = '0,0' - @deposit.amount.should == 0.0 - end - - it 'should replace comma with points' do - @deposit.amount = '10,11' - @deposit.amount.should == 10.11 - end - - it 'should work with float as well' do - @deposit.amount = 0.123 - @deposit.amount.should == 0.123 - end - end -end diff --git a/test/models/deposit_test.rb b/test/models/deposit_test.rb new file mode 100644 index 000000000..09f7de7f3 --- /dev/null +++ b/test/models/deposit_test.rb @@ -0,0 +1,59 @@ +require 'test_helper' + +class DepositTest < ActiveSupport::TestCase + def setup + super + + @deposit = Deposit.new(registrar: registrars(:bestnames)) + @minimum_deposit = Setting.minimum_deposit + Setting.minimum_deposit = 1.00 + end + + def teardown + super + + Setting.minimum_deposit = @minimum_deposit + end + + def test_validate_amount_cannot_be_lower_than_0_01 + Setting.minimum_deposit = 0.0 + @deposit.amount = -10 + refute(@deposit.valid?) + assert(@deposit.errors.full_messages.include?("Amount is too small. Minimum deposit is 0.01 EUR")) + end + + def test_validate_amount_cannot_be_lower_than_minimum_deposit + @deposit.amount = 0.10 + refute(@deposit.valid?) + + assert(@deposit.errors.full_messages.include?("Amount is too small. Minimum deposit is 1.0 EUR")) + end + + def test_registrar_must_be_set + deposit = Deposit.new(amount: 120) + refute(deposit.valid?) + + assert(deposit.errors.full_messages.include?("Registrar is missing")) + end + + def test_amount_is_converted_from_string + @deposit.amount = "12.00" + assert_equal(BigDecimal.new("12.00"), @deposit.amount) + + @deposit.amount = "12,00" + assert_equal(BigDecimal.new("12.00"), @deposit.amount) + end + + def test_amount_is_converted_from_float + @deposit.amount = 12.0044 + assert_equal(BigDecimal.new("12.0044"), @deposit.amount) + + @deposit.amount = 12.0144 + assert_equal(BigDecimal.new("12.0144"), @deposit.amount) + end + + def test_amount_is_converted_from_nil + @deposit.amount = nil + assert_equal(BigDecimal.new("0.00"), @deposit.amount) + end +end diff --git a/test/system/registrar_area/invoices/new_test.rb b/test/system/registrar_area/invoices/new_test.rb index b9b6b6db4..282b109dd 100644 --- a/test/system/registrar_area/invoices/new_test.rb +++ b/test/system/registrar_area/invoices/new_test.rb @@ -29,20 +29,16 @@ class NewInvoiceTest < ApplicationSystemTestCase assert_text 'Pay invoice' end - # This test case should fail once issue #651 gets fixed - def test_create_new_invoice_with_amount_0_goes_through + def test_create_new_invoice_fails_when_amount_is_0 visit registrar_invoices_path click_link_or_button 'Add deposit' fill_in 'Amount', with: '0.00' fill_in 'Description', with: 'My first invoice' - assert_difference 'Invoice.count', 1 do + assert_no_difference 'Invoice.count' do click_link_or_button 'Add' end - assert_text 'Please pay the following invoice' - assert_text 'Invoice no. 131050' - assert_text 'Subtotal 0,00 €' - assert_text 'Pay invoice' + assert_text 'Amount is too small. Minimum deposit is 0.01 EUR' end end From 0ea10452593cf5251b53fd0f95b476c23d896647 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Wed, 15 Aug 2018 12:46:36 +0300 Subject: [PATCH 2/5] Fix rubocop issues --- app/models/deposit.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/deposit.rb b/app/models/deposit.rb index 589856437..05197c5a1 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -14,7 +14,7 @@ class Deposit minimum_deposit = [0.01, Setting.minimum_deposit].max return if amount >= minimum_deposit errors.add(:amount, I18n.t(:is_too_small_minimum_deposit_is, amount: minimum_deposit, - currency: 'EUR')) + currency: 'EUR')) end def initialize(attributes = {}) @@ -28,8 +28,8 @@ class Deposit end def amount - return BigDecimal.new('0.0') if @amount.blank? - BigDecimal.new(@amount, 10) + return BigDecimal('0.0') if @amount.blank? + BigDecimal(@amount, 10) end def issue_prepayment_invoice From b8c082090e223e3a77f8c26f0dd56464db256fe2 Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Wed, 15 Aug 2018 16:32:42 +0300 Subject: [PATCH 3/5] Rename a variable from minimum_deposit to minimum_allowed_amount --- app/models/deposit.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/deposit.rb b/app/models/deposit.rb index 05197c5a1..41d59ac4c 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -11,9 +11,9 @@ class Deposit validate :validate_amount def validate_amount - minimum_deposit = [0.01, Setting.minimum_deposit].max - return if amount >= minimum_deposit - errors.add(:amount, I18n.t(:is_too_small_minimum_deposit_is, amount: minimum_deposit, + minimum_allowed_amount = [0.01, Setting.minimum_deposit].max + return if amount >= minimum_allowed_amount + errors.add(:amount, I18n.t(:is_too_small_minimum_deposit_is, amount: minimum_allowed_amount, currency: 'EUR')) end From c028c0e4771ac1f7815156a10b57d5790572693d Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Wed, 15 Aug 2018 20:15:17 +0300 Subject: [PATCH 4/5] Add more tests to deposit handling --- app/models/deposit.rb | 2 +- test/models/deposit_test.rb | 4 +-- .../registrar_area/invoices/new_test.rb | 29 +++++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/app/models/deposit.rb b/app/models/deposit.rb index 41d59ac4c..a3b898047 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -29,7 +29,7 @@ class Deposit def amount return BigDecimal('0.0') if @amount.blank? - BigDecimal(@amount, 10) + BigDecimal(@amount.to_s.gsub(/,/, '.'), 10) end def issue_prepayment_invoice diff --git a/test/models/deposit_test.rb b/test/models/deposit_test.rb index 09f7de7f3..b7510b960 100644 --- a/test/models/deposit_test.rb +++ b/test/models/deposit_test.rb @@ -40,8 +40,8 @@ class DepositTest < ActiveSupport::TestCase @deposit.amount = "12.00" assert_equal(BigDecimal.new("12.00"), @deposit.amount) - @deposit.amount = "12,00" - assert_equal(BigDecimal.new("12.00"), @deposit.amount) + @deposit.amount = "12,11" + assert_equal(BigDecimal.new("12.11"), @deposit.amount) end def test_amount_is_converted_from_float diff --git a/test/system/registrar_area/invoices/new_test.rb b/test/system/registrar_area/invoices/new_test.rb index 282b109dd..a5a72fbe8 100644 --- a/test/system/registrar_area/invoices/new_test.rb +++ b/test/system/registrar_area/invoices/new_test.rb @@ -29,6 +29,22 @@ class NewInvoiceTest < ApplicationSystemTestCase assert_text 'Pay invoice' end + def test_create_new_invoice_with_comma_in_number + visit registrar_invoices_path + click_link_or_button 'Add deposit' + fill_in 'Amount', with: '200,00' + fill_in 'Description', with: 'My first invoice' + + assert_difference 'Invoice.count', 1 do + click_link_or_button 'Add' + end + + assert_text 'Please pay the following invoice' + assert_text 'Invoice no. 131050' + assert_text 'Subtotal 200,00 €' + assert_text 'Pay invoice' + end + def test_create_new_invoice_fails_when_amount_is_0 visit registrar_invoices_path click_link_or_button 'Add deposit' @@ -41,4 +57,17 @@ class NewInvoiceTest < ApplicationSystemTestCase assert_text 'Amount is too small. Minimum deposit is 0.01 EUR' end + + def test_create_new_invoice_fails_when_amount_is_negative + visit registrar_invoices_path + click_link_or_button 'Add deposit' + fill_in 'Amount', with: '-120.00' + fill_in 'Description', with: 'My first invoice' + + assert_no_difference 'Invoice.count' do + click_link_or_button 'Add' + end + + assert_text 'Amount is too small. Minimum deposit is 0.01 EUR' + end end From 066123bcf731fb568c1dda01c41344e1ec572bbc Mon Sep 17 00:00:00 2001 From: Maciej Szlosarczyk Date: Thu, 16 Aug 2018 13:33:09 +0300 Subject: [PATCH 5/5] Replace gsub with tr for better performance --- app/models/deposit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/deposit.rb b/app/models/deposit.rb index a3b898047..2eff26bcc 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -29,7 +29,7 @@ class Deposit def amount return BigDecimal('0.0') if @amount.blank? - BigDecimal(@amount.to_s.gsub(/,/, '.'), 10) + BigDecimal(@amount.to_s.tr(',', '.'), 10) end def issue_prepayment_invoice