Low-impact cleanup of IDisplay (#4818)

Remove unused help-related display code. When NcursesDisplay was
removed[1], help was deprecated. This change removes the remaining
bits and pieces of code.

Remove unused escape-related display code. When NcursesDisplay was
removed[1], escape was deprecated. This change removes the remaining
bits and pieces of code.

Remove uses of unused menu parameters.

Remove unused default_status/default_state argument from checklist.
(This seems safe because not only is it unused, the parameter has
different names in the interface and implementation)

1 - d54cb76432

Resolves #4795.

certbot-apache/certbot_apache/display_ops.py

@@ -27,9 +27,7 @@ def select_vhost(domain, vhosts):
         return None
     while True:
         code, tag = _vhost_menu(domain, vhosts)
-        if code == display_util.HELP:
+        if code == display_util.OK:
-            _more_info_vhost(vhosts[tag])
-        elif code == display_util.OK:
             return vhosts[tag]
         else:
             return None
@@ -85,8 +83,7 @@ def _vhost_menu(domain, vhosts):
             "or Address of {0}.{1}Which virtual host would you "
             "like to choose?\n(note: conf files with multiple "
             "vhosts are not yet supported)".format(domain, os.linesep),
-            choices, help_label="More Info",
+            choices, force_interactive=True)
-            ok_label="Select", force_interactive=True)
     except errors.MissingCommandlineFlag:
         msg = ("Encountered vhost ambiguity but unable to ask for user guidance in "
                "non-interactive mode. Currently Certbot needs each vhost to be "
@@ -96,10 +93,3 @@ def _vhost_menu(domain, vhosts):
         raise errors.MissingCommandlineFlag(msg)
 
     return code, tag
-
-
-def _more_info_vhost(vhost):
-    zope.component.getUtility(interfaces.IDisplay).notification(
-        "Virtual Host Information:{0}{1}{0}{2}".format(
-            os.linesep, "-" * (display_util.WIDTH - 4), str(vhost)),
-        force_interactive=True)

certbot-apache/certbot_apache/tests/display_ops_test.py

@@ -43,13 +43,10 @@ class SelectVhostTest(unittest.TestCase):
     @certbot_util.patch_get_utility()
     def test_more_info_cancel(self, mock_util):
         mock_util().menu.side_effect = [
-            (display_util.HELP, 1),
-            (display_util.HELP, 0),
             (display_util.CANCEL, -1),
         ]
 
         self.assertEqual(None, self._call(self.vhosts))
-        self.assertEqual(mock_util().notification.call_count, 2)
 
     def test_no_vhosts(self):
         self.assertEqual(self._call([]), None)

certbot/cert_manager.py

@@ -157,7 +157,7 @@ def _get_certname(config, verb):
         if not choices:
             raise errors.Error("No existing certificates found.")
         code, index = disp.menu("Which certificate would you like to {0}?".format(verb),
-                                choices, ok_label="Select", flag="--cert-name",
+                                choices, flag="--cert-name",
                                 force_interactive=True)
         if code != display_util.OK or not index in range(0, len(choices)):
             raise errors.Error("User ended interaction.")

certbot/display/util.py

@@ -24,10 +24,10 @@ CANCEL = "cancel"
 """Display exit code for a user canceling the display."""
 
 HELP = "help"
-"""Display exit code when for when the user requests more help."""
+"""Display exit code when for when the user requests more help. (UNUSED)"""
 
 ESC = "esc"
-"""Display exit code when the user hits Escape"""
+"""Display exit code when the user hits Escape (UNUSED)"""
 
 
 def _wrap_lines(msg):
@@ -123,8 +123,8 @@ class FileDisplay(object):
             else:
                 logger.debug("Not pausing for user confirmation")
 
-    def menu(self, message, choices, ok_label="", cancel_label="",
+    def menu(self, message, choices, ok_label=None, cancel_label=None,
-             help_label="", default=None,
+             help_label=None, default=None,
              cli_flag=None, force_interactive=False, **unused_kwargs):
         # pylint: disable=unused-argument
         """Display a menu.
@@ -228,14 +228,13 @@ class FileDisplay(object):
                     ans.startswith(no_label[0].upper())):
                 return False
 
-    def checklist(self, message, tags, default_status=True, default=None,
+    def checklist(self, message, tags, default=None,
                   cli_flag=None, force_interactive=False, **unused_kwargs):
         # pylint: disable=unused-argument
         """Display a checklist.
 
         :param str message: Message to display to user
         :param list tags: `str` tags to select, len(tags) > 0
-        :param bool default_status: Not used for FileDisplay
         :param default: default value to return (if one exists)
         :param str cli_flag: option used to set this value with the CLI
         :param bool force_interactive: True if it's safe to prompt the user

certbot/interfaces.py

@@ -396,8 +396,8 @@ class IDisplay(zope.interface.Interface):
 
         """
 
-    def menu(message, choices, ok_label="OK",
+    def menu(message, choices, ok_label=None,
-             cancel_label="Cancel", help_label="",
+             cancel_label=None, help_label=None,
              default=None, cli_flag=None, force_interactive=False):
         """Displays a generic menu.
 
@@ -409,9 +409,9 @@ class IDisplay(zope.interface.Interface):
         :param choices: choices
         :type choices: :class:`list` of :func:`tuple` or :class:`str`
 
-        :param str ok_label: label for OK button
+        :param str ok_label: label for OK button (UNUSED)
-        :param str cancel_label: label for Cancel button
+        :param str cancel_label: label for Cancel button (UNUSED)
-        :param str help_label: label for Help button
+        :param str help_label: label for Help button (UNUSED)
         :param int default: default (non-interactive) choice from the menu
         :param str cli_flag: to automate choice from the menu, eg "--keep"
         :param bool force_interactive: True if it's safe to prompt the user
@@ -470,8 +470,7 @@ class IDisplay(zope.interface.Interface):
 
         """
 
-    def checklist(message, tags, default_state,
+    def checklist(message, tags, default=None, cli_args=None, force_interactive=False):
-                  default=None, cli_args=None, force_interactive=False):
         """Allow for multiple selections from a menu.
 
         When not setting force_interactive=True, you must provide a
@@ -479,7 +478,6 @@ class IDisplay(zope.interface.Interface):
 
         :param str message: message to display to the user
         :param list tags: where each is of type :class:`str` len(tags) > 0
-        :param bool default_status: If True, items are in a selected state by default.
         :param str default: default (non-interactive) state of the checklist
         :param str cli_flag: to automate choice from the menu, eg "--domains"
         :param bool force_interactive: True if it's safe to prompt the user

certbot/main.py

@@ -161,7 +161,7 @@ def _handle_identical_cert_request(config, lineage):
                "Renew & replace the cert (limit ~5 per 7 days)"]
 
     display = zope.component.getUtility(interfaces.IDisplay)
-    response = display.menu(question, choices, "OK", "Cancel",
+    response = display.menu(question, choices,
                             default=0, force_interactive=True)
     if response[0] == display_util.CANCEL:
         # TODO: Add notification related to command-line options for

certbot/plugins/selection.py

@@ -112,7 +112,7 @@ def choose_plugin(prepared, question):
     while True:
         disp = z_util(interfaces.IDisplay)
         code, index = disp.menu(
-            question, opts, help_label="More Info", force_interactive=True)
+            question, opts, force_interactive=True)
 
         if code == display_util.OK:
             plugin_ep = prepared[index]
@@ -123,13 +123,6 @@ def choose_plugin(prepared, question):
                     "was:\n\n{0}".format(plugin_ep.prepare()), pause=False)
             else:
                 return plugin_ep
-        elif code == display_util.HELP:
-            if prepared[index].misconfigured:
-                msg = "Reported Error: %s" % prepared[index].prepare()
-            else:
-                msg = prepared[index].init().more_info()
-            z_util(interfaces.IDisplay).notification(msg,
-                                                     force_interactive=True)
         else:
             return None
 

certbot/plugins/selection_test.py

@@ -137,13 +137,10 @@ class ChoosePluginTest(unittest.TestCase):
     @test_util.patch_get_utility("certbot.plugins.selection.z_util")
     def test_more_info(self, mock_util):
         mock_util().menu.side_effect = [
-            (display_util.HELP, 0),
-            (display_util.HELP, 1),
             (display_util.OK, 1),
         ]
 
         self.assertEqual(self.mock_stand, self._call())
-        self.assertEqual(mock_util().notification.call_count, 2)
 
     @test_util.patch_get_utility("certbot.plugins.selection.z_util")
     def test_no_choice(self, mock_util):

certbot/plugins/webroot.py

@@ -117,22 +117,11 @@ to serve all files under specified web root ({0})."""
             code, index = display.menu(
                 "Select the webroot for {0}:".format(domain),
                 ["Enter a new webroot"] + known_webroots,
-                help_label="Help", cli_flag=path_flag, force_interactive=True)
+                cli_flag=path_flag, force_interactive=True)
             if code == display_util.CANCEL:
                 raise errors.PluginError(
                     "Every requested domain must have a "
                     "webroot when using the webroot plugin.")
-            elif code == display_util.HELP:
-                display.notification(
-                    "To use the webroot plugin, you need to have an "
-                    "HTTP server running on this system serving files "
-                    "for the requested domain. Additionally, this "
-                    "server should be serving all files contained in a "
-                    "public_html or webroot directory. The webroot "
-                    "plugin works by temporarily saving necessary "
-                    "resources in the HTTP server's webroot directory "
-                    "to pass domain validation challenges.",
-                    force_interactive=True)
             else:  # code == display_util.OK
                 return None if index == 0 else known_webroots[index - 1]
 
@@ -141,10 +130,7 @@ to serve all files under specified web root ({0})."""
             _validate_webroot,
             "Input the webroot for {0}:".format(domain),
             force_interactive=True)
-        if code == display_util.HELP:
+        if code == display_util.CANCEL:
-            # Displaying help is not currently implemented
-            return None
-        elif code == display_util.CANCEL or code == display_util.ESC:
             return None
         else:  # code == display_util.OK
             return _validate_webroot(webroot)

certbot/plugins/webroot_test.py

@@ -84,10 +84,8 @@ class AuthenticatorTest(unittest.TestCase):
         self.config.webroot_map = {"otherthing.com": self.path}
 
         mock_display = mock_get_utility()
-        mock_display.menu.side_effect = ((display_util.HELP, -1),
+        mock_display.menu.side_effect = ((display_util.CANCEL, -1),)
-                                         (display_util.CANCEL, -1),)
         self.assertRaises(errors.PluginError, self.auth.perform, [self.achall])
-        self.assertTrue(mock_display.notification.called)
         self.assertTrue(mock_display.menu.called)
         for call in mock_display.menu.call_args_list:
             self.assertTrue(self.achall.domain in call[0][0])
@@ -103,8 +101,7 @@ class AuthenticatorTest(unittest.TestCase):
         mock_display = mock_get_utility()
         mock_display.menu.return_value = (display_util.OK, 0,)
         with mock.patch('certbot.display.ops.validated_directory') as m:
-            m.side_effect = ((display_util.HELP, -1),
+            m.side_effect = ((display_util.CANCEL, -1),
-                             (display_util.CANCEL, -1),
                              (display_util.OK, self.path,))
 
             self.auth.perform([self.achall])