diff --git a/neon_utils/user_utils.py b/neon_utils/user_utils.py index 03a62fa5..6edcb0fa 100644 --- a/neon_utils/user_utils.py +++ b/neon_utils/user_utils.py @@ -26,6 +26,7 @@ # NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from collections.abc import MutableMapping from os.path import isfile, join from ovos_bus_client import Message, MessageBusClient from ovos_utils.log import LOG @@ -72,6 +73,21 @@ def apply_local_user_profile_updates(updated_profile: dict, _DEFAULT_USER_CONFIG = user_config.content +def _drop_null_values(profile: MutableMapping) -> MutableMapping: + """ + Recursively remove keys with null values so a merge against default + configuration fills them, rather than treating null as a set value. + :param profile: user profile to modify and return + :returns: profile with all null-valued keys removed + """ + for key, value in list(profile.items()): + if isinstance(value, MutableMapping): + _drop_null_values(value) + elif value is None: + profile.pop(key) + return profile + + @resolve_message def get_user_prefs(message: Message = None) -> dict: """ @@ -104,6 +120,9 @@ def get_user_prefs(message: Message = None) -> dict: for profile in message.context.get(profile_key): try: if profile["user"]["username"] == username: + # `dict_update_keys` fills absent keys only, so any key sent + # explicitly as null would mask the configured default + _drop_null_values(profile) return dict(dict_update_keys(profile, default_user_config)) except KeyError: LOG.error(f"Malformed profile in message context: {profile}") diff --git a/tests/user_util_tests.py b/tests/user_util_tests.py index 196ef9c0..22a767ed 100644 --- a/tests/user_util_tests.py +++ b/tests/user_util_tests.py @@ -228,6 +228,157 @@ def test_get_default_user_config(self): {'lat', 'lng', 'city', 'state', 'country', 'tz', 'utc'}) + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_heals_null_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + null_location_profile = { + "user": {"username": "null_location_user"}, + "location": {"lat": None, "lng": None, "city": None, + "state": None, "country": None, "tz": None, + "utc": None}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "null_location_user", + "user_profiles": [null_location_profile]})) + self.assertEqual(prefs["location"], + {"lat": '38.971669', + "lng": '-95.23525', + "tz": 'America/Chicago', + "utc": '-6.0', + "city": 'Kirkland', + "state": 'Washington', + "country": "United States"}) + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_keeps_configured_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + user_location_profile = { + "user": {"username": "located_user"}, + "location": {"lat": '29.4241', "lng": '-98.4936', + "city": 'San Antonio', "state": 'Texas', + "country": "United States", "tz": 'America/Chicago', + "utc": '-6.0'}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "located_user", + "user_profiles": [user_location_profile]})) + self.assertEqual(prefs["location"]["city"], 'San Antonio') + self.assertEqual(prefs["location"]["lat"], '29.4241') + self.assertEqual(prefs["location"]["state"], 'Texas') + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_heals_null_outside_location(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs, \ + get_default_user_config + + # A null anywhere in the profile inherits its configured default + null_profile = {"user": {"username": "null_prefs_user", + "email": None}, + "units": {"measure": None, "date": "YMD"}, + "speech": {"tts_language": None}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "null_prefs_user", + "user_profiles": [null_profile]})) + default = get_default_user_config() + # These defaults are non-null, so the null cannot have survived + self.assertEqual(prefs["units"]["measure"], + default["units"]["measure"]) + self.assertEqual(prefs["speech"]["tts_language"], + default["speech"]["tts_language"]) + self.assertIsNotNone(prefs["units"]["measure"]) + self.assertIsNotNone(prefs["speech"]["tts_language"]) + # `user.email` defaults to an empty string, so assert the type changed + self.assertEqual(prefs["user"]["email"], default["user"]["email"]) + self.assertIsNotNone(prefs["user"]["email"]) + # A non-null value is still preferred over the default + self.assertEqual(prefs["units"]["date"], "YMD") + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_keeps_falsy_values(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + # Only null is absent; False, 0, and '' are deliberate user values and + # must not inherit a truthy default (i.e. a privacy opt-out reverting) + falsy_profile = {"user": {"username": "falsy_user", "email": ""}, + "privacy": {"save_audio": False, "save_text": False}, + "units": {"time": 0}} + prefs = get_user_prefs(Message("test_message", {}, { + "username": "falsy_user", + "user_profiles": [falsy_profile]})) + self.assertFalse(prefs["privacy"]["save_audio"]) + self.assertFalse(prefs["privacy"]["save_text"]) + self.assertEqual(prefs["user"]["email"], "") + self.assertEqual(prefs["units"]["time"], 0) + + @patch("ovos_config.config.Configuration") + def test_get_user_prefs_heals_null_in_message_context(self, config): + from ovos_config.models import LocalConf + test_config_dir = os.path.join(os.path.dirname(__file__), + "user_util_test_config") + config.return_value = LocalConf(join(test_config_dir, "mycroft", + "mycroft.conf")) + import importlib + from neon_utils import user_utils + importlib.reload(user_utils) + from neon_utils.user_utils import get_user_prefs + + # `get_user_prefs` back-fills the context profile in place, so the + # healed value must land there too rather than a null being restored + null_profile = {"user": {"username": "healed_user"}, + "units": {"measure": None}} + message = Message("test_message", {}, { + "username": "healed_user", + "user_profiles": [null_profile]}) + prefs = get_user_prefs(message) + self.assertEqual(message.context["user_profiles"][0], prefs) + self.assertIsNotNone( + message.context["user_profiles"][0]["units"]["measure"]) + + def test_dict_update_keys_preserves_explicit_null(self): + from neon_utils.configuration_utils import dict_update_keys + + # Skill settings merge through this helper and persist to disk, so a + # deliberately-nulled setting must not inherit the metadata default + settings = {"api_key": None, "endpoint": "https://custom"} + merged = dict_update_keys(settings, {"api_key": "DEFAULT_KEY", + "endpoint": "https://default", + "added_key": "added_value"}) + self.assertIsNone(merged["api_key"]) + self.assertEqual(merged["endpoint"], "https://custom") + self.assertEqual(merged["added_key"], "added_value") + if __name__ == '__main__': unittest.main()