Skip to content

Conversation

@mfo
Copy link
Contributor

@mfo mfo commented Oct 31, 2025

@sentry
Copy link

sentry bot commented Oct 31, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: app/models/champs/siret_champ.rb

Function Unhandled Issue
fetch_external_data NoMethodError: undefined method 'delete' for nil (NoMethodError) Sidekiq/ChampFetc...
Event Count: 1

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.48%. Comparing base (41eca96) to head (84ea4dd).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
app/models/champs/siret_champ.rb 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12273      +/-   ##
==========================================
- Coverage   89.50%   89.48%   -0.02%     
==========================================
  Files        1338     1341       +3     
  Lines       30341    30369      +28     
  Branches     6351     6354       +3     
==========================================
+ Hits        27156    27177      +21     
- Misses       3185     3192       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mfo mfo force-pushed the US/fix-siret-champ-reason branch 2 times, most recently from ab40327 to 4022378 Compare October 31, 2025 16:48
@mfo mfo force-pushed the US/fix-siret-champ-reason branch from 4022378 to 9dabfe2 Compare October 31, 2025 17:01
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

ok, ca va faire du bien a court terme.

J'ai l'impression qu'il va falloir des changements plus conséquents pour avoir un truc lisible, dans des prs à venir.

  1. aujourd'hui, pour qu un job soit rejoué il faut 2 choses : un Failure(retryable: true) et une error qui soit dans TRANSIENT_ERRORS. C'est compliqué. Je propose qu'on modifie le système pour que juste retryable: true suffise 💡
  2. reason doit s'appeler error ou inner_error sinon on ne comprend pas qu'il faut mettre dedans une ... Error 🤦
  3. on fait une ici une mécanique compliquée ou APIEntreprise::API wrap une première fois des erreurs Typhoeus dans des ApiEntrepriseError. Puis ces erreurs sont à nouveau wrappés dans des erreurs Excon 🤯

LeSim added 3 commits November 1, 2025 18:12
Now, returning a Failure(retryable: true) is enough to trigger a retry
- use original error if possible
- use StandardError otherwise
- token error is not retryable
@LeSim LeSim force-pushed the US/fix-siret-champ-reason branch from 02bff97 to 84ea4dd Compare November 1, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants