Refactoring: Clean up Ticket::Number & Ticket::Number::Increment classes

app/models/ticket/number.rb

@@ -16,13 +16,11 @@ returns
 =end
 
   def self.generate
-
-    # generate number
     49_999.times do
       number = adapter.generate
-      ticket = Ticket.find_by(number: number)
+      return number if !Ticket.exists?(number: number)
-      return number if !ticket
     end
+
     raise "Can't generate new ticket number!"
   end
 
@@ -42,12 +40,9 @@ returns
     adapter.check(string)
   end
 
-  def self.adapter
-
   # load backend based on config
-    adapter_name = Setting.get('ticket_number')
+  def self.adapter
-    raise 'Missing ticket_number setting option' if !adapter_name
+    Setting.get('ticket_number')&.constantize ||
-
+      raise('Missing ticket_number setting option')
-    adapter_name.constantize
   end
 end

app/models/ticket/number/base.rb

@@ -0,0 +1,27 @@
+# Copyright (C) 2012-2019 Zammad Foundation, http://zammad-foundation.org/
+module Ticket::Number::Base
+  extend self
+
+  private
+
+  # The algorithm to calculate the checksum is derived from the one
+  # Deutsche Bundesbahn (german railway company) uses for calculation
+  # of the check digit of their vehikel numbering.
+  # The checksum is calculated by alternately multiplying the digits
+  # with 1 and 2 and adding the resulsts from left to right of the
+  # vehikel number. The modulus to 10 of this sum is substracted from
+  # 10. See: http://www.pruefziffernberechnung.de/F/Fahrzeugnummer.shtml
+  # (german)
+  def checksum(number)
+    chksum = 0
+    mult   = 1
+
+    number.to_s.split('').map(&:to_i).each do |digit|
+      chksum += digit * mult
+      mult    = (mult % 3) + 1
+    end
+
+    chksum = 10 - (chksum % 10)
+    chksum.to_s[0]
+  end
+end

app/models/ticket/number/date.rb

@@ -1,78 +1,30 @@
 # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
 module Ticket::Number::Date
-  module_function
+  extend Ticket::Number::Base
 
-  def generate
+  def self.generate
+    date = Time.zone.now.strftime('%F')
 
-    # get config
+    counter = Ticket::Counter.create_with(content: '0')
-    config = Setting.get('ticket_number_date')
+                             .find_or_create_by(generator: 'Date')
 
-    t = Time.zone.now
+    counter.with_lock do
-    date = t.strftime('%Y-%m-%d')
+      counter_increment = if counter.content.end_with?(date)
-
+                            counter.content.split(';').first.to_i.next
-    # read counter
-    counter_increment = nil
-    Ticket::Counter.transaction do
-      counter = Ticket::Counter.where(generator: 'Date').lock(true).first
-      if !counter
-        counter = Ticket::Counter.new(generator: 'Date', content: '0')
-      end
-
-      # increase counter
-      counter_increment, date_file = counter.content.to_s.split(';')
-      counter_increment = if date_file == date
-                            counter_increment.to_i + 1
                           else
                             1
                           end
 
-      # store new counter value
+      counter.update(content: "#{counter_increment};#{date}")
-      counter.content = counter_increment.to_s + ';' + date
-      counter.save
     end
 
-    system_id = Setting.get('system_id') || ''
+    number  = date.delete('-') + Setting.get('system_id').to_s + format('%04d', counter.content.split(';').first)
-    number = t.strftime('%Y%m%d') + system_id.to_s + format('%04d', counter_increment)
+    number += checksum(number) if config[:checksum]
 
-    # calculate a checksum
-    # The algorithm to calculate the checksum is derived from the one
-    # Deutsche Bundesbahn (german railway company) uses for calculation
-    # of the check digit of their vehikel numbering.
-    # The checksum is calculated by alternately multiplying the digits
-    # with 1 and 2 and adding the resulsts from left to right of the
-    # vehikel number. The modulus to 10 of this sum is substracted from
-    # 10. See: http://www.pruefziffernberechnung.de/F/Fahrzeugnummer.shtml
-    # (german)
-
-    # fix for https://github.com/zammad/zammad/issues/413 - can be removed later
-    if config.class == FalseClass || config.class == TrueClass
-      config = {
-        checksum: config
-      }
-    end
-
-    if config[:checksum]
-      chksum = 0
-      mult   = 1
-      (1..number.length).each do |i|
-        digit = number.to_s[i, 1]
-        chksum = chksum + (mult * digit.to_i)
-        mult += 1
-        if mult == 3
-          mult = 1
-        end
-      end
-      chksum %= 10
-      chksum = 10 - chksum
-      if chksum == 10
-        chksum = 1
-      end
-      number += chksum.to_s
-    end
     number
   end
 
-  def check(string)
+  def self.check(string)
     return if string.blank?
 
     # get config
@@ -100,4 +52,13 @@ module Ticket::Number::Date
     end
     ticket
   end
+
+  def self.config
+    config = Setting.get('ticket_number_date')
+    return config if !config.in?([true, false])
+
+    # fix for https://github.com/zammad/zammad/issues/413 - can be removed later
+    { checksum: config }
+  end
+  private_class_method :config
 end

app/models/ticket/number/increment.rb

@@ -1,87 +1,40 @@
 # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
 
 module Ticket::Number::Increment
-  module_function
+  extend Ticket::Number::Base
 
-  def generate
+  def self.generate
+    counter = Ticket::Counter.create_with(content: '0')
+                             .find_or_create_by(generator: 'Increment')
 
-    # get config
+    counter.with_lock do
-    config = Setting.get('ticket_number_increment')
+      counter.update(content: counter.content.to_i.next.to_s)
-
-    # read counter
-    min_digs = config[:min_size] || 4
-    counter_increment = nil
-    Ticket::Counter.transaction do
-      counter = Ticket::Counter.where(generator: 'Increment').lock(true).first
-      if !counter
-        counter = Ticket::Counter.new(generator: 'Increment', content: '0')
-      end
-      counter_increment = counter.content.to_i
-
-      # increase counter
-      counter_increment += 1
-
-      # store new counter value
-      counter.content = counter_increment.to_s
-      counter.save
     end
 
     # fill up number counter
-    if config[:checksum]
+    head = (Setting.get('system_id') || 1).to_s
-      min_digs = min_digs.to_i - 1
+    tail = counter.content
-    end
-    fillup = Setting.get('system_id').to_s || '1'
-    99.times do
 
-      next if (fillup.length.to_i + counter_increment.to_s.length.to_i) >= min_digs.to_i
+    padding_length  = (config[:min_size] || 4).to_i - head.length - tail.length
+    padding_length -= 1 if config[:checksum]
+    padding_length  = 0 if padding_length.negative?
+    padding_length  = 99 if padding_length > 99
 
-      fillup = fillup + '0'
+    number  = head + ('0' * padding_length) + tail
-    end
+    number += checksum(number) if config[:checksum]
-    number = fillup.to_s + counter_increment.to_s
 
-    # calculate a checksum
-    # The algorithm to calculate the checksum is derived from the one
-    # Deutsche Bundesbahn (german railway company) uses for calculation
-    # of the check digit of their vehikel numbering.
-    # The checksum is calculated by alternately multiplying the digits
-    # with 1 and 2 and adding the resulsts from left to right of the
-    # vehikel number. The modulus to 10 of this sum is substracted from
-    # 10. See: http://www.pruefziffernberechnung.de/F/Fahrzeugnummer.shtml
-    # (german)
-    if config[:checksum]
-      chksum = 0
-      mult   = 1
-      (1..number.length).each do |i|
-        digit = number.to_s[i, 1]
-        chksum = chksum + (mult * digit.to_i)
-        mult += 1
-        if mult == 3
-          mult = 1
-        end
-      end
-      chksum %= 10
-      chksum = 10 - chksum
-      if chksum == 10
-        chksum = 1
-      end
-      number += chksum.to_s
-    end
     number
   end
 
-  def check(string)
+  def self.check(string)
     return if string.blank?
 
     # get config
-    system_id           = Setting.get('system_id') || ''
+    system_id           = Setting.get('ticket_number_ignore_system_id') ? '' : Setting.get('system_id').to_s
     ticket_hook         = Setting.get('ticket_hook')
-    ticket_hook_divider = Setting.get('ticket_hook_divider') || ''
+    ticket_hook_divider = Setting.get('ticket_hook_divider').to_s
     ticket              = nil
 
-    if Setting.get('ticket_number_ignore_system_id') == true
-      system_id = ''
-    end
-
     # probe format
     # NOTE: we use `(?<=\W|^)` at the start of the regular expressions below
     # because `\b` fails when ticket_hook begins with a non-word character (like '#')
@@ -89,12 +42,19 @@ module Ticket::Number::Increment
       ticket = Ticket.find_by(number: $1)
       break if ticket
     end
+
     if !ticket
       string.scan(/(?<=\W|^)#{Regexp.quote(ticket_hook)}\s{0,2}(#{system_id}\d{2,48})\b/i) do
         ticket = Ticket.find_by(number: $1)
         break if ticket
       end
     end
+
     ticket
   end
+
+  def self.config
+    Setting.get('ticket_number_increment')
+  end
+  private_class_method :config
 end

spec/models/ticket/number/date_spec.rb

@@ -6,6 +6,11 @@ RSpec.describe Ticket::Number::Date do
 
     before { travel_to(Time.zone.parse('1955-11-05')) }
 
+    it 'updates the "Date" Ticket::Counter' do
+      expect { number }
+        .to change { Ticket::Counter.find_by(generator: 'Date')&.content }
+    end
+
     context 'with a "ticket_number_date" setting with checksum: false (default)' do
       context 'and a single-digit system_id' do
         before { Setting.set('system_id', 1) }

spec/models/ticket/number/increment_spec.rb

@@ -6,6 +6,11 @@ RSpec.describe Ticket::Number::Increment do
     let(:system_id) { Setting.get('system_id') }
     let(:ticket_count) { Ticket::Counter.find_by(generator: 'Increment').content }
 
+    it 'updates the "Increment" Ticket::Counter' do
+      expect { number }
+        .to change { Ticket::Counter.find_by(generator: 'Increment').content }
+    end
+
     context 'with a "ticket_number_increment" setting with...' do
       context 'min_size: 5' do
         before { Setting.set('ticket_number_increment', { min_size: 5 }) }