chore: Extract CSV parsing logic and optimize site import flow

- Introduce `CsvSiteParser`
- Simplify `SiteUpload` model
- Add `ProcessSiteUploadJob` and `ProcessSingleSiteJob` for parallelized site processing
- Fix audit callback to use `after_create` instead of `after_create_commit`

Author: Josyann

app/controllers/sites_controller.rb

@@ -42,7 +42,7 @@ def create
   def upload
     @upload = SiteUpload.new(site_upload_params)
     if @upload.save
-      redirect_to sites_path, notice: t(".uploaded", count: @upload.count)
+      redirect_to sites_path, notice: t(".started")
     else
       render :new, status: :unprocessable_content
     end

app/jobs/process_single_site_job.rb

@@ -0,0 +1,25 @@
+class ProcessSingleSiteJob < ApplicationJob
+  queue_as :default
+
+  def perform(site_data, team_id, tag_ids)
+    team = Team.find(team_id)
+
+    url = site_data["url"]
+    name = site_data["name"]
+    tag_names = site_data["tag_names"] || []
+
+    row_tag_ids = tag_names.map { |tag_name| team.tags.find_or_create_by(name: tag_name).id }
+    combined_tag_ids = (tag_ids + row_tag_ids).uniq
+
+    site = team.sites.find_by_url(url: url)
+
+    if site
+      site.tag_ids = combined_tag_ids.union(site.tag_ids)
+      site.name = name if name.present? && site.name.blank?
+      site.save!
+      site.audit!
+    else
+      Site.create!(url: url, team: team, name: name, tag_ids: combined_tag_ids)
+    end
+  end
+end

app/jobs/process_site_upload_job.rb

@@ -0,0 +1,13 @@
+class ProcessSiteUploadJob < ApplicationJob
+  queue_as :default
+
+  def perform(sites_data, team_id, tag_ids)
+    site_jobs = []
+
+    sites_data.each do |site_data|
+      site_jobs << ProcessSingleSiteJob.new(site_data, team_id, tag_ids)
+    end
+
+    ActiveJob.perform_all_later(site_jobs) if site_jobs.any?
+  end
+end

app/models/audit.rb

@@ -2,7 +2,7 @@ class Audit < ApplicationRecord
   belongs_to :site, touch: true, counter_cache: true
   has_many :checks, -> { prioritized }, dependent: :destroy
 
-  after_create_commit :create_checks
+  after_create :create_checks
   after_create_commit :schedule
 
   validates :url, presence: true, url: true

app/models/site.rb

@@ -43,6 +43,7 @@ def url=(new_url)
   def url_without_scheme_and_www = Link.url_without_scheme_and_www(audit.url)
 
   def name_with_fallback = name.presence || url_without_scheme_and_www
+
   alias to_title name_with_fallback
   alias to_s name_with_fallback
 

app/models/site_upload.rb

@@ -15,32 +15,23 @@ class SiteUpload
   ].freeze
   MAX_FILE_SIZE = 5.megabytes
   REQUIRED_HEADERS = ["url"].freeze
-  SUPPORTED_SEPARATORS = [",", ";"].freeze
-  BOM = /^\xEF\xBB\xBF/
 
-  attr_accessor :file, :team, :tag_ids, :tags, :new_sites, :existing_sites
+  attr_accessor :file, :team, :tag_ids
 
   validates :file, :team, presence: true
   validate :valid_file_size, :valid_file_format, :valid_headers, if: :file
 
-  delegate :create!, :transaction, :human, to: :Site
-
   def initialize(attributes = {})
     super
     @tag_ids ||= []
-    @new_sites = {}
-    @existing_sites = {}
   end
 
   def save
     return false unless valid?
+    sites_data = parser.parse
 
-    parse_sites
+    ProcessSiteUploadJob.perform_later(sites_data, team.id, tag_ids)
 
-    transaction do
-      create!(new_sites.values) if new_sites.any?
-      existing_sites.values.each { |site| site.save && site.audit! }
-    end
     true
   end
 
@@ -57,44 +48,10 @@ def assign_attributes(attributes)
     super(attributes.slice(:team).merge(attributes))
   end
 
-  def count
-    (new_sites&.length || 0) + (existing_sites&.length || 0)
-  end
-
-  def parse_sites
-    require "csv"
-
-    CSV.foreach(file.path, headers: true, encoding: "bom|utf-8", col_sep:) do |row|
-      row = row.to_h.transform_keys { |header| header.to_s.downcase } # Case-insensitive headers
-
-      url = Link.normalize(row["url"])
-      name = row["nom"] || row["name"]
-      tag_names = row["tags"].present? ? row["tags"].split(",").map(&:strip).compact_blank.uniq : []
-
-      row_tag_ids = tag_names.map { |n| team.tags.find_or_create_by(name: n).id }
-      combined_tag_ids = (tag_ids + row_tag_ids).uniq
-      existing_site = team.sites.find_by_url(url:)
-
-      if existing_site
-        existing_site.assign_attributes(tag_ids: combined_tag_ids.union(existing_site.tag_ids))
-        existing_site.assign_attributes(name:) unless existing_site.name
-        self.existing_sites[url] = existing_site
-      else
-        self.new_sites[url] = { url:, team:, name:, tag_ids: combined_tag_ids }
-      end
-    end
-  end
-
   private
 
-  def first_line
-    @first_line ||= File.open(file.path, &:gets)&.strip&.sub(BOM, "") || ""
-  end
-
-  def col_sep
-    SUPPORTED_SEPARATORS.max_by { |sep| first_line.count(sep) }
-  rescue StandardError
-    SUPPORTED_SEPARATORS.first
+  def parser
+    @parser ||= CsvSiteParser.new(file)
   end
 
   def valid_file_size
@@ -107,8 +64,7 @@ def valid_file_format
   end
 
   def valid_headers
-    headers = CSV.parse_line(first_line, col_sep:) || []
-    missing_headers = REQUIRED_HEADERS - headers.compact.collect(&:downcase)
+    missing_headers = REQUIRED_HEADERS - parser.headers
     errors.add(:file, :invalid_headers) unless missing_headers.empty?
   rescue CSV::MalformedCSVError, StandardError
     errors.add(:file, :invalid_headers)

app/services/csv_site_parser.rb

@@ -0,0 +1,50 @@
+class CsvSiteParser
+  require "csv"
+
+  BOM = /^\xEF\xBB\xBF/
+  SUPPORTED_SEPARATORS = [",", ";"].freeze
+
+  def initialize(file)
+    @file = file
+  end
+
+  def parse
+    sites_by_url = {}
+
+    CSV.foreach(@file.path, headers: true, encoding: "bom|utf-8", col_sep: detect_col_sep) do |row|
+      row = row.to_h.transform_keys { |header| header.to_s.downcase }
+
+      url = Link.normalize(row["url"]).to_s
+      name = row["nom"] || row["name"]
+      tag_names = row["tags"].present? ? row["tags"].split(",").map(&:strip).compact_blank.uniq : []
+
+      if sites_by_url[url]
+        sites_by_url[url]["tag_names"] = (sites_by_url[url]["tag_names"] + tag_names).uniq
+      else
+        sites_by_url[url] = {
+          "url" => url,
+          "name" => name,
+          "tag_names" => tag_names
+        }
+      end
+    end
+
+    sites_by_url.values
+  end
+
+  def headers
+    @headers ||= (CSV.parse_line(first_line, col_sep: detect_col_sep) || []).compact_blank.map(&:downcase)
+  end
+
+  private
+
+  def first_line
+    @first_line ||= File.open(@file.path, &:gets)&.strip&.sub(BOM, "") || ""
+  end
+
+  def detect_col_sep
+    SUPPORTED_SEPARATORS.max_by { |sep| first_line.count(sep) }
+  rescue StandardError
+    SUPPORTED_SEPARATORS.first
+  end
+end

config/locales/fr.yml

@@ -328,6 +328,7 @@ fr:
       new_audit: Site existant, nouvel audit programmé
     upload:
       title: Importer des sites
+      started: Import démarré
       uploaded:
         zero: Aucun site ajouté
         one: Un site ajouté

spec/jobs/process_single_site_job_spec.rb

@@ -0,0 +1,135 @@
+require "rails_helper"
+
+RSpec.describe ProcessSingleSiteJob do
+  subject(:job) { described_class.new }
+
+  let(:team) { create(:team) }
+  let(:site_data) { { "url" => "https://example.com/", "name" => "Example Site", "tag_names" => ["tag1", "tag2"] } }
+  let(:tag_ids) { [] }
+
+  describe "#perform" do
+    context "when site does not exist" do
+      it "creates a new site with tags" do
+        expect {
+          job.perform(site_data, team.id, tag_ids)
+        }.to change(Site, :count).by(1)
+
+        site = Site.last
+        expect(site.url).to eq("https://example.com/")
+        expect(site.name).to eq("Example Site")
+        expect(site.tags.pluck(:name)).to match_array(["tag1", "tag2"])
+      end
+
+      it "creates tags if they don't exist" do
+        expect {
+          job.perform(site_data, team.id, tag_ids)
+        }.to change(Tag, :count).by(2)
+      end
+
+      context "with form tag_ids" do
+        let!(:form_tag) { create(:tag, name: "form_tag", team: team) }
+        let(:tag_ids) { [form_tag.id] }
+
+        it "combines CSV tags with form tags" do
+          job.perform(site_data, team.id, tag_ids)
+
+          site = Site.last
+          expect(site.tags.pluck(:name)).to match_array(["tag1", "tag2", "form_tag"])
+        end
+      end
+
+      context "without name in CSV" do
+        let(:site_data) { { "url" => "https://example.com/", "name" => nil, "tag_names" => [] } }
+
+        it "creates site without name" do
+          job.perform(site_data, team.id, tag_ids)
+
+          site = Site.last
+          expect(site.name).to be_nil
+        end
+      end
+    end
+
+    context "when site already exists" do
+      let!(:existing_site) { create(:site, url: "https://example.com/", name: nil, team: team) }
+      let!(:existing_tag) { create(:tag, name: "existing_tag", team: team) }
+
+      before do
+        existing_site.tags << existing_tag
+      end
+
+      it "does not create a new site" do
+        expect {
+          job.perform(site_data, team.id, tag_ids)
+        }.not_to change(Site, :count)
+      end
+
+      it "merges tags with existing tags" do
+        job.perform(site_data, team.id, tag_ids)
+
+        existing_site.reload
+        expect(existing_site.tags.pluck(:name)).to match_array(["existing_tag", "tag1", "tag2"])
+      end
+
+      it "sets name if site has no name" do
+        job.perform(site_data, team.id, tag_ids)
+
+        existing_site.reload
+        expect(existing_site.name).to eq("Example Site")
+      end
+
+      it "does not overwrite existing name" do
+        existing_site.update!(name: "Old Name")
+
+        job.perform(site_data, team.id, tag_ids)
+
+        existing_site.reload
+        expect(existing_site.name).to eq("Old Name")
+      end
+
+      it "triggers audit on existing site" do
+        expect(existing_site).to receive(:audit!)
+
+        job.perform(site_data, team.id, tag_ids)
+      end
+
+      context "with form tag_ids" do
+        let!(:form_tag) { create(:tag, name: "form_tag", team: team) }
+        let(:tag_ids) { [form_tag.id] }
+
+        it "combines all tags" do
+          job.perform(site_data, team.id, tag_ids)
+
+          existing_site.reload
+          expect(existing_site.tags.pluck(:name)).to match_array(["existing_tag", "tag1", "tag2", "form_tag"])
+        end
+      end
+    end
+
+    context "with duplicate tags in CSV" do
+      let(:site_data) { { "url" => "https://example.com/", "name" => "Example Site", "tag_names" => ["tag1", "tag1", "tag2"] } }
+
+      it "deduplicates tags" do
+        expect {
+          job.perform(site_data, team.id, tag_ids)
+        }.to change(Tag, :count).by(1)
+
+        site = Site.last
+        expect(site.tags.pluck(:name)).to match_array(["tag1", "tag2"])
+      end
+    end
+
+    context "with existing tags" do
+      let!(:existing_tag) { create(:tag, name: "tag1", team: team) }
+
+      it "reuses existing tags instead of creating new ones" do
+        expect {
+          job.perform(site_data, team.id, tag_ids)
+        }.to change(Tag, :count).by(1)
+
+        site = Site.last
+        expect(site.tags).to include(existing_tag)
+      end
+    end
+  end
+end