Improvements from code review

- Yast.import at top level, like `require`
- Don't `include Yast`
- Less nesting
- Specify return value in a corner case
- UIShortcuts via require "yast/rspec"

src/lib/installation/select_system_role.rb

@@ -18,17 +18,15 @@
 
 require "yast"
 require "ui/installation_dialog"
+Yast.import "ProductControl"
+Yast.import "ProductFeatures"
 
 module Installation
   class SelectSystemRole < ::UI::InstallationDialog
-    include Yast
-
     def initialize
       super
 
       textdomain "installation"
-      Yast.import "ProductControl"
-      Yast.import "ProductFeatures"
     end
 
     def run
@@ -40,8 +38,6 @@ def run
       super
     end
 
-    public                      # called by parent class
-
     def dialog_title
       _("System Role")
     end
@@ -51,32 +47,27 @@ def help_text
     end
 
     def dialog_content
-      RadioButtonGroup(
-        Id(:roles),
-        VBox(
-          * ui_roles.map do |r|
-              VBox(
-                Left(RadioButton(Id(r[:id]), r[:label])),
-                HBox(
-                  HSpacing(4),
-                  Left(Label(r[:description]))
-                ),
-                VSpacing(2)
-              )
-            end
+      ui_roles = role_attributes.each_with_object(VBox()) do |r, vbox|
+        vbox << Left(RadioButton(Id(r[:id]), r[:label]))
+        vbox << HBox(
+          HSpacing(4),
+          Left(Label(r[:description]))
         )
-      )
+        vbox << VSpacing(2)
+      end
+
+      RadioButtonGroup(Id(:roles), ui_roles)
     end
 
     def create_dialog
       clear_role
       ok = super
-      UI.ChangeWidget(Id(:roles), :CurrentButton, ui_roles.first[:id])
+      Yast::UI.ChangeWidget(Id(:roles), :CurrentButton, role_attributes.first[:id])
       ok
     end
 
     def next_handler
-      role_id = UI.QueryWidget(Id(:roles), :CurrentButton)
+      role_id = Yast::UI.QueryWidget(Id(:roles), :CurrentButton)
       apply_role(role_id)
 
       super
@@ -85,15 +76,15 @@ def next_handler
     private
 
     def clear_role
-      ProductFeatures.ClearOverlay
+      Yast::ProductFeatures.ClearOverlay
     end
 
     def apply_role(role_id)
       log.info "Applying system role '#{role_id}'"
       features = raw_roles.find { |r| r["id"] == role_id }
       features = features.dup
       features.delete("id")
-      ProductFeatures.SetOverlay(features)
+      Yast::ProductFeatures.SetOverlay(features)
     end
 
     # the contents is an overlay for ProductFeatures sections
@@ -103,17 +94,17 @@ def apply_role(role_id)
     # ]
     # @return [Array<Hash{String => Object}>]
     def raw_roles
-      ProductControl.productControl.fetch("system_roles", [])
+      Yast::ProductControl.productControl.fetch("system_roles", [])
     end
 
-    def ui_roles
+    def role_attributes
       raw_roles.map do |r|
         id = r["id"]
 
         {
           id:          id,
-          label:       ProductControl.GetTranslatedText(id),
-          description: ProductControl.GetTranslatedText(id + "_description")
+          label:       Yast::ProductControl.GetTranslatedText(id),
+          description: Yast::ProductControl.GetTranslatedText(id + "_description")
         }
       end
     end

test/select_system_role_test.rb

@@ -23,15 +23,13 @@
           .and_return("system_roles" => [])
       end
 
-      it "does not display dialog" do
+      it "does not display dialog, and returns :auto" do
         expect(Yast::Wizard).to_not receive(:SetContents)
-        subject.run
+        expect(subject.run).to eq(:auto)
       end
     end
 
     context "when some roles are defined" do
-      include Yast::UIShortcuts
-
       let(:control_file_roles) do
         [
           { "id" => "foo", "partitioning" => { "format" => true } },

test/test_helper.rb

@@ -3,6 +3,7 @@
 ENV["Y2DIR"] = y2dirs.unshift(srcdir).join(":")
 
 require "yast"
+require "yast/rspec"
 
 # fake AutoinstConfigClass class which is not supported by Ubuntu
 module Yast