Fix reinstall message (#3784)

* Changed informational messages because of confusing message on reinstallation.

Certbot prompts the user when it detects that an appropriately fresh certificate
is already available:

        You have an existing certificate that contains exactly the same domains you requested and isn't close to expiry.
        (ref: <path>)

        What would you like to do?
        -------------------------------------------------------------------------------
        1: Attempt to reinstall this existing certificate
        2: Renew & replace the cert (limit ~5 per 7 days)
        -------------------------------------------------------------------------------
        Select the appropriate number [1-2] then [enter] (press 'c' to cancel): 1

On selecting '1' (reinstall), the resulting message is:

        -------------------------------------------------------------------------------
        Your existing certificate has been successfully reinstalled, and the new
        certificate has been installed.

        The new certificate covers the following domains: https://<whatever>

        You should test your configuration at:
        https://www.ssllabs.com/ssltest/analyze.html?d=<whatever>
        -------------------------------------------------------------------------------

"Your existing certificate has been successfully reinstalled" <-- Okay

"and the new certificate has been installed." <-- Wait, what?

The issue appears to come from assumptions in certbot/certbot/main.py

It uses `len(lineage.available_versions("cert"))` to determine if this was a
fresh install or renewal, and then calls either `display_ops.success_renewal()`
(which produces the "existing certificate ... and the new certificate" language)
or `display_ops.success_installation()` (which has no messaging about existing
vs. new certificates).

The len(lineage) test isn't the right way to make this choice. The certificate's
lineage length doesn't imply anything about whether we've just obtained a new
certificate, because there is no new certificate in the case of a "reinstall"
action.

The new logic calls `display_ops.success_installation()` on all "reinstall"
actions, and otherwise employs the existing `len(lineage)` test.

Additionally the `display_ops.success_installation()` has been enhanced to
accept an action parameter, and has the message reworded slightly to make
sense regardless of the action passed. The messaging is mostly unchanged if it's
called without the action parameter:

Original message:
        -------------------------------------------------------------------------------
        Congratulations! You have successfully enabled https://<whatever>

        You should test your configuration at:
        https://www.ssllabs.com/ssltest/analyze.html?d=<whatever>
        -------------------------------------------------------------------------------

New message on initial install:
        -------------------------------------------------------------------------------
        Congratulations! You have successfully installed a certificate for
        https://<whatever>

        You should test your configuration at:
        https://www.ssllabs.com/ssltest/analyze.html?d=<whatever>
        -------------------------------------------------------------------------------

New message on re-install:
        -------------------------------------------------------------------------------
        Congratulations! You have successfully reinstalled a certificate for
        https://<whatever>

        You should test your configuration at:
        https://www.ssllabs.com/ssltest/analyze.html?d=<whatever>
        -------------------------------------------------------------------------------

* Typo in display message.

* Typo, characters transposed.

* undo changes to certbot/display/ops.py

* remove invalid todos

* Test success_installation() called for reinstall

* Simplify display_ops.success* functions

* refactor and expand run() tests

certbot/display/ops.py

@@ -209,8 +209,6 @@ def _choose_names_manually(prompt_prefix=""):
 def success_installation(domains):
     """Display a box confirming the installation of HTTPS.
 
-    .. todo:: This should be centered on the screen
-
     :param list domains: domain names which were enabled
 
     """
@@ -223,24 +221,20 @@ def success_installation(domains):
         pause=False)
 
 
-def success_renewal(domains, action):
+def success_renewal(domains):
     """Display a box confirming the renewal of an existing certificate.
 
-    .. todo:: This should be centered on the screen
-
     :param list domains: domain names which were renewed
-    :param str action: can be "reinstall" or "renew"
 
     """
     z_util(interfaces.IDisplay).notification(
-        "Your existing certificate has been successfully {3}ed, and the "
+        "Your existing certificate has been successfully renewed, and the "
         "new certificate has been installed.{1}{1}"
         "The new certificate covers the following domains: {0}{1}{1}"
         "You should test your configuration at:{1}{2}".format(
             _gen_https_names(domains),
             os.linesep,
-            os.linesep.join(_gen_ssl_lab_urls(domains)),
-            action),
+            os.linesep.join(_gen_ssl_lab_urls(domains))),
         pause=False)
 
 

certbot/main.py

@@ -528,10 +528,10 @@ def run(config, plugins):  # pylint: disable=too-many-branches,too-many-locals
 
     le_client.enhance_config(domains, config, lineage.chain)
 
-    if len(lineage.available_versions("cert")) == 1:
+    if action in ("newcert", "reinstall",):
         display_ops.success_installation(domains)
     else:
-        display_ops.success_renewal(domains, action)
+        display_ops.success_renewal(domains)
 
     _suggest_donation_if_appropriate(config, action)
 

certbot/tests/display/ops_test.py

@@ -321,7 +321,7 @@ class SuccessRenewalTest(unittest.TestCase):
     @classmethod
     def _call(cls, names):
         from certbot.display.ops import success_renewal
-        success_renewal(names, "renew")
+        success_renewal(names)
 
     @mock.patch("certbot.display.ops.z_util")
     def test_success_renewal(self, mock_util):

certbot/tests/main_test.py

@@ -13,9 +13,11 @@ from certbot import configuration
 from certbot import errors
 from certbot.plugins import disco as plugins_disco
 
+
 class MainTest(unittest.TestCase):
     def setUp(self):
         pass
+
     def tearDown(self):
         pass
 
@@ -27,6 +29,54 @@ class MainTest(unittest.TestCase):
         ret = main._handle_identical_cert_request(mock.Mock(), mock_lineage)
         self.assertEqual(ret, ("reinstall", mock_lineage))
 
+
+class RunTest(unittest.TestCase):
+    """Tests for certbot.main.run."""
+
+    def setUp(self):
+        self.domain = 'example.org'
+        self.patches = [
+            mock.patch('certbot.main._auth_from_domains'),
+            mock.patch('certbot.main.display_ops.success_installation'),
+            mock.patch('certbot.main.display_ops.success_renewal'),
+            mock.patch('certbot.main._init_le_client'),
+            mock.patch('certbot.main._suggest_donation_if_appropriate')]
+
+        self.mock_auth = self.patches[0].start()
+        self.mock_success_installation = self.patches[1].start()
+        self.mock_success_renewal = self.patches[2].start()
+        self.mock_init = self.patches[3].start()
+        self.mock_suggest_donation = self.patches[4].start()
+
+    def tearDown(self):
+        for patch in self.patches:
+            patch.stop()
+
+    def _call(self):
+        args = '-a webroot -i null -d {0}'.format(self.domain).split()
+        plugins = plugins_disco.PluginsRegistry.find_all()
+        config = configuration.NamespaceConfig(
+            cli.prepare_and_parse_args(plugins, args))
+
+        from certbot.main import run
+        run(config, plugins)
+
+    def test_newcert_success(self):
+        self.mock_auth.return_value = ('newcert', mock.Mock())
+        self._call()
+        self.mock_success_installation.assert_called_once_with([self.domain])
+
+    def test_reinstall_success(self):
+        self.mock_auth.return_value = ('reinstall', mock.Mock())
+        self._call()
+        self.mock_success_installation.assert_called_once_with([self.domain])
+
+    def test_renewal_success(self):
+        self.mock_auth.return_value = ('renewal', mock.Mock())
+        self._call()
+        self.mock_success_renewal.assert_called_once_with([self.domain])
+
+
 class ObtainCertTest(unittest.TestCase):
     """Tests for certbot.main.obtain_cert."""