Merge e6bcc3e into 25f51ac

package/yast2-ntp-client.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Tue Mar  3 16:38:54 CET 2020 - schubi@suse.de
+
+- Supporting more than one ntp server in the time setting module.
+  (bsc#1164547)
+- 4.1.10
+
 -------------------------------------------------------------------
 Thu Jul  4 10:29:27 UTC 2019 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
 

package/yast2-ntp-client.spec

@@ -17,7 +17,7 @@
 
 
 Name:           yast2-ntp-client
-Version:        4.1.9
+Version:        4.1.10
 Release:        0
 Summary:        YaST2 - NTP Client Configuration
 License:        GPL-2.0-or-later

src/clients/ntp-client_proposal.rb

@@ -128,7 +128,7 @@ def ui_help_text
     end
 
     def ui_enable_disable_widgets(enabled)
-      UI.ChangeWidget(Id(:ntp_address), :Enabled, enabled)
+      UI.ChangeWidget(Id(:ntp_address), :Enabled, enabled) if select_ntp_server
       UI.ChangeWidget(Id(:run_service), :Enabled, enabled)
       # FIXME: With chronyd, we cannot synchronize if the service is already
       # running, we could force a makestep in this case, but then the button
@@ -175,8 +175,6 @@ def ValidateSingleServer(ntp_server)
     end
 
     def MakeProposal
-      ntp_items = []
-
       # On the running system, read all the data, otherwise firewall and other
       # stuff outside ntp.conf may not be initialized correctly (#375877)
       if !Stage.initial
@@ -188,18 +186,14 @@ def MakeProposal
         NtpClient.ProcessNtpConf
       end
 
-      if NtpClient.config_has_been_read || NtpClient.ntp_selected
-        log.info("ntp_items will be filled from /etc/chrony.conf")
-        # grr, GUNS means all of them are used and here we just pick one
-        ntp_items = configured_ntp_items
-      end
-
-      ntp_items = fallback_ntp_items if ntp_items.empty?
-      # Once read or proposed any config we consider it as read (bnc#427712)
-      NtpClient.config_has_been_read = true
+      if select_ntp_server
+        ntp_items = fallback_ntp_items
+        # Once read or proposed any config we consider it as read (bnc#427712)
+        NtpClient.config_has_been_read = true
 
-      log.info "ntp_items :#{ntp_items}"
-      UI.ChangeWidget(Id(:ntp_address), :Items, ntp_items)
+        log.info "ntp_items :#{ntp_items}"
+        UI.ChangeWidget(Id(:ntp_address), :Items, ntp_items)
+      end
 
       nil
     end
@@ -208,19 +202,38 @@ def MakeProposal
     # @return should our radio button be selected
     def ui_init(rp, first_time)
       rp = deep_copy(rp)
+
+      if select_ntp_server
+        ntp_server_widget = ComboBox(
+          Id(:ntp_address),
+          Opt(:editable, :hstretch),
+          # TRANSLATORS: combo box label
+          _("&NTP Server Address")
+        )
+      else
+        # Only show all ntp servers
+        text = _("Synchronization Servers:\n").dup
+        counter = NtpClient.GetUsedNtpServers.size > 3 ? 3 : NtpClient.GetUsedNtpServers.size
+        counter.times do |i|
+          text << NtpClient.GetUsedNtpServers[i]
+          text << "\n"
+        end
+        if NtpClient.GetUsedNtpServers.size > 3
+          # TRANSLATOR %1 number of additional servers
+          text << Builtins.sformat(_("... (%1 more servers)"),
+            NtpClient.GetUsedNtpServers.size - counter)
+        end
+        ntp_server_widget = Label(text)
+      end
+
       cont = VBox(
         VSpacing(0.5),
         HBox(
           HSpacing(3),
           HWeight(
             1,
             Left(
-              ComboBox(
-                Id(:ntp_address),
-                Opt(:editable, :hstretch),
-                # TRANSLATORS: combo box label
-                _("&NTP Server Address")
-              )
+              ntp_server_widget
             )
           ),
           HWeight(
@@ -289,16 +302,19 @@ def ui_init(rp, first_time)
 
     def AskUser
       ret = nil
-      ntp_server = Convert.to_string(UI.QueryWidget(Id(:ntp_address), :Value))
-      if !ValidateSingleServer(ntp_server)
-        ret = :invalid_hostname
-      else
+      if select_ntp_server
+        # The user can select ONE ntp server.
+        # So we Initialize the ntp client module with the selected ntp server.
+        ntp_server = Convert.to_string(UI.QueryWidget(Id(:ntp_address), :Value))
+        return :invalid_hostname unless ValidateSingleServer(ntp_server)
         NtpClient.ntp_conf.clear_pools
         NtpClient.ntp_conf.add_pool(ntp_server)
-        retval = Convert.to_boolean(WFM.CallFunction("ntp-client"))
-        ret = :next if retval
-        MakeProposal()
       end
+      # Calling ntp client module.
+      ret = :next if WFM.CallFunction("ntp-client")
+      # Initialize the rest
+      MakeProposal()
+
       ret
     end
 
@@ -356,25 +372,41 @@ def Write(params)
       params.compact!
 
       ntp_server  = params.fetch("server", "")
-      ntp_servers = params.fetch("servers", [])
+      ntp_servers = params.fetch("servers", NtpClient.GetUsedNtpServers)
       run_service = params.fetch("run_service", NtpClient.run_service)
 
       # Get the ntp_server value from UI only if isn't present (probably wasn't given as parameter)
-      ntp_server = UI.QueryWidget(Id(:ntp_address), :Value) if ntp_server.strip.empty?
+      if ntp_server.strip.empty? && select_ntp_server
+        ntp_server = UI.QueryWidget(Id(:ntp_address), :Value) || ""
+      end
 
-      return :invalid_hostname unless ValidateSingleServer(ntp_server)
+      if !ntp_server.empty? && !ValidateSingleServer(ntp_server)
+        return :invalid_hostname
+      end
 
       add_or_install_required_package unless params["write_only"]
 
       WriteNtpSettings(ntp_servers, ntp_server, run_service) unless params["ntpdate_only"]
 
       return :success if params["write_only"]
 
-      # Only if network is running try to synchronize the ntp server
+      # Only if network is running try to synchronize
+      # the ntp server.
       if NetworkService.isNetworkRunning && !Service.Active(NtpClient.service_name)
-        Popup.ShowFeedback("", _("Synchronizing with NTP server..."))
-        exit_code = NtpClient.sync_once(ntp_server)
-        Popup.ClearFeedback
+        ntp_servers = [ntp_server]
+        if !select_ntp_server
+          # Taking also the rest of the ntp servers, configured in the ntp client module.
+          ntp_servers += NtpClient.GetUsedNtpServers unless NtpClient.GetUsedNtpServers.nil?
+        end
+        ntp_servers.delete("")
+        ntp_servers.uniq
+        exit_code = 0
+        ntp_servers.each do |server|
+          Popup.ShowFeedback("", _("Synchronizing with NTP server...") + server)
+          exit_code = NtpClient.sync_once(server)
+          Popup.ClearFeedback
+          break if exit_code.zero?
+        end
 
         return :ntpdate_failed unless exit_code.zero?
       end
@@ -392,6 +424,8 @@ def ui_handle(ui)
             UI.QueryWidget(Id(:ntp_address), :Value)
           )
         elsif rv == :next && !Stage.initial
+          # Updating UI for the changed ntp servers
+          ui_init(Id(:rp), false)
           # show the 'save' status after configuration
           UI.ChangeWidget(Id(:ntp_save), :Value, GetNTPEnabled())
         end
@@ -436,8 +470,11 @@ def ui_try_save
 
       rv = Write(argmap)
 
-      server = Convert.to_string(UI.QueryWidget(Id(:ntp_address), :Value))
+      # The user has not had the possibility to change the ntp server.
+      # So we are done here.
+      return true unless select_ntp_server
 
+      server = Convert.to_string(UI.QueryWidget(Id(:ntp_address), :Value))
       Builtins.y2milestone("ui_try_save argmap %1", argmap)
       if rv == :invalid_hostname
         handle_invalid_hostname(server)
@@ -486,13 +523,6 @@ def add_or_install_required_package
       end
     end
 
-    # Configured ntp servers Yast::Term items with the ntp address ID and label
-    #
-    # @return [Yast::Term] ntp address table Item
-    def configured_ntp_items
-      NtpClient.GetUsedNtpServers.map { |s| Item(Id(s), s) }
-    end
-
     # Public list of ntp servers Yast::Term items with the ntp address ID and
     # label
     #
@@ -514,16 +544,34 @@ def dhcp_ntp_items
     #
     # @return [Array<Yast::Term>] ntp address table Item
     def fallback_ntp_items
-      dhcp_items = dhcp_ntp_items
-
-      log.info("Nothing found in /etc/chrony.conf")
-      if dhcp_items.empty?
+      return @cached_fallback_ntp_items if @cached_fallback_ntp_items
+      @cached_fallback_ntp_items = dhcp_ntp_items
+      if !@cached_fallback_ntp_items.empty?
+        log.info("Proposing NTP server list provided by DHCP")
+      else
         log.info("Proposing current timezone-based NTP server list")
-        return timezone_ntp_items
+        @cached_fallback_ntp_items = timezone_ntp_items
       end
+      @cached_fallback_ntp_items
+    end
 
-      log.info("Proposing NTP server list provided by DHCP")
-      dhcp_items
+    # Checking if the user can select one ntp server from the list
+    # of proposed servers.
+    # It does not make sense if there are more than one ntp server
+    # defined.
+    #
+    # @return [Boolean] true if the user should select a server
+    def select_ntp_server
+      ret = NtpClient.GetUsedNtpServers.nil? || NtpClient.GetUsedNtpServers.empty?
+      # It could be that the user has defined an own ntp server in the ntp-client
+      # module which is not defined in the combo box. In that case we do not offer
+      # a selection. The user should go back to ntp-client to change it.
+      if NtpClient.GetUsedNtpServers.size == 1
+        ret = fallback_ntp_items.any? do |item|
+          item.params[1] == NtpClient.GetUsedNtpServers.first
+        end
+      end
+      ret
     end
   end
 end

test/ntp_client_proposal_test.rb

@@ -36,10 +36,11 @@
       allow(Yast::Report).to receive(:Error)
       allow(Yast::NetworkService).to receive(:isNetworkRunning).and_return(network_running)
       allow(Yast::Service).to receive(:Active).with(ntp_client.service_name).and_return(false)
+      allow(Yast::NtpClient).to receive(:dhcp_ntp_servers).and_return([])
     end
 
     context "with a not valid hostname" do
-      let(:ntp_server) { nil }
+      let(:ntp_server) { "not_valid" }
 
       it "does not write settings" do
         expect(subject).to_not receive(:WriteNtpSettings)
@@ -138,13 +139,13 @@
         let(:network_running) { true }
 
         it "returns :ntpdate_failed if synchronization fails" do
-          allow(Yast::NtpClient).to receive(:sync_once).with(ntp_server).and_return(1)
+          allow(Yast::NtpClient).to receive(:sync_once).and_return(1)
 
           expect(subject.Write(params)).to eq(:ntpdate_failed)
         end
 
         it "returns :success if synchronization was successfully" do
-          allow(Yast::NtpClient).to receive(:sync_once).with(ntp_server).and_return(0)
+          allow(Yast::NtpClient).to receive(:sync_once).and_return(0)
 
           expect(subject.Write(params)).to eq(:success)
         end
@@ -176,4 +177,44 @@
       end
     end
   end
+
+  describe "#select_ntp_server" do
+    context "there are already more than one ntp server defined" do
+      it "returns false" do
+        allow(Yast::NtpClient).to receive(:GetUsedNtpServers).and_return(["n1", "n2"])
+        expect(subject.send(:select_ntp_server)).to eq(false)
+      end
+    end
+
+    context "there is no ntp server defined" do
+      it "returns true" do
+        allow(Yast::NtpClient).to receive(:GetUsedNtpServers).and_return([])
+        expect(subject.send(:select_ntp_server)).to eq(true)
+      end
+    end
+
+    context "there is ONE ntp server defined" do
+      before do
+        allow(Yast::NtpClient).to receive(:dhcp_ntp_servers).and_return([])
+      end
+
+      context "and defined server is not in the selection list" do
+        it "returns false" do
+          allow(Yast::NtpClient).to receive(:GetUsedNtpServers).and_return(["not_found"])
+          expect(subject.send(:select_ntp_server)).to eq(false)
+        end
+      end
+
+      context "and defined server is in the selection list" do
+        it "returns true" do
+          allow(Yast::NtpClient).to receive(:GetNtpServersByCountry).and_return(
+            [Item(Id("de.pool.ntp.org"), "de.pool.ntp.org", true)]
+          )
+          allow(Yast::NtpClient).to receive(:GetUsedNtpServers).and_return(["de.pool.ntp.org"])
+          expect(subject.send(:select_ntp_server)).to eq(true)
+        end
+      end
+    end
+
+  end
 end