Fix add-on handling

src/modules/AddOnProduct.rb

@@ -1572,24 +1572,16 @@ def AddRepo(url, pth, priority)
       new_repo = { "enabled" => true, "base_urls" => [url], "prod_dir" => pth }
 
       # BNC #714027: Possibility to adjust repository priority (usually higher)
-      Ops.set(new_repo, "priority", priority) if Ops.greater_than(priority, -1)
+      new_repo["priority"] = priority if priority > -1
 
-      Builtins.y2milestone(
-        "Adding Repository: %1, product path: %2",
-        URL.HidePassword(url),
-        pth
-      )
+      log.info "Adding Repository: #{URL.HidePassword(url)}, product path: #{pth}"
       new_repo_id = Pkg.RepositoryAdd(new_repo)
+      log.info "New repository id: #{new_repo_id}"
 
-      if new_repo_id == nil || Ops.less_than(new_repo_id, 0)
-        Builtins.y2error("Unable to add product: %1", URL.HidePassword(url))
+      if new_repo_id == nil || new_repo_id < 0
+        log.error "Unable to add product: %1", URL.HidePassword(url)
         # TRANSLATORS: error message, %1 is replaced with product URL
-        Report.Error(
-          Builtins.sformat(
-            _("Unable to add product %1."),
-            URL.HidePassword(url)
-          )
-        )
+        Report.Error(format(_("Unable to add product %s."), URL.HidePassword(url)))
         return nil
       end
 
@@ -1714,45 +1706,16 @@ def AddPreselectedAddOnProducts(filelist)
             Builtins.y2milestone("inst_network_check ret: %1", inc_ret)
           end
           # a CD/DVD repository
-          if Builtins.contains(["cd", "dvd"], scheme)
-            # if the CD/DVD product is known just try if it's there
-            # and ask if not
-            if prodname != ""
-              found = false
-
-              while !found
-                repo_id = AddRepo(url, pth, priority)
-                next false if repo_id == nil
-
-                prod2 = Pkg.SourceProductData(repo_id)
-                if Ops.get_string(prod2, "label", "") == prodname
-                  found = true
-                else
-                  Builtins.y2milestone(
-                    "Removing repo %1: Add-on found: %2, expected: %3",
-                    repo_id,
-                    Ops.get_string(prod2, "label", ""),
-                    prodname
-                  )
-                  Pkg.SourceDelete(repo_id)
-
-                  # ask for a different medium
-                  url = AskForCD(url, prodname)
-                  next false if url == nil
-                end
-              end
+          repo_id =
+            if Builtins.contains(["cd", "dvd"], scheme)
+              prodname.empty? ? add_repo_from_cd(url, pth, priority) :
+                add_product_from_cd(url, pth, priority, prodname)
             else
-              result = AskForCD(url, prodname)
-              next false if result == nil
-
-              repo_id = AddRepo(result, pth, priority)
-              next false if repo_id == nil
+              # a non CD/DVD repository
+              AddRepo(url, pth, priority)
             end
-          else
-            # a non CD/DVD repository
-            repo_id = AddRepo(url, pth, priority)
-            next false if repo_id == nil
-          end
+          next false unless repo_id
+
           if !AcceptedLicenseAndInfoFile(repo_id)
             Builtins.y2warning("License not accepted, delete the repository")
             Pkg.SourceDelete(repo_id)
@@ -2256,6 +2219,52 @@ def add_rename(old_name, new_name)
     publish :function => :ImportGpgKeyCallback, :type => "boolean (map <string, any>, integer)"
     publish :function => :AcceptNonTrustedGpgKeyCallback, :type => "boolean (map <string, any>)"
     publish :function => :SetSignatureCallbacks, :type => "void (string)"
+
+  private
+
+    # Adds a product from a CD/DVD
+    #
+    # To add the product, the name should match +prodname+ argument.
+    #
+    # @param url      [String]  Repository URL (cd: or dvd: schemas are expected)
+    # @param pth      [String]  Repository path (in the media)
+    # @param priority [Integer] Repository priority
+    # @param prodname [String]  Expected product's name
+    # @return         [Integer,nil] Repository id; nil if product was not found and user cancelled.
+    #
+    # @see add_repo_from_cd
+    def add_product_from_cd(url, pth, priority, prodname)
+      current_url = url
+      loop do
+        # just try if it's there and ask if not
+        repo_id = AddRepo(current_url, pth, priority)
+        if repo_id
+          found_product = Pkg.SourceProductData(repo_id)
+          return repo_id if found_product["label"] == prodname
+          log.info("Removing repo #{repo_id}: Add-on found #{found_product["label"]}, expected: #{prodname}")
+          Pkg.SourceDelete(repo_id)
+        end
+
+        # ask for a different medium
+        current_url = AskForCD(current_url, prodname)
+        return nil if current_url.nil?
+      end
+    end
+
+    # Adds a repository from a CD/DVD
+    #
+    # The product contained in the CD/DVD does not matter.
+    #
+    # @param url      [String]  Repository URL (cd: or dvd: schemas are expected)
+    # @param pth      [String]  Repository path (in the media)
+    # @param priority [Integer] Repository priority
+    # @return         [Integer,nil] Repository id; nil if product was not found and user cancelled.
+    def add_repo_from_cd(url, pth, priority)
+      result = AskForCD(url, "")
+      return result if result.nil?
+
+      AddRepo(result, pth, priority)
+    end
   end
 
   AddOnProduct = AddOnProductClass.new

test/addon_product_test.rb

@@ -5,6 +5,8 @@
 Yast.import "AddOnProduct"
 
 describe Yast::AddOnProduct do
+  subject { Yast::AddOnProduct }
+
   describe "#renamed?" do
     it "returns true if product has been renamed" do
       expect(Yast::AddOnProduct.renamed?("SUSE_SLES", "SLES")).to eq(true)
@@ -87,4 +89,167 @@
       end
     end
   end
+
+  describe "#AddPreselectedAddOnProducts" do
+    BASE_URL = "cd:/?devices=/dev/disk/by-id/ata-QEMU_DVD-ROM_QM00001".freeze
+    ADDON_REPO = {
+      "path" => "/foo", "priority" => 50, "url" => "cd:/?alias=Foo"
+    }.freeze
+
+    let(:repo) { ADDON_REPO }
+    let(:filelist) do
+      [ { "file" => "/add_on_products.xml", "type" => "xml" } ]
+    end
+
+    before do
+      subject.SetBaseProductURL(BASE_URL)
+      allow(subject).to receive(:ParseXMLBasedAddOnProductsFile).and_return([repo])
+    end
+
+    context "when filelist is empty" do
+      let(:filelist) { [] }
+
+      it "just returns true" do
+        expect(subject).to_not receive(:GetBaseProductURL)
+        subject.AddPreselectedAddOnProducts(filelist)
+      end
+    end
+
+    context "when filelist is nil" do
+      let(:filelist) { nil }
+
+      it "just returns true" do
+        expect(subject).to_not receive(:GetBaseProductURL)
+        subject.AddPreselectedAddOnProducts(filelist)
+      end
+    end
+
+    context "when filelist contains XML files" do
+      it "parses the XML file" do
+        expect(subject).to receive(:ParseXMLBasedAddOnProductsFile).and_return([])
+        subject.AddPreselectedAddOnProducts(filelist)
+      end
+    end
+
+    context "when filelist contains plain-text files" do
+      let(:filelist) do
+        [ { "file" => "/add_on_products.xml", "type" => "plain" } ]
+      end
+
+      it "parses the plain file" do
+        expect(subject).to receive(:ParsePlainAddOnProductsFile).and_return([])
+        subject.AddPreselectedAddOnProducts(filelist)
+      end
+    end
+
+    context "when the add-on is on a CD/DVD" do
+      let(:repo_id) { 1 }
+      let(:cd_url) { "cd:///?device=/dev/sr0" }
+
+      before do
+        allow(subject).to receive(:AskForCD).and_return(cd_url)
+        allow(subject).to receive(:AcceptedLicenseAndInfoFile).and_return(true)
+        allow(Yast::Pkg). to receive(:SourceProductData).with(repo_id)
+        allow(subject).to receive(:InstallProductsFromRepository)
+        allow(subject).to receive(:ReIntegrateFromScratch)
+        allow(subject).to receive(:Integrate)
+      end
+
+      context "and no product name was given" do
+        let(:repo) { ADDON_REPO }
+
+        it "adds the repository" do
+          allow(subject).to receive(:AddRepo).with(cd_url, anything, anything)
+          subject.AddPreselectedAddOnProducts(filelist)
+        end
+
+        it "does not add the repository if user cancels the dialog" do
+          allow(subject).to receive(:AskForCD).and_return(nil)
+
+          expect(subject).to_not receive(:AddRepo)
+          subject.AddPreselectedAddOnProducts(filelist)
+        end
+
+        it "does not integrate the repository if it was not added" do
+          allow(subject).to receive(:AddRepo).with(cd_url, anything, anything)
+            .and_return(nil)
+
+          expect(subject).to_not receive(:Integrate).with(repo_id)
+          subject.AddPreselectedAddOnProducts(filelist)
+        end
+      end
+
+      context "and a network scheme is used" do
+        let(:repo) { ADDON_REPO.merge("url" => "http://example.net/repo") }
+
+        it "checks whether the network is working" do
+          allow(subject).to receive(:AddRepo).and_return(nil)
+          expect(Yast::WFM).to receive(:CallFunction).with("inst_network_check", [])
+          subject.AddPreselectedAddOnProducts(filelist)
+        end
+      end
+
+      context "and a product name was given" do
+        let(:repo) { ADDON_REPO.merge("name" => "Foo") }
+        let(:matching_product) { { "label" => repo["name"] } }
+        let(:other_product) { { "label" => "other" } }
+        let(:other_repo_id) { 2 }
+
+        context "and the product is found in the CD/DVD" do
+          before do
+            allow(Yast::Pkg).to receive(:SourceProductData).with(repo_id)
+              .and_return(matching_product)
+          end
+
+          it "adds the product without asking" do
+            expect(subject).to_not receive(:AskForCD)
+            expect(subject).to receive(:AddRepo).with(repo["url"], anything, anything)
+              .and_return(repo_id)
+            subject.AddPreselectedAddOnProducts(filelist)
+          end
+        end
+
+        context "and the product is not found in the CD/DVD" do
+          before do
+            allow(Yast::Pkg).to receive(:SourceProductData).with(repo_id)
+              .and_return(matching_product)
+            allow(Yast::Pkg).to receive(:SourceProductData).with(other_repo_id)
+              .and_return(other_product)
+          end
+
+          it "does not add the repository if the user cancels the dialog" do
+            allow(subject).to receive(:AddRepo).with(repo["url"], anything, anything)
+              .and_return(other_repo_id)
+            allow(subject).to receive(:AskForCD).and_return(nil)
+
+            expect(Yast::Pkg).to receive(:SourceDelete).with(other_repo_id)
+            expect(subject).to_not receive(:Integrate).with(other_repo_id)
+            subject.AddPreselectedAddOnProducts(filelist)
+          end
+
+          it "adds the product if the user points to a valid CD/DVD" do
+            allow(subject).to receive(:AddRepo).with(repo["url"], anything, anything)
+              .and_return(other_repo_id)
+            allow(subject).to receive(:AskForCD).and_return(cd_url)
+            allow(subject).to receive(:AddRepo).with(cd_url, anything, anything)
+              .and_return(repo_id)
+
+            expect(Yast::Pkg).to receive(:SourceDelete).with(other_repo_id)
+            expect(Yast::Pkg).to_not receive(:SourceDelete).with(repo_id)
+            expect(subject).to receive(:Integrate).with(repo_id)
+            subject.AddPreselectedAddOnProducts(filelist)
+          end
+        end
+      end
+
+      it "removes the product is the license is not accepted" do
+        allow(subject).to receive(:AddRepo).with(cd_url, anything, anything)
+          .and_return(repo_id)
+        expect(subject).to receive(:AcceptedLicenseAndInfoFile).and_return(false)
+        expect(Yast::Pkg).to receive(:SourceDelete).with(repo_id)
+        subject.AddPreselectedAddOnProducts(filelist)
+        expect(subject.add_on_products).to be_empty
+      end
+    end
+  end
 end

test/data/add_on_products_cd.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<add_on_products xmlns="http://www.suse.com/1.0/yast2ns"
+    xmlns:config="http://www.suse.com/1.0/configns">
+    <product_items config:type="list">
+        <product_item>
+            <url>cd:/?alias=FooXXX</url>
+            <path>/foo</path>
+            <priority config:type="integer">50</priority>
+        </product_item>
+    </product_items>
+</add_on_products>