Add private flag to workflow state; closes #28

Visibility of metadata records is now determined by that of the workflow state they're in. By default (before any workflow state has been assigned) metadata records are private.

ckanext/metadata/logic/action/create.py

@@ -306,6 +306,7 @@ def metadata_record_create(context, data_dict):
         'validated': False,
         'errors': '{}',
         'workflow_state_id': '',
+        'private': True,
     })
     context.update({
         'schema': schema.metadata_record_create_schema(),
@@ -339,6 +340,8 @@ def workflow_state_create(context, data_dict):
     :type title: string
     :param description: the description of the workflow state (optional)
     :type description: string
+    :param private: determines the private/public status of metadata records that are in this workflow state
+    :type private: boolean
     :param revert_state_id: the id or name of the state to which a metadata record is
         reverted in case it no longer fulfils the rules for this state (nullable)
     :type revert_state_id: string

ckanext/metadata/logic/action/update.py

@@ -366,6 +366,7 @@ def metadata_record_update(context, data_dict):
         'validated': asbool(metadata_record.extras['validated']),
         'errors': metadata_record.extras['errors'],
         'workflow_state_id': metadata_record.extras['workflow_state_id'],
+        'private': metadata_record.private,
     })
     context.update({
         'schema': schema.metadata_record_update_schema(),
@@ -595,6 +596,7 @@ def metadata_record_workflow_state_override(context, data_dict):
     tk.check_access('metadata_record_workflow_state_override', context, data_dict)
 
     metadata_record.extras['workflow_state_id'] = workflow_state_id
+    metadata_record.private = workflow_state.private
 
     rev = model.repo.new_revision()
     rev.author = user
@@ -644,6 +646,8 @@ def workflow_state_update(context, data_dict):
 
     tk.check_access('workflow_state_update', context, data_dict)
 
+    old_private = workflow_state.private
+
     data_dict.update({
         'id': workflow_state_id,
     })
@@ -659,6 +663,19 @@ def workflow_state_update(context, data_dict):
 
     workflow_state = model_save.workflow_state_dict_save(data, context)
 
+    if workflow_state.private != old_private:
+        # cascade change in 'private' status to metadata records that are in this workflow state
+        metadata_records = session.query(model.Package) \
+            .join(model.PackageExtra, model.Package.id == model.PackageExtra.package_id) \
+            .filter(model.PackageExtra.key == 'workflow_state_id') \
+            .filter(model.PackageExtra.value == workflow_state_id) \
+            .filter(model.Package.type == 'metadata_record') \
+            .filter(model.Package.state != 'deleted') \
+            .all()
+
+        for metadata_record in metadata_records:
+            metadata_record.private = workflow_state.private
+
     rev = model.repo.new_revision()
     rev.author = user
     if 'message' in context:
@@ -889,6 +906,7 @@ def metadata_record_workflow_state_transition(context, data_dict):
 
     if success:
         metadata_record.extras['workflow_state_id'] = target_workflow_state_id
+        metadata_record.private = target_workflow_state.private
 
     activity_context = context.copy()
     activity_context.update({

ckanext/metadata/logic/schema.py

@@ -86,6 +86,7 @@ def metadata_record_create_schema():
         'state': [ignore_not_package_admin, ignore_missing],
         'owner_org': [v.not_empty, v.group_exists('organization'), owner_org_validator, unicode],
         'type': [],
+        'private': [],
 
         # extension-specific fields
         'metadata_collection_id': [v.not_empty, unicode, v.group_exists('metadata_collection'), convert_to_extras],
@@ -288,6 +289,7 @@ def workflow_state_create_schema():
         'name': [v.not_empty, unicode, name_validator, v.workflow_state_name_validator],
         'title': [ignore_missing, unicode],
         'description': [ignore_missing, unicode],
+        'private': [v.not_missing, boolean_validator],
         'revert_state_id': [v.not_missing, unicode, v.workflow_state_exists],
         'state': [ignore_not_sysadmin, ignore_missing],
     }

ckanext/metadata/model/workflow_state.py

@@ -12,6 +12,7 @@
     Column('name', types.UnicodeText, nullable=False, unique=True),
     Column('title', types.UnicodeText),
     Column('description', types.UnicodeText),
+    Column('private', types.Boolean, nullable=False),
     # we implement the self-relation "softly", otherwise revision table
     # auto-generation gets confused about how to join to this table
     Column('revert_state_id', types.UnicodeText),  # ForeignKey('workflow_state.id')),

ckanext/metadata/tests/__init__.py

@@ -79,6 +79,16 @@ def assert_package_has_extra(package_id, key, value, state='active', is_json=Fal
     assert extra_value == value
 
 
+def assert_package_has_attribute(package_id, attr, value):
+    """
+    Check that a package has the specified native attribute value.
+    """
+    obj = ckan_model.Package.get(package_id)
+    assert obj
+    assert hasattr(obj, attr)
+    assert getattr(obj, attr) == value
+
+
 def assert_group_has_extra(group_id, key, value, state='active'):
     """
     Check that a group has the specified extra key-value pair.

ckanext/metadata/tests/factories.py

@@ -145,6 +145,7 @@ class WorkflowState(factory.Factory):
     title = factory.LazyAttribute(lambda obj: obj.name.replace('_', ' ').title())
     description = 'A test description for this test workflow state.'
     revert_state_id = ''
+    private = False
 
     @classmethod
     def _build(cls, target_class, *args, **kwargs):

ckanext/metadata/tests/test_metadata_record_actions.py

@@ -85,6 +85,7 @@ def _assert_metadata_record_ok(self, obj, input_dict, **kwargs):
         assert obj.title == kwargs.pop('title', input_dict.get('title'))
         assert obj.name == kwargs.pop('name', 'metadata-' + obj.id)
         assert obj.owner_org == kwargs.pop('owner_org', self.owner_org['id'])
+        assert obj.private == kwargs.pop('private', True)
         assert_package_has_extra(obj.id, 'metadata_collection_id', kwargs.pop('metadata_collection_id', self.metadata_collection['id']))
         assert_package_has_extra(obj.id, 'metadata_schema_id', kwargs.pop('metadata_schema_id', self.metadata_schema['id']))
         assert_package_has_extra(obj.id, 'metadata_json', input_dict['metadata_json'], is_json=True)
@@ -96,10 +97,13 @@ def _assert_metadata_record_ok(self, obj, input_dict, **kwargs):
 
     def test_create_valid(self):
         input_dict = self._make_input_dict()
-        input_dict['type'] = 'ignore'
-        input_dict['validated'] = 'ignore'
-        input_dict['errors'] = 'ignore'
-        input_dict['workflow_state_id'] = 'ignore'
+        input_dict.update({
+            'type': 'ignore',
+            'validated': 'ignore',
+            'errors': 'ignore',
+            'workflow_state_id': 'ignore',
+            'private': 'ignore',
+        })
         result, obj = self._test_action('metadata_record_create', **input_dict)
         self._assert_metadata_record_ok(obj, input_dict)
 
@@ -243,6 +247,7 @@ def test_update_valid(self):
             'validated': 'ignore',
             'errors': 'ignore',
             'workflow_state_id': 'ignore',
+            'private': 'ignore',
         }
         result, obj = self._test_action('metadata_record_update', **input_dict)
 

ckanext/metadata/tests/test_workflow_state_actions.py

@@ -8,6 +8,7 @@
     make_uuid,
     assert_object_matches_dict,
     assert_package_has_extra,
+    assert_package_has_attribute,
     assert_error,
     factories as ckanext_factories,
 )
@@ -21,6 +22,7 @@ def test_create_valid(self):
             'title': 'Test Workflow State',
             'description': 'This is a test workflow state',
             'revert_state_id': '',
+            'private': True,
         }
         result, obj = self._test_action('workflow_state_create', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -30,6 +32,7 @@ def test_create_valid_with_revert(self):
         input_dict = {
             'name': 'test-workflow-state',
             'revert_state_id': workflow_state['id'],
+            'private': False,
         }
         result, obj = self._test_action('workflow_state_create', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -39,6 +42,7 @@ def test_create_valid_with_revert_byname(self):
         input_dict = {
             'name': 'test-workflow-state',
             'revert_state_id': workflow_state['name'],
+            'private': False,
         }
         result, obj = self._test_action('workflow_state_create', **input_dict)
         input_dict['revert_state_id'] = workflow_state['id']
@@ -49,6 +53,7 @@ def test_create_valid_sysadmin_setid(self):
             'id': make_uuid(),
             'name': 'test-workflow-state',
             'revert_state_id': '',
+            'private': True,
         }
         result, obj = self._test_action('workflow_state_create', sysadmin=True, check_auth=True, **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -63,6 +68,7 @@ def test_create_invalid_missing_params(self):
         result, obj = self._test_action('workflow_state_create', should_error=True)
         assert_error(result, 'name', 'Missing parameter')
         assert_error(result, 'revert_state_id', 'Missing parameter')
+        assert_error(result, 'private', 'Missing parameter')
 
     def test_create_invalid_missing_values(self):
         result, obj = self._test_action('workflow_state_create', should_error=True,
@@ -110,6 +116,7 @@ def test_update_valid(self):
             'title': 'Updated Test Workflow State',
             'description': 'Updated test workflow state description',
             'revert_state_id': '',
+            'private': False,
         }
         result, obj = self._test_action('workflow_state_update', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -120,6 +127,7 @@ def test_update_valid_partial(self):
             'id': workflow_state['id'],
             'name': 'updated-test-workflow-state',
             'revert_state_id': '',
+            'private': False,
         }
         result, obj = self._test_action('workflow_state_update', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -133,6 +141,7 @@ def test_update_valid_change_revert_1(self):
             'id': workflow_state1['id'],
             'name': workflow_state1['name'],
             'revert_state_id': workflow_state2['id'],
+            'private': True,
         }
         result, obj = self._test_action('workflow_state_update', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -145,6 +154,7 @@ def test_update_valid_change_revert_2(self):
             'id': workflow_state3['id'],
             'name': workflow_state3['name'],
             'revert_state_id': workflow_state1['id'],
+            'private': True,
         }
         result, obj = self._test_action('workflow_state_update', **input_dict)
         assert_object_matches_dict(obj, input_dict)
@@ -159,10 +169,55 @@ def test_update_valid_change_revert_3(self):
             'id': workflow_state3['id'],
             'name': workflow_state3['name'],
             'revert_state_id': workflow_state1['id'],
+            'private': True,
         }
         result, obj = self._test_action('workflow_state_update', **input_dict)
         assert_object_matches_dict(obj, input_dict)
 
+    def test_update_private_cascade_1(self):
+        """
+        Test that the visibility of a metadata record is determined by that of its workflow state.
+        """
+        metadata_record = ckanext_factories.MetadataRecord()
+        workflow_state = ckanext_factories.WorkflowState(private=False)
+        call_action('metadata_record_workflow_state_override',
+                    id=metadata_record['id'],
+                    workflow_state_id=workflow_state['id'])
+        assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id'])
+        assert_package_has_attribute(metadata_record['id'], 'private', False)
+
+        input_dict = {
+            'id': workflow_state['id'],
+            'revert_state_id': '',
+            'private': True,
+        }
+        result, obj = self._test_action('workflow_state_update', **input_dict)
+        assert_object_matches_dict(obj, input_dict)
+        assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id'])
+        assert_package_has_attribute(metadata_record['id'], 'private', True)
+
+    def test_update_private_cascade_2(self):
+        """
+        Test that the visibility of a metadata record is determined by that of its workflow state.
+        """
+        metadata_record = ckanext_factories.MetadataRecord()
+        workflow_state = ckanext_factories.WorkflowState(private=True)
+        call_action('metadata_record_workflow_state_override',
+                    id=metadata_record['id'],
+                    workflow_state_id=workflow_state['id'])
+        assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id'])
+        assert_package_has_attribute(metadata_record['id'], 'private', True)
+
+        input_dict = {
+            'id': workflow_state['id'],
+            'revert_state_id': '',
+            'private': False,
+        }
+        result, obj = self._test_action('workflow_state_update', **input_dict)
+        assert_object_matches_dict(obj, input_dict)
+        assert_package_has_extra(metadata_record['id'], 'workflow_state_id', workflow_state['id'])
+        assert_package_has_attribute(metadata_record['id'], 'private', False)
+
     def test_update_invalid_duplicate_name(self):
         workflow_state1 = ckanext_factories.WorkflowState()
         workflow_state2 = ckanext_factories.WorkflowState()
@@ -178,6 +233,7 @@ def test_update_invalid_missing_params(self):
         result, obj = self._test_action('workflow_state_update', should_error=True,
                                         id=workflow_state['id'])
         assert_error(result, 'revert_state_id', 'Missing parameter')
+        assert_error(result, 'private', 'Missing parameter')
 
     def test_update_invalid_circular_revert(self):
         workflow_state1 = ckanext_factories.WorkflowState()