Handle errors in upload and sync form

app/controllers/articles_controller.rb

@@ -160,48 +160,40 @@ class ArticlesController < ApplicationController
     if @updated_article_pairs.empty? && @outlisted_articles.empty? && @new_articles.empty?
       redirect_to supplier_articles_path(@supplier), :notice => I18n.t('articles.controller.sync.notice')
     end
-    @ignored_article_count = @supplier.articles.where(order_number: [nil, '']).count
   end
 
   # Updates, deletes articles when upload or sync form is submitted
   def update_synchronized
-    begin
+    @outlisted_articles = Article.find(params[:outlisted_articles].try(:keys)||[])
+    @updated_articles = Article.find(params[:articles].try(:keys)||[])
+    @updated_articles.map{|a| a.assign_attributes(params[:articles][a.id.to_s]) }
+    @new_articles = (params[:new_articles]||[]).map{|a| @supplier.articles.build(a) }
+
+    has_error = false
     Article.transaction do
       # delete articles
-        if params[:outlisted_articles]
-          Article.find(params[:outlisted_articles].keys).each(&:mark_as_deleted)
-        end
-
+      @outlisted_articles.each(&:mark_as_deleted)
       # Update articles
-        if params[:articles]
-          params[:articles].each do |id, attrs|
-            Article.find(id).update_attributes! attrs
-          end
-        end
-
+      @updated_articles.map{|a| a.save or has_error=true }
       # Add new articles
-        if params[:new_articles]
-          params[:new_articles].each do |attrs|
-            article = Article.new attrs
-            article.supplier = @supplier
+      @new_articles.each do |article|
         article.availability = true if @supplier.shared_sync_method == 'all_available'
         article.availability = false if @supplier.shared_sync_method == 'all_unavailable'
-            article.save!
-          end
-        end
+        article.save or has_error=true
       end
 
-      # Successfully done.
+      raise ActiveRecord::Rollback if has_error
+    end
+
+    if !has_error
       redirect_to supplier_articles_path(@supplier), notice: I18n.t('articles.controller.update_sync.notice')
-
-    rescue ActiveRecord::RecordInvalid => invalid
-      # An error has occurred, transaction has been rolled back.
-      redirect_to supplier_articles_path(@supplier),
-                  alert: I18n.t('articles.controller.error_update', :article => invalid.record.name, :msg => invalid.record.errors.full_messages)
-
-    rescue => error
-      redirect_to supplier_articles_path(@supplier),
-                  alert: I18n.t('errors.general_msg', :msg => error.message)
+    else
+      @updated_article_pairs = @updated_articles.map do |article|
+        orig_article = Article.find(article.id)
+        [article, orig_article.unequal_attributes(article)]
+      end
+      flash.now.alert = I18n.t('articles.controller.error_invalid')
+      render params[:from_action] == 'sync' ? :sync : :parse_upload
     end
   end
 
@@ -227,4 +219,16 @@ class ArticlesController < ApplicationController
       render :action => 'new', :layout => false
     end
   end
+
+  private
+
+  # @return [Number] Number of articles not taken into account when syncing (having no number)
+  helper_method \
+  def ignored_article_count
+    if action_name == 'sync' || params[:from_action] == 'sync'
+      @ignored_article_count ||= @supplier.articles.where(order_number: [nil, '']).count
+    else
+      0
+    end
+  end
 end

app/views/articles/_sync.html.haml

@@ -27,6 +27,6 @@
   = render 'sync_table', articles: @new_articles, field: 'new_articles', hidden_fields: %w(shared_updated_on order_number)
   %hr/
 
-- if @ignored_article_count > 0
+- if ignored_article_count > 0
   %p
-    %i= t '.outlist.body_ignored', count: @ignored_article_count
+    %i= t '.outlist.body_ignored', count: ignored_article_count

app/views/articles/_sync_table.html.haml

@@ -51,3 +51,6 @@
               = form.text_field 'deposit', class: 'input-mini', style: 'width: 45px'
           %td= form.select :article_category_id, ArticleCategory.all.map {|a| [ a.name, a.id ] },
             {include_blank: true}, class: 'input-small'
+      - unless changed_article.errors.empty?
+        %tr.alert
+          %td(colspan=11)= changed_article.errors.full_messages.join(', ')

app/views/articles/parse_upload.html.haml

@@ -1,6 +1,7 @@
 - title t('.title', supplier: @supplier.name)
 
 = form_tag update_synchronized_supplier_articles_path(@supplier) do
+  = hidden_field_tag :from_action, 'parse_upload'
   = render 'sync'
   .form-actions
     = submit_tag t('.submit'), class: 'btn btn-primary'

app/views/articles/sync.html.haml

@@ -1,6 +1,8 @@
 - title t('.title')
 
 = form_tag update_synchronized_supplier_articles_path(@supplier) do
+  = hidden_field_tag :from_action, 'sync'
   = render 'sync'
+  .form-actions
     = submit_tag t('.submit'), class: 'btn btn-primary'
     = link_to t('ui.or_cancel'), supplier_articles_path(@supplier)

spec/integration/articles_spec.rb

@@ -72,6 +72,19 @@ describe ArticlesController, :type => :feature do
       end
     end
 
+    describe "handles missing data" do
+      it do
+        find('input[type="submit"]').click # to overview
+        find('input[type="submit"]').click # missing category, re-show form
+        expect(find('tr.alert')).to be_present
+        expect(supplier.articles.count).to eq 0
+
+        all("tr select > option")[1].select_option
+        find('input[type="submit"]').click # now it should succeed
+        expect(supplier.articles.count).to eq 1
+      end
+    end
+
     #describe "can remove an existing article" do
     #  let!(:article) { create :article, supplier: supplier, name: 'Foobar', order_number: 99999 }
     #  it do