[SYNPY-1861] remove submissionInstructionsMessage and submissionReceiptMessage from required attributes

[jaymedina]

Problem:

The Evaluation OOP model's store() method incorrectly required submission_instructions_message and
submission_receipt_message to be set before creating or updating an evaluation. These are optional display messages
shown to submitters. The Synapse REST API accepts PUT requests without them.

Solution:

• Remove submission_instructions_message and submission_receipt_message from the required attributes validated in to_synapse_request().
• Updated field and class-level docstrings to clearly mark name, description, and content_source as the true required-to-store attributes.
• Added unit test coverage verifying the request body is built correctly without the message fields
• Added integration tests for creating and updating evaluations without them, including a test confirming that submissions can be stored and retrieved against an evaluation that was created without these fields.

Testing:

Tests are passing locally:

[Copilot]

Pull request overview

This pull request updates the Evaluation OOP model so that submission_instructions_message and submission_receipt_message are treated as truly optional when building the REST request body for create/update operations, aligning client-side validation with the Synapse REST API’s accepted payloads.

Changes:

• Removed submission_instructions_message and submission_receipt_message from the set of attributes required by Evaluation.to_synapse_request().
• Updated to_synapse_request() to omit submission message fields from the request body when they are None.
• Added/updated unit and integration tests to verify evaluations (and submissions against them) can be created/updated without these optional message fields.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
synapseclient/models/evaluation.py	Makes submission message fields optional in request validation and omits them from request bodies when unset.
tests/unit/synapseclient/models/unit_test_evaluation.py	Updates required-field tests and adds a unit test asserting request bodies omit optional message keys when absent.
tests/integration/synapseclient/models/async/test_evaluation_async.py	Adds integration coverage for creating and updating evaluations without optional message fields.
tests/integration/synapseclient/models/async/test_submission_async.py	Adds integration coverage for creating/retrieving submissions against an evaluation created without optional message fields.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jaymedina]

Note: This comment has been generated with AI assistance and reviewed by me ✍️

Reviewed evaluation.py, unit_test_evaluation.py, test_evaluation_async.py, and test_submission_async.py. The core logic change is correct — the two message fields are properly removed from the required list and are now conditionally included in the request body only when non-None. Docstrings and examples are updated consistently. Two minor findings left as inline comments.

[linglp]

@jaymedina Hi Jenny! It looks good overall. I found some issues in the tests and docstring that should probably be fixed before merging.

[linglp]

Nit: the submission tests shouldn't care whether submission_instructions_message or submission_receipt_message are set. What about just removing the message fields from test_evaluation, delete test_store_submission_to_evaluation_without_message_fields_async

[jaymedina]

Note: This comment has been generated with AI assistance and reviewed by me ✍️

The intent of test_store_submission_to_evaluation_without_message_fields_async is to ensure that the submission service doesn't require submission_instructions_message or submission_receipt_message to be set on the evaluation — since those attributes surface to submitters, it's worth explicitly verifying that submissions can be created against an evaluation that omits them. Happy to keep the test and clarify the comment if that makes the intent clearer.

[linglp]

I think the entire TestEvaluationValidation class should also probably be moved to unit test

[linglp]

Also, test_create_evaluation_missing_required_fields only covers when the description is missing. But missing name and missing content_source aren't tested.

[linglp]

I think def _build_evaluation_with_all_fields(self) -> Evaluation and probably some other tests related to test_to_synapse_request_create should also be updated.

[linglp]

I saw that under to_synapse_request, we have:

        for attribute in required_attributes:
            if not getattr(self, attribute):
                raise ValueError(
                    f"Your evaluation object is missing the '{attribute}' attribute. This attribute is required to {request_type.value} an evaluation"
                )

But this would only report the first missing field. I think the consistent pattern in the code base is to check per field:

if not self.x: raise ValueError(...)

[thomasyu888]

For discussion, should there be an Optional[str] here if it is "required"? Is Required to store a new semantic within the client?

[linglp]

I think the idea is that "name" is required when calling evaluation.store(). But I think we don't have the convention of doing something like **Required to store.** in the code base. My understanding is that we will just raise an error like here

[andrewelamb]

I agree with Lingling here on both counts.

[thomasyu888]

Should type hinting be...

name: None
    """The name of this Evaluation."""

instead of

name: Optional[str] = None

[andrewelamb]

Shouldn't delete_none_keys(request_body) be used here?