From fd5678139ee5e384562ae6da254aaa3a06bcefa9 Mon Sep 17 00:00:00 2001 From: Chris Veilleux Date: Wed, 3 Jul 2019 15:53:21 -0500 Subject: [PATCH] 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 | 6 +-- .../endpoints/device_skill_settings.py | 53 ++++++++++++++----- .../features/device_skill_settings.feature | 16 ++++-- ...s_settings.py => device_skill_settings.py} | 39 ++++++++++++-- .../data/device/repository/device_skill.py | 25 ++++----- .../get_device_skill_settings_for_account.sql | 1 + .../sql/update_device_skill_settings.sql | 8 --- .../sql/upsert_device_skill_settings.sql | 19 +++++++ shared/selene/testing/device_skill.py | 2 +- 9 files changed, 126 insertions(+), 43 deletions(-) rename api/public/tests/features/steps/{device_skills_settings.py => device_skill_settings.py} (80%) delete mode 100644 shared/selene/data/device/repository/sql/update_device_skill_settings.sql create mode 100644 shared/selene/data/device/repository/sql/upsert_device_skill_settings.sql diff --git a/api/public/public_api/api.py b/api/public/public_api/api.py index 632463ea..49368de7 100644 --- a/api/public/public_api/api.py +++ b/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//skill/', - 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//skill', - view_func=DeviceSkillsEndpoint.as_view('device_skill_api'), + view_func=DeviceSkillSettingsEndpoint.as_view('device_skill_api'), methods=['GET', 'PUT'] ) diff --git a/api/public/public_api/endpoints/device_skill_settings.py b/api/public/public_api/endpoints/device_skill_settings.py index 142d7a7d..e77b6384 100644 --- a/api/public/public_api/endpoints/device_skill_settings.py +++ b/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: - if device_skill.skill_id == self.skill.id: - if device_skill.install_method in ('voice', 'cli'): - device_ids = [self.device_id] + return skill_settings + + def _update_skill_settings(self, skill_settings): + 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 - self.device_skill_repo.update_device_skill_settings( - device_ids, + devices_to_update = skill_setting.device_ids + self.device_skill_repo.upsert_device_skill_settings( + 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 diff --git a/api/public/tests/features/device_skill_settings.feature b/api/public/tests/features/device_skill_settings.feature index 521e9b05..d9bd6220 100644 --- a/api/public/tests/features/device_skill_settings.feature +++ b/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 diff --git a/api/public/tests/features/steps/device_skills_settings.py b/api/public/tests/features/steps/device_skill_settings.py similarity index 80% rename from api/public/tests/features/steps/device_skills_settings.py rename to api/public/tests/features/steps/device_skill_settings.py index 85aaf224..52a2234b 100644 --- a/api/public/tests/features/steps/device_skills_settings.py +++ b/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') -def update_skill_settings(context): - _, bar_settings_display = context.skills['bar'] +@when('the device sends a request to update the {skill} skill settings') +def update_skill_settings(context, skill): + _, 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 diff --git a/shared/selene/data/device/repository/device_skill.py b/shared/selene/data/device/repository/device_skill.py index 31951e6e..c5ce6b09 100644 --- a/shared/selene/data/device/repository/device_skill.py +++ b/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( - sql_file_name='update_device_skill_settings.sql', - args=dict( - device_ids=tuple(device_ids), - skill_id=settings_display.skill_id, - settings_values=json.dumps(settings_values), - settings_display_id=settings_display.id + for device_id in device_ids: + db_request = self._build_db_request( + sql_file_name='upsert_device_skill_settings.sql', + args=dict( + device_id=device_id, + skill_id=settings_display.skill_id, + settings_values=json.dumps(settings_values), + settings_display_id=settings_display.id + ) ) - ) - self.cursor.update(db_request) + self.cursor.insert(db_request) def get_skill_manifest_for_device( self, device_id: str diff --git a/shared/selene/data/device/repository/sql/get_device_skill_settings_for_account.sql b/shared/selene/data/device/repository/sql/get_device_skill_settings_for_account.sql index 90029a12..e3a6918c 100644 --- a/shared/selene/data/device/repository/sql/get_device_skill_settings_for_account.sql +++ b/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, diff --git a/shared/selene/data/device/repository/sql/update_device_skill_settings.sql b/shared/selene/data/device/repository/sql/update_device_skill_settings.sql deleted file mode 100644 index ea2f6d3e..00000000 --- a/shared/selene/data/device/repository/sql/update_device_skill_settings.sql +++ /dev/null @@ -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 diff --git a/shared/selene/data/device/repository/sql/upsert_device_skill_settings.sql b/shared/selene/data/device/repository/sql/upsert_device_skill_settings.sql new file mode 100644 index 00000000..85345399 --- /dev/null +++ b/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 diff --git a/shared/selene/testing/device_skill.py b/shared/selene/testing/device_skill.py index 4bde7463..4808fb7a 100644 --- a/shared/selene/testing/device_skill.py +++ b/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