From 64adcdb2e97f3189338845980936117ebe1e25e0 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Fri, 5 Jan 2018 16:13:07 -0800 Subject: [PATCH 1/4] Add "is_temporary" field to narrative object metadata --- lib/NarrativeService/NarrativeManager.py | 12 +++++++++--- test/NarrativeService_server_test.py | 10 +++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/lib/NarrativeService/NarrativeManager.py b/lib/NarrativeService/NarrativeManager.py index 783d3fa..a9b114e 100644 --- a/lib/NarrativeService/NarrativeManager.py +++ b/lib/NarrativeService/NarrativeManager.py @@ -207,6 +207,10 @@ def copy_narrative(self, newName, workspaceRef, workspaceId): newNarMetadata['ws_name'] = newWsName newNarMetadata['job_info'] = json.dumps({'queue_time': 0, 'running': 0, 'completed': 0, 'run_time': 0, 'error': 0}) + is_temporary = 'false' + if newNarMetadata['name'] == 'Untitled' or newNarMetadata['name'] is None: + is_temporary = 'true' + newNarMetadata['is_temporary'] = is_temporary currentNarrative['data']['metadata']['name'] = newName currentNarrative['data']['metadata']['ws_name'] = newWsName @@ -283,6 +287,11 @@ def _create_temp_narrative(self, cells, parameters, importData, includeIntroCell [narrativeObject, metadataExternal] = self._fetchNarrativeObjects( workspaceName, cells, parameters, includeIntroCell, title ) + is_temporary = 'true' + if title is not None and title != 'Untitled': + is_temporary = 'false' + + metadataExternal['is_temporary'] = is_temporary objectInfo = ws.save_objects({'workspace': workspaceName, 'objects': [{'type': 'KBaseNarrative.Narrative', 'data': narrativeObject, @@ -293,9 +302,6 @@ def _create_temp_narrative(self, cells, parameters, importData, includeIntroCell 'Workspace/Narrative bundle.'}], 'hidden': 0}]})[0] objectInfo = ServiceUtils.objectInfoToObject(objectInfo) - is_temporary = 'true' - if title is not None and title != 'Untitled': - is_temporary = 'false' ws_info = self._completeNewNarrative(ws_info[0], objectInfo['id'], importData, is_temporary, title) return { diff --git a/test/NarrativeService_server_test.py b/test/NarrativeService_server_test.py index 2ed2355..24c1671 100644 --- a/test/NarrativeService_server_test.py +++ b/test/NarrativeService_server_test.py @@ -421,6 +421,10 @@ def test_create_new_temp_narrative(self): self.assertEqual(ws_meta['is_temporary'], 'true') self.assertEqual(ws_meta['narrative'], str(ret['narrativeInfo']['id'])) self.assertNotIn('narrative_nice_name', ws_meta) + + self.assertIn('narrativeInfo', ret) + info = ret['narrativeInfo'] + self.assertEqual(info['metadata']['is_temporary'], 'true') finally: new_ws_id = ret['workspaceInfo']['id'] ws.delete_workspace({'id': new_ws_id}) @@ -433,7 +437,7 @@ def test_create_new_titled_narrative(self): title: title }) try: - self.assertTrue('workspaceInfo' in ret) + self.assertIn('workspaceInfo', ret) info = ret['workspaceInfo'] self.assertIn('id', info) self.assertIn('name', info) @@ -449,6 +453,10 @@ def test_create_new_titled_narrative(self): self.assertEqual(ws_meta['is_temporary'], 'false') self.assertEqual(ws_meta['narrative'], str(ret['narrativeInfo']['id'])) self.assertEqual(ws_meta['narrative_nice_name'], title) + + self.assertIn('narrativeInfo', ret) + info = ret['narrativeInfo'] + self.assertEqual(info['metadata']['is_temporary'], 'false') finally: new_ws_id = ret['workspaceInfo']['id'] ws.delete_workspace({'id': new_ws_id}) From 5a6bae0c8d22b00ad1d6a04d3f7d77baefb981e3 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Fri, 5 Jan 2018 17:33:39 -0800 Subject: [PATCH 2/4] is_temporary metadata not set on copy if the source obj already has it --- lib/NarrativeService/NarrativeManager.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/NarrativeService/NarrativeManager.py b/lib/NarrativeService/NarrativeManager.py index a9b114e..933354d 100644 --- a/lib/NarrativeService/NarrativeManager.py +++ b/lib/NarrativeService/NarrativeManager.py @@ -207,10 +207,11 @@ def copy_narrative(self, newName, workspaceRef, workspaceId): newNarMetadata['ws_name'] = newWsName newNarMetadata['job_info'] = json.dumps({'queue_time': 0, 'running': 0, 'completed': 0, 'run_time': 0, 'error': 0}) - is_temporary = 'false' - if newNarMetadata['name'] == 'Untitled' or newNarMetadata['name'] is None: - is_temporary = 'true' - newNarMetadata['is_temporary'] = is_temporary + if 'is_temporary' not in newNarMetadata: + is_temporary = 'false' + if newNarMetadata['name'] == 'Untitled' or newNarMetadata['name'] is None: + is_temporary = 'true' + newNarMetadata['is_temporary'] = is_temporary currentNarrative['data']['metadata']['name'] = newName currentNarrative['data']['metadata']['ws_name'] = newWsName From 9dd35cff8e7053327faab8f4d31ed231ef116d79 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 8 Jan 2018 14:05:07 -0800 Subject: [PATCH 3/4] add tests to ensure copying a temp narrative retains the temp status --- test/NarrativeService_server_test.py | 60 ++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/test/NarrativeService_server_test.py b/test/NarrativeService_server_test.py index 24c1671..644c188 100644 --- a/test/NarrativeService_server_test.py +++ b/test/NarrativeService_server_test.py @@ -40,6 +40,7 @@ def setUpClass(cls): token = environ.get('KB_AUTH_TOKEN', None) os.environ['USE_DP'] = "1" config_file = environ.get('KB_DEPLOYMENT_CONFIG', None) + cls.NARRATIVE_TYPE = "KBaseNarrative.Narrative-4.0" cls.cfg = {} config = ConfigParser() config.read(config_file) @@ -228,7 +229,6 @@ def test_copy_narrative(self): nar_obj_data['metadata']['kbase']['creator'] = user_id nar_obj_data['metadata']['kbase']['ws_name'] = ws_name nar_obj_name = "Narrative." + str(int(round(time.time() * 1000))) - nar_obj_type = "KBaseNarrative.Narrative-4.0" job_info = json.dumps({"queue_time": 0, "running": 0, "completed": 0, "run_time": 0, "error": 0}) nar_obj_meta = {"description": "", @@ -241,7 +241,7 @@ def test_copy_narrative(self): "type": "KBaseNarrative.Narrative", "name": "NarrativeCopyTest"} ws.save_objects({'workspace': ws_name, 'objects': - [{'type': nar_obj_type, + [{'type': self.NARRATIVE_TYPE, 'data': nar_obj_data, 'name': nar_obj_name, 'meta': nar_obj_meta}]}) @@ -316,6 +316,59 @@ def test_copy_narrative(self): # Cleaning up new created workspace ws.delete_workspace({'id': copy_ws_id}) + def test_copy_temp_narrative(self): + """ + Now copy a Narrative flagged as temporary. E.g. create a new narrative, then make + a copy right away. Test that it contains the right 'is_temporary' key in metadata. + """ + ws = self.getWsClient() + + source_nar_info = self.getImpl().create_new_narrative(self.getContext(), {})[0] + source_ws_id = source_nar_info['workspaceInfo']['id'] + source_nar_id = source_nar_info['narrativeInfo']['id'] + + # first, make a copy from the source as-is and make sure it has the temp tag. + copied_nar_info = self.getImpl().copy_narrative(self.getContext(), { + 'workspaceRef': '{}/{}'.format(source_ws_id, source_nar_id), + 'newName': 'Untitled' + })[0] + copied_nar = ws.get_objects2({ + 'objects': [{ + 'ref': '{}/{}'.format(copied_nar_info['newWsId'], copied_nar_info['newNarId']) + }] + }) + + # second, tweak the source to remove its 'is_temporary' field all together. then + # copy and test it. + source_nar = ws.get_objects2({ + 'objects': [{ + 'ref': '{}/{}'.format(source_ws_id, source_nar_id) + }] + })['data'][0] + if 'is_temporary' in source_nar['info'][10]: + del source_nar['info'][10]['is_temporary'] + ws.save_objects({'workspace': source_nar_info['workspaceInfo']['name'], 'objects': + [{'type': self.NARRATIVE_TYPE, + 'data': source_nar['data'], + 'name': source_nar['info'][1], + 'meta': source_nar['info'][10]}]}) + copied_nar_info2 = self.getImpl().copy_narrative(self.getContext(), { + 'workspaceRef': '{}/{}'.format(source_ws_id, source_nar_id), + 'newName': 'Untitled' + })[0] + copied_nar2 = ws.get_objects2({ + 'objects': [{ + 'ref': '{}/{}'.format(copied_nar_info2['newWsId'], copied_nar_info2['newNarId']) + }] + }) + + try: + self.assertEquals(copied_nar['data'][0]['info'][10]['is_temporary'], 'true') + self.assertEquals(copied_nar2['data'][0]['info'][10]['is_temporary'], 'true') + finally: + self.getWsClient().delete_workspace({'id': source_ws_id}) + self.getWsClient().delete_workspace({'id': copied_nar_info['newWsId']}) + self.getWsClient().delete_workspace({'id': copied_nar_info2['newWsId']}) def test_copy_narrative_two_users(self): # Create workspace with Reads object for user1 @@ -342,7 +395,6 @@ def test_copy_narrative_two_users(self): nar_obj_data['metadata']['kbase']['creator'] = user_id nar_obj_data['metadata']['kbase']['ws_name'] = ws_name2 nar_obj_name = "Narrative." + str(int(round(time.time() * 1000))) - nar_obj_type = "KBaseNarrative.Narrative-4.0" job_info = json.dumps({"queue_time": 0, "running": 0, "completed": 0, "run_time": 0, "error": 0}) nar_obj_meta = {"description": "", @@ -355,7 +407,7 @@ def test_copy_narrative_two_users(self): "type": "KBaseNarrative.Narrative", "name": "NarrativeCopyTest"} ws2.save_objects({'workspace': ws_name2, 'objects': - [{'type': nar_obj_type, + [{'type': self.NARRATIVE_TYPE, 'data': nar_obj_data, 'name': nar_obj_name, 'meta': nar_obj_meta}]}) From aed0b0c5e5249bf8e4d07c4cb043c05f416549e9 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 8 Jan 2018 14:44:15 -0800 Subject: [PATCH 4/4] fix and test issue with is_temporary field in ws metadata --- lib/NarrativeService/NarrativeManager.py | 16 +++++++++--- test/NarrativeService_server_test.py | 32 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/lib/NarrativeService/NarrativeManager.py b/lib/NarrativeService/NarrativeManager.py index 933354d..039ddb6 100644 --- a/lib/NarrativeService/NarrativeManager.py +++ b/lib/NarrativeService/NarrativeManager.py @@ -158,7 +158,7 @@ def copy_narrative(self, newName, workspaceRef, workspaceId): time_ms = int(round(time.time() * 1000)) newWsName = self.user_id + ':narrative_' + str(time_ms) # add the 'narrative' field to newWsMeta later. - newWsMeta = {"is_temporary": "false", "narrative_nice_name": newName} + newWsMeta = {"narrative_nice_name": newName} # start with getting the existing narrative object. currentNarrative = self.ws.get_objects([{'ref': workspaceRef}])[0] @@ -207,8 +207,9 @@ def copy_narrative(self, newName, workspaceRef, workspaceId): newNarMetadata['ws_name'] = newWsName newNarMetadata['job_info'] = json.dumps({'queue_time': 0, 'running': 0, 'completed': 0, 'run_time': 0, 'error': 0}) + + is_temporary = newNarMetadata.get('is_temporary', 'false') if 'is_temporary' not in newNarMetadata: - is_temporary = 'false' if newNarMetadata['name'] == 'Untitled' or newNarMetadata['name'] is None: is_temporary = 'true' newNarMetadata['is_temporary'] = is_temporary @@ -228,8 +229,15 @@ def copy_narrative(self, newName, workspaceRef, workspaceId): # now, just update the workspace metadata to point # to the new narrative object newNarId = newNarInfo[0][0] - self.ws.alter_workspace_metadata({'wsi': {'id': newWsId}, - 'new': {'narrative': str(newNarId)}}) + self.ws.alter_workspace_metadata({ + 'wsi': { + 'id': newWsId + }, + 'new': { + 'narrative': str(newNarId), + 'is_temporary': is_temporary + } + }) return {'newWsId': newWsId, 'newNarId': newNarId} except: # let's delete copy of workspace so it's out of the way - it's broken diff --git a/test/NarrativeService_server_test.py b/test/NarrativeService_server_test.py index 644c188..0c4ae86 100644 --- a/test/NarrativeService_server_test.py +++ b/test/NarrativeService_server_test.py @@ -320,6 +320,8 @@ def test_copy_temp_narrative(self): """ Now copy a Narrative flagged as temporary. E.g. create a new narrative, then make a copy right away. Test that it contains the right 'is_temporary' key in metadata. + + Also, for kicks, copy one that's NOT temporary and make sure the 'false' propagates. """ ws = self.getWsClient() @@ -337,6 +339,7 @@ def test_copy_temp_narrative(self): 'ref': '{}/{}'.format(copied_nar_info['newWsId'], copied_nar_info['newNarId']) }] }) + copied_ws_info = ws.get_workspace_info({'id': copied_nar_info['newWsId']}) # second, tweak the source to remove its 'is_temporary' field all together. then # copy and test it. @@ -361,14 +364,43 @@ def test_copy_temp_narrative(self): 'ref': '{}/{}'.format(copied_nar_info2['newWsId'], copied_nar_info2['newNarId']) }] }) + copied_ws_info2 = ws.get_workspace_info({'id': copied_nar_info2['newWsId']}) + + # Finally, create a new non-temporary narrative, copy it, and ensure everything has + # 'is_temporary': 'false' in the right metadata places. + source_nar_info2 = self.getImpl().create_new_narrative(self.getContext(), { + 'title': 'Not Temporary' + })[0] + source_ws_id2 = source_nar_info2['workspaceInfo']['id'] + source_nar_id2 = source_nar_info2['narrativeInfo']['id'] + copied_nar_info3 = self.getImpl().copy_narrative(self.getContext(), { + 'workspaceRef': '{}/{}'.format(source_ws_id2, source_nar_id2), + 'newName': 'Still Not Temporary' + })[0] + copied_nar3 = ws.get_objects2({ + 'objects': [{ + 'ref': '{}/{}'.format(copied_nar_info3['newWsId'], copied_nar_info3['newNarId']) + }] + }) + copied_ws_info3 = ws.get_workspace_info({'id': copied_nar_info3['newWsId']}) try: + self.assertEquals(source_nar_info['workspaceInfo']['metadata']['is_temporary'], 'true') + self.assertEquals(source_nar_info['narrativeInfo']['metadata']['is_temporary'], 'true') self.assertEquals(copied_nar['data'][0]['info'][10]['is_temporary'], 'true') + self.assertEquals(copied_ws_info[8]['is_temporary'], 'true') self.assertEquals(copied_nar2['data'][0]['info'][10]['is_temporary'], 'true') + self.assertEquals(copied_ws_info2[8]['is_temporary'], 'true') + self.assertEquals(source_nar_info2['workspaceInfo']['metadata']['is_temporary'], 'false') + self.assertEquals(source_nar_info2['narrativeInfo']['metadata']['is_temporary'], 'false') + self.assertEquals(copied_nar3['data'][0]['info'][10]['is_temporary'], 'false') + self.assertEquals(copied_ws_info3[8]['is_temporary'], 'false') finally: self.getWsClient().delete_workspace({'id': source_ws_id}) + self.getWsClient().delete_workspace({'id': source_ws_id2}) self.getWsClient().delete_workspace({'id': copied_nar_info['newWsId']}) self.getWsClient().delete_workspace({'id': copied_nar_info2['newWsId']}) + self.getWsClient().delete_workspace({'id': copied_nar_info3['newWsId']}) def test_copy_narrative_two_users(self): # Create workspace with Reads object for user1