Update from code review

library/packages/src/lib/y2packager/license.rb

@@ -96,8 +96,8 @@ def cache
     # Bear in mind that `fetcher` will be ignored if `content` is specified.
     #
     # @param product_name [String] Product name to retrieve license information
-    # @param content      [String] License content. If this argument is given, this
-    #   string is used as the license's content (and `product_name` is ignored).
+    # @param content      [String] License content. When given, this string is used as the license's
+    #   content, ignoring the `product_name`
     # @param fetcher      [LicensesFetchers::Base] The license's fetcher
     # @param handler      [LicensesHandlers::Base] The license's handler
     def initialize(product_name: nil, content: nil, fetcher: nil, handler: nil)

library/packages/src/lib/y2packager/licenses_fetchers.rb

@@ -22,6 +22,8 @@ module Y2Packager
   module LicensesFetchers
     include Yast::Logger
 
+    # Candidate sources to retrieve the license content. Note that order matters because it will be
+    # chosen the first source able to fetch the content.
     KNOWN_SOURCES = [:libzypp, :rpm].freeze
 
     # Return the proper license fetcher

library/packages/src/lib/y2packager/licenses_fetchers/base.rb

@@ -40,10 +40,6 @@ def found?
         !default_content.empty?
       end
 
-      def content(lang)
-        return @default_content if default_lang?(lang) && @default_content
-      end
-
     private
 
       # Return (and caches) the content found for the default language

library/packages/src/lib/y2packager/licenses_fetchers/libzypp.rb

@@ -23,7 +23,7 @@ class Libzypp < Base
       #
       # @return [String, nil] Product's license; nil if the product or the license were not found.
       def content(lang)
-        super
+        return @default_content if default_lang?(lang) && @default_content
 
         Yast::Pkg.PrdGetLicenseToConfirm(product_name, lang)
       end

library/packages/src/lib/y2packager/licenses_fetchers/rpm.rb

@@ -23,7 +23,7 @@ class Rpm < Base
       #
       # @return [String, nil] Product's license; nil if the product or the license were not found
       def content(lang)
-        super
+        return @default_content if default_lang?(lang) && @default_content
 
         if package.nil?
           log.info("No package found for #{product_name}")
@@ -50,9 +50,13 @@ def locales
             package.extract_to(tmpdir)
             # TODO: Use rpm -qpl file.rpm instead?
             license_files = Dir.glob(File.join(tmpdir, "**", "LICENSE.*.TXT"), File::FNM_CASEFOLD)
+            # NOTE: despite the use of the case-insensitive flag, the captured group will be
+            # returned as it is.
             languages = license_files.map { |path| path[/LICENSE.(\w*).TXT/i, 1] }
             languages << DEFAULT_LANG
             languages.compact.uniq
+          ensure
+            FileUtils.remove_entry_secure(tmpdir)
           end
       end
 
@@ -71,72 +75,81 @@ def locales
       # @return [Array<String, String>, nil] Array containing content and language code
       def license_content_for(lang)
         tmpdir = Dir.mktmpdir
+        package.extract_to(tmpdir)
+        license_file = license_path(tmpdir, lang) || fallback_path(tmpdir)
 
-        begin
-          package.extract_to(tmpdir)
-          license_file = license_path(tmpdir, lang) || fallback_path(tmpdir)
+        if license_file.nil?
+          log.error("#{lang} license file not found for #{product_name}")
 
-          if license_file.nil?
-            log.error("License file not found")
-
-            return
-          end
-
-          File.read(license_file)
-        ensure
-          FileUtils.remove_entry_secure(tmpdir)
+          return
         end
+
+        File.read(license_file)
+      ensure
+        FileUtils.remove_entry_secure(tmpdir)
       end
 
-      # Return license file path for a given package and language
+      # Return license file path for the given languages
       #
       # When a license for a language "xx_XX" is not found, it will fallback to "xx".
       #
       # @param directory [String] Directory where licenses were uncompressed
-      # @param lang      [String] Searched language
+      # @param lang      [String] Searched translation
       #
-      # @return [Array<String, String>] Array containing the path and language code
+      # @return [String, lang] The first licence path for given languages or nil
       def license_path(directory, lang)
         candidate_langs = [lang]
         candidate_langs << lang.split("_", 2).first if lang
         candidate_langs.uniq!
 
         log.info("Searching for a #{candidate_langs.join(",")} license translations in #{directory}")
 
-        Dir.glob(
-          File.join(directory, "**", "LICENSE.{#{candidate_langs.join(",")}}.TXT"),
-          File::FNM_CASEFOLD
-        ).first
+        find_path_for(directory, "LICENSE.{#{candidate_langs.join(",")}}.TXT")
       end
 
       # Fallback license file
       FALLBACK_LICENSE_FILE = "LICENSE.TXT".freeze
 
+      # Return the fallback license file path
+      #
+      # Looking for a license file without language code
+      #
+      # @param directory [String] Directory where licenses were uncompressed
+      #
+      # @return [String, nil] The fallback license path
       def fallback_path(directory)
         log.info("Searching for a fallback #{FALLBACK_LICENSE_FILE} file in #{directory}")
 
-        Dir.glob(
-          File.join(directory, "**", FALLBACK_LICENSE_FILE),
-          File::FNM_CASEFOLD
-        ).first
+        find_path_for(directory, FALLBACK_LICENSE_FILE)
+      end
+
+      # Return the path for the given file in specified directory
+      #
+      # @param directory [String] Directory where licenses were uncompressed
+      # @param file      [String] Searched file
+      #
+      # @return [String, nil] The file path; nil if was not found
+      def find_path_for(directory, file)
+        Dir.glob(File.join(directory, "**", file), File::FNM_CASEFOLD).first
       end
 
       # Valid statuses for packages containing licenses
       AVAILABLE_STATUSES = [:available, :selected].freeze
 
-      # Find the latest available/selected product package
+      # Find the highest version of available/selected product package
       #
       # @return [Y2Packager::Package, nil] Package containing licenses; nil if not found
       def package
         return nil if package_name.nil?
 
-        @package ||=
-          Y2Packager::Package
-          .find(package_name)
-          .compact
-          .select { |i| AVAILABLE_STATUSES.include?(i.status) }
-          .sort { |a, b| Yast::Pkg.CompareVersions(a.version, b.version) }
-          .last
+        @package ||= begin
+          found_packages = Y2Packager::Package.find(package_name)
+          return unless found_packages
+
+          found_packages.select { |i| AVAILABLE_STATUSES.include?(i.status) }
+                        .sort { |a, b| Yast::Pkg.CompareVersions(a.version, b.version) }
+                        .last
+        end
       end
 
       # Find the package name
@@ -145,7 +158,9 @@ def package
       def package_name
         return @package_name if @package_name
 
-        package_properties = Yast::Pkg.ResolvableProperties(product_name, :product, "").first || {}
+        package_properties = Yast::Pkg.ResolvableProperties(product_name, :product, "")
+        package_properties = package_properties.find { |props| props.key?("product_package") }
+        package_properties ||= {}
 
         @package_name = package_properties.fetch("product_package", nil)
       end

library/packages/src/lib/y2packager/licenses_handlers.rb

@@ -25,7 +25,7 @@ module LicensesHandlers
     # @param fetcher      [LicensesFetchers::Base] Fetcher used as source to fetch license
     # @param product_name [String]                 Product's name
     #
-    # @return [Object]
+    # @return [LicenseHandlers::Base]
     def self.for(fetcher, product_name)
       type = fetcher.class.name.split("::").last
       klass = const_get(type.to_s.capitalize)

library/packages/src/lib/y2packager/licenses_handlers/rpm.rb

@@ -25,11 +25,16 @@ class Rpm < Base
       #
       # @return [Boolean] true if the license acceptance is required
       def confirmation_required?
-        @package ||= find_package
+        return false unless package
 
-        return false unless @package
+        begin
+          tmpdir = Dir.mktmpdir
+          package.extract_to(tmpdir)
 
-        !ignore_acceptance?
+          Dir.glob(File.join(tmpdir, "**", NO_ACCEPTANCE_FILE), File::FNM_CASEFOLD).empty?
+        ensure
+          FileUtils.remove_entry_secure(tmpdir)
+        end
       end
 
       # Set the license confirmation for the product
@@ -45,34 +50,23 @@ def confirmation=(confirmed)
 
     private
 
-      attr_reader :package
-
-      def ignore_acceptance?
-        tmpdir = Dir.mktmpdir
-
-        begin
-          package.extract_to(tmpdir)
-
-          Dir.glob(File.join(tmpdir, "**", NO_ACCEPTANCE_FILE), File::FNM_CASEFOLD).any?
-        ensure
-          FileUtils.remove_entry_secure(tmpdir)
-        end
-      end
-
       # Valid statuses for packages containing licenses
       AVAILABLE_STATUSES = [:available, :selected].freeze
 
-      # Find the latest available/selected product package
+      # Find the highest version of available/selected product package
       #
       # @return [Y2Packager::Package, nil] Package containing licenses; nil if not found
-      def find_package
+      def package
         return nil if package_name.nil?
 
-        Y2Packager::Package
-          .find(package_name)
-          .select { |i| AVAILABLE_STATUSES.include?(i.status) }
-          .sort { |a, b| Yast::Pkg.CompareVersions(a.version, b.version) }
-          .last
+        @package ||= begin
+          found_packages = Y2Packager::Package.find(package_name)
+          return unless found_packages
+
+          found_packages.select { |i| AVAILABLE_STATUSES.include?(i.status) }
+                        .sort { |a, b| Yast::Pkg.CompareVersions(a.version, b.version) }
+                        .last
+        end
       end
 
       # Find the package name
@@ -81,7 +75,9 @@ def find_package
       def package_name
         return @package_name if @package_name
 
-        package_properties = Yast::Pkg.ResolvableProperties(product_name, :product, "").first || {}
+        package_properties = Yast::Pkg.ResolvableProperties(product_name, :product, "")
+        package_properties = package_properties.find { |props| props.key?("product_package") }
+        package_properties ||= {}
 
         @package_name = package_properties.fetch("product_package", nil)
       end

library/packages/test/y2packager/licenses_fetchers/rpm_test.rb

@@ -26,6 +26,7 @@
   let(:package_properties) { [{ "product_package" => package_name }] }
   let(:package_status) { :selected }
   let(:package) { instance_double(Y2Packager::Package, status: package_status, extract_to: nil) }
+  let(:found_packages) { [package] }
 
   before do
     allow(Yast::Pkg).to receive(:ResolvableProperties)
@@ -34,7 +35,7 @@
 
     allow(Y2Packager::Package).to receive(:find)
       .with(package_name)
-      .and_return([package])
+      .and_return(found_packages)
   end
 
   it_behaves_like "a fetcher"
@@ -106,7 +107,7 @@
     end
 
     context "when package is not found" do
-      let(:package) { nil }
+      let(:found_packages) { nil }
       let(:available_license_files) { [] }
 
       it "returns an empty list" do

library/packages/test/y2packager/licenses_handlers/rpm_test.rb

@@ -28,7 +28,7 @@
       allow(Dir).to receive(:glob).and_return(found_paths)
       allow(File).to receive(:join)
       allow(FileUtils).to receive(:remove_entry_secure)
-      allow(subject).to receive(:find_package).and_return(package)
+      allow(subject).to receive(:package).and_return(package)
     end
 
     context "when 'no-acceptance-neeed' file is present" do