Feat catch unlikely long names (#103)

* Catch longer than expected names and prompt confirmation to prevent invalid intent matches modifying a user profile
Prevent speaking back a 'username' that is a UID

* Update skill.json

* Update unit tests to use shared base class

* Fix breaking change to unit test class

---------

Co-authored-by: Daniel McKnight <[email protected]>
Co-authored-by: NeonDaniel <[email protected]>

__init__.py

@@ -423,13 +423,13 @@ def handle_say_my_name(self, message: Message):
             return
         utterance = message.data.get("utterance")
         profile = get_user_prefs(message)
-
+        # a 32-char username is a generated UID, not a real username
+        real_username = len(profile["user"]["username"]) < 32
         if not any((profile["user"]["first_name"],
                     profile["user"]["middle_name"],
                     profile["user"]["last_name"],
                     profile["user"]["preferred_name"],
-                    profile["user"]["full_name"],
-                    profile["user"]["username"])):
+                    profile["user"]["full_name"])) and not real_username:
             # TODO: Use get_response to ask for the user's name
             self.speak_dialog(
                 "name_not_known",
@@ -680,11 +680,14 @@ def _on_close(message):
                     .require("rx_name").build())
     def handle_set_my_name(self, message: Message):
         """
-        Handle a request to set a user's name
+        Handle a request to set a user's name. Some considerations for name
+        parsing can be found from w3c:
+        https://www.w3.org/International/questions/qa-personal-names
         :param message: Message associated with request
         """
         if not self.neon_in_request(message):
             return
+        confirmed = True
         utterance = message.data.get("utterance")
         name = message.data.get("rx_setting") or message.data.get("rx_name")
         if self.voc_match(utterance, "first_name"):
@@ -708,13 +711,23 @@ def handle_set_my_name(self, message: Message):
 
         user_profile = get_user_prefs(message)["user"]
 
-        if request:
+        # Catch an invalid intent match
+        if (request and len(name.split()) > 3) or len(name.split()) > 4:
+            LOG.warning(f"'{name}' does not look like a {request} name.")
+            confirmed = self.ask_yesno("name_confirm_change",
+                                       {"position": self.translate(
+                                           f"word_{request or 'name'}"),
+                                        "name": name})
+
+        if not confirmed:
+            self.speak_dialog("name_not_confirmed", private=True)
+        elif request:
             if name == user_profile[request]:
                 self.speak_dialog(
                     "name_not_changed",
                     {"position": self.translate(f"word_{request}"),
                      "name": name}, private=True)
-            else:
+            elif confirmed:
                 name_parts = (name if request == n else user_profile.get(n)
                               for n in ("first_name", "middle_name",
                                         "last_name"))
@@ -1085,6 +1098,7 @@ def _get_name_parts(name: str, user_profile: dict) -> dict:
                     "last_name": name_parts[2]}
         else:
             LOG.warning(f"Longer name than expected: {name}")
+            # TODO: Better logic here
             name = {"first_name": name_parts[0],
                     "middle_name": name_parts[1],
                     "last_name": " ".join(name_parts[2:])}

locale/en-us/dialog/name_confirm_change.dialog

@@ -0,0 +1 @@
+Would you like to update your {{position}} to {{name}}?

locale/en-us/dialog/name_not_confirmed.dialog

@@ -0,0 +1 @@
+Okay, no changes have been made.

requirements/test.txt

@@ -0,0 +1 @@
+neon-minerva~=0.0, >=0.0.2a1

setup.py

@@ -89,6 +89,7 @@ def find_resource_files():
     url=f'https://github.com/NeonGeckoCom/{SKILL_NAME}',
     license='BSD-3-Clause',
     install_requires=get_requirements("requirements.txt"),
+    extras_require={"test": get_requirements("requirements/test.txt")},
     author='Neongecko',
     author_email='[email protected]',
     long_description=long_description,

skill.json

@@ -25,7 +25,6 @@
         "Tell me my name.",
         "Tell me my first name.",
         "Tell me my last name.",
-        "Tell me my username.",
         "Tell me my email address.",
         "Where am I?",
         "My birthday is...",

test/test_resources.yaml

@@ -138,6 +138,8 @@ dialog:
   - 'word_email_title'
   - 'date_format_already_set'
   - 'date_format_changed'
+  - 'name_confirm_change'
+  - 'name_not_confirmed'
 # regex entities, not necessarily filenames
 regex:
   - 'rx_language'

test/test_skill.py

@@ -25,30 +25,24 @@
 # LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
 # NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 # SOFTWARE,  EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import json
-import os
-import shutil
+
 import unittest
+import mock
 
 from copy import deepcopy
 from datetime import datetime
-from os import mkdir
-from os.path import dirname, join, exists
 from typing import Optional
-
-import mock
 from dateutil.tz import gettz
 from mock import Mock
 from mock.mock import call
 from neon_utils.user_utils import get_user_prefs
 from neon_utils.language_utils import SupportedLanguages
-from ovos_utils.messagebus import FakeBus
 from ovos_bus_client import Message
 
-from mycroft.skills.skill_loader import SkillLoader
+from neon_minerva.tests.skill_unit_test_base import SkillTestCase
 
 
-class TestSkill(unittest.TestCase):
+class TestSkill(SkillTestCase):
     test_message = Message("test", {}, {"neon_in_request": True})
     default_config = deepcopy(get_user_prefs())
     default_config['location']['country'] = "United States"
@@ -57,35 +51,10 @@ class TestSkill(unittest.TestCase):
     @mock.patch('neon_utils.language_utils.get_supported_languages')
     def setUpClass(cls, get_langs) -> None:
         get_langs.return_value = SupportedLanguages({'en'}, {'en'}, {'en'})
-        # Define a directory to use for testing
-        cls.test_fs = join(dirname(__file__), "skill_fs")
-        if not exists(cls.test_fs):
-            mkdir(cls.test_fs)
-        os.environ["NEON_CONFIG_PATH"] = cls.test_fs
-
-        bus = FakeBus()
-        bus.run_in_thread()
-        skill_loader = SkillLoader(bus, dirname(dirname(__file__)))
-        skill_loader.load()
-        cls.skill = skill_loader.instance
-
-        # Override the configuration and fs paths to use the test directory
-        cls.skill.settings_write_path = cls.test_fs
-        cls.skill.file_system.path = cls.test_fs
-        cls.skill._init_settings()
-        cls.skill.initialize()
-
-        # Override speak and speak_dialog to test passed arguments
-        cls.skill.speak = Mock()
-        cls.skill.speak_dialog = Mock()
-
-    @classmethod
-    def tearDownClass(cls) -> None:
-        shutil.rmtree(cls.test_fs)
+        SkillTestCase.setUpClass()
 
     def setUp(self):
-        self.skill.speak.reset_mock()
-        self.skill.speak_dialog.reset_mock()
+        SkillTestCase.setUp(self)
         self.user_config = deepcopy(self.default_config)
 
     def test_00_skill_init(self):
@@ -979,6 +948,74 @@ def test_handle_set_my_name(self):
         self.assertEqual(test_message.context["user_profiles"][0]
                          ["user"]["preferred_name"], "Dan")
 
+        real_ask_yesno = self.skill.ask_yesno
+        self.skill.ask_yesno = Mock(return_value=False)
+
+        # Test first name too long, unconfirmed
+        test_message.data['utterance'] = ("my first name is some misfired "
+                                          "intent match")
+        test_message.data["rx_setting"] = "some misfired intent match"
+        self.skill.handle_set_my_name(test_message)
+        self.skill.ask_yesno.assert_called_with("name_confirm_change",
+                                                {"position": "first name",
+                                                 "name": test_message.data[
+                                                     "rx_setting"].title()})
+        self.skill.speak_dialog.assert_called_with("name_not_confirmed",
+                                                   private=True)
+
+        # Test full name too long, unconfirmed
+        test_message.data['utterance'] = ("my name is some other misfired "
+                                          "intent match")
+        test_message.data["rx_setting"] = "some other misfired intent match"
+        self.skill.handle_set_my_name(test_message)
+        self.skill.ask_yesno.assert_called_with("name_confirm_change",
+                                                {"position": "name",
+                                                 "name": test_message.data[
+                                                     "rx_setting"].title()})
+        self.skill.speak_dialog.assert_called_with("name_not_confirmed",
+                                                   private=True)
+
+        self.skill.ask_yesno.return_value = True
+        # Test full name too long, confirmed
+        test_message.data['utterance'] = ("my name is José Eduardo Santos "
+                                          "Tavares Melo Silva")
+        test_message.data["rx_setting"] = "José Eduardo Santos Tavares Melo Silva"
+        self.skill.handle_set_my_name(test_message)
+        self.skill.ask_yesno.assert_called_with("name_confirm_change",
+                                                {"position": "name",
+                                                 "name": test_message.data[
+                                                     "rx_setting"].title()})
+        self.skill.speak_dialog.assert_called_with(
+            "name_set_full",
+            {"nick": test_message.context["user_profiles"][0]["user"]
+             ["preferred_name"],
+             "name": test_message.data["rx_setting"]}, private=True)
+        self.assertEqual(test_message.context["user_profiles"][0]
+                         ["user"]["full_name"], test_message.data["rx_setting"])
+        self.assertEqual(test_message.context["user_profiles"][0]
+                         ["user"]["first_name"], "José")
+        self.assertEqual(test_message.context["user_profiles"][0]
+                         ["user"]["middle_name"], "Eduardo")
+        self.assertEqual(test_message.context["user_profiles"][0]
+                         ["user"]["last_name"], "Santos Tavares Melo Silva")
+
+        # Test last name too long, confirmed no change
+        test_message.data['utterance'] = ("my last name is Santos "
+                                          "Tavares Melo Silva")
+        test_message.data["rx_setting"] = "Santos Tavares Melo Silva"
+        self.skill.handle_set_my_name(test_message)
+        self.skill.ask_yesno.assert_called_with("name_confirm_change",
+                                                {"position": "last name",
+                                                 "name": test_message.data[
+                                                     "rx_setting"].title()})
+        self.skill.speak_dialog.assert_called_with("name_not_changed",
+                                                   {"position": "last name",
+                                                    "name": test_message.data[
+                                                        "rx_setting"]},
+                                                   private=True)
+
+        self.skill.ask_yesno = real_ask_yesno
+
     def test_handle_say_my_language_settings(self):
         test_profile = self.user_config
         test_profile["user"]["username"] = "test_user"