Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions app/components/dsfr/input_status_message_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,16 @@ def compute_statut_message
case @champ.type_de_champ.type_champ
when TypeDeChamp.type_champs[:siret]
# TODO: use fetched? after T20251029backfillChampSiretExternalStateTask
if @champ.etablissement && @champ.etablissement.entreprise_capital_social.present?
if @champ.etablissement&.as_degraded_mode?
# When the champ feeds a condition, submission is blocked until the
# backfill completes: no "you can continue" message in that case.
if !@champ.external_data_required_for_conditions?
{ state: :info, text: t('.siret.technical_error') }
end
elsif @champ.etablissement && @champ.etablissement.entreprise_capital_social.present?
{ state: :info, text: t('.siret.fetched_with_capital', raison_sociale_or_name: displayable_raison_sociale_or_name(@champ.etablissement), forme_juridique: @champ.etablissement.entreprise_forme_juridique, capital_sociale: pretty_currency(@champ.etablissement.entreprise_capital_social)) }
elsif @champ.etablissement && @champ.etablissement.entreprise_capital_social.blank?
{ state: :info, text: t('.siret.fetched', raison_sociale_or_name: displayable_raison_sociale_or_name(@champ.etablissement), forme_juridique: @champ.etablissement.entreprise_forme_juridique) }
elsif @champ.external_error?
{ state: :warning, text: t('.siret.error', value: pretty_siret(@champ.external_id)) }
elsif @champ.pending?
{ state: :info, text: t('.siret.pending', value: pretty_siret(@champ.external_id)) }
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ en:
pending: "Search in progress for RNA %{value}…"
success: "This RNA number is linked to: %{title}, %{address}."
siret:
technical_error: "Automatic verification is temporarily unavailable. You can continue submitting; the information will be retrieved as soon as possible."
pending: "Search in progress for SIRET %{value}…"
fetched_with_capital: "%{raison_sociale_or_name} %{forme_juridique} with a share capital of %{capital_sociale}"
fetched: "%{raison_sociale_or_name} %{forme_juridique}"
error: "An error occurred while retrieving SIRET information."
referentiel:
not_configured: "This field has not been configured by the administrator yet."
fetching: "Search in progress."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ fr:
pending: "Recherche en cours pour le RNA %{value}…"
success: "Ce RNA correspond à : %{title}, %{address}"
siret:
technical_error: "La vérification automatique est temporairement indisponible. Vous pouvez poursuivre le dépôt ; les informations seront récupérées dès que possible."
pending: "Recherche en cours pour le SIRET %{value}…"
fetched_with_capital: "%{raison_sociale_or_name} %{forme_juridique} au capital social de %{capital_sociale}"
fetched: "%{raison_sociale_or_name} %{forme_juridique}"
error: "Une erreur est survenue lors de la récupération des informations SIRET."
referentiel:
not_configured: "Ce champ n’est pas encore configuré par l’administrateur."
fetching: "Recherche en cours."
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/champ_fetch_external_data_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class ChampFetchExternalDataJob < ApplicationJob

retry_on RetryableFetchError, attempts: 3, wait: :polynomially_longer do |job, err|
champ = job.arguments.first
champ.external_data_error!
champ.handle_exhausted_external_data_retries!
Comment thread
E-L-T marked this conversation as resolved.

# Don't raise, otherwise it will pop forever as "working" queue without doing anything
Sentry.capture_exception(err.cause)
Expand Down
6 changes: 6 additions & 0 deletions app/models/champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ class Champ < ApplicationRecord
# look like it was edited. Revert blank-equivalent JSON columns before saving.
before_save :nullify_blank_json_columns

# Overridable extension point. Override to true in a champ subclass to opt into
# permissive external data validation (see ExternalDataChampValidator).
def permissive_external_data_validation?
false
end

def type_de_champ
@type_de_champ ||= dossier.revision
.types_de_champ
Expand Down
49 changes: 45 additions & 4 deletions app/models/champs/siret_champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ def has_async_external_data?
true
end

# A condition based on one of this champ's columns (value_json) cannot be
# evaluated until the data is fetched: restore the blocking validation.
def permissive_external_data_validation?
!external_data_required_for_conditions?
end

def external_data_required_for_conditions?
dependent_conditions?
Comment thread
E-L-T marked this conversation as resolved.
end

def focusable_input_id(attribute = :value)
[input_id, :value].compact.join('-')
end
Expand All @@ -34,17 +44,33 @@ def ready_for_external_call?

def fetch_external_data
case APIEntrepriseService.create_etablissement_with_fallback(self, external_id.delete(" "), dossier.user&.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on creait déjà l'etb en mode degradé quand api down ou 429, j'ai donc pas trop compris ces changements (on gere les 401/403/409/422/451...). Or dans ces cas la, je ne suis pas certains qu'on souhaite passer le dossier (a la limite, le 409 arrive quand on requete le meme objet au mm moment, limite faudrait remonter le cas dans APIEntrepriseService.create_etablissement_with_fallback).

Propale pour simplifier :
0001-simplify-siret_champ-remove-redundant-degraded-mode-.patch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effectivement, c'est peut-être pas une bonne idée de laisser passer sans distinction tout ce qui est 401/403/409/422/451...
Peut-être qu'il faut traiter différemment selon les erreurs.
Vu que le but de la fonctionnalité est de diminuer la gêne pour les usagers dans des cas où ça peut être rattrapé ensuite, je verrais bien :

  • 401 (Unauthorized) / 403 (Forbidden) / 409 (collision de requête) -> dépôt possible avec degraded_mode (par ex. via create_etablissement_with_fallback) + sentry -> rattrapé par les jobs mais surtout par le cron Cron::BackfillSiretDegradedModeJob une fois que le problème est réparé côté administration.
  • 451 (indispo pour raison légale) / 422 (requête invalide) : blocage du dépôt + sentry

in Success(etablissement) if etablissement.as_degraded_mode?
Failure(retryable: true, error: StandardError.new("API Entreprise: degraded mode"), code: 503)
in Success(etablissement)
# A degraded-mode etablissement (API Entreprise down) is accepted as is:
# the user is not blocked, background jobs backfill the data later and
# Dossier#any_etablissement_as_degraded_mode? keeps instructeurs from
# accepting the dossier in the meantime.
Success(etablissement:, value: external_id)
in Failure(type: :not_found, **)
Failure(retryable: false, error: StandardError.new('NotFound'), code: 404)
in Failure(type:, code:, retryable:, **)
Failure(retryable:, error: StandardError.new("API Entreprise: #{type}"), code:)
in Failure(type:, code:, retryable: true, **)
Failure(retryable: true, error: StandardError.new("API Entreprise: #{type}"), code:)
in Failure(type:, code:, **)
# Any other technical error also converges to degraded mode: only a 404
# (nonexistent SIRET) leaves the champ in external_error.
Sentry.capture_message("SiretChamp: degraded mode fallback after API Entreprise error", extra: { champ: id, type:, code: })
Success(etablissement: create_degraded_etablissement, value: external_id)
end
end

# Retries exhausted (API Entreprise flapping while the health checker still
# reports it up): converge to a degraded etablissement instead of
# external_error, so that Dossier#any_etablissement_as_degraded_mode? blocks
# acceptance and the backfill jobs recover the data later.
def handle_exhausted_external_data_retries!
update_external_data!(etablissement: create_degraded_etablissement, value: external_id)
external_data_fetched!
end

def search_terms
etablissement.present? ? etablissement.search_terms : [value]
end
Expand All @@ -57,14 +83,29 @@ def save_additional_job_exception(exception, code)

private

def create_degraded_etablissement
APIEntrepriseService.create_etablissement_as_degraded_mode(self, external_id.delete(" "), dossier.user&.id)
end

# We want to validate if SIRET really exists
# It's valid when an etablissement have been created in turbo with SIRET controller
# When API Entreprise is down, user won't be stuck because
# SIRET controller creates an etablissement in degraded mode
def validate_etablissement
return if external_id.blank?

# A degraded-mode etablissement carries no data yet: block until the
# backfill jobs complete it, so the condition can be evaluated.
if external_data_required_for_conditions? && etablissement&.as_degraded_mode?
errors.add(:external_id, :api_response_pending)
return
end

return if etablissement.present?
return if pending?
# Blocking on external_error is ExternalDataChampValidator's job;
# this validation only covers the synchronous idle/format-check path.
return if external_error?

validator = ActiveModel::Validations::SiretValidator.new(attributes: { value: true })

Expand Down
9 changes: 8 additions & 1 deletion app/models/concerns/champ_external_data_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ module ChampExternalDataConcern
end

event :external_data_fetched do
transitions from: [:fetching], to: :fetched
# waiting_for_job: a champ can converge to fetched from the retry_on
# exhaustion block (see #handle_exhausted_external_data_retries!)
transitions from: [:fetching, :waiting_for_job], to: :fetched

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis un peu embété d'avoir un état fetched pour un champ ... qui ne l'ai pas. De plus, on va chercher a étendre cette mécanique à d'autres champs connectés.

Du coup, je te proposerai bien, dans la machine à état, d'introduire un nouvelle état degraded et une nouvelle transition exhausted_retry! qui irait de waiting_for_job -> degraded

end

event :external_data_error do
Expand All @@ -72,6 +74,11 @@ def has_async_external_data? = false

def external_data_needed_for_validation? = has_async_external_data?

# Called by ChampFetchExternalDataJob when all retry attempts are exhausted.
def handle_exhausted_external_data_retries!
external_data_error!
end

private

def ready_for_external_call? = external_id.present?
Expand Down
3 changes: 2 additions & 1 deletion app/models/procedure_revision.rb
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ def parent_of(tdc)
def dependent_conditions(tdc)
stable_id = tdc.stable_id

(tdc.public? ? types_de_champ_public : types_de_champ_private).filter do |other_tdc|
tdcs = tdc.public? ? types_de_champ_public + types_de_champ_private : types_de_champ_private
tdcs.filter do |other_tdc|
next if !other_tdc.condition?

other_tdc.condition.sources.include?(stable_id)
Expand Down
5 changes: 4 additions & 1 deletion app/models/types_de_champ/siret_type_de_champ.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ def estimated_fill_duration(revision)
FILL_DURATION_MEDIUM
end

def champ_blank_or_invalid?(champ) = Siret.new(siret: champ.value).invalid?
# A syntactically valid external_id is enough to satisfy the mandatory
# check while the async fetch is pending or failed non-blockingly:
# the actual blocking decision belongs to ExternalDataChampValidator.
def champ_blank_or_invalid?(champ) = Siret.new(siret: champ.external_id).invalid?

def columns(procedure_id:, displayable: true, prefix: nil)
super
Expand Down
5 changes: 3 additions & 2 deletions app/services/ocr_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,17 @@ def self.fetch_ban_address(query)
def self.ocr_url = ENV.fetch("OCR_SERVICE_URL", nil)
def self.document_ia_url = ENV.fetch("DOCUMENT_IA_URL", nil)

# code: is always present: ChampExternalDataConcern#handle_result pattern-matches on it
def self.not_configured(message)
Failure(retryable: false, error: StandardError.new("#{message} not configured"))
Failure(retryable: false, error: StandardError.new("#{message} not configured"), code: nil)
end

def self.to_not_retryable_failure(data)
case data
in code:, error:
Failure(retryable: false, error:, code:)
else
Failure(retryable: false, error: StandardError.new('Unknown error'))
Failure(retryable: false, error: StandardError.new('Unknown error'), code: nil)
end
end
end
29 changes: 23 additions & 6 deletions app/validators/external_data_champ_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,39 @@
class ExternalDataChampValidator < ActiveModel::Validator
# Required checks are delegated to check_mandatory_and_visible_champs_public.
def validate(record)
if record.permissive_external_data_validation?
permissive_validate(record)
else
strict_validate(record)
end
end

private

# Historical behavior: any pending or external_error blocks submission.
def strict_validate(record)
if record.pending?
# User filled the field, but background job is still running.
record.errors.add(:external_id, :api_response_pending)
elsif record.external_error?
# User filled the field, but background job failed.
record.errors.add(:external_id, error_key_for_api_response_code(record))
record.errors.add(:external_id, error_key_for(record, record.fetch_external_data_exceptions&.last))
end
end

private
# Permissive behavior: only a not-found result (HTTP 404) blocks submission.
# Pending and technical errors let the user submit.
# Exceptions accumulate across retries: only the most recent one is conclusive.
def permissive_validate(record)
return unless record.external_data_not_found?

record.errors.add(:value, error_key_for(record, record.fetch_external_data_exceptions.last))
end

def error_key_for_api_response_code(record)
first_exception = record.fetch_external_data_exceptions&.first
return :code_unknown if first_exception.nil?
def error_key_for(record, exception)
return :code_unknown if exception.nil?

http_status = first_exception.code
http_status = exception.code
error_key = :"code_#{http_status}"

if http_status && translation_exists_for?(error_key, record)
Expand Down
36 changes: 36 additions & 0 deletions spec/components/input_status_message_component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,42 @@
end
end

context "when external fetch failed with a technical error (non-blocking)" do
before do
champ.update!(etablissement: nil)
champ.update_columns(
external_id: '80879023200025',
external_state: 'external_error',
fetch_external_data_exceptions: [ExternalDataException.new(error: 'API down', code: 503)]
)
end

it "does not render a warning message (submission is not blocked)" do
expect(subject).not_to have_css(".fr-message--warning")
end
end

context "when the etablissement is in degraded mode (API Entreprise was down at fetch time)" do
before do
champ.update!(value: '44011762001530', external_id: '44011762001530', external_state: 'fetched', etablissement: Etablissement.new(siret: '44011762001530'))
end

it "renders an info message explaining verification is postponed" do
expect(subject).to have_css(".fr-message--info", text: "La vérification automatique est temporairement indisponible")
expect(subject).not_to have_text("Non-diffusible")
end

context "when the champ feeds a condition (submission is blocked until backfill)" do
before do
allow(champ).to receive(:external_data_required_for_conditions?).and_return(true)
end

it "does not render the 'you can continue' message" do
expect(subject).not_to have_css(".fr-message--info")
end
end
end

context "when etablissement is non-diffusible (diffusable_commercialement: false)" do
before do
etablissement = create(:etablissement, :non_diffusable, entreprise_raison_sociale: "SECRET EI", entreprise_forme_juridique: "Entrepreneur individuel")
Expand Down
25 changes: 25 additions & 0 deletions spec/jobs/champ_fetch_external_data_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,30 @@
expect(champ.fetch_external_data_exceptions.size).to eq(3)
end
end

context 'when retries are exhausted on a siret champ' do
let(:procedure) { create(:procedure, :published, types_de_champ_public: [{ type: :siret }]) }
let(:dossier) { create(:dossier, procedure:) }
let(:external_id) { '41816609600051' }

before do
champ.update_columns(external_id:, external_state: 'waiting_for_job')
allow_any_instance_of(Champs::SiretChamp).to receive(:fetch_external_data).and_return(failure)
end

it 'converges to a degraded etablissement instead of external_error' do
described_class.perform_later(champ, external_id)

3.times do
perform_enqueued_jobs(only: ChampFetchExternalDataJob)
rescue StandardError
end

champ.reload

expect(champ).to be_fetched
expect(champ.etablissement).to be_as_degraded_mode
end
end
end
end
Loading
Loading