Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions api/collections/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ def update(self, obj, validated_data):
obj.grade_levels = validated_data.pop('grade_levels')

obj.save()
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR):
obj.sync_cedar_metadata()
return obj


Expand Down Expand Up @@ -405,6 +407,8 @@ def update(self, obj, validated_data):
obj.grade_levels = validated_data.pop('grade_levels')

obj.save()
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR):
obj.sync_cedar_metadata()
return obj


Expand All @@ -429,15 +433,17 @@ def create(self, validated_data):
raise exceptions.ValidationError('"creator" must be specified.')
if not (creator.has_perm('write_collection', collection) or (hasattr(guid.referent, 'has_permission') and guid.referent.has_permission(creator, WRITE))):
raise exceptions.PermissionDenied('Must have write permission on either collection or collected object to collect.')
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR) and collection.provider_id:
try:
collection.provider.validate_required_metadata(guid.referent)
except ValidationError as e:
raise InvalidModelValueError(e.message)
Comment on lines -432 to -436
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about calling validate_required_metadata after sync_cedar_metadata, instead of removing the validation entirely? (looks like now it's never validated)

try:
obj = collection.collect_object(guid.referent, creator, **validated_data)
except ValidationError as e:
raise InvalidModelValueError(e.message)
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR):
obj.sync_cedar_metadata()
if collection.provider_id:
try:
collection.provider.validate_required_metadata(guid.referent)
except ValidationError as e:
raise InvalidModelValueError(e.message)
if subjects:
auth = get_user_auth(self.context['request'])
try:
Expand Down Expand Up @@ -470,15 +476,17 @@ def create(self, validated_data):
raise exceptions.ValidationError('"creator" must be specified.')
if not (creator.has_perm('write_collection', collection) or (hasattr(guid.referent, 'has_permission') and guid.referent.has_permission(creator, WRITE))):
raise exceptions.PermissionDenied('Must have write permission on either collection or collected object to collect.')
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR) and collection.provider_id:
try:
collection.provider.validate_required_metadata(guid.referent)
except ValidationError as e:
raise InvalidModelValueError(e.message)
try:
obj = collection.collect_object(guid.referent, creator, **validated_data)
except ValidationError as e:
raise InvalidModelValueError(e.message)
if waffle.switch_is_active(features.COLLECTION_SUBMISSION_WITH_CEDAR):
obj.sync_cedar_metadata()
if collection.provider_id:
try:
collection.provider.validate_required_metadata(guid.referent)
except ValidationError as e:
raise InvalidModelValueError(e.message)
if subjects:
auth = get_user_auth(self.context['request'])
try:
Expand Down
79 changes: 70 additions & 9 deletions api_tests/collections/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from api_tests.subjects.mixins import UpdateSubjectsMixin, SubjectsFilterMixin, SubjectsListMixin, \
SubjectsRelationshipMixin
from api_tests.utils import disconnected_from_listeners
from tests.utils import capture_notifications
from framework.auth.core import Auth
from osf import features
from osf.models import Collection, VersionedGuidMixin
Expand Down Expand Up @@ -4450,16 +4451,76 @@ def test_switch_active_no_provider_submission_succeeds(self, app, user_one, proj
)
assert res.status_code == 201

def test_switch_active_missing_cedar_record_submission_fails(self, app, user_one, project, url, payload):
def test_switch_active_no_provider_no_cedar_record_created(self, app, user_one, project, url_no_provider, payload):
from osf.models import CedarMetadataRecord
with mock_update_share():
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=True):
app.post_json_api(url_no_provider, payload(guid=project._id), auth=user_one.auth)
assert not CedarMetadataRecord.objects.filter(guid__in=project.guids.all()).exists()

def test_switch_active_submission_creates_cedar_record(self, app, user_one, project, url, payload, cedar_template):
from osf.models import CedarMetadataRecord
with capture_notifications():
with mock_update_share():
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=True):
res = app.post_json_api(
url,
payload(guid=project._id),
auth=user_one.auth,
)
assert res.status_code == 201
record = CedarMetadataRecord.objects.filter(
guid__in=project.guids.all(),
template=cedar_template,
is_published=True,
).first()
assert record is not None

def test_switch_active_submission_with_custom_fields_syncs_cedar_metadata(
self, app, user_one, project, url, payload, cedar_template, collection):
from osf.models import CedarMetadataRecord
collection.status_choices = ['pending']
collection.volume_choices = ['1']
collection.save()
with capture_notifications():
with mock_update_share():
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=True):
app.post_json_api(
url,
payload(guid=project._id, status='pending', volume='1'),
auth=user_one.auth,
)
record = CedarMetadataRecord.objects.get(guid__in=project.guids.all(), template=cedar_template)
assert record.metadata['status'] == 'pending'
assert record.metadata['volume'] == '1'

def test_switch_inactive_submission_does_not_create_cedar_record(
self, app, user_one, project, url, payload, cedar_template):
from osf.models import CedarMetadataRecord
with capture_notifications():
with mock_update_share():
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=False):
res = app.post_json_api(url, payload(guid=project._id), auth=user_one.auth)
assert res.status_code == 201
assert not CedarMetadataRecord.objects.filter(guid__in=project.guids.all()).exists()

def test_switch_active_update_syncs_cedar_metadata(
self, app, user_one, project, url, payload, cedar_template, collection, provider):
from osf.models import CedarMetadataRecord
collection.status_choices = ['pending', 'approved']
collection.save()
with capture_notifications():
with mock_update_share():
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=True):
res = app.post_json_api(url, payload(guid=project._id, status='pending'), auth=user_one.auth)
assert res.status_code == 201

detail_url = f'/{API_BASE}collections/{collection._id}/collected_metadata/{project._id}/'
with override_switch(features.COLLECTION_SUBMISSION_WITH_CEDAR, active=True):
res = app.post_json_api(
url,
payload(guid=project._id),
auth=user_one.auth,
expect_errors=True,
)
assert res.status_code == 400
assert 'CEDAR metadata record' in res.json['errors'][0]['detail']
app.patch_json_api(detail_url, payload(status='approved'), auth=user_one.auth)

record = CedarMetadataRecord.objects.get(guid__in=project.guids.all(), template=cedar_template)
assert record.metadata['status'] == 'approved'


class TestCollectedMetaSubjectFiltering(SubjectsFilterMixin):
Expand Down
20 changes: 20 additions & 0 deletions osf/models/collection_submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@

logger = logging.getLogger(__name__)

CEDAR_METADATA_FIELDS = [
'collected_type', 'status', 'volume', 'issue',
'program_area', 'school_type', 'study_design',
'data_type', 'disease', 'grade_levels',
]


class CollectionSubmission(TaxonomizableMixin, BaseModel):
primary_identifier_name = 'guid___id'
Expand Down Expand Up @@ -94,6 +100,20 @@ def is_submitted_by_moderator_contributor(self, event_data):
def state(self, new_state):
self.machine_state = new_state.value

def sync_cedar_metadata(self):
"""Create or update a CedarMetadataRecord from this submission's custom metadata fields."""

from osf.models import CedarMetadataRecord
if not (self.collection.provider_id and self.collection.provider.required_metadata_template):
return
template = self.collection.provider.required_metadata_template
metadata = {f: getattr(self, f) for f in CEDAR_METADATA_FIELDS if getattr(self, f, '')}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main thing this has to do is make a valid cedar record (is why calling validate_required_metadata seems relevant) -- to index for search, will at least need a "@context" to be parsable as json-ld (either build one on the fly from the template jsonschema or hard-code one based on the specific properties used for the existing collection fields)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have a question: why don't we use the Cedar Metadata Editor on the FE and change the collection submission process so that the FE would create a Cedar Metadata Record by posting to the respective endpoints, instead of having the BE do the "sync"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the Cedar Metadata Editor on the FE would give cleaner separation of concerns, but the custom fields (status, volume, issue, etc.) are already part of the existing collection submission payload — splitting that into a separate cedar record creation step adds coordination complexity and risk of partial failure on the client side. The BE sync keeps the existing submission flow unchanged for all clients while ensuring the cedar record stays consistent with the submission fields.

metadata['@context'] = template.cedar_id
record, _ = CedarMetadataRecord.objects.get_or_create(guid=self.guid, template=template)
record.metadata = metadata
record.is_published = True
record.save()

def _notify_contributors_pending(self, event_data):
user = event_data.kwargs.get('user')
for contributor in self.guid.referent.contributors:
Expand Down
17 changes: 0 additions & 17 deletions osf/models/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,23 +167,6 @@ def update_or_create_from_json(cls, provider_data, user):
related_name='required_by_providers',
)

def validate_required_metadata(self, obj):
"""
Raises ValidationError if obj does not have a CedarMetadataRecord for
this provider's required_metadata_template.
Does nothing when required_metadata_template is not set.
"""
if not self.required_metadata_template_id:
return
guid = obj.guids.first()
if guid is None or not guid.cedar_metadata_records.filter(
template_id=self.required_metadata_template_id
).exists():
raise ValidationError(
f'Submitted object must have a CEDAR metadata record for template '
f'"{self.required_metadata_template.schema_name}" to be submitted to this collection.'
)

def __repr__(self):
return ('(name={self.name!r}, default_license={self.default_license!r}, '
'allow_submissions={self.allow_submissions!r}) with id {self.id!r}').format(self=self)
Expand Down