Make RNAFUSION samples be always tumour by diitaz93 · Pull Request #4537 · Clinical-Genomics/cg

diitaz93 · 2025-08-01T08:57:09Z

Description

Closes #4528
Closes #4530
This PR adds validation to RNAFUSION samples so that they are always tumour

Added

Boolean tumour field in the RNAFusionSample defaulting to True
Field validator over "tumour" that verifies that it is always true
Overriding of __setattr__ method of the RNAFusionSample so that it raises an error if tumour is set to False
Overriding of model_copy method of the RNAFusionSample so that it raises an error if tumour is set to False when updating
Tests for above scenarios

How to prepare for test

Ssh to relevant server (depending on type of change)
Use stage: us
Paxa the environment: paxa

Install on stage (example for Hasta):

bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_cg -t cg -b tumor-validation-rnafusion -a

How to test

Submit an RNAFUSION order using the order form with samples without specifying tumour value -> verify that is persisted as True in StatusDB

Submit an RNAFUSION order through the order portal using an existing normal (non-tumour) sample -> verify that it raises an error with a meaningful message
Submit an RNAFUSION order using the order form specifying false as tumour status -> verify that it raises an error with a meaningful message (not very meaningful message, is there something we can do about this? @islean )

Review

Tests executed by
"Merge and deploy" approved by
Thanks for filling in who performed the code review and the test!

This version is a

PATCH - when you make backwards compatible bug fixes or documentation/instructions

Implementation Plan

Deployed to stage:

Deployed to production:

cg/services/orders/validation/order_types/rna_fusion/models/sample.py

mogmi

Looks good!

cg/services/orders/validation/order_types/rna_fusion/models/sample.py

sonarqubecloud · 2025-08-01T13:27:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

diitaz93 · 2025-08-01T13:28:05Z

cg/services/orders/validation/models/existing_sample.py

+    def default_and_validate_tumour(cls, v) -> bool:
+        """Ensure that the tumour field is always set to True.
+        If it is not provided, it defaults to True.
+        If it is provided and not True, raise a ValueError.
+        """
+        if v is None:
+            return True
+        if v is not True:
+            raise ValueError("Can't perform RNAFUSION analysis on non-tumour samples")
+        return v


we might want this to fail even if is None

diitaz93 · 2025-08-01T13:33:03Z

cg/services/orders/validation/order_types/rna_fusion/models/sample.py

+    def __setattr__(self, name, value):
+        if name == "tumour" and hasattr(self, name):
+            raise ValueError(TUMOUR_ERROR_MESSAGE)
+        super().__setattr__(name, value)
+
+    def model_copy(self, *args, update=None, **kwargs):
+        if update and "tumour" in update and update["tumour"] is not True:
+            raise ValueError(TUMOUR_ERROR_MESSAGE)
+        return super().model_copy(*args, update=update, **kwargs)


Is this really needed? The RNAFusionSample is like a DTO which is never modified

diitaz93 · 2025-08-01T13:33:30Z

cg/services/orders/validation/service.py

        if not errors.is_empty:
            LOG.error(errors.get_error_message())
-            raise OrderValidationError(message="Order contained errors")
+            raise OrderValidationError(message=errors.get_error_message())


Is this too crazy @islean? This will improve the error message

If we do that change, we should remove the logging of the error above though? Now we will just have the same thing twice.

The thing is, this message is the one the customer sees, not the logging. So during the tests, I got "Order contained errors" in the order portal, which is very confusing if you don't know what is wrong with your order

Oh, but this is not something that the customer can address either way, since the error concerns a field which is not editable for them. So I would say it is not an issue that the "Order contained errors" is nondescript.

It is editable in the order form

they also could pick up an existing non-tumour sample

How can they edit the tumour status? Or you mean that they can do it in the Excel form? Will we even persist that?

Might be worth to inform about existing samples, although I feel that would be better as a filter in the endpoint for fetching old samples

diitaz93 · 2025-08-01T13:34:30Z

tests/services/orders/validation_service/test_model_validator.py

+def test_set_tumour_to_false_fails_rnafusion_sample(rnafusion_order_to_submit: dict):
+    """Test that setting tumour to False for a RNAFUSION sample raises an error."""
+    # GIVEN a parsed RNAFUSION order
+    order, _ = ModelValidator.validate(order=rnafusion_order_to_submit, model=RNAFusionOrder)
+
+    # WHEN setting the tumour field to False for a RNAFUSION sample
+
+    # THEN it should raise a ValueError saying that RNAFUSION samples must be tumour samples
+    with pytest.raises(ValueError) as exc_info:
+        order.cases[0].samples[0].tumour = False
+    assert str(exc_info.value) == "RNAFUSION samples must always be tumour samples"


If we remove the overriden functions in RNAFusionSample, this test might not be needed anymore

islean · 2025-08-04T08:00:38Z

cg/services/orders/validation/models/existing_sample.py

+class ExistingRNAFusionSample(ExistingSample):
+    """Model with special tumour validation for existing RNAFUSION samples."""
+
+    tumour: bool = True


I think we want this to be an internal attribute which customers can not access at all. Then we can type hint it as a literal True. Or we simply remove it from the model. Leaving it open for modification just makes it necessary to add all these validators. If we call it _tumour, customers can not modify it: https://docs.pydantic.dev/latest/concepts/models/#private-model-attributes

That is a great suggestion, I will try to implement it 👍

The problem is we want an error to be shown to the customer if they try to use non-tumour samples

Then we need to design how it should look. Given that the tumour status is not rendered for new samples it should likely be in the sample_errors or something of the case?

Just noticed that this is the existing sample. We should fetch tumour status from statusDB for existing samples IMO

Just remembered - we could add a warning that we have overruled the tumour status, similar to what was done for the capture kit here

islean · 2025-08-04T08:01:12Z

cg/services/orders/validation/order_types/rna_fusion/models/sample.py

    source: str
    subject_id: str = Field(pattern=NAME_PATTERN, min_length=1, max_length=128)
    tissue_block_size: TissueBlockEnum | None = None
+    tumour: bool = True


Same thing here

This model is for new samples which currently do not have a tumour field in the order portal. We should either align them or make the field private

add validation for tumour value in rnafusion samples

c0557de

diitaz93 commented Aug 1, 2025

View reviewed changes

cg/services/orders/validation/order_types/rna_fusion/models/sample.py Outdated Show resolved Hide resolved

diitaz93 added 2 commits August 1, 2025 11:05

small renaming of test functions

3118ce6

fix sonar cloud isuue

587004a

diitaz93 marked this pull request as ready for review August 1, 2025 09:14

diitaz93 requested a review from a team as a code owner August 1, 2025 09:14

diitaz93 added 2 commits August 1, 2025 11:17

fix further more sonar cloud isuue

971b7ce

change scope of fixture to function to make test work

57fb0a8

mogmi approved these changes Aug 1, 2025

View reviewed changes

cg/services/orders/validation/order_types/rna_fusion/models/sample.py Outdated Show resolved Hide resolved

clingen-sthlm temporarily deployed to stage August 1, 2025 11:11 Inactive

clingen-sthlm temporarily deployed to stage August 1, 2025 11:12 Inactive

make the field validator before to include None values of tumour

4b8fdb3

clingen-sthlm temporarily deployed to stage August 1, 2025 11:46 Inactive

add validation for existing samples

3bfbb32

diitaz93 commented Aug 1, 2025

View reviewed changes

diitaz93 requested a review from islean August 1, 2025 13:35

diitaz93 marked this pull request as draft August 1, 2025 13:35

islean reviewed Aug 4, 2025

View reviewed changes

Conversation

diitaz93 commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Added

How to prepare for test

How to test

Review

This version is a

Implementation Plan

Uh oh!

Uh oh!

mogmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Aug 1, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

diitaz93 commented Aug 1, 2025 •

edited

Loading