fixed a bug when a device sends skill settings before it sends the skill manifest. to address the bug, if settings are sent for a skill not yet assigned to a device, insert the device_skill row

api/public/public_api/api.py

@@ -19,7 +19,7 @@ from .endpoints.device_refresh_token import DeviceRefreshTokenEndpoint
 from .endpoints.device_setting import DeviceSettingEndpoint
 from .endpoints.device_skill import DeviceSkillEndpoint
 from .endpoints.device_skill_manifest import DeviceSkillManifestEndpoint
-from .endpoints.device_skill_settings import DeviceSkillsEndpoint
+from .endpoints.device_skill_settings import DeviceSkillSettingsEndpoint
 from .endpoints.device_subscription import DeviceSubscriptionEndpoint
 from .endpoints.google_stt import GoogleSTTEndpoint
 from .endpoints.oauth_callback import OauthCallbackEndpoint
@@ -40,13 +40,13 @@ public.register_blueprint(selene_api)
 
 public.add_url_rule(
     '/v1/device/<string:device_id>/skill/<string:skill_gid>',
-    view_func=DeviceSkillsEndpoint.as_view('device_skill_delete_api'),
+    view_func=DeviceSkillSettingsEndpoint.as_view('device_skill_delete_api'),
     methods=['DELETE']
 )
 
 public.add_url_rule(
     '/v1/device/<string:device_id>/skill',
-    view_func=DeviceSkillsEndpoint.as_view('device_skill_api'),
+    view_func=DeviceSkillSettingsEndpoint.as_view('device_skill_api'),
     methods=['GET', 'PUT']
 )
 

api/public/public_api/endpoints/device_skill_settings.py

@@ -66,7 +66,7 @@ class SkillSettingUpdater(object):
         self._extract_settings_values()
         self._get_skill_id()
         self._ensure_settings_display_exists()
-        self._update_device_skill()
+        self._upsert_device_skill()
 
     def _extract_settings_values(self):
         """Extract the settings values from the skillMetadata
@@ -120,27 +120,56 @@ class SkillSettingUpdater(object):
 
         return new_settings_display
 
-    def _update_device_skill(self):
+    def _upsert_device_skill(self):
         """Update the account's devices with the skill to have new settings"""
+        skill_settings = self._get_account_skill_settings()
+        device_skill_found = self._update_skill_settings(skill_settings)
+        if not device_skill_found:
+            self._add_skill_to_device()
+
+    def _get_account_skill_settings(self):
+        """Get all the permutations of settings for a skill"""
         account_repo = AccountRepository(self.db)
         account = account_repo.get_account_by_device_id(self.device_id)
-        device_skill_settings = (
+        skill_settings = (
             self.device_skill_repo.get_device_skill_settings_for_account(
-                account.id
+                account.id,
+                self.skill.id
             )
         )
 
-        for device_skill in device_skill_settings:
+        return skill_settings
-            if device_skill.skill_id == self.skill.id:
+
-                if device_skill.install_method in ('voice', 'cli'):
+    def _update_skill_settings(self, skill_settings):
-                    device_ids = [self.device_id]
+        device_skill_found = False
+        for skill_setting in skill_settings:
+            if self.device_id in skill_setting.device_ids:
+                device_skill_found = True
+                if skill_setting.install_method in ('voice', 'cli'):
+                    devices_to_update = [self.device_id]
                 else:
-                    device_ids = device_skill.device_ids
+                    devices_to_update = skill_setting.device_ids
-                self.device_skill_repo.update_device_skill_settings(
+                self.device_skill_repo.upsert_device_skill_settings(
-                    device_ids,
+                    devices_to_update,
                     self.settings_display,
                     self.settings_values if self.settings_values else None
                 )
+                break
+
+        return device_skill_found
+
+    def _add_skill_to_device(self):
+        """Add a device_skill row for this skill.
+
+        In theory, the skill manifest endpoint handles adding skills to a
+        device but testing shows that this endpoint gets called before the
+        manifest endpoint in some cases.
+        """
+        self.device_skill_repo.upsert_device_skill_settings(
+            [self.device_id],
+            self.settings_display,
+            self.settings_values if self.settings_values else None
+        )
 
 
 class RequestSkillField(Model):
@@ -186,7 +215,7 @@ class RequestSkill(Model):
         return value
 
 
-class DeviceSkillsEndpoint(PublicEndpoint):
+class DeviceSkillSettingsEndpoint(PublicEndpoint):
     """Fetch all skills associated with a device using the API v1 format"""
     _device_skill_repo = None
     _skill_repo = None

api/public/tests/features/device_skill_settings.feature

@@ -26,21 +26,31 @@ Feature: Upload and fetch skills and their settings
     And an E-tag is generated for these settings
     And device last contact timestamp is updated
 
-  Scenario: A device uploads a change to a single skill setting value
+  Scenario: A device uploads a change to a skill setting value
     Given an authorized device
     And a valid device skill E-tag
     And skill settings with a new value
-    When the device sends a request to update the skill settings
+    When the device sends a request to update the bar skill settings
     Then the request will be successful
     And the skill settings are updated with the new value
     And the device skill E-tag is expired
     And device last contact timestamp is updated
 
+  Scenario: A device uploads a change to a skill not assigned to it
+    Given an authorized device
+    And a valid device skill E-tag
+    And settings for a skill not assigned to the device
+    When the device sends a request to update the foobar skill settings
+    Then the request will be successful
+    And the skill is assigned to the device with the settings populated
+    And the device skill E-tag is expired
+    And device last contact timestamp is updated
+
   Scenario: A device uploads skill settings with a field deleted from the settings
     Given an authorized device
     And a valid device skill E-tag
     And skill settings with a deleted field
-    When the device sends a request to update the skill settings
+    When the device sends a request to update the bar skill settings
     Then the request will be successful
     And the field is no longer in the skill settings
     And the device skill E-tag is expired

api/public/tests/features/steps/device_skill_settings.py

@@ -6,6 +6,7 @@ from hamcrest import assert_that, equal_to, is_not, is_in
 from selene.api.etag import ETAG_REQUEST_HEADER_KEY
 from selene.data.device import DeviceSkillRepository
 from selene.data.skill import SkillSettingRepository
+from selene.testing.skill import add_skill, build_label_field, build_text_field
 from selene.util.cache import DEVICE_SKILL_ETAG_KEY
 
 
@@ -41,6 +42,19 @@ def expire_skill_setting_etag(context):
     )
 
 
+@given('settings for a skill not assigned to the device')
+def add_skill_not_assigned_to_device(context):
+    foobar_skill, foobar_settings_display = add_skill(
+        context.db,
+        skill_global_id='foobar-skill|19.02',
+        settings_fields=[build_label_field(), build_text_field()]
+    )
+    section = foobar_settings_display.display_data['skillMetadata']['sections'][0]
+    field_with_value = section['fields'][1]
+    field_with_value['value'] = 'New skill text value'
+    context.skills.update(foobar=(foobar_skill, foobar_settings_display))
+
+
 @when('a device requests the settings for its skills')
 def get_device_skill_settings(context):
     if hasattr(context, 'device_skill_etag'):
@@ -54,12 +68,12 @@ def get_device_skill_settings(context):
     )
 
 
-@when('the device sends a request to update the skill settings')
+@when('the device sends a request to update the {skill} skill settings')
-def update_skill_settings(context):
+def update_skill_settings(context, skill):
-    _, bar_settings_display = context.skills['bar']
+    _, settings_display = context.skills[skill]
     context.response = context.client.put(
         '/v1/device/{device_id}/skill'.format(device_id=context.device_id),
-        data=json.dumps(bar_settings_display.display_data),
+        data=json.dumps(settings_display.display_data),
         content_type='application/json',
         headers=context.request_header
     )
@@ -131,6 +145,23 @@ def validate_updated_skill_setting_value(context):
     )
 
 
+@then('the skill is assigned to the device with the settings populated')
+def validate_updated_skill_setting_value(context):
+    settings_repo = SkillSettingRepository(context.db)
+    device_skill_settings = settings_repo.get_skill_settings_for_device(
+        context.device_id
+    )
+    device_settings_values = [
+        dss.settings_values for dss in device_skill_settings
+    ]
+    assert_that(len(device_skill_settings), equal_to(3))
+    expected_settings_values = dict(textfield='New skill text value')
+    assert_that(
+        expected_settings_values,
+        is_in(device_settings_values)
+    )
+
+
 @then('an E-tag is generated for these settings')
 def get_skills_etag(context):
     response_headers = context.response.headers

shared/selene/data/device/repository/device_skill.py

@@ -13,12 +13,12 @@ class DeviceSkillRepository(RepositoryBase):
         super(DeviceSkillRepository, self).__init__(db, __file__)
 
     def get_device_skill_settings_for_account(
-            self, account_id: str
+            self, account_id: str, skill_id: str
     ) -> List[DeviceSkillSettings]:
         return self._select_all_into_dataclass(
             DeviceSkillSettings,
             sql_file_name='get_device_skill_settings_for_account.sql',
-            args=dict(account_id=account_id)
+            args=dict(account_id=account_id, skill_id=skill_id)
         )
 
     def get_device_skill_settings_for_device(
@@ -43,22 +43,23 @@ class DeviceSkillRepository(RepositoryBase):
         )
         self.cursor.update(db_request)
 
-    def update_device_skill_settings(
+    def upsert_device_skill_settings(
             self,
             device_ids: List[str],
             settings_display: SettingsDisplay,
             settings_values: str,
     ):
-        db_request = self._build_db_request(
+        for device_id in device_ids:
-            sql_file_name='update_device_skill_settings.sql',
+            db_request = self._build_db_request(
-            args=dict(
+                sql_file_name='upsert_device_skill_settings.sql',
-                device_ids=tuple(device_ids),
+                args=dict(
-                skill_id=settings_display.skill_id,
+                    device_id=device_id,
-                settings_values=json.dumps(settings_values),
+                    skill_id=settings_display.skill_id,
-                settings_display_id=settings_display.id
+                    settings_values=json.dumps(settings_values),
+                    settings_display_id=settings_display.id
+                )
             )
-        )
+            self.cursor.insert(db_request)
-        self.cursor.update(db_request)
 
     def get_skill_manifest_for_device(
             self, device_id: str

shared/selene/data/device/repository/sql/get_device_skill_settings_for_account.sql

@@ -9,6 +9,7 @@ FROM
     INNER JOIN device.device_skill dds ON dd.id = dds.device_id
 WHERE
     dd.account_id = %(account_id)s
+    AND dds.skill_id = %(skill_id)s
 GROUP BY
     dds.skill_settings_display_id,
     dds.settings::jsonb,

shared/selene/data/device/repository/sql/update_device_skill_settings.sql

@@ -1,8 +0,0 @@
-UPDATE
-    device.device_skill
-SET
-    skill_settings_display_id = %(settings_display_id)s,
-    settings = %(settings_values)s
-WHERE
-    skill_id = %(skill_id)s
-    AND device_id IN %(device_ids)s

shared/selene/data/device/repository/sql/upsert_device_skill_settings.sql

@@ -0,0 +1,19 @@
+INSERT INTO
+    device.device_skill (
+        device_id,
+        skill_id,
+        skill_settings_display_id,
+        settings
+    )
+VALUES
+    (
+        %(device_id)s,
+        %(skill_id)s,
+        %(settings_display_id)s,
+        %(settings_values)s
+    )
+ON CONFLICT
+    (device_id, skill_id)
+DO UPDATE SET
+    skill_settings_display_id = %(settings_display_id)s,
+    settings = %(settings_values)s

shared/selene/testing/device_skill.py

@@ -22,7 +22,7 @@ def add_device_skill(db, device_id, skill):
 
 def add_device_skill_settings(db, device_id, settings_display, settings_values):
     device_skill_repo = DeviceSkillRepository(db)
-    device_skill_repo.update_device_skill_settings(
+    device_skill_repo.upsert_device_skill_settings(
         [device_id],
         settings_display,
         settings_values