From 5738c1773107eb3e8c5c83f76073b669e29eca67 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 1 Oct 2018 13:43:50 +0300 Subject: [PATCH] Use Estonian reference number format instead of ISO 11649 --- .../admin/registrars_controller.rb | 1 + app/models/billing/reference_no.rb | 10 ++++ app/models/billing/reference_no/base.rb | 48 ++++++++++++++++++ app/models/registrar.rb | 25 +--------- app/views/admin/registrars/_billing.html.erb | 4 ++ app/views/admin/registrars/_details.html.erb | 3 -- .../admin/registrars/form/_billing.html.erb | 18 +++++++ config/locales/admin/registrars.en.yml | 8 +-- config/locales/en.yml | 1 + ...1090536_change_reference_no_to_not_null.rb | 6 +++ db/structure.sql | 6 ++- ...egenerate_registrar_reference_numbers.rake | 18 +++++++ lib/tasks/import.rake | 1 - spec/factories/invoice.rb | 1 + spec/factories/registrar.rb | 1 + spec/models/bank_statement_spec.rb | 4 +- spec/models/bank_transaction_spec.rb | 4 +- test/fixtures/invoices.yml | 1 + test/fixtures/registrars.yml | 3 ++ ...nerate_registrar_reference_numbers_test.rb | 50 +++++++++++++++++++ test/models/billing/reference_no/base_test.rb | 29 +++++++++++ test/models/billing/reference_no_test.rb | 13 +++++ test/models/registrar_test.rb | 28 +++++++++-- .../admin_area/contact_versions_test.rb | 4 +- .../system/admin_area/domain_versions_test.rb | 4 +- 25 files changed, 244 insertions(+), 47 deletions(-) create mode 100644 app/models/billing/reference_no.rb create mode 100644 app/models/billing/reference_no/base.rb create mode 100644 db/migrate/20181001090536_change_reference_no_to_not_null.rb create mode 100644 lib/tasks/data_migrations/regenerate_registrar_reference_numbers.rake create mode 100644 test/integration/tasks/data_migrations/regenerate_registrar_reference_numbers_test.rb create mode 100644 test/models/billing/reference_no/base_test.rb create mode 100644 test/models/billing/reference_no_test.rb diff --git a/app/controllers/admin/registrars_controller.rb b/app/controllers/admin/registrars_controller.rb index 25e9eec40..6eb2e0bcf 100644 --- a/app/controllers/admin/registrars_controller.rb +++ b/app/controllers/admin/registrars_controller.rb @@ -14,6 +14,7 @@ module Admin def create @registrar = Registrar.new(registrar_params) + @registrar.reference_no = ::Billing::ReferenceNo.generate if @registrar.valid? @registrar.transaction do diff --git a/app/models/billing/reference_no.rb b/app/models/billing/reference_no.rb new file mode 100644 index 000000000..23812214c --- /dev/null +++ b/app/models/billing/reference_no.rb @@ -0,0 +1,10 @@ +module Billing + class ReferenceNo + REGEXP = /\A\d{2,20}\z/ + + def self.generate + base = Base.generate + "#{base}#{base.check_digit}" + end + end +end diff --git a/app/models/billing/reference_no/base.rb b/app/models/billing/reference_no/base.rb new file mode 100644 index 000000000..d36efa114 --- /dev/null +++ b/app/models/billing/reference_no/base.rb @@ -0,0 +1,48 @@ +module Billing + class ReferenceNo + class Base + def self.generate + new(SecureRandom.random_number(1..1_000_000)) + end + + def initialize(base) + @base = base.to_s + end + + def check_digit + amount = amount_after_multiplication + next_number_ending_with_zero(amount) - amount + end + + def to_s + base + end + + private + + attr_reader :base + + def next_number_ending_with_zero(number) + next_number = number + + loop do + next_number = next_number.next + return next_number if next_number.to_s.end_with?('0') + end + end + + def amount_after_multiplication + multipliers = [7, 3, 1] + enumerator = multipliers.cycle + amount = 0 + + base.reverse.each_char do |char| + digit = char.to_i + amount += (digit * enumerator.next) + end + + amount + end + end + end +end diff --git a/app/models/registrar.rb b/app/models/registrar.rb index 5b948b41c..cb633acb1 100644 --- a/app/models/registrar.rb +++ b/app/models/registrar.rb @@ -14,9 +14,10 @@ class Registrar < ActiveRecord::Base delegate :balance, to: :cash_account, allow_nil: true validates :name, :reg_no, :country_code, :email, :code, presence: true - validates :name, :reference_no, :code, uniqueness: true + validates :name, :code, uniqueness: true validates :accounting_customer_code, presence: true validates :language, presence: true + validates :reference_no, format: Billing::ReferenceNo::REGEXP validate :forbid_special_code validates :vat_rate, presence: true, if: 'foreign_vat_payer? && vat_no.blank?' @@ -29,7 +30,6 @@ class Registrar < ActiveRecord::Base attribute :vat_rate, ::Type::VATRate.new after_initialize :set_defaults - before_validation :generate_iso_11649_reference_no validates :email, :billing_email, email_format: { message: :invalid }, @@ -167,27 +167,6 @@ class Registrar < ActiveRecord::Base errors.add(:code, :forbidden) if code == 'CID' end - def generate_iso_11649_reference_no - return if reference_no.present? - - loop do - base = nil - loop do - base = SecureRandom.random_number.to_s.last(8) - break if base.to_i != 0 && base.length == 8 - end - - control_base = (base + '2715' + '00').to_i - reminder = control_base % 97 - check_digits = 98 - reminder - - check_digits = check_digits < 10 ? "0#{check_digits}" : check_digits.to_s - - self.reference_no = "RF#{check_digits}#{base}" - break unless self.class.exists?(reference_no: reference_no) - end - end - def home_vat_payer? country == Registry.instance.legal_address_country end diff --git a/app/views/admin/registrars/_billing.html.erb b/app/views/admin/registrars/_billing.html.erb index 7a7e49fea..8b2e83ee9 100644 --- a/app/views/admin/registrars/_billing.html.erb +++ b/app/views/admin/registrars/_billing.html.erb @@ -2,6 +2,7 @@
<%= t '.header' %>
+
<%= Registrar.human_attribute_name :vat_no %>
@@ -15,6 +16,9 @@
<%= Registrar.human_attribute_name :billing_email %>
<%= registrar.billing_email %>
+ +
<%= Registrar.human_attribute_name :reference_no %>
+
<%= registrar.reference_no %>
diff --git a/app/views/admin/registrars/_details.html.erb b/app/views/admin/registrars/_details.html.erb index c640075ba..3fc884e5c 100644 --- a/app/views/admin/registrars/_details.html.erb +++ b/app/views/admin/registrars/_details.html.erb @@ -11,9 +11,6 @@
<%= Registrar.human_attribute_name :reg_no %>
<%= registrar.reg_no %>
-
<%= Registrar.human_attribute_name :reference_no %>
-
<%= registrar.reference_no %>
-
<%= Registrar.human_attribute_name :code %>
<%= registrar.code %>
diff --git a/app/views/admin/registrars/form/_billing.html.erb b/app/views/admin/registrars/form/_billing.html.erb index c87da8b6c..03dfc22c6 100644 --- a/app/views/admin/registrars/form/_billing.html.erb +++ b/app/views/admin/registrars/form/_billing.html.erb @@ -47,6 +47,24 @@ <%= f.email_field :billing_email, class: 'form-control' %> + +
+ <% if f.object.new_record? %> +
+ <%= t '.no_reference_no_hint' %> +
+ <% else %> +
+ <%= f.label :reference_no %> +
+ +
+ <%= f.text_field :reference_no, disabled: true, + title: t('.disabled_reference_no_hint'), + class: 'form-control' %> +
+ <% end %> +
diff --git a/config/locales/admin/registrars.en.yml b/config/locales/admin/registrars.en.yml index 3586960b0..47a443e06 100644 --- a/config/locales/admin/registrars.en.yml +++ b/config/locales/admin/registrars.en.yml @@ -19,12 +19,6 @@ en: billing: header: Billing - edit: - header: Edit registrar - - billing: - header: Billing - create: created: Registrar has been successfully created @@ -41,6 +35,8 @@ en: billing: header: Billing + no_reference_no_hint: Reference number will be generated automatically + disabled_reference_no_hint: Reference number cannot be changed preferences: header: Preferences diff --git a/config/locales/en.yml b/config/locales/en.yml index f204e336e..e9e499def 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -746,3 +746,4 @@ en: vat_rate: VAT rate ipv4: IPv4 ipv6: IPv6 + reference_no: Reference number diff --git a/db/migrate/20181001090536_change_reference_no_to_not_null.rb b/db/migrate/20181001090536_change_reference_no_to_not_null.rb new file mode 100644 index 000000000..9dc993179 --- /dev/null +++ b/db/migrate/20181001090536_change_reference_no_to_not_null.rb @@ -0,0 +1,6 @@ +class ChangeReferenceNoToNotNull < ActiveRecord::Migration + def change + change_column_null :registrars, :reference_no, false + change_column_null :invoices, :reference_no, false + end +end diff --git a/db/structure.sql b/db/structure.sql index 6eede259a..92a775966 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1048,7 +1048,7 @@ CREATE TABLE public.invoices ( payment_term character varying, currency character varying NOT NULL, description character varying, - reference_no character varying, + reference_no character varying NOT NULL, vat_rate numeric(4,3), paid_at timestamp without time zone, seller_id integer, @@ -2202,7 +2202,7 @@ CREATE TABLE public.registrars ( website character varying, accounting_customer_code character varying NOT NULL, legacy_id integer, - reference_no character varying, + reference_no character varying NOT NULL, test_registrar boolean DEFAULT false, language character varying NOT NULL, vat_rate numeric(4,3) @@ -4859,5 +4859,7 @@ INSERT INTO schema_migrations (version) VALUES ('20180825232819'); INSERT INTO schema_migrations (version) VALUES ('20180826162821'); +INSERT INTO schema_migrations (version) VALUES ('20181001090536'); + INSERT INTO schema_migrations (version) VALUES ('20181002090319'); diff --git a/lib/tasks/data_migrations/regenerate_registrar_reference_numbers.rake b/lib/tasks/data_migrations/regenerate_registrar_reference_numbers.rake new file mode 100644 index 000000000..aff296d83 --- /dev/null +++ b/lib/tasks/data_migrations/regenerate_registrar_reference_numbers.rake @@ -0,0 +1,18 @@ +namespace :data_migrations do + task regenerate_registrar_reference_numbers: [:environment] do + processed_registrar_count = 0 + + Registrar.transaction do + Registrar.all.each do |registrar| + next unless registrar.reference_no.start_with?('RF') + + registrar.reference_no = Billing::ReferenceNo.generate + registrar.save! + + processed_registrar_count += 1 + end + end + + puts "Registrars processed: #{processed_registrar_count}" + end +end diff --git a/lib/tasks/import.rake b/lib/tasks/import.rake index a3b884935..ebd40a65e 100644 --- a/lib/tasks/import.rake +++ b/lib/tasks/import.rake @@ -97,7 +97,6 @@ namespace :import do puts "-----> Generating reference numbers" Registrar.all.each do |x| - x.send(:generate_iso_11649_reference_no) x.save(validate: false) end diff --git a/spec/factories/invoice.rb b/spec/factories/invoice.rb index 42ac54812..06293c7ad 100644 --- a/spec/factories/invoice.rb +++ b/spec/factories/invoice.rb @@ -9,6 +9,7 @@ FactoryBot.define do seller_street { 'Paldiski mnt. 123' } vat_rate 0.2 buyer { FactoryBot.create(:registrar) } + sequence(:reference_no) { |n| "1234#{n}" } after :build do |invoice| invoice.invoice_items << FactoryBot.create_pair(:invoice_item) diff --git a/spec/factories/registrar.rb b/spec/factories/registrar.rb index c77e8dc66..82c492bb0 100644 --- a/spec/factories/registrar.rb +++ b/spec/factories/registrar.rb @@ -11,6 +11,7 @@ FactoryBot.define do country_code 'US' accounting_customer_code 'test' language 'en' + sequence(:reference_no) { |n| "1234#{n}" } factory :registrar_with_unlimited_balance do after :create do |registrar| diff --git a/spec/models/bank_statement_spec.rb b/spec/models/bank_statement_spec.rb index 8b10307fc..d1d54d01f 100644 --- a/spec/models/bank_statement_spec.rb +++ b/spec/models/bank_statement_spec.rb @@ -36,7 +36,7 @@ describe BankStatement do end it 'should not bind transactions with invalid match data' do - r = create(:registrar, reference_no: 'RF7086666663') + r = create(:registrar, reference_no: '1234') create(:account, registrar: r, account_type: 'cash', balance: 0) @@ -50,7 +50,7 @@ describe BankStatement do }), create(:bank_transaction, { sum: 240.0, - reference_no: 'RF7086666663', + reference_no: '1234', description: 'Invoice no. 4948934' }) ]) diff --git a/spec/models/bank_transaction_spec.rb b/spec/models/bank_transaction_spec.rb index 9a559bbd6..7b3cb57f6 100644 --- a/spec/models/bank_transaction_spec.rb +++ b/spec/models/bank_transaction_spec.rb @@ -35,7 +35,7 @@ describe BankTransaction do end it 'should not bind transaction with mismatching sums' do - r = create(:registrar, reference_no: 'RF7086666663') + r = create(:registrar) invoice = r.issue_prepayment_invoice(200, 'add some money') bt = create(:bank_transaction, { sum: 10 }) @@ -45,7 +45,7 @@ describe BankTransaction do end it 'should not bind transaction with cancelled invoice' do - r = create(:registrar, reference_no: 'RF7086666663') + r = create(:registrar) invoice = r.issue_prepayment_invoice(200, 'add some money') invoice.cancel diff --git a/test/fixtures/invoices.yml b/test/fixtures/invoices.yml index 6b9359343..52d7cb06e 100644 --- a/test/fixtures/invoices.yml +++ b/test/fixtures/invoices.yml @@ -8,6 +8,7 @@ DEFAULTS: &DEFAULTS buyer_name: Jane Doe vat_rate: 0.1 total: 16.50 + reference_no: 13 valid: <<: *DEFAULTS diff --git a/test/fixtures/registrars.yml b/test/fixtures/registrars.yml index d6e18982f..8f7a9581b 100644 --- a/test/fixtures/registrars.yml +++ b/test/fixtures/registrars.yml @@ -12,6 +12,7 @@ bestnames: language: en billing_email: billing@example.com website: bestnames.test + reference_no: 13 goodnames: name: Good Names @@ -22,6 +23,7 @@ goodnames: vat_no: DE123456789 accounting_customer_code: goodnames language: en + reference_no: 26 not_in_use: name: any @@ -31,3 +33,4 @@ not_in_use: country_code: US accounting_customer_code: any language: en + reference_no: 39 diff --git a/test/integration/tasks/data_migrations/regenerate_registrar_reference_numbers_test.rb b/test/integration/tasks/data_migrations/regenerate_registrar_reference_numbers_test.rb new file mode 100644 index 000000000..7700b3fb6 --- /dev/null +++ b/test/integration/tasks/data_migrations/regenerate_registrar_reference_numbers_test.rb @@ -0,0 +1,50 @@ +require 'test_helper' + +class RegenerateRegistrarReferenceNumbersTaskTest < ActiveSupport::TestCase + def test_regenerates_registrar_reference_numbers_to_estonian_format + registrar = registrars(:bestnames) + registrar.update_column(:reference_no, 'RF1111') + + capture_io { run_task } + registrar.reload + + assert_not registrar.reference_no.start_with?('RF') + end + + def test_does_not_regenerate_when_the_task_is_run_again + registrar = registrars(:bestnames) + registrar.update!(reference_no: '1111') + + capture_io { run_task } + registrar.reload + + assert_equal '1111', registrar.reference_no + end + + def test_keeps_iso_reference_number_on_the_invoice_unchanged + registrar = registrars(:bestnames) + registrar.update_column(:reference_no, 'RF1111') + invoice = registrar.invoices.first + invoice.update!(reference_no: 'RF2222') + + capture_io { run_task } + invoice.reload + + assert_equal 'RF2222', invoice.reference_no + end + + def test_output + registrar = registrars(:bestnames) + registrar.update_column(:reference_no, 'RF1111') + + assert_output "Registrars processed: 1\n" do + run_task + end + end + + private + + def run_task + Rake::Task['data_migrations:regenerate_registrar_reference_numbers'].execute + end +end diff --git a/test/models/billing/reference_no/base_test.rb b/test/models/billing/reference_no/base_test.rb new file mode 100644 index 000000000..27c7d9e27 --- /dev/null +++ b/test/models/billing/reference_no/base_test.rb @@ -0,0 +1,29 @@ +require 'test_helper' + +# https://www.pangaliit.ee/settlements-and-standards/reference-number-of-the-invoice +class ReferenceNoBaseTest < ActiveSupport::TestCase + def test_generates_random_base + assert_not_equal Billing::ReferenceNo::Base.generate, Billing::ReferenceNo::Base.generate + end + + def test_randomly_generated_base_conforms_to_standard + base = Billing::ReferenceNo::Base.generate + format = /\A\d{1,19}\z/ + assert_match format, base.to_s + end + + def test_generates_check_digit_for_a_given_base + assert_equal 3, Billing::ReferenceNo::Base.new('1').check_digit + assert_equal 7, Billing::ReferenceNo::Base.new('1234567891234567891').check_digit + end + + def test_returns_string_representation + base = Billing::ReferenceNo::Base.new('1') + assert_equal '1', base.to_s + end + + def test_normalizes_non_string_values + base = Billing::ReferenceNo::Base.new(1) + assert_equal '1', base.to_s + end +end diff --git a/test/models/billing/reference_no_test.rb b/test/models/billing/reference_no_test.rb new file mode 100644 index 000000000..d7bf78597 --- /dev/null +++ b/test/models/billing/reference_no_test.rb @@ -0,0 +1,13 @@ +require 'test_helper' + +class ReferenceNoTest < ActiveSupport::TestCase + def test_returns_format_regexp + format = /\A\d{2,20}\z/ + assert_equal format, Billing::ReferenceNo::REGEXP + end + + def test_generated_reference_number_conforms_to_format + reference_no = Billing::ReferenceNo.generate + assert_match Billing::ReferenceNo::REGEXP, reference_no + end +end diff --git a/test/models/registrar_test.rb b/test/models/registrar_test.rb index 756dabc83..bc50e4071 100644 --- a/test/models/registrar_test.rb +++ b/test/models/registrar_test.rb @@ -6,7 +6,7 @@ class RegistrarTest < ActiveSupport::TestCase end def test_valid - assert @registrar.valid? + assert @registrar.valid?, proc { @registrar.errors.full_messages } end def test_invalid_without_name @@ -55,8 +55,28 @@ class RegistrarTest < ActiveSupport::TestCase assert_equal 'Main Street, New York, New York, 12345', @registrar.address end - def test_reference_number_generation - @registrar.validate - refute_empty @registrar.reference_no + def test_validates_reference_number_format + @registrar.reference_no = '1' + assert @registrar.invalid? + + @registrar.reference_no = '11' + assert @registrar.valid? + + @registrar.reference_no = '1' * 20 + assert @registrar.valid? + + @registrar.reference_no = '1' * 21 + assert @registrar.invalid? + + @registrar.reference_no = '1a' + assert @registrar.invalid? + end + + def test_disallows_non_unique_reference_numbers + registrars(:bestnames).update!(reference_no: '1234') + + assert_raises ActiveRecord::RecordNotUnique do + registrars(:goodnames).update!(reference_no: '1234') + end end end diff --git a/test/system/admin_area/contact_versions_test.rb b/test/system/admin_area/contact_versions_test.rb index 456fbd5fc..d35d72e36 100644 --- a/test/system/admin_area/contact_versions_test.rb +++ b/test/system/admin_area/contact_versions_test.rb @@ -17,9 +17,9 @@ class ContactVersionsTest < ApplicationSystemTestCase def create_contact_with_history sql = <<-SQL.squish INSERT INTO registrars (id, name, reg_no, email, country_code, code, - accounting_customer_code, language) + accounting_customer_code, language, reference_no) VALUES (75, 'test_registrar', 'test123', 'test@test.com', 'EE', 'TEST123', - 'test123', 'en'); + 'test123', 'en', '1234'); INSERT INTO contacts (id, code, email, auth_info, registrar_id) VALUES (75, 'test_code', 'test@inbox.test', '8b4d462aa04194ca78840a', 75); diff --git a/test/system/admin_area/domain_versions_test.rb b/test/system/admin_area/domain_versions_test.rb index 6d158aaf7..ba07db3f7 100644 --- a/test/system/admin_area/domain_versions_test.rb +++ b/test/system/admin_area/domain_versions_test.rb @@ -17,9 +17,9 @@ class DomainVersionsTest < ApplicationSystemTestCase def create_domain_with_history sql = <<-SQL.squish INSERT INTO registrars (id, name, reg_no, email, country_code, code, - accounting_customer_code, language) + accounting_customer_code, language, reference_no) VALUES (54, 'test_registrar', 'test123', 'test@test.com', 'EE', 'TEST123', - 'test123', 'en'); + 'test123', 'en', '1234'); INSERT INTO contacts (id, code, email, auth_info, registrar_id) VALUES (54, 'test_code', 'test@inbox.test', '8b4d462aa04194ca78840a', 54);