Merge pull request #998 from internetee/change-reference-number-algo

Use Estonian reference number format instead of ISO 11649

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

app/models/bank_transaction.rb

@@ -31,7 +31,7 @@ class BankTransaction < ActiveRecord::Base
   end
 
   def registrar
-    @registrar ||= Registrar.find_by(reference_no: reference_no)
+    @registrar ||= Invoice.find_by(reference_no: reference_no)&.buyer
   end
 
 

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

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

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

app/views/admin/registrars/_billing.html.erb

@@ -2,6 +2,7 @@
     <div class="panel-heading">
         <%= t '.header' %>
     </div>
+
     <div class="panel-body">
         <dl class="dl-horizontal">
             <dt><%= Registrar.human_attribute_name :vat_no %></dt>
@@ -15,6 +16,9 @@
 
             <dt><%= Registrar.human_attribute_name :billing_email %></dt>
             <dd><%= registrar.billing_email %></dd>
+
+            <dt><%= Registrar.human_attribute_name :reference_no %></dt>
+            <dd><%= registrar.reference_no %></dd>
         </dl>
     </div>
 </div>

app/views/admin/registrars/_details.html.erb

@@ -11,9 +11,6 @@
             <dt><%= Registrar.human_attribute_name :reg_no %></dt>
             <dd><%= registrar.reg_no %></dd>
 
-            <dt><%= Registrar.human_attribute_name :reference_no %></dt>
-            <dd><%= registrar.reference_no %></dd>
-
             <dt><%= Registrar.human_attribute_name :code %></dt>
             <dd><%= registrar.code %></dd>
 

app/views/admin/registrars/form/_billing.html.erb

@@ -47,6 +47,24 @@
                         <%= f.email_field :billing_email, class: 'form-control' %>
                     </div>
                 </div>
+
+                <div class="form-group">
+                    <% if f.object.new_record? %>
+                        <div class="col-md-offset-4 col-md-8">
+                            <%= t '.no_reference_no_hint' %>
+                        </div>
+                    <% else %>
+                        <div class="col-md-4 control-label">
+                            <%= f.label :reference_no %>
+                        </div>
+
+                        <div class="col-md-7">
+                            <%= f.text_field :reference_no, disabled: true,
+                                             title: t('.disabled_reference_no_hint'),
+                                             class: 'form-control' %>
+                        </div>
+                    <% end %>
+                </div>
             </div>
         </div>
     </div>

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

config/locales/en.yml

@@ -746,3 +746,4 @@ en:
     vat_rate: VAT rate
     ipv4: IPv4
     ipv6: IPv6
+    reference_no: Reference number

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

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');
 

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

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
 

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)

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|

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'
         })
       ])

spec/models/bank_transaction_spec.rb

@@ -1,68 +0,0 @@
-require 'rails_helper'
-
-describe BankTransaction do
-  context 'with invalid attribute' do
-    before :all do
-      @bank_transaction = BankTransaction.new
-    end
-
-    it 'should not be valid' do
-      @bank_transaction.valid?
-      @bank_transaction.errors.full_messages.should match_array([
-      ])
-    end
-
-    it 'should not have any versions' do
-      @bank_transaction.versions.should == []
-    end
-  end
-
-  context 'with valid attributes' do
-    before :all do
-      @bank_transaction = create(:bank_transaction)
-      create(:registrar)
-    end
-
-    it 'should be valid' do
-      @bank_transaction.valid?
-      @bank_transaction.errors.full_messages.should match_array([])
-    end
-
-    it 'should be valid twice' do
-      @bank_transaction = create(:bank_statement)
-      @bank_transaction.valid?
-      @bank_transaction.errors.full_messages.should match_array([])
-    end
-
-    it 'should not bind transaction with mismatching sums' do
-      r = create(:registrar, reference_no: 'RF7086666663')
-      invoice = r.issue_prepayment_invoice(200, 'add some money')
-
-      bt = create(:bank_transaction, { sum: 10 })
-      bt.bind_invoice(invoice.number)
-
-      bt.errors.full_messages.should match_array(["Invoice and transaction sums do not match"])
-    end
-
-    it 'should not bind transaction with cancelled invoice' do
-      r = create(:registrar, reference_no: 'RF7086666663')
-      invoice = r.issue_prepayment_invoice(200, 'add some money')
-      invoice.cancel
-
-      bt = create(:bank_transaction, { sum: 240 })
-      bt.bind_invoice(invoice.number)
-
-      bt.errors.full_messages.should match_array(["Cannot bind cancelled invoice"])
-    end
-
-    it 'should have one version' do
-      with_versioning do
-        @bank_transaction.versions.should == []
-        @bank_transaction.bank_reference = '123'
-        @bank_transaction.save
-        @bank_transaction.errors.full_messages.should match_array([])
-        @bank_transaction.versions.size.should == 1
-      end
-    end
-  end
-end

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

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
@@ -29,5 +31,7 @@ not_in_use:
   code: any
   email: any@example.com
   country_code: US
+  vat_no: any
   accounting_customer_code: any
   language: en
+  reference_no: 39

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

test/models/bank_transaction_test.rb

@@ -0,0 +1,51 @@
+require 'test_helper'
+
+class BankTransactionTest < ActiveSupport::TestCase
+  def test_matches_against_invoice_reference_number
+    invoices(:valid).update!(number: '2222', total: 10, reference_no: '1111')
+    transaction = BankTransaction.new(description: 'invoice #2222', sum: 10, reference_no: '1111')
+
+    assert_difference 'AccountActivity.count' do
+      transaction.autobind_invoice
+    end
+  end
+
+  def test_does_not_match_against_registrar_reference_number
+    registrars(:bestnames).update!(reference_no: '1111')
+    transaction = BankTransaction.new(description: 'invoice #2222', sum: 10, reference_no: '1111')
+
+    assert_no_difference 'AccountActivity.count' do
+      transaction.autobind_invoice
+    end
+  end
+
+  def test_underpayment_is_not_matched_with_invoice
+    invoices(:valid).update!(number: '2222', total: 10)
+    transaction = BankTransaction.new(sum: 9)
+
+    assert_no_difference 'AccountActivity.count' do
+      transaction.bind_invoice('2222')
+    end
+    assert transaction.errors.full_messages.include?('Invoice and transaction sums do not match')
+  end
+
+  def test_overpayment_is_not_matched_with_invoice
+    invoices(:valid).update!(number: '2222', total: 10)
+    transaction = BankTransaction.new(sum: 11)
+
+    assert_no_difference 'AccountActivity.count' do
+      transaction.bind_invoice('2222')
+    end
+    assert transaction.errors.full_messages.include?('Invoice and transaction sums do not match')
+  end
+
+  def test_cancelled_invoice_is_not_matched
+    invoices(:valid).update!(number: '2222', total: 10, cancelled_at: '2010-07-05')
+    transaction = BankTransaction.new(sum: 10)
+
+    assert_no_difference 'AccountActivity.count' do
+      transaction.bind_invoice('2222')
+    end
+    assert transaction.errors.full_messages.include?('Cannot bind cancelled invoice')
+  end
+end

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

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

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

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);

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);