diff --git a/app/controllers/providers_controller.rb b/app/controllers/providers_controller.rb index f8a6ba5d..0206e513 100644 --- a/app/controllers/providers_controller.rb +++ b/app/controllers/providers_controller.rb @@ -39,10 +39,12 @@ def update end def destroy - @provider.destroy! - respond_to do |format| - format.html { redirect_to providers_path, status: :see_other, notice: "Provider was successfully destroyed." } + if @provider.destroy + format.html { redirect_to providers_path, status: :see_other, notice: "Provider was successfully destroyed." } + else + format.html { redirect_to providers_path, status: :see_other, alert: @provider.errors.full_messages.to_sentence } + end end end diff --git a/app/jobs/file_upload_job.rb b/app/jobs/file_upload_job.rb index 028ee74f..3692e541 100644 --- a/app/jobs/file_upload_job.rb +++ b/app/jobs/file_upload_job.rb @@ -3,14 +3,8 @@ class FileUploadJob < ApplicationJob # or use a more specific key to avoid blocking all jobs for a language limits_concurrency to: 3, key: ->(_language_id, content_id, _content_type) { "hard-limit" } - retry_on AzureFileShares::Errors::ApiError, wait: :exponentially_longer, attempts: 3 - retry_on Timeout::Error, wait: :exponentially_longer, attempts: 2 - - discard_on StandardError do |job, error| - Rails.logger.error "FileUploadJob failed permanently: #{error.message}" - Rails.logger.error "Job arguments: #{job.arguments}" - Rails.logger.error "Suggestion: Check provider names for invalid characters if Azure API errors" - end + retry_on AzureFileShares::Errors::ApiError, wait: :polynomially_longer, attempts: 3 + retry_on Timeout::Error, wait: :polynomially_longer, attempts: 2 def perform(language_id, content_id, content_type, share = ENV["AZURE_STORAGE_SHARE_NAME"]) @language = Language.find(language_id) diff --git a/app/models/provider.rb b/app/models/provider.rb index b5c65806..d4c8e97e 100644 --- a/app/models/provider.rb +++ b/app/models/provider.rb @@ -20,7 +20,7 @@ class Provider < ApplicationRecord has_many :regions, through: :branches has_many :contributors has_many :users, through: :contributors - has_many :topics + has_many :topics, dependent: :restrict_with_error has_many :beacon_providers, dependent: :destroy has_many :beacons, through: :beacon_providers diff --git a/spec/jobs/file_upload_job_spec.rb b/spec/jobs/file_upload_job_spec.rb index 198446d8..70208856 100644 --- a/spec/jobs/file_upload_job_spec.rb +++ b/spec/jobs/file_upload_job_spec.rb @@ -24,6 +24,23 @@ end end + context "when generating the file fails" do + before { allow(CsvGenerator::Topics).to receive(:new).and_raise(NoMethodError, "undefined method 'name' for nil") } + + it "raises so the job is recorded as failed" do + expect { described_class.perform_now(language.id, "topics", "file") }.to raise_error(NoMethodError) + end + end + + context "when Azure returns an API error" do + before { allow(FileWorker).to receive(:new).and_raise(AzureFileShares::Errors::ApiError.new("ServerBusy")) } + + it "retries the job" do + expect { described_class.perform_now(language.id, "topics", "file") } + .to have_enqueued_job(described_class).with(language.id, "topics", "file") + end + end + context "when provider specific file" do let(:provider) { create(:provider, name: "Test Provider") } diff --git a/spec/requests/providers_spec.rb b/spec/requests/providers_spec.rb index 69d0cbc2..32f59931 100644 --- a/spec/requests/providers_spec.rb +++ b/spec/requests/providers_spec.rb @@ -139,5 +139,19 @@ expect(response).to redirect_to(providers_url) end + + context "when the provider has topics" do + it "keeps the provider and explains why" do + provider = Provider.create! valid_attributes + create(:topic, provider:) + + expect { + delete provider_url(provider) + }.not_to change(Provider, :count) + + expect(response).to redirect_to(providers_url) + expect(flash[:alert]).to match(/topics/i) + end + end end end