MandatoryAttachment: group missing errors logic together

and show errors only if not draft

app/controllers/backoffice/private_formulaires_controller.rb

@@ -5,7 +5,8 @@ class Backoffice::PrivateFormulairesController < ApplicationController
     dossier = current_gestionnaire.dossiers.find(params[:dossier_id])
 
     unless params[:champs].nil?
-      champs_service_errors = ChampsService.save_champs dossier.champs_private, params
+      ChampsService.save_champs dossier.champs_private, params
+      champs_service_errors = ChampsService.build_error_messages(dossier.champs_private)
 
       if champs_service_errors.empty?
         flash[:notice] = "Formulaire enregistré"

app/controllers/users/description_controller.rb

@@ -28,13 +28,8 @@ class Users::DescriptionController < UsersController
     procedure = dossier.procedure
 
     return head :forbidden unless dossier.can_be_initiated?
-    check_mandatory_fields = !draft_submission?
 
-    if params[:champs]
-      champs_service_errors =
-        ChampsService.save_champs(dossier.champs, params, check_mandatory_fields)
-      return redirect_to_description_with_errors(dossier, champs_service_errors) if champs_service_errors.any?
-    end
+    ChampsService.save_champs(dossier.champs, params) if params[:champs]
 
     if procedure.cerfa_flag? && params[:cerfa_pdf]
       cerfa = Cerfa.new(content: params[:cerfa_pdf], dossier: dossier, user: current_user)
@@ -44,6 +39,13 @@ class Users::DescriptionController < UsersController
     errors_upload = PiecesJustificativesService.upload!(dossier, current_user, params)
     return redirect_to_description_with_errors(dossier, errors_upload) if errors_upload.any?
 
+    if params[:champs] && !draft_submission?
+      errors =
+        ChampsService.build_error_messages(dossier.champs) +
+        PiecesJustificativesService.missing_pj_error_messages(dossier)
+      return redirect_to_description_with_errors(dossier, errors) if errors.any?
+    end
+
     if draft_submission?
       flash.notice = 'Votre brouillon a bien été sauvegardé.'
       redirect_to url_for(controller: :dossiers, action: :index, liste: :brouillon)

app/services/champs_service.rb

@@ -1,11 +1,14 @@
 class ChampsService
   class << self
-    def save_champs(champs, params, check_mandatory = true)
+    def save_champs(champs, params)
       fill_champs(champs, params)
 
       champs.select(&:changed?).each(&:save)
+    end
 
-      check_mandatory ? build_error_messages(champs) : []
+    def build_error_messages(champs)
+      champs.select(&:mandatory_and_blank?)
+            .map { |c| "Le champ #{c.libelle} doit être rempli." }
     end
 
     private
@@ -28,10 +31,5 @@ class ChampsService
     def extract_minute(champ_id, h)
       h[:time_minute]["'#{champ_id}'"]
     end
-
-    def build_error_messages(champs)
-      champs.select(&:mandatory_and_blank?)
-            .map { |c| "Le champ #{c.libelle} doit être rempli." }
-    end
   end
 end

app/services/pieces_justificatives_service.rb

@@ -13,8 +13,6 @@ class PiecesJustificativesService
     errors += without_virus
               .map { |tpj, content| save_pj(content, dossier, tpj, user) }
               .compact()
-
-    errors += missing_pj_error_messages(dossier)
   end
 
   def self.upload_one! dossier, user, params

spec/services/champs_service_spec.rb

@@ -1,7 +1,6 @@
 require 'spec_helper'
 
 describe ChampsService do
-  describe 'save_champs' do
   let!(:champ) { Champ.create(value: 'toto', type_de_champ: TypeDeChamp.new) }
   let!(:champ_mandatory_empty) { Champ.create(type_de_champ: TypeDeChamp.new(libelle: 'mandatory', mandatory: true)) }
   let!(:champ_datetime) do
@@ -10,6 +9,7 @@ describe ChampsService do
   end
   let!(:champs) { [champ, champ_mandatory_empty, champ_datetime] }
 
+  describe 'save_champs' do
     before :each do
       params_hash = {
         champs: {
@@ -19,12 +19,10 @@ describe ChampsService do
         time_hour:  { "'#{champ_datetime.id}'" => '12' },
         time_minute: { "'#{champ_datetime.id}'" => '24' }
       }
-      @errors = ChampsService.save_champs(champs, params_hash, check_mandatory)
+      ChampsService.save_champs(champs, params_hash)
       champs.each(&:reload)
     end
 
-    context 'check_mandatory is true' do
-      let(:check_mandatory) { true }
     it 'saves the changed champ' do
       expect(champ.value).to eq('yop')
     end
@@ -32,18 +30,11 @@ describe ChampsService do
     it 'parses and save the date' do
       expect(champ_datetime.value).to eq('d 12:24')
     end
+  end
 
+  describe 'build_error_message' do
     it 'adds error for the missing mandatory champ' do
-        expect(@errors).to match(['Le champ mandatory doit être rempli.'])
-      end
-    end
-
-    context 'check_mandatory is false' do
-      let(:check_mandatory) { false }
-
-      it 'does not add errors' do
-        expect(@errors).to match([])
-      end
+      expect(ChampsService.build_error_messages(champs)).to match(['Le champ mandatory doit être rempli.'])
     end
   end
 end

spec/services/pieces_justificatives_service_spec.rb

@@ -8,7 +8,6 @@ describe PiecesJustificativesService do
     allow(ClamavService).to receive(:safe_file?).and_return(safe_file)
   end
 
-  describe 'self.upload!' do
   let(:hash) { {} }
   let!(:tpj_not_mandatory) do
     TypeDePieceJustificative.create(libelle: 'not mandatory', mandatory: false)
@@ -19,15 +18,14 @@ describe PiecesJustificativesService do
   let(:procedure) { Procedure.create(types_de_piece_justificative: tpjs) }
   let(:dossier) { Dossier.create(procedure: procedure) }
   let(:errors) { PiecesJustificativesService.upload!(dossier, user, hash) }
-
-    context 'when no piece justificative is required' do
   let(:tpjs) { [tpj_not_mandatory] }
 
+  describe 'self.upload!' do
     context 'when no params are given' do
       it { expect(errors).to eq([]) }
     end
 
-      context 'when sometihing wrong with file save' do
+    context 'when there is something wrong with file save' do
       let(:hash) do
         {
           "piece_justificative_#{tpj_not_mandatory.id}" =>
@@ -49,9 +47,30 @@ describe PiecesJustificativesService do
 
       it { expect(errors).to match(['bad_file : virus détecté']) }
     end
+
+    context 'when a regular file is provided' do
+      let(:content) { double(path: '', original_filename: 'filename') }
+      let(:hash) do
+        {
+          "piece_justificative_#{tpj_not_mandatory.id}" =>
+            content
+        }
       end
 
-    context 'when a piece justificative is required' do
+      before :each do
+        expect(PiecesJustificativesService).to receive(:save_pj)
+          .with(content, dossier, tpj_not_mandatory, user)
+          .and_return(nil)
+      end
+
+      it 'is saved' do
+        expect(errors).to match([])
+      end
+    end
+  end
+
+  describe 'missing_pj_error_messages' do
+    let(:errors) { PiecesJustificativesService.missing_pj_error_messages(dossier) }
     let(:tpjs) { [tpj_mandatory] }
 
     context 'when no params are given' do
@@ -63,19 +82,11 @@ describe PiecesJustificativesService do
         # we are messing around piece_justificative
         # because directly doubling carrierwave params seems complicated
 
-          allow(PiecesJustificativesService).to receive(:save_pj).and_return(nil)
         piece_justificative_double = double(type_de_piece_justificative: tpj_mandatory)
         expect(dossier).to receive(:pieces_justificatives).and_return([piece_justificative_double])
       end
 
-        let(:hash) do
-          {
-            "piece_justificative_#{tpj_mandatory.id}" => double(path: '')
-          }
-        end
-
       it { expect(errors).to match([]) }
     end
   end
-  end
 end