Merge pull request #1675 from urbanrotnik/ur-sci-3345-export-all-counter-fix

Fix for exports_left, fix for some edge cases [SCI-3345]

app/controllers/teams_controller.rb

@@ -251,15 +251,13 @@ class TeamsController < ApplicationController
 
   def export_projects_modal
     if @exp_projects.present?
-      limit = (ENV['EXPORT_ALL_LIMIT_24_HOURS'] || 3).to_i
-      curr_num = current_user.export_vars['num_of_export_all_last_24_hours']
       if current_user.has_available_exports?
         render json: {
           html: render_to_string(
             partial: 'projects/export/modal.html.erb',
             locals: { num_projects: @exp_projects.size,
-                      limit: limit,
-                      num_of_requests_left: limit - curr_num - 1 }
+                      limit: TeamZipExport.exports_limit,
+                      num_of_requests_left: current_user.exports_left }
           ),
           title: t('projects.export_projects.modal_title')
         }
@@ -267,7 +265,7 @@ class TeamsController < ApplicationController
         render json: {
           html: render_to_string(
             partial: 'projects/export/error.html.erb',
-            locals: { limit: limit }
+            locals: { limit: TeamZipExport.exports_limit }
           ),
           title: t('projects.export_projects.error_title'),
           status: 'error'

app/models/team_zip_export.rb

@@ -44,6 +44,10 @@ class TeamZipExport < ZipExport
   handle_asynchronously :generate_exportable_zip,
                         queue: :team_zip_export
 
+  def self.exports_limit
+    (Rails.application.secrets.export_all_limit_24h || 3).to_i
+  end
+
   private
 
   # Export all functionality

app/models/user.rb

@@ -518,24 +518,32 @@ class User < ApplicationRecord
   def increase_daily_exports_counter!
     range = Time.now.utc.beginning_of_day.to_i..Time.now.utc.end_of_day.to_i
     last_export = export_vars[:last_export_timestamp] || 0
+    export_vars[:num_of_export_all_last_24_hours] ||= 0
 
     if range.cover?(last_export)
       export_vars[:num_of_export_all_last_24_hours] += 1
     else
-      export_vars[:last_export_timestamp] = Time.now.utc.to_i
       export_vars[:num_of_export_all_last_24_hours] = 1
     end
+    export_vars[:last_export_timestamp] = Time.now.utc.to_i
     save
   end
 
   def has_available_exports?
-    limit = (Rails.application.secrets.export_all_limit_24h || 3).to_i
-    last_export = export_vars[:last_export_timestamp]
+    last_export_timestamp = export_vars[:last_export_timestamp] || 0
 
     # limit 0 means unlimited exports
-    return true if limit.zero? || last_export < Time.now.utc.beginning_of_day.to_i
+    return true if TeamZipExport.exports_limit.zero? || last_export_timestamp < Time.now.utc.beginning_of_day.to_i
 
-    export_vars[:num_of_export_all_last_24_hours] < limit
+    exports_left.positive?
+  end
+
+  def exports_left
+    if (export_vars[:last_export_timestamp] || 0) < Time.now.utc.beginning_of_day.to_i
+      return TeamZipExport.exports_limit
+    end
+
+    TeamZipExport.exports_limit - export_vars[:num_of_export_all_last_24_hours]
   end
 
   def global_activity_filter(filters, search_query)

spec/models/user_spec.rb

@@ -259,19 +259,71 @@ describe User, type: :model do
           }.from(2).to(1)
       end
     end
+
+    context 'when num_of_export not exists' do
+      it 'starts count reports with 1' do
+        user.export_vars.delete('num_of_export_all_last_24_hours')
+        user.save
+
+        expect { user.increase_daily_exports_counter! }
+          .to change {
+            user.reload.export_vars['num_of_export_all_last_24_hours']
+          }.from(nil).to(1)
+      end
+    end
   end
 
-  describe 'has_available_exports?' do
+  describe '#has_available_exports?' do
     let(:user) { create :user }
 
-    it 'returns true when user has avaiable export' do
-      expect(user.has_available_exports?).to be_truthy
+    context 'when user have export_vars' do
+      it 'returns true when user has avaiable export' do
+        expect(user.has_available_exports?).to be_truthy
+      end
+
+      it 'returns false when user has no avaiable export' do
+        user.export_vars['num_of_export_all_last_24_hours'] = 3
+
+        expect(user.has_available_exports?).to be_falsey
+      end
     end
 
-    it 'returns false when user has no avaiable export' do
-      user.export_vars['num_of_export_all_last_24_hours'] = 3
+    context 'when user do not have export_vars' do
+      it 'returns false when user has no avaiable export' do
+        user.export_vars = {}
 
-      expect(user.has_available_exports?).to be_falsey
+        expect(user.has_available_exports?).to be_truthy
+      end
+    end
+  end
+
+  describe '#exports_left' do
+    let(:user) { create :user }
+    context 'when user do have export_vars' do
+      it 'returns 3 when user has all exports avaible' do
+        expect(user.exports_left).to be == 3
+      end
+
+      it 'returns 0 when user has no avaiable export' do
+        user.export_vars['num_of_export_all_last_24_hours'] = 3
+
+        expect(user.exports_left).to be == 0
+      end
+
+      it 'returns 3 when user has invalid number from the past' do
+        user.export_vars['num_of_export_all_last_24_hours'] = -4
+        user.export_vars['last_export_timestamp'] = Date.yesterday.to_time.utc.to_i
+
+        expect(user.exports_left).to be == 3
+      end
+    end
+
+    context 'when user do not have export_vars' do
+      it 'returns false when user has no avaiable export' do
+        user.export_vars = {}
+
+        expect(user.exports_left).to be == 3
+      end
     end
   end