Use the new ServiceWidget #26

dgdavid · 2018-07-27T12:56:04Z

More info: https://trello.com/c/uAe4i9Ru/107-5-fate319428-allow-socket-activation-widget

From	To

jreidinger · 2018-07-27T12:58:38Z

src/include/dhcp-server/dialogs.rb

+    # @return [Boolean] true if settings are saved successfully; false otherwise
+    def write_settings
+      if DhcpServer.Write
+        dhcp_service.save


this looks wrong for me. It should be part of DhcpServer. So this service belongs to backend and not UI. Ideally DhcpServer holds service and see below.

jreidinger · 2018-07-27T12:59:01Z

src/include/dhcp-server/widgets.rb

+    #
+    # @return [Yast2::SystemService] status service
+    def dhcp_service
+      @service ||= Yast2::SystemService.find(DhcpServer.ServiceName)


here I would like to ideally see just Yast::DhcpServer.service

jreidinger · 2018-07-27T12:59:43Z

src/include/dhcp-server/widgets.rb

-      else
-        nil
-      end
+      @status_widget ||= dhcp_service ? Yast2::ServiceWidget.new(dhcp_service) : nil


I think we should not silently ignore that service is missing and simply report error. As having DHCP server without service is useless.

coveralls · 2018-07-27T13:00:08Z

Coverage increased (+0.4%) to 16.345% when pulling 0620039 on feature/service-widget into 09ffd50 on master.

jreidinger · 2018-07-27T13:00:21Z

src/include/dhcp-server/widgets.rb

-    def restart_after_writing?
-      # If ServiceStatus is used, DhcpServer must be set to write-only
-      DhcpServer.GetWriteOnly() && status_widget && status_widget.reload_flag?
+      nil
    end


those methods is no longer needed.

jreidinger · 2018-07-27T13:01:43Z

src/include/dhcp-server/widgets.rb

        "init"   => fun_ref(method(:init_service_status), "void (string)"),
-        "handle" => fun_ref(method(:handle_service_status), "symbol (string, map)"),
        "store"  => fun_ref(method(:store_service_status), "void (string, map)")
      }


this is wrong. Here it is CWM and old hash based one. So ideally you should here just do:

service_widget = CWM::ServiceWidget.new(dhcp_service) @widgets[service_widget.widget_id] = service_widget.cwm_definitions

same as you can see in example for old hash based CWM.

mvidner · 2018-07-31T08:29:27Z

src/include/dhcp-server/widgets.rb

@@ -7,7 +7,9 @@
 # Authors:	Jiri Srain <jsrain@suse.cz>

 require "yast"
-require "ui/service_status"
+require "yast2/system_service"


Please add a dependency to the RPM spec file on the yast2.rpm version that provides these new APIs

joseivanlopez · 2018-08-07T07:27:07Z

src/include/dhcp-server/dialogs.rb

+      Wizard.RestoreHelp(help_text)
+
+      return :next if write_settings
+      return :back if Popup.YesNo(_("Saving the configuration failed. Change the settings?"))


Only for your information: we have a new Yast2::Popup class. Maybe a good opportunity to use it here. You can see an example in storage-ng

Good to know, thank you 👍

joseivanlopez · 2018-08-07T07:31:12Z

src/include/dhcp-server/dialogs.rb


      nil
    end

+    # Write DHCP server settings and save the service
+    #
+    # NOTE: currently, the DhpcServer is a Perl module, reason why the write of


NP: please, use @note instead of NOTE: to tell to YARD that this is a note. YARD will render it with a different style.

joseivanlopez · 2018-08-07T07:33:28Z

src/include/dhcp-server/dialogs.rb

+
+    # Validates and saves CWM widgets
+    #
+    # @param [Hash] event map that triggered saving


NP: please, document return value.

joseivanlopez · 2018-08-07T09:17:24Z

src/include/dhcp-server/dialogs.rb

+    # @return [Boolean] true if user decides to go back to change settings; false otherwise
+    def back_to_change_setting?
+      change_settings_message = _("Saving the configuration failed. Change the settings?")
+      Yast2::Popup.show(change_settings_message, headline: :warning, buttons: :yes_no)


Be careful. This Yast2::Popup returns a Symbol (:yes, :no), so you have to check it:

Yast2::Popup.show(change_settings_message, headline: :warning, buttons: :yes_no) == :yes

Ooops! Thank you.

joseivanlopez · 2018-08-07T09:19:08Z

package/yast2-dhcp-server.spec

-# UI::ServiceStatus
-BuildRequires:  yast2 >= 3.1.161
+# Yast2::ServiceWidget
+BuildRequires:  yast2 >= 4.1.0


Only BuildRequres ? I guess it also needs it as Requires.

joseivanlopez · 2018-08-07T09:20:07Z

src/modules/DhcpServer.pm

-	    Service->Stop ($SERVICE);
-	}
-	Service->Disable ($SERVICE);
+    if (Mode->auto() || Mode->config())


I would add a # TODO here with something similar to you have explained in the commit.

joseivanlopez

Please, update version and add changelog entry.

Adapt old test suite to new services management (2)

Despite services will be managed by Yast2::SystemService from now on, it is necessary to keep the related Perl code for `auto` and `config` modes until them will be also managed from Ruby.

Using the `keep_state` option of Yast2::SystemService#save method. See commit 7acbe06.

imobachgs · 2018-08-13T14:32:36Z

src/include/dhcp-server/dialogs.rb

-    # @return `abort if aborted and `next otherwise
+    #
+    # @return [Symbol] :next whether configuration is saved successfully
+    #                  :back if user decided go back to change settings


decided to go

imobachgs · 2018-08-13T14:33:03Z

src/include/dhcp-server/dialogs.rb

-        return :back
-      end
-      ret ? :next : :abort
+      help_text = @HELPS.fetch("write") { "" }


What about @HELPS.fetch("write", "") ?

Yep, it is more simple and readable.

I usually use the block notation because performance: in this case the empty string only will be created if Hash has no the write key. However, I must admit that in this case the difference it will be not significant. So, I will to change it :)

imobachgs · 2018-08-13T14:34:28Z

src/include/dhcp-server/dialogs.rb

-      CWM.save_current_widgets(event)
+      return nil unless validate_and_save_widgets(event)
+
+      help_text = @HELPS.fetch("write") { "" }


Same than above. You do not need to use a block here.

imobachgs · 2018-08-13T14:41:51Z

test/dialog_test.rb

+  let(:service) { instance_double(Yast2::SystemService, save: true, currently_active?: true) }
+
+  before do
+    allow_any_instance_of(TestDialog).to receive(:fun_ref)


I am not a big fan of allow_any_instance_of. Actually, its usage is discouraged. What about setting this expectation in the subject (if possible) or just adding the method to the TestDialog class? Anyway, we are working with legacy code, so it is not that bad.

I did not know that. Thank you.

I will to update/improve it

imobachgs · 2018-08-13T14:43:15Z

test/dialog_test.rb

+        dialog.WriteDialog
+      end
+
+      it "aks for changing the current settings" do


jreidinger reviewed Jul 27, 2018

View reviewed changes

dgdavid force-pushed the feature/service-widget branch 2 times, most recently from b7f35d8 to 2134df1 Compare July 31, 2018 08:09

dgdavid changed the title ~~WIP: replacing ServiceStatus widget by ServiceWidget~~ Use the new ServiceWidget Jul 31, 2018

mvidner reviewed Jul 31, 2018

View reviewed changes

dgdavid force-pushed the feature/service-widget branch 3 times, most recently from c44c308 to 548f7e0 Compare July 31, 2018 13:54

joseivanlopez reviewed Aug 7, 2018

View reviewed changes

dgdavid force-pushed the feature/service-widget branch from 4a091b5 to 5429ff4 Compare August 7, 2018 09:04

joseivanlopez reviewed Aug 7, 2018

View reviewed changes

dgdavid force-pushed the feature/service-widget branch 2 times, most recently from 68017b4 to 4bd94a0 Compare August 7, 2018 10:23

joseivanlopez reviewed Aug 7, 2018

View reviewed changes

dgdavid force-pushed the feature/service-widget branch 4 times, most recently from 85af08d to 489def5 Compare August 10, 2018 12:01

imobachgs and others added 7 commits August 13, 2018 11:30

Drop TriggeredBy property from old testsuite

fcdd8de

Bump version and update changes file

2764952

Merge pull request #30 from yast/drop-triggered-by-sle-15-ga

7f4762c

Adapt old test suite to new services management (2)

Use CWM::ServiceWidget instead ServiceStatus

f9d8fa3

Use CWM::ServiceWidget also in the wizard dialogs (first run)

0d326c4

Avoid DhcpServer Perl module to overlap with Yast2::SystemService

7acbe06

Despite services will be managed by Yast2::SystemService from now on, it is necessary to keep the related Perl code for `auto` and `config` modes until them will be also managed from Ruby.

Update from code review

f3a0c3b

dgdavid added 2 commits August 13, 2018 11:53

Update Yast2 required version

ca7cb79

Bump version and update changes file

7655ad6

dgdavid force-pushed the feature/service-widget branch from fff997c to 7655ad6 Compare August 13, 2018 10:55

dgdavid added 2 commits August 13, 2018 12:01

Merge branch 'SLE-15-GA' into feature/service-widget

9007e59

Manage the service status and start mode directly from Ruby code

9c4ca86

Using the `keep_state` option of Yast2::SystemService#save method. See commit 7acbe06.

dgdavid force-pushed the feature/service-widget branch 4 times, most recently from 74fd537 to 34fa445 Compare August 13, 2018 14:23

Drop no longer necessaries systemctl commands from old testsuite

0396ebc

dgdavid force-pushed the feature/service-widget branch from 34fa445 to 0396ebc Compare August 13, 2018 14:29

imobachgs reviewed Aug 13, 2018

View reviewed changes

Update from code review

0620039

imobachgs approved these changes Aug 13, 2018

View reviewed changes

dgdavid merged commit 6cf1c69 into master Aug 14, 2018

dgdavid deleted the feature/service-widget branch August 14, 2018 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new ServiceWidget #26

Use the new ServiceWidget #26

dgdavid commented Jul 27, 2018 •

edited

Loading

jreidinger Jul 27, 2018

jreidinger Jul 27, 2018

jreidinger Jul 27, 2018

coveralls commented Jul 27, 2018 •

edited

Loading

jreidinger Jul 27, 2018

jreidinger Jul 27, 2018

mvidner Jul 31, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez left a comment

imobachgs Aug 13, 2018

imobachgs Aug 13, 2018

dgdavid Aug 13, 2018

imobachgs Aug 13, 2018

imobachgs Aug 13, 2018

dgdavid Aug 13, 2018

imobachgs Aug 13, 2018

dgdavid Aug 13, 2018

Use the new ServiceWidget #26

Use the new ServiceWidget #26

Conversation

dgdavid commented Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseivanlopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgdavid commented Jul 27, 2018 •

edited

Loading

coveralls commented Jul 27, 2018 •

edited

Loading