Walkthrough

Adds robust remote-attachment error handling to assemblies and participatory_processes importers: validates URLs, replaces existence checks with a remote_file_error flow, wraps downloads in rescue blocks, normalizes error messages via format_error, and adds attachment_title for display; translations and tests updated accordingly.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• Show a warning message when the space images URL have errors #16026: Modifies the same importer codepaths to improve remote image/attachment error handling and warning messages.

Suggested labels

module: core

Suggested reviewers

• alecslupu

Poem

🐰 I nibble URLs with careful paws,
I catch the timeout, note the cause,
I name the file so you can see,
“(404 Not Found)” — I whisper gently.
Hoppity-hop, imports safer now! 🥕

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• 📝 Generate docstrings

decidim-assemblies/app/serializers/decidim/assemblies/assembly_importer.rb (2)

147-165: Consider adding explicit timeouts to the HTTP connection.

Net::HTTP.new defaults are 60 seconds for open/read timeouts, but the connection has no explicit timeout set. If the remote server is slow to respond to the HEAD request, this will block the import worker for up to 120 seconds per attachment. Given that imports can contain many attachments, this could cause significant delays or thread starvation.

Proposed fix

         http_connection = Net::HTTP.new(url.host, url.port)
         http_connection.use_ssl = true if url.scheme == "https"
+        http_connection.open_timeout = 5
+        http_connection.read_timeout = 5
         http_connection.start do |http|

---

71-122: Significant code duplication with ParticipatoryProcessImporter.

The methods remote_file_error, attachment_title, format_error, and the core logic of import_folders_and_attachments are virtually identical between AssemblyImporter and ParticipatoryProcessImporter. This makes maintenance error-prone — any future bug fix or enhancement must be applied to both files.

Consider extracting these shared methods into a common concern (e.g., Decidim::Importers::RemoteFileConcern) that both importers include.

Also applies to: 147-202

decidim-participatory_processes/app/serializers/decidim/participatory_processes/participatory_process_importer.rb (1)

174-192: Same missing timeout concern as in AssemblyImporter.

This remote_file_error implementation is identical to the one in AssemblyImporter and has the same lack of explicit timeouts on the HTTP connection. See the comment on the assembly importer for details.

Proposed fix

         http_connection = Net::HTTP.new(url.host, url.port)
         http_connection.use_ssl = true if url.scheme == "https"
+        http_connection.open_timeout = 5
+        http_connection.read_timeout = 5
         http_connection.start do |http|

decidim-assemblies/spec/system/admin/admin_imports_assembly_spec.rb (1)

97-313: Significant test setup duplication across contexts.

The json_data / json_file / uploaded_file let-blocks and the before block pattern (fill_in form, attach file, click Import) are repeated verbatim across five contexts (lines 97, 150, 203, 258, 315). Consider extracting these into shared let declarations and a shared before block at a higher scope, with per-context overrides for the URL modifications and stubs.

This is entirely optional for test files where readability can be more important than DRYness.

• X
• Mastodon
• Reddit
• LinkedIn