Fix validation for threshold metrics (#41)

* Fix validation for threshold metrics

* Fix failing test

* Update package version

Author: MaloChevillotte

README.md

@@ -92,6 +92,7 @@ Each advanced aggregation type requires its specific parameter and cannot use ot
   - `aggregation_type`: "sum" or "count" (**not** count_distinct)
   - `breach_value`: numeric threshold value
 - **Cannot use**: `retention_threshold_days`, `conversion_threshold_days`
+- **Cannot use**: Timeframe parameters (`aggregation_timeframe_start_value`, `aggregation_timeframe_end_value`, `aggregation_timeframe_unit`)
 
 #### Retention Metrics  
 - **Required**: `retention_threshold_days` (numeric)

eppo_metrics_sync/validation.py

@@ -217,10 +217,13 @@ def aggregation_is_valid(aggregation):
                 matched[0] + ' specified, but operation is ' + aggregation['operation']
             )
 
-    # if threshold:
+    # threshold operations cannot use timeframe parameters
     if aggregation['operation'] == 'threshold':
-        # TODO
-        pass
+        matched = [p for p in timeframe_parameters if p in aggregation]
+        if matched:
+            error_message.append(
+                f"Cannot specify {', '.join(matched)} for operation threshold"
+            )
 
     distinct_advanced_aggregation_parameter_set(
         aggregation,

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "eppo_metrics_sync"
-version = "0.1.12"
+version = "0.1.13"
 description = "Sync metrics to Eppo"
 readme = "README.md"
 requires-python = ">=3.7"

tests/test_validation.py

@@ -133,6 +133,38 @@ def test_extra_parameter_on_retention_metric():
     assert res == 'Invalid parameter for retention aggregation: conversion_threshold_days'
 
 
+def test_threshold_with_timeframe_parameters():
+    test_agg = {
+        'operation': 'threshold',
+        'aggregation_timeframe_end_value': 7,
+        'aggregation_timeframe_unit': 'days',
+        'threshold_metric_settings': {
+            'comparison_operator': 'gt',
+            'aggregation_type': 'sum',
+            'breach_value': 100
+        }
+    }
+
+    res = aggregation_is_valid(test_agg)
+    assert res == 'Cannot specify aggregation_timeframe_end_value, aggregation_timeframe_unit for operation threshold'
+
+
+def test_threshold_with_single_timeframe_parameter():
+    test_agg = {
+        'operation': 'threshold',
+        'aggregation_timeframe_start_value': 1,
+        'aggregation_timeframe_unit': 'days',  # Add unit to avoid the unit validation error
+        'threshold_metric_settings': {
+            'comparison_operator': 'gte',
+            'aggregation_type': 'count',
+            'breach_value': 5
+        }
+    }
+
+    res = aggregation_is_valid(test_agg)
+    assert res == 'Cannot specify aggregation_timeframe_start_value, aggregation_timeframe_unit for operation threshold'
+
+
 def test_count_distinct():
     test_agg = {
         'operation': 'count_distinct',