diff --git a/synapseclient/models/evaluation.py b/synapseclient/models/evaluation.py index 5b34d32c3..105564ece 100644 --- a/synapseclient/models/evaluation.py +++ b/synapseclient/models/evaluation.py @@ -6,7 +6,7 @@ from synapseclient import Synapse from synapseclient.core.async_utils import async_to_sync -from synapseclient.core.utils import merge_dataclass_entities +from synapseclient.core.utils import delete_none_keys, merge_dataclass_entities from synapseclient.models.protocols.evaluation_protocol import ( EvaluationSynchronousProtocol, ) @@ -33,11 +33,11 @@ class Evaluation(EvaluationSynchronousProtocol): etag: Synapse employs an Optimistic Concurrency Control (OCC) scheme to handle concurrent updates. The eTag changes every time an Evaluation is updated; it is used to detect when a client's copy of an Evaluation is out-of-date. - name: The name of this Evaluation. - description: A text description of this Evaluation. + name: **Required to store.** The name of this Evaluation. + description: **Required to store.** A text description of this Evaluation. owner_id: The ID of the Synapse user who created this Evaluation. created_on: The date on which Evaluation was created. - content_source: The Synapse ID of the Entity to which this Evaluation belongs, + content_source: **Required to store.** The Synapse ID of the Entity to which this Evaluation belongs, e.g. a reference to a Synapse project. submission_instructions_message: Message to display to users detailing acceptable formatting for Submissions to this Evaluation. submission_receipt_message: Message to display to users upon successful submission to this Evaluation. @@ -56,8 +56,6 @@ class Evaluation(EvaluationSynchronousProtocol): name="My Challenge Evaluation", description="Evaluation for my data challenge", content_source="syn123456", - submission_instructions_message="Submit CSV files only", - submission_receipt_message="Thank you for your submission!", ) created = evaluation.store() ``` @@ -123,10 +121,10 @@ class Evaluation(EvaluationSynchronousProtocol): of an Evaluation is out-of-date.""" name: Optional[str] = None - """The name of this Evaluation.""" + """**Required to store.** The name of this Evaluation.""" description: Optional[str] = None - """A text description of this Evaluation.""" + """**Required to store.** A text description of this Evaluation.""" owner_id: Optional[str] = None """The ID of the Synapse user who created this Evaluation.""" @@ -135,7 +133,7 @@ class Evaluation(EvaluationSynchronousProtocol): """The date on which Evaluation was created.""" content_source: Optional[str] = None - """The Synapse ID of the Entity to which this Evaluation belongs, + """**Required to store.** The Synapse ID of the Entity to which this Evaluation belongs, e.g. a reference to a Synapse project.""" submission_instructions_message: Optional[str] = None @@ -250,23 +248,26 @@ def to_synapse_request(self, request_type: RequestType): ValueError: If any required attributes are missing. """ - # These attributes are required in our PUT requests for creating or updating an evaluation - required_attributes = [ - "name", - "description", - "content_source", - "submission_instructions_message", - "submission_receipt_message", - ] - - # For "update" requests, add id and etag + if not self.name: + raise ValueError( + f"Your evaluation object is missing the 'name' attribute. This attribute is required to {request_type.value} an evaluation" + ) + if not self.description: + raise ValueError( + f"Your evaluation object is missing the 'description' attribute. This attribute is required to {request_type.value} an evaluation" + ) + if not self.content_source: + raise ValueError( + f"Your evaluation object is missing the 'content_source' attribute. This attribute is required to {request_type.value} an evaluation" + ) if request_type == RequestType.UPDATE: - required_attributes.extend(["id", "etag"]) - - for attribute in required_attributes: - if not getattr(self, attribute): + if not self.id: + raise ValueError( + f"Your evaluation object is missing the 'id' attribute. This attribute is required to {request_type.value} an evaluation" + ) + if not self.etag: raise ValueError( - f"Your evaluation object is missing the '{attribute}' attribute. This attribute is required to {request_type.value} an evaluation" + f"Your evaluation object is missing the 'etag' attribute. This attribute is required to {request_type.value} an evaluation" ) # Build a request body for storing a brand new evaluation @@ -277,6 +278,7 @@ def to_synapse_request(self, request_type: RequestType): "submissionInstructionsMessage": self.submission_instructions_message, "submissionReceiptMessage": self.submission_receipt_message, } + delete_none_keys(request_body) # For UPDATE request types, add id and etag if request_type == RequestType.UPDATE: @@ -307,7 +309,7 @@ async def store_async( Example: Creating a new evaluation   - Create a new evaluation on Synapse by storing an evaluation object with the required fields. If there are any fields missing, an error will be raised. + Create a new evaluation on Synapse by storing an evaluation object with the required fields. ```python from synapseclient.models import Evaluation from synapseclient import Synapse @@ -321,8 +323,6 @@ async def create_evaluation(): name="My Challenge Evaluation", description="Evaluation for my data challenge", content_source="syn123456", - submission_instructions_message="Submit CSV files only", - submission_receipt_message="Thank you for your submission!" ).store_async() return evaluation diff --git a/tests/integration/synapseclient/models/async/test_evaluation_async.py b/tests/integration/synapseclient/models/async/test_evaluation_async.py index d2a26e078..4918ee4ff 100644 --- a/tests/integration/synapseclient/models/async/test_evaluation_async.py +++ b/tests/integration/synapseclient/models/async/test_evaluation_async.py @@ -17,18 +17,12 @@ def init(self, syn: Synapse, schedule_for_cleanup: Callable[..., None]) -> None: self.syn = syn self.schedule_for_cleanup = schedule_for_cleanup - async def test_create_evaluation(self): - # GIVEN a project to work with - project = await Project(name=f"test_project_{uuid.uuid4()}").store_async( - synapse_client=self.syn - ) - self.schedule_for_cleanup(project.id) - + async def test_create_evaluation(self, project_model: Project): # WHEN I create an evaluation using the dataclass method evaluation = Evaluation( name=f"test_evaluation_{uuid.uuid4()}", description="A test evaluation for testing purposes", - content_source=project.id, + content_source=project_model.id, submission_instructions_message="Please submit your results in CSV format", submission_receipt_message="Thank you for your submission!", ) @@ -42,10 +36,45 @@ async def test_create_evaluation(self): assert ( created_evaluation.description == "A test evaluation for testing purposes" ) - assert created_evaluation.content_source == project.id + assert created_evaluation.content_source == project_model.id assert created_evaluation.owner_id is not None # Check that owner_id is set assert created_evaluation.created_on is not None # Check that created_on is set + async def test_create_evaluation_without_optional_message_fields( + self, project_model: Project + ): + # WHEN I create an evaluation without submission_instructions_message or submission_receipt_message + evaluation = Evaluation( + name=f"test_evaluation_{uuid.uuid4()}", + description="A test evaluation without optional message fields", + content_source=project_model.id, + ) + created_evaluation = await evaluation.store_async(synapse_client=self.syn) + self.schedule_for_cleanup(created_evaluation.id) + + # THEN the evaluation should be created successfully + assert created_evaluation.id is not None + assert created_evaluation.etag is not None + assert created_evaluation.name == evaluation.name + assert created_evaluation.content_source == project_model.id + assert created_evaluation.submission_instructions_message is None + assert created_evaluation.submission_receipt_message is None + + # WHEN I update a field without setting message fields + new_description = f"Updated description {uuid.uuid4()}" + old_etag = created_evaluation.etag + created_evaluation.description = new_description + updated_evaluation = await created_evaluation.store_async( + synapse_client=self.syn + ) + + # THEN the update should also succeed without message fields + assert updated_evaluation.description == new_description + assert updated_evaluation.id == created_evaluation.id + assert updated_evaluation.etag != old_etag + assert updated_evaluation.submission_instructions_message is None + assert updated_evaluation.submission_receipt_message is None + class TestGetEvaluation: @pytest.fixture(autouse=True, scope="function") @@ -519,54 +548,3 @@ async def test_update_acl_async_with_full_dictionary( user_access is not None ), f"User {current_user_id} not found in updated ACL" assert set(user_access["accessType"]) == set(["READ", "DELETE", "SUBMIT"]) - - -class TestEvaluationValidation: - @pytest.fixture(autouse=True, scope="function") - def init(self, syn: Synapse, schedule_for_cleanup: Callable[..., None]) -> None: - self.syn = syn - self.schedule_for_cleanup = schedule_for_cleanup - - async def test_create_evaluation_missing_required_fields(self): - # WHEN I try to create an evaluation with missing required fields - evaluation = Evaluation(name="test_evaluation") - - # THEN it should raise a ValueError - with pytest.raises(ValueError, match="missing the 'description' attribute"): - await evaluation.store_async(synapse_client=self.syn) - - async def test_get_evaluation_missing_id_and_name(self): - # WHEN I try to get an evaluation without id or name - evaluation = Evaluation() - - # THEN it should raise a ValueError - with pytest.raises( - ValueError, match="Either id or name must be set to get an evaluation" - ): - await evaluation.get_async(synapse_client=self.syn) - - async def test_delete_evaluation_missing_id(self): - # WHEN I try to delete an evaluation without an id - evaluation = Evaluation(name="test_evaluation") - - # THEN it should raise a ValueError - with pytest.raises(ValueError, match="id must be set to delete an evaluation"): - await evaluation.delete_async(synapse_client=self.syn) - - async def test_get_acl_missing_id(self): - # WHEN I try to get ACL for an evaluation without an id - evaluation = Evaluation(name="test_evaluation") - - # THEN it should raise a ValueError - with pytest.raises(ValueError, match="id must be set to get evaluation ACL"): - await evaluation.get_acl_async(synapse_client=self.syn) - - async def test_get_permissions_missing_id(self): - # WHEN I try to get permissions for an evaluation without an id - evaluation = Evaluation(name="test_evaluation") - - # THEN it should raise a ValueError - with pytest.raises( - ValueError, match="id must be set to get evaluation permissions" - ): - await evaluation.get_permissions_async(synapse_client=self.syn) diff --git a/tests/integration/synapseclient/models/async/test_submission_async.py b/tests/integration/synapseclient/models/async/test_submission_async.py index f8ae95f9c..71ca65f96 100644 --- a/tests/integration/synapseclient/models/async/test_submission_async.py +++ b/tests/integration/synapseclient/models/async/test_submission_async.py @@ -98,6 +98,40 @@ async def test_store_submission_successfully_async( assert created_submission.created_on is not None assert created_submission.version_number is not None + async def test_store_submission_to_evaluation_without_message_fields_async( + self, + test_project: Project, + test_file: File, + ): + # GIVEN an evaluation created without submission_instructions_message or submission_receipt_message + evaluation = Evaluation( + name=f"test_evaluation_{uuid.uuid4()}", + description="Evaluation without optional message fields", + content_source=test_project.id, + ) + created_evaluation = await evaluation.store_async(synapse_client=self.syn) + self.schedule_for_cleanup(created_evaluation.id) + + # WHEN I submit to that evaluation + submission = Submission( + entity_id=test_file.id, + evaluation_id=created_evaluation.id, + name=f"Test Submission {uuid.uuid4()}", + ) + created_submission = await submission.store_async(synapse_client=self.syn) + self.schedule_for_cleanup(created_submission.id) + + # THEN the submission should be created successfully + assert created_submission.id is not None + assert created_submission.entity_id == test_file.id + assert created_submission.evaluation_id == created_evaluation.id + + # AND retrieving the submission should also work + retrieved = await Submission(id=created_submission.id).get_async( + synapse_client=self.syn + ) + assert retrieved.id == created_submission.id + async def test_store_submission_without_entity_id_async( self, test_evaluation: Evaluation ): diff --git a/tests/unit/synapseclient/models/async/unit_test_evaluation_async.py b/tests/unit/synapseclient/models/async/unit_test_evaluation_async.py index 71ee2346f..def3f2e96 100644 --- a/tests/unit/synapseclient/models/async/unit_test_evaluation_async.py +++ b/tests/unit/synapseclient/models/async/unit_test_evaluation_async.py @@ -92,7 +92,7 @@ def get_example_permissions_response(self) -> Dict[str, Union[str, List, bool]]: } def _build_evaluation_with_all_fields(self) -> Evaluation: - """Build an Evaluation instance with all required fields for store.""" + """Build an Evaluation instance with all fields set.""" return Evaluation( name=EVALUATION_NAME, description=EVALUATION_DESCRIPTION, diff --git a/tests/unit/synapseclient/models/unit_test_evaluation.py b/tests/unit/synapseclient/models/unit_test_evaluation.py index 730e8771f..2851ef2ae 100644 --- a/tests/unit/synapseclient/models/unit_test_evaluation.py +++ b/tests/unit/synapseclient/models/unit_test_evaluation.py @@ -99,8 +99,6 @@ def test_to_synapse_request_missing_required_fields(self): Evaluation( description="Test", content_source="syn123", - submission_instructions_message="Inst", - submission_receipt_message="Rec", ), "name", ), @@ -108,8 +106,6 @@ def test_to_synapse_request_missing_required_fields(self): Evaluation( name="Test", content_source="syn123", - submission_instructions_message="Inst", - submission_receipt_message="Rec", ), "description", ), @@ -117,29 +113,9 @@ def test_to_synapse_request_missing_required_fields(self): Evaluation( name="Test", description="Test", - submission_instructions_message="Inst", - submission_receipt_message="Rec", ), "content_source", ), - ( - Evaluation( - name="Test", - description="Test", - content_source="syn123", - submission_receipt_message="Rec", - ), - "submission_instructions_message", - ), - ( - Evaluation( - name="Test", - description="Test", - content_source="syn123", - submission_instructions_message="Inst", - ), - "submission_receipt_message", - ), ] # Test each case @@ -149,6 +125,36 @@ def test_to_synapse_request_missing_required_fields(self): ): evaluation.to_synapse_request(RequestType.CREATE) + def test_to_synapse_request_without_optional_message_fields(self): + """Test that submission_instructions_message and submission_receipt_message + are not required — the request body omits them when absent.""" + # GIVEN an evaluation without message fields + evaluation = Evaluation( + name="Test Evaluation", + description="This is a test evaluation", + content_source="syn123456", + ) + + # WHEN we generate a request body for create + request_body = evaluation.to_synapse_request(RequestType.CREATE) + + # THEN the body should not include the optional message keys + assert request_body == { + "name": "Test Evaluation", + "description": "This is a test evaluation", + "contentSource": "syn123456", + } + assert "submissionInstructionsMessage" not in request_body + assert "submissionReceiptMessage" not in request_body + + # AND the same applies to an update request + evaluation.id = "9614112" + evaluation.etag = "abc-123-xyz" + update_body = evaluation.to_synapse_request(RequestType.UPDATE) + + assert "submissionInstructionsMessage" not in update_body + assert "submissionReceiptMessage" not in update_body + def test_set_last_persistent_instance(self): """Test setting the last persistent instance of an evaluation.""" # GIVEN a new evaluation @@ -577,3 +583,57 @@ def test_log_message_when_removing_principal(self): mock_logger.info.assert_called_once_with( "Principal ID 12345 will be removed from ACL due to empty access_type" ) + + @pytest.mark.parametrize( + "evaluation,missing_field", + [ + (Evaluation(description="Test", content_source="syn123"), "name"), + (Evaluation(name="Test", content_source="syn123"), "description"), + (Evaluation(name="Test", description="Test"), "content_source"), + ], + ) + async def test_create_evaluation_missing_required_fields( + self, evaluation: Evaluation, missing_field: str, syn: Synapse + ): + # WHEN I try to create an evaluation with a missing required field + # THEN it should raise a ValueError + with pytest.raises( + ValueError, match=f"missing the '{missing_field}' attribute" + ): + await evaluation.store_async(synapse_client=syn) + + async def test_get_evaluation_missing_id_and_name(self, syn: Synapse): + # WHEN I try to get an evaluation without id or name + evaluation = Evaluation() + + # THEN it should raise a ValueError + with pytest.raises( + ValueError, match="Either id or name must be set to get an evaluation" + ): + await evaluation.get_async(synapse_client=syn) + + async def test_delete_evaluation_missing_id(self, syn: Synapse): + # WHEN I try to delete an evaluation without an id + evaluation = Evaluation(name="test_evaluation") + + # THEN it should raise a ValueError + with pytest.raises(ValueError, match="id must be set to delete an evaluation"): + await evaluation.delete_async(synapse_client=syn) + + async def test_get_acl_missing_id(self, syn: Synapse): + # WHEN I try to get ACL for an evaluation without an id + evaluation = Evaluation(name="test_evaluation") + + # THEN it should raise a ValueError + with pytest.raises(ValueError, match="id must be set to get evaluation ACL"): + await evaluation.get_acl_async(synapse_client=syn) + + async def test_get_permissions_missing_id(self, syn: Synapse): + # WHEN I try to get permissions for an evaluation without an id + evaluation = Evaluation(name="test_evaluation") + + # THEN it should raise a ValueError + with pytest.raises( + ValueError, match="id must be set to get evaluation permissions" + ): + await evaluation.get_permissions_async(synapse_client=syn)