diff --git a/ckanext/metadata/logic/action/update.py b/ckanext/metadata/logic/action/update.py index e6b7cc0..e83b0a9 100644 --- a/ckanext/metadata/logic/action/update.py +++ b/ckanext/metadata/logic/action/update.py @@ -753,7 +753,7 @@ def workflow_transition_update(context, data_dict): does not define any properties of its own; to "update" a transition, delete it and create a new one. """ - raise tk.ValidationError("A workflow transition cannot be updated. Delete it and create a new one instead.") + raise tk.ValidationError(_("A workflow transition cannot be updated. Delete it and create a new one instead.")) def metadata_record_workflow_state_transition(context, data_dict): @@ -863,3 +863,87 @@ def metadata_record_workflow_state_transition(context, data_dict): model.repo.commit() return activity_dict + + +def metadata_record_workflow_state_revert(context, data_dict): + """ + Revert a metadata record to the predecessor to its current workflow state (or to None + if the current state has no revert state), and log the change to the metadata record's + activity stream. + + You must be authorized to revert the metadata record's workflow state. + + :param id: the id or name of the metadata record to revert + :type id: string + + :returns: the workflow activity record + :rtype: dictionary + """ + log.info("Reverting workflow state of metadata record: %r", data_dict) + + model = context['model'] + session = context['session'] + user = context['user'] + defer_commit = context.get('defer_commit', False) + + metadata_record_id = tk.get_or_bust(data_dict, 'id') + metadata_record = model.Package.get(metadata_record_id) + if metadata_record is not None and metadata_record.type == 'metadata_record': + metadata_record_id = metadata_record.id + else: + raise tk.ObjectNotFound('%s: %s' % (_('Not found'), _('Metadata Record'))) + + tk.check_access('metadata_record_workflow_state_revert', context, data_dict) + + current_workflow_state_id = session.query(model.PackageExtra.value) \ + .filter_by(package_id=metadata_record_id, key='workflow_state_id').scalar() + + # already on null state - do nothing + if not current_workflow_state_id: + return + + target_workflow_state = ckanext_model.WorkflowState.get_revert_state(current_workflow_state_id) + if target_workflow_state: + target_workflow_state_id = target_workflow_state.id + metadata_record_private = target_workflow_state.metadata_records_private + else: + target_workflow_state_id ='' + metadata_record_private = True + + metadata_record.extras['workflow_state_id'] = target_workflow_state_id + metadata_record.private = metadata_record_private + + activity_context = context.copy() + activity_context.update({ + 'defer_commit': True, + 'schema': { + 'user_id': [unicode, tk.get_validator('convert_user_name_or_id_to_id')], + 'object_id': [], + 'revision_id': [], + 'activity_type': [], + 'data': [], + }, + }) + activity_dict = { + 'user_id': model.User.by_name(user.decode('utf8')).id, + 'object_id': metadata_record_id, + 'activity_type': METADATA_WORKFLOW_ACTIVITY_TYPE, + 'data': { + 'action': 'metadata_record_workflow_state_revert', + 'workflow_state_id': target_workflow_state_id, + } + } + + rev = model.repo.new_revision() + rev.author = user + if 'message' in context: + rev.message = context['message'] + else: + rev.message = _(u'REST API: Revert workflow state of metadata record %s') % metadata_record_id + + activity_dict = tk.get_action('activity_create')(activity_context, activity_dict) + + if not defer_commit: + model.repo.commit() + + return activity_dict diff --git a/ckanext/metadata/logic/auth/update.py b/ckanext/metadata/logic/auth/update.py index 7855ea1..93e9f1d 100644 --- a/ckanext/metadata/logic/auth/update.py +++ b/ckanext/metadata/logic/auth/update.py @@ -65,6 +65,10 @@ def metadata_record_workflow_state_override(context, data_dict): return {'success': True} +def metadata_record_workflow_state_revert(context, data_dict): + return {'success': True} + + def workflow_state_update(context, data_dict): return {'success': True} diff --git a/ckanext/metadata/model/workflow_state.py b/ckanext/metadata/model/workflow_state.py index ea045ce..759e52e 100644 --- a/ckanext/metadata/model/workflow_state.py +++ b/ckanext/metadata/model/workflow_state.py @@ -40,6 +40,18 @@ def get(cls, reference): workflow_state = cls.by_name(reference) return workflow_state + @classmethod + def get_revert_state(cls, reference): + """ + Returns a workflow_state object referenced by the revert_state_id of the object + with the given id or name. + """ + workflow_state = cls.get(reference) + if not workflow_state: + return None + + return cls.get(workflow_state.revert_state_id) + @classmethod def revert_path_exists(cls, from_state_id, to_state_id): """ diff --git a/ckanext/metadata/tests/__init__.py b/ckanext/metadata/tests/__init__.py index dcbec6b..5e81f98 100644 --- a/ckanext/metadata/tests/__init__.py +++ b/ckanext/metadata/tests/__init__.py @@ -266,16 +266,19 @@ def _assert_metadata_model_has_dependent_records(self, metadata_model_id, *metad dependent_record_list = call_action('metadata_model_dependent_record_list', id=metadata_model_id) assert set(dependent_record_list) == set(metadata_record_ids) - def _assert_workflow_activity_logged(self, metadata_record_id, workflow_state_id, *jsonpatch_ids, **workflow_errors): + def _assert_workflow_activity_logged(self, action_suffix, metadata_record_id, workflow_state_id, + *jsonpatch_ids, **workflow_errors): """ + :param action_suffix: 'transition' | 'revert' | 'override' :param workflow_errors: dictionary mapping workflow annotation (flattened) keys to expected error patterns """ activity_dict = call_action('metadata_record_workflow_activity_show', id=metadata_record_id) assert activity_dict['user_id'] == self.normal_user['id'] assert activity_dict['object_id'] == metadata_record_id assert activity_dict['activity_type'] == 'metadata workflow' + assert activity_dict['data']['action'] == 'metadata_record_workflow_state_' + action_suffix assert activity_dict['data']['workflow_state_id'] == workflow_state_id - assert activity_dict['data']['jsonpatch_ids'] == list(jsonpatch_ids) - logged_errors = activity_dict['data']['errors'] + assert activity_dict['data'].get('jsonpatch_ids', []) == list(jsonpatch_ids) + logged_errors = activity_dict['data'].get('errors', {}) for error_key, error_pattern in workflow_errors.items(): assert_error(logged_errors, error_key, error_pattern) diff --git a/ckanext/metadata/tests/test_metadata_record_actions.py b/ckanext/metadata/tests/test_metadata_record_actions.py index 322d4e6..3718db5 100644 --- a/ckanext/metadata/tests/test_metadata_record_actions.py +++ b/ckanext/metadata/tests/test_metadata_record_actions.py @@ -13,6 +13,7 @@ assert_group_has_member, assert_error, assert_object_matches_dict, + assert_package_has_attribute, factories as ckanext_factories, load_example, ) @@ -595,8 +596,7 @@ def test_workflow_transition_submitted(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_submitted['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_submitted['id'], + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_submitted['id'], data_agreement='is a required property', terms_and_conditions='is a required property', capture_method='is a required property') @@ -617,10 +617,8 @@ def test_workflow_transition_submitted(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_submitted['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_submitted['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_submitted['id'], + *jsonpatch_ids, **{ 'data_agreement/accepted': 'True was expected', 'data_agreement/href': 'is not a .*url', 'terms_and_conditions': 'is not of type .*object', @@ -643,10 +641,8 @@ def test_workflow_transition_submitted(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_submitted['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_submitted['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_submitted['id'], + *jsonpatch_ids, **{ 'data_agreement/href': 'is a required property', }) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', '') @@ -658,8 +654,7 @@ def test_workflow_transition_submitted(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_submitted['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_submitted['id'], + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_submitted['id'], *jsonpatch_ids) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state_submitted['id']) @@ -692,8 +687,7 @@ def test_workflow_transition_captured(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_captured['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_captured['id'], + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_captured['id'], *jsonpatch_ids, validated='True was expected', quality_control='is not of type .*array') @@ -707,10 +701,8 @@ def test_workflow_transition_captured(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_captured['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_captured['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_captured['id'], + *jsonpatch_ids, **{ 'quality_control/0/userid': 'Not found.? User', 'quality_control/0/date': 'is not a .*date', }) @@ -723,10 +715,8 @@ def test_workflow_transition_captured(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_captured['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_captured['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_captured['id'], + *jsonpatch_ids, **{ 'quality_control/0/userid': 'Must use object id not name', }) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', '') @@ -739,8 +729,7 @@ def test_workflow_transition_captured(self): # TODO: the following depends on implementation of role_validator() in json_validator_functions # self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], # workflow_state_id=workflow_state_captured['id']) - # self._assert_workflow_activity_logged(metadata_record['id'], - # workflow_state_captured['id'], + # self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_captured['id'], # *jsonpatch_ids) # assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state_captured['id']) @@ -781,10 +770,8 @@ def test_workflow_transition_published(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_published['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_published['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_published['id'], + *jsonpatch_ids, **{ 'errors/identifier': 'This key may not be present in the dictionary', 'quality_control/__minItems': 'Array has too few items', }) @@ -801,10 +788,8 @@ def test_workflow_transition_published(self): self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], workflow_state_id=workflow_state_published['id']) - self._assert_workflow_activity_logged(metadata_record['id'], - workflow_state_published['id'], - *jsonpatch_ids, - **{ + self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_published['id'], + *jsonpatch_ids, **{ 'quality_control/__uniqueObjects': 'Array has non-unique objects', }) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', '') @@ -818,7 +803,31 @@ def test_workflow_transition_published(self): # TODO: the following depends on implementation of role_validator() in json_validator_functions # self._test_action('metadata_record_workflow_state_transition', id=metadata_record['id'], # workflow_state_id=workflow_state_published['id']) - # self._assert_workflow_activity_logged(metadata_record['id'], - # workflow_state_published['id'], + # self._assert_workflow_activity_logged('transition', metadata_record['id'], workflow_state_published['id'], # *jsonpatch_ids) # assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state_published['id']) + + def test_workflow_state_revert(self): + metadata_record = self._generate_metadata_record() + workflow_state1 = ckanext_factories.WorkflowState(metadata_records_private=False) + workflow_state2 = ckanext_factories.WorkflowState(revert_state_id=workflow_state1['id'], metadata_records_private=True) + + call_action('metadata_record_workflow_state_override', context={'user': self.normal_user['name']}, + id=metadata_record['id'], workflow_state_id=workflow_state2['id']) + assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state2['id']) + assert_package_has_attribute(metadata_record['id'], 'private', True) + + self._test_action('metadata_record_workflow_state_revert', id=metadata_record['id']) + self._assert_workflow_activity_logged('revert', metadata_record['id'], workflow_state1['id']) + assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state1['id']) + assert_package_has_attribute(metadata_record['id'], 'private', False) + + self._test_action('metadata_record_workflow_state_revert', id=metadata_record['id']) + self._assert_workflow_activity_logged('revert', metadata_record['id'], '') + assert_package_has_extra(metadata_record['id'], 'workflow_state_id', '') + assert_package_has_attribute(metadata_record['id'], 'private', True) + + # reverting a record already in the null workflow state should not error or change anything + self._test_action('metadata_record_workflow_state_revert', id=metadata_record['id']) + assert_package_has_extra(metadata_record['id'], 'workflow_state_id', '') + assert_package_has_attribute(metadata_record['id'], 'private', True) diff --git a/ckanext/metadata/tests/test_workflow_state_actions.py b/ckanext/metadata/tests/test_workflow_state_actions.py index 484a6ee..2159feb 100644 --- a/ckanext/metadata/tests/test_workflow_state_actions.py +++ b/ckanext/metadata/tests/test_workflow_state_actions.py @@ -192,7 +192,7 @@ def test_update_metadata_records_private_cascade_1(self): """ metadata_record = ckanext_factories.MetadataRecord() workflow_state = ckanext_factories.WorkflowState(metadata_records_private=False) - call_action('metadata_record_workflow_state_override', + call_action('metadata_record_workflow_state_override', context={'user': self.normal_user['name']}, id=metadata_record['id'], workflow_state_id=workflow_state['id']) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id']) @@ -215,7 +215,7 @@ def test_update_metadata_records_private_cascade_2(self): """ metadata_record = ckanext_factories.MetadataRecord() workflow_state = ckanext_factories.WorkflowState(metadata_records_private=True) - call_action('metadata_record_workflow_state_override', + call_action('metadata_record_workflow_state_override', context={'user': self.normal_user['name']}, id=metadata_record['id'], workflow_state_id=workflow_state['id']) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id']) @@ -322,7 +322,7 @@ def test_delete_valid(self): def test_delete_with_dependencies(self): metadata_record = ckanext_factories.MetadataRecord() workflow_state = ckanext_factories.WorkflowState() - call_action('metadata_record_workflow_state_override', + call_action('metadata_record_workflow_state_override', context={'user': self.normal_user['name']}, id=metadata_record['id'], workflow_state_id=workflow_state['id']) assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id'])