Add errors handling support to AutoinstStorage.Import

src/modules/AutoinstStorage.rb

@@ -8,6 +8,8 @@
 # $Id$
 require "yast"
 require "autoinstall/storage_proposal"
+require "autoinstall/dialogs/question"
+require "autoinstall/storage_proposal_issues_presenter"
 
 module Yast
   class AutoinstStorageClass < Module
@@ -149,66 +151,6 @@ def Import(settings)
 
     end
 
-    # Determines whether the proposal is valid and inform the user if not valid
-    #
-    # When proposal is not valid:
-    #
-    # * If it only contain warnings: asks the user for confirmation.
-    # * If it contains some important problem, inform the user.
-    #
-    # @return [Boolean] True if the proposal is valid or the user accepted an invalid one.
-    def valid_proposal?(proposal)
-      return true if proposal.valid?
-
-      message = proposal_problems_message(proposal.problems_list)
-
-      if proposal.problems_list.fatal?
-        Popup.LongError(message)
-        false
-      else
-        Report.ErrorAnyQuestion(
-          _("Profile problems"),
-          message,
-          Label.ContinueButton,
-          Label.AbortButton,
-          :yes
-        )
-      end
-    end
-
-    def proposal_problems_message(problems_list)
-      fatal, non_fatal = problems_list.partition(&:fatal?)
-
-      parts = []
-      parts << proposal_warnings_message(non_fatal) unless non_fatal.empty?
-
-      if fatal.empty?
-        parts << _("Do you want to continue?")
-      else
-        parts << proposal_errors_message(fatal)
-        parts << _("Please, correct these problems and try again.")
-      end
-
-      parts.join("\n\n")
-    end
-
-    def proposal_warnings_message(problems)
-      _("Some minor problems where detected while creating the partitioning plan:") +
-        problems_list_to_message(problems)
-    end
-
-    def proposal_errors_message(problems)
-      _("Some minor problems where detected while creating the partitioning plan:") +
-        problems_list_to_message(problems)
-    end
-
-    def problems_list_to_message(problems)
-      lines = problems.map do |p|
-        "* #{p.message}"
-      end
-      lines.join("\n")
-    end
-
     # Import settings from the general/storage section
     #
     # General settings are imported with a different method because:
@@ -452,6 +394,58 @@ def build_id(disk_dev_name, nr)
 
   private
 
+    # Determine whether the proposal is valid and inform the user if not valid
+    #
+    # When proposal is not valid:
+    #
+    # * If it only contain warnings: asks the user for confirmation.
+    # * If it contains some important problem, inform the user.
+    #
+    # @param [StorageProposal] Storage proposal to check
+    # @return [Boolean] True if the proposal is valid or the user accepted an invalid one.
+    def valid_proposal?(proposal)
+      return true if proposal.valid?
+
+      report_settings = Report.Export
+      if proposal.issues_list.fatal?
+        # On fatal errors, the message should be displayed and without timeout
+        level = :error
+        buttons_set = :abort
+        display_message = true
+        log_message = report_settings["errors"]["log"]
+        timeout = 0
+      else
+        # On non-fatal issues, obey report settings for warnings
+        level = :warn
+        buttons_set = :question
+        display_message = report_settings["warnings"]["show"]
+        log_message = report_settings["warnings"]["log"]
+        timeout = report_settings["warnings"]["timeout"]
+      end
+
+      presenter = Y2Autoinstallation::StorageProposalIssuesPresenter.new(proposal.issues_list)
+      log_proposal_issues(level, presenter.to_plain) if log_message
+      return true unless display_message
+
+      dialog = Y2Autoinstallation::Dialogs::Question.new(
+        presenter.to_html,
+        timeout: timeout,
+        buttons_set: buttons_set
+      )
+      dialog.run == :ok
+    end
+
+
+    # Log proposal issues message
+    #
+    # @param level   [Symbol] Message level (:error, :warn)
+    # @param content [String] Text to log
+    def log_proposal_issues(level, content)
+      settings_name = level == :error ? :error : :warning
+      log.send(level, content)
+    end
+
+
     attr_accessor :general_settings
   end
 

test/autoinst_storage_test.rb

@@ -9,19 +9,30 @@
   describe "#Import" do
     let(:storage_proposal) do
       instance_double(
-        Y2Autoinstallation::StorageProposal, valid?: valid?, problems_list: problems_list
+        Y2Autoinstallation::StorageProposal, valid?: valid?, issues_list: issues_list
       )
     end
-
-    let(:problems_list) { Y2Storage::AutoinstProblems::List.new }
+    let(:issues_dialog) { instance_double(Y2Autoinstallation::Dialogs::Question, run: :abort) }
+    let(:issues_list) { Y2Storage::AutoinstIssues::List.new }
     let(:valid?) { true }
+    let(:errors_settings) { { "show" => false, "timeout" => 10 } }
+    let(:warnings_settings) { { "show" => true, "timeout" => 5 } }
 
     before do
       allow(Y2Autoinstallation::StorageProposal).to receive(:new)
         .and_return(storage_proposal)
+      allow(Y2Autoinstallation::Dialogs::Question).to receive(:new)
+        .and_return(issues_dialog)
       allow(storage_proposal).to receive(:save)
     end
 
+    around do |example|
+      old_settings = Yast::Report.Export
+      Yast::Report.Import({"warnings" => warnings_settings, "errors" => errors_settings})
+      example.run
+      Yast::Report.Import(old_settings)
+    end
+
     it "creates a proposal" do
       expect(Y2Autoinstallation::StorageProposal).to receive(:new)
       subject.Import({})
@@ -40,65 +51,115 @@
       end
     end
 
-    context "when the proposal is not valid" do
+    context "when the proposal contains fatal issues" do
+      let(:valid?) { false }
+
+      before do
+        issues_list.add(:missing_root)
+        allow(subject.log).to receive(:error).and_call_original
+      end
+
+      it "shows errors to the user with not timeout" do
+        expect(Y2Autoinstallation::Dialogs::Question).to receive(:new)
+          .with(/Some important problems/, timeout: 0, buttons_set: :abort)
+          .and_return(issues_dialog)
+        expect(issues_dialog).to receive(:run)
+        subject.Import({})
+      end
+
+      it "returns false" do
+        allow(issues_dialog).to receive(:run).and_return(:abort)
+        expect(subject.Import({})).to eq(false)
+      end
+
+      context "and errors logging is enabled" do
+        let(:errors_settings) { { "log" => true } }
+
+        it "logs the error" do
+          expect(subject.log).to receive(:error)
+            .with(/Some important problems/)
+          subject.Import({})
+        end
+      end
+
+      context "and errors logging is disabled" do
+        let(:errors_settings) { { "log" => false } }
+
+        it "does not log the error" do
+          expect(subject.log).to_not receive(:error)
+            .with(/Some important problems/)
+          subject.Import({})
+        end
+      end
+    end
+
+    context "when the proposal contains non fatal issues" do
       let(:valid?) { false }
       let(:continue) { true }
 
       before do
-        allow(Yast::Report).to receive(:ErrorAnyQuestion)
-          .and_return(continue)
+        issues_list.add(:invalid_value, "/", :size, "auto")
+        allow(subject.log).to receive(:warn).and_call_original
       end
 
-      context "and there are no fatal errors" do
-        it "asks the user to continue" do
-          expect(Yast::Report).to receive(:ErrorAnyQuestion)
+      context "and warnings reporting is enabled" do
+        it "asks the user for confirmation" do
+          expect(Y2Autoinstallation::Dialogs::Question).to receive(:new)
+            .with(/Some minor problems/, timeout: 5, buttons_set: :question)
+            .and_return(issues_dialog)
+          expect(issues_dialog).to receive(:run)
           subject.Import({})
         end
 
-        context "and the user decides to continue" do
-          it "returns true" do
-            expect(subject.Import({})).to eq(true)
+        context "and the user confirms the proposal" do
+          before do
+            allow(issues_dialog).to receive(:run).and_return(:ok)
           end
 
-          it "saves the proposal" do
-            expect(storage_proposal).to receive(:save)
-            subject.Import({})
+          it "returns true" do
+            expect(subject.Import({})).to eq(true)
           end
         end
 
-        context "and the user decides to abort" do
-          let(:continue) { false }
+        context "and the user dismisses the proposal" do
+          before do
+            allow(issues_dialog).to receive(:run).and_return(:abort)
+          end
 
           it "returns false" do
             expect(subject.Import({})).to eq(false)
           end
-
-          it "does not save the proposal" do
-            expect(storage_proposal).to_not receive(:save)
-            subject.Import({})
-          end
         end
       end
 
-      context "and there are fatal errors" do
-        before do
-          problems_list.add(:missing_root)
-        end
+      context "and warnings reporting is disabled" do
+        let(:warnings_settings) { { "show" => false } }
 
-        it "returns false" do
-          expect(subject.Import({})).to eq(false)
+        it "returns true" do
+          expect(subject.Import({})).to eq(true)
         end
+      end
+
+      context "and warnings logging is enabled" do
+        let(:warnings_settings) { { "log" => true } }
 
-        it "notifies the user" do
-          expect(Yast::Popup).to receive(:LongError)
-            .with(/No root partition/)
+        it "logs the warning" do
+          expect(subject.log).to receive(:warn)
+            .with(/Some minor problems/)
           subject.Import({})
         end
+      end
+
+      context "and warnings logging is disabled" do
+        let(:warnings_settings) { { "log" => false } }
 
-        it "does not save the proposal" do
-          expect(storage_proposal).to_not receive(:save)
+        it "does not log the warning" do
+          expect(subject.log).to_not receive(:warn)
+            .with(/Some minor problems/)
+          subject.Import({})
         end
       end
+
     end
   end