Implement workflow state revert

Closes #37

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

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}
 

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):
         """

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)

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)

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'])