SACGF / SACGF/variantgrid

Auto-detected bugs - Classifications

Open
#1,446 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
30
Forks
3
Avg merge
9h 28m
Merged PRs (30d)
42

Description

**Feel free to split this up**

BUG 1 — AttributeError: wrong attribute name in as_json()
FILE: classification/models/evidence_mixin.py
LINE: 72
IMPACT: HIGH — runtime AttributeError any time as_json() is called on a
SomaticClinicalSignificanceValue that has an amp_level

```
SomaticClinicalSignificanceValue is a frozen dataclass with fields:
tier_level: str
amp_level: Optional[str]

But as_json() references self.level which does not exist:

def as_json(self):
return {
"somatic_clinical_significance": self.tier_level,
"amp_level": self.level # <-- BUG: should be self.amp_level
}
```

Fix: change self.level → self.amp_level

--------------------------------------------------------------------------------

BUG 2 — Operator precedence error in ```__lt__``` (somatic value sort is broken)
FILE: classification/models/evidence_mixin.py
LINE: 67
IMPACT: HIGH — somatic classification values sort incorrectly

```
def __lt__(self, other):
return self.sort_value or 0 < other.sort_value or 0

Python operator precedence makes this parse as:
return (self.sort_value) or (0 < other.sort_value) or (0)

Which is a boolean/truthy evaluation, NOT a numeric comparison. The intended
logic is clearly:
return (self.sort_value or 0) < (other.sort_value or 0)
```

Fix: add parentheses around each "or 0" clause.

--------------------------------------------------------------------------------

BUG 3 — Typo in type annotation (copy/paste)
FILE: classification/models/evidence_mixin.py
LINE: 54
IMPACT: LOW — only affects type checkers / IDEs, not runtime

```
@property
def without_amp_level(self) -> 'SopmaticClinicalSignificanceValue':
^^^^^^^^^^
Typo: "Sopmatic" instead of "Somatic". Should be:
-> 'SomaticClinicalSignificanceValue'
```

--------------------------------------------------------------------------------

BUG 4 — Typo in ORM lookup path causes FieldError at runtime
FILE: classification/views/classification_grouping_datatables.py
LINE: 203
IMPACT: HIGH — any user filtering by protein position gets a Django FieldError
(500 error or unhandled exception)

```
filters.append(Q(allele_origin_grouping__allele_grouing__allele__variantallele__variant__in=variant_qs))
^^^^^^^^^^^^
Typo: "allele_grouing" instead of "allele_grouping".

The actual FK field on AlleleOriginGrouping is `allele_grouping`
(confirmed in classification/models/classification_grouping.py line 97).

Fix: rename allele_grouing → allele_grouping in the Q() lookup.
```

-----------

Bugs 5 and 6 have been moved to #1466

--------------------------------------------------------------------------------

BUG 7 — Wrong settings attribute name in error message path
FILE: classification/tasks/classification_import_map_and_insert_task.py
LINES: 50–52
IMPACT: MEDIUM — when an invalid file_type_override is supplied, the
AttributeError on the wrong settings name masks the original
validation error

```
if file_type_override not in settings.CLASSIFICATION_OMNI_IMPORTER_PARSERS:
valid_parsers = ",".join(settings.OMNI_IMPORTER_PARSERS) # <-- BUG
raise ValueError(f'{file_type_override=} must be one of {valid_parsers}')
```

Line 50 correctly uses settings.CLASSIFICATION_OMNI_IMPORTER_PARSERS.
Line 51 uses settings.OMNI_IMPORTER_PARSERS (different name, likely does not
exist), causing an AttributeError instead of the intended ValueError.

Fix: change settings.OMNI_IMPORTER_PARSERS → settings.CLASSIFICATION_OMNI_IMPORTER_PARSERS

--------------------------------------------------------------------------------

BUG 8 — Nullable FK accessed without None check in preview property
FILE: classification/models/clinvar_export_models.py
LINE: 233
IMPACT: MEDIUM — AttributeError when ClinVarExport.preview is called on a
record where classification_based_on is None

```
classification_based_on = models.ForeignKey(
ClassificationModification, null=True, blank=True, on_delete=models.CASCADE
) # field is nullable

@property
def preview(self) -> PreviewData:
extra = [
PreviewKeyValue(key="Lab:", value=self.classification_based_on.lab.name), # BUG
]
```

classification_based_on can be None (null=True), but the property dereferences
it unconditionally. Compare with line 139 elsewhere in the same file which
correctly guards with: if self.classification_based_on_id is None.

Fix: guard with a None check, e.g.:
lab_name = self.classification_based_on.lab.name if self.classification_based_on_id else "Unknown"

--------------------------------------------------------------------------------

BUG 9 — Pretty label compared against raw key value (comparison always fails)
FILE: classification/signals/classification_hooks_significant_change.py
LINE: 122
IMPACT: MEDIUM — the close message always incorrectly appends
", expected " even when the new classification
matches the pending change expectation

```
pending_change_label = EvidenceKeyMap.cached_key(...).pretty_value(pending_change_value)
if classification_change.new_label != pending_change_value:
close_message += f", expected {pending_change_label}"
```

classification_change.new_label is a human-readable string (e.g. "Pathogenic").
pending_change_value is a raw EvidenceKey key value (e.g. "P").
These will never be equal, so the "expected X" clause is always appended.

The comparison should be one of:
- classification_change.new_value != pending_change_value (both raw values)
- classification_change.new_label != pending_change_label (both pretty labels)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.