Merge pull request #728 from akvo/feature/727-feedback-automatic-segmentation-ui

[#727] feat: implement manual range authority and refined segmentation UI

Author: wayangalihpratama

GEMINI.md

@@ -16,6 +16,20 @@ Income Driver Calculator (IDC) is a web application designed to help companies t
 - **CI/CD**: Automated deployment to test cluster on push to `main`.
 
 ## Recent Changes
+- **Segmentation UI Refinement (Issue #727)**:
+    - Implemented "Manual Range Authority" model: user-defined Min and Max values are strictly respected during recalculation and final segment generation.
+    *   Updated backend Pydantic models to support explicit `min` and `max` fields in segmentation requests.
+    *   Enhanced `recalculate_numerical_segments` and `process_confirmed_segmentation` to prioritize manual boundaries over automatic cascading logic.
+    *   Decoupled "Segment range" display from live form fields, ensuring it remains static/calculated until an explicitly triggered update.
+    *   Prioritized manual segment addition by swapping button order in `DataUploadSegmentForm.js`.
+    *   Implemented full bi-directional cascading helpers for numerical range editing (Segment N Min updates Segment N-1 Max, and Segment N Max updates Segment N+1 Min) using a robust 0.01 precision offset.
+    *   Refined "Adjust" button logic to send both boundaries simultaneously, preventing range reversion bugs.
+    *   Resolved a "Save Case" React crash by implementing robust error stringification for Pydantic/FastAPI validation errors and sanitizing segment payloads to exclude redundant `min`/`max` fields for categorical data.
+    *   Fixed missing segment answer values in Step 2 by implementing inclusive lower bounds (`>=`) for the starting segment of each variable, ensuring all farmers are captured.
+    *   Fixed `UnboundLocalError` in `process_confirmed_segmentation` by resolving a scoping bug in numerical segment processing.
+    *   Enforced manual, unique naming for numerical segments with real-time frontend validation and empty-by-default inputs.
+    *   Simplified Step 2 layout: adjusted grid spans (12/12), added "Min/Max" input prefixes, and made the "Segment range" display optional via toggle.
+    *   Resolved frontend lint warnings by replacing `undefined` checks with `typeof` checks.
 - **Authentication Improvements (PR #705)**:
     - Fixed infinite redirect loop for unauthenticated users accessing protected routes.
     - Implemented "Redirect After Login" feature to return users to their originally requested page.
@@ -223,7 +237,15 @@ Income Driver Calculator (IDC) is a web application designed to help companies t
         - Consolidated workflow rules for Git, PRs, and activity logging.
     - **Analytics**: Migrated from Piwik Pro to Matomo (Issue #idc-analytics) with environment-based site selection.
     - **Time Analysis**: Enhanced `analyze_time.py` with issue grouping and idle time analysis; updated `/check_time` workflow to be interactive, proactively prompting for analysis criteria.
-    - **Documentation**: Created `INCOME_CALCULATION.md` and reorganized `docs/` folder.
+    - **Segmentation UI Refinement (Issue #727)**:
+        - Defined UAC and TAC for improving farmer visibility and manual adjustment flow.
+        - Implemented colorized IDC Green tag style (`.farmer-count-tag`) for farmer counts for better visibility.
+        - Standardized tag sizes for both farmer count and segment range (`.segment-info-tag`) for visual balance.
+        - Implemented segment name initialization: numerical segments start with blank names and a `"Please specify the segment name"` placeholder.
+        - Reorganized documentation into `docs/segmentation_ui_refinement` and provided technical context in `docs/segmentation_fix`.
+        - Refactored Numerical Range UI using Ant Design `Space` and `InputNumber`, centering Min/Max labels below inputs and adding an IDC Green "Adjust" button.
+        - Prioritized the manual flow by making the "Add segment manually" button primary and moving it before the generator button.
+- **Documentation**: Created `INCOME_CALCULATION.md` and reorganized `docs/` folder.
 
 ## Codebase Structure
 - `backend/`: FastAPI application code.

backend/models/case_import.py

@@ -41,6 +41,8 @@ class SegmentDefinition(BaseModel):
     value: Union[int, float, str]
     number_of_farmers: Optional[int] = 0
     is_manual: bool = False
+    min: Optional[float] = None
+    max: Optional[float] = None
     segmentation_variable: Optional[str] = None
     variable_type: Optional[Literal["categorical", "numerical"]] = None
 
@@ -78,6 +80,8 @@ class SegmentationPreviewResponse(BaseModel):
 class SegmentValueInput(BaseModel):
     index: int = Field(..., ge=1)
     value: float | str
+    min: Optional[float] = None
+    max: Optional[float] = None
     segmentation_variable: Optional[str] = None
     variable_type: Optional[Literal["categorical", "numerical"]] = None
 

backend/utils/case_import_process_confirmed_segmentation.py

@@ -145,7 +145,11 @@ def process_confirmed_segmentation(
                 pd.DataFrame()
             )  # Empty DF for manual segments, will skip aggregation
         elif seg_type == "numerical" and seg_is_numeric:
-            # Find the previous segment of the SAME variable for lower bound
+            # Use explicit min/max if provided (Manual Authority)
+            explicit_min = seg.min
+            explicit_max = seg.max if seg.max is not None else float(seg.value)
+
+            # Find previous segment of same variable for lower bound logic
             prev_seg_same_var = next(
                 (
                     s
@@ -158,15 +162,29 @@ def process_confirmed_segmentation(
                 ),
                 None,
             )
-            lower = (
-                float(prev_seg_same_var.value) if prev_seg_same_var else None
-            )
-            upper = float(seg.value)
+
+            if explicit_min is not None:
+                lower = float(explicit_min)
+            else:
+                # Fallback to previous segment logic
+                lower = (
+                    float(prev_seg_same_var.value)
+                    if prev_seg_same_var
+                    else None
+                )
+
+            upper = float(explicit_max)
 
             if lower is None:
                 mask = seg_series <= upper
             else:
-                mask = (seg_series > lower) & (seg_series <= upper)
+                # Standard (Lower, Upper] interval for subsequent segments.
+                # Use [Lower, Upper] for the very first segment of a variable
+                # to ensure the minimum value is included.
+                if prev_seg_same_var is None:
+                    mask = (seg_series >= lower) & (seg_series <= upper)
+                else:
+                    mask = (seg_series > lower) & (seg_series <= upper)
 
             seg_df = data_df[mask]
             number_of_farmers = int(len(seg_df))

backend/utils/case_import_processing.py

@@ -210,7 +210,7 @@ def calculate_numerical_segments_from_cuts(
         segments.append(
             {
                 "index": idx,
-                "name": column,
+                "name": "",  # Empty by default for numerical segments
                 "operator": "<=",
                 "value": float(cut),
                 "number_of_farmers": int(count),
@@ -301,26 +301,47 @@ def recalculate_numerical_segments(
             None,
         )
 
-        lower_bound = (
-            float(prev_seg_same_var["value"]) if prev_seg_same_var else None
-        )
-        upper_bound = float(seg["value"])
+        # Use explicit min/max if provided (Manual Authority)
+        explicit_min = seg.get("min")
+        explicit_max = seg.get("max") or float(seg.get("value"))
+
+        if explicit_min is not None:
+            lower_bound = float(explicit_min)
+            # When using manual min, we want to respect it EXACTLY.
+            # However, to avoid overlapping counts
+            # (if user sets Min = Prev Max),
+            # we must decide on the interval. Standard is (Min, Max].
+            # If we use > lower_bound, then a value of exactly 1.0
+            # (with Min=1.0) would be excluded.
+            # This is consistent with (1.0, 2.0].
+            seg_min = lower_bound
+        elif prev_seg_same_var:
+            lower_bound = float(prev_seg_same_var["value"])
+            seg_min = lower_bound + 0.01
+        else:
+            lower_bound = None
+            # min is min of data or upper_bound (to avoid inversion)
+            data_min = np.min(series) if series.size > 0 else 0
+            seg_min = min(data_min, explicit_max)
+
+        upper_bound = float(explicit_max)
 
         # Calculate count
         if seg_type == "numerical" and is_numeric:
             if lower_bound is None:
                 count = np.sum(series <= upper_bound)
-                # min is min of data or upper_bound (to avoid inversion)
-                data_min = np.min(series) if series.size > 0 else 0
-                seg_min = min(data_min, upper_bound)
             else:
-                count = np.sum(
-                    (series > lower_bound) & (series <= upper_bound)
-                )
-                if is_integer_data:
-                    seg_min = np.floor(lower_bound) + 1
+                # Standard (Lower, Upper] interval for subsequent segments.
+                # Use [Lower, Upper] for the very first segment of a variable
+                # to ensure the minimum value is included.
+                if prev_seg_same_var is None:
+                    count = np.sum(
+                        (series >= lower_bound) & (series <= upper_bound)
+                    )
                 else:
-                    seg_min = lower_bound + 0.01
+                    count = np.sum(
+                        (series > lower_bound) & (series <= upper_bound)
+                    )
 
             seg_max = np.floor(upper_bound) if is_integer_data else upper_bound
             if is_integer_data:
@@ -329,7 +350,7 @@ def recalculate_numerical_segments(
             processed_segments.append(
                 {
                     "index": seg["index"],
-                    "name": seg_var,
+                    "name": seg.get("name") or "",
                     "operator": "<=",
                     "value": float(upper_bound),
                     "number_of_farmers": int(count),

docs/segmentation_ui_refinement/implementation_plan.md

@@ -0,0 +1,117 @@
+# Segmentation UI and Flow Improvements
+
+This task aims to improve the visibility of farmer counts per segment and refine the manual segment adjustment process to make it more intuitive and prioritized.
+
+## User Review Required
+
+> [!IMPORTANT]
+> **Cascading Exclusivity**: As requested in the feedback, adjusting the **Maximum** of Segment N will automatically update the **Minimum** of Segment N+1. This ensures segments remain exclusive without manual entry for both sides.
+
+> [!NOTE]
+> **Scope Clarification**: This plan focuses strictly on the UI refinement (visibility, layout, and cascading boundaries). The separate technical issue of "Inverted Ranges" (Auto-Sorting) is documented in [docs/segmentation_fix/implementation_plan.md](file:///Users/galihpratama/Sites/IDH-IDC/docs/segmentation_fix/implementation_plan.md) and is considered out of scope for *this* UI task unless explicitly combined.
+
+> [!NOTE]
+> **Manual Preference**: I will move the "Add segment manually" button to be more prominent, suggesting it as the primary way to define segments if automatic generation isn't used.
+
+## User Acceptance Criteria (UAC)
+
+1.  **Enhanced Farmer Visibility**: [x]
+    *   Each segment card displays the farmer count in a prominent box.
+    *   **Style**: IDC Green background (`#00605A`), white text, bold font.
+    *   **Label**: `Number of Farmers: [Count]`.
+2.  **Segment Name Initialization**: [x]
+    *   New segments have blank names by default for numerical variables.
+    *   Placeholder: `"Please specify the segment name"`.
+3.  **Numerical Range Configuration UI**: [x]
+    *   Minimum and Maximum thresholds are displayed in two clearly labeled white boxes.
+    *   An **"Adjust"** button (IDC Green) is placed next to these boxes.
+    *   The thresholds are editable upon clicking "Adjust" or directly (as per existing logic, but with improved styling).
+4.  **Bi-directional Segment Exclusivity**: [x]
+    *   Modifying the **Maximum** of Segment N automatically updates the **Minimum** of Segment N+1.
+    *   Modifying the **Minimum** of Segment N+1 automatically updates the **Maximum** of Segment N.
+    *   This ensures segments remain exclusive and continuous regardless of which side is adjusted.
+5.  **Prioritized Manual Flow**: [x]
+    *   The option to adjust segments manually is presented as the preferred/first method.
+6.  **Recalculation Trigger**: [x]
+    *   Inputting values should not auto-trigger recalculation until "Adjust" is clicked.
+7.  **Static Calculated Range Display**: [x]
+    *   The "Segment range: X - Y" label should show the *calculated* range from the backend.
+    *   It should NOT update when the user is typing into the Min or Max boxes.
+    *   It should only update when the "Adjust" operation is completed and new backend data is received.
+
+## Numerical Range Behavior: Manual Authority
+
+To ensure the UI respects user intent exactly as discussed, we will adopt the **Manual Authority** model:
+
+1.  **Ground Truth**: The values currently visible in the **Min** and **Max** input boxes are the only values used to filter data for a segment.
+2.  **Recalculation**: When "Adjust" is clicked, the backend will count farmers where `Min < Value <= Max`.
+3.  **Full Bi-directional Cascading**:
+    *   **Backward**: Adjusting Segment N **Min** to `V` updates Segment N-1 **Max** to `V - offset`.
+    *   **Forward**: Adjusting Segment N **Max** to `V` updates Segment N+1 **Min** to `V + offset`.
+    *   *Note*: `offset` is constant at `0.01` for all numerical data types to ensure maximum mathematical precision and prevent data gaps.
+4.  **Payload Synchronization**: Both `min` and `max` fields must be explicitly synchronized in the API payload to prevent stale values from overriding the new `value` (reversion bug).
+5.  **No Reversion**: The backend must accept the provided `Min` and `Max` and return them as-is, rather than recalculating the `Min` based on a previous segment.
+
+## Bug Fixes
+
+### Answer Value Generation Fix
+- [x] **Inclusive Lower Bound**: Modified `recalculate_numerical_segments` and `process_confirmed_segmentation` to use `>=` for the first segment of each variable. This ensures the minimum value (e.g., 0) is included and answer values are generated.
+
+### Save Case React Crash Fix
+- [ ] **Error Detail Stringification**: Update `CaseSettings.js` to handle `detail` objects (422 errors) from FastAPI, preventing React from crashing when an object is rendered as a child.
+- [ ] **Payload Sanitization**: Ensure `min` and `max` are only sent as numbers for numerical segments. For categorical segments, these should be `null` to avoid Pydantic validation errors (422).
+
+### UnboundLocalError Fix
+- [x] **Scoping Fix**: Move `prev_seg_same_var` lookup outside the `if/else` block in `process_confirmed_segmentation.py` to ensure it is always defined before being referenced.
+
+## Technical Acceptance Criteria (TAC)
+
+1.  **Frontend Component Updates**:
+    *   Modify `renderSegmentCard` in [DataUploadSegmentForm.js](file:///Users/galihpratama/Sites/IDH-IDC/frontend/src/pages/cases/components/DataUploadSegmentForm.js).
+    *   Implement the horizontal layout for thresholds: Range Boxes + Adjust Button.
+    *   Update [SegmentConfigurationForm.js](file:///Users/galihpratama/Sites/IDH-IDC/frontend/src/pages/cases/components/SegmentConfigurationForm.js) to handle both `min` and `max` cascading range updates in `handleOnChangeFieldValue`.
+2.  **Styling**:
+    *   Define new utility classes in `steps.scss` for the "Threshold Box" styles.
+    *   Apply IDC branding colors: `$primary-color` (`#00605A`) for backgrounds and buttons.
+3.  **Data Logic**:
+    *   Ensure the `segments` array in the form state correctly initializes `name` to `null` or `""` for numerical variables.
+    *   Implement boundary checks to prevent overlapping logic in the frontend before sending to the backend for recalculation.
+4.  **Verification**:
+    *   Verify cascading logic for at least 3 segments.
+    *   Verify farmer count visibility across different screen sizes.
+
+## Estimation
+
+| Component | Task | Effort |
+| :--- | :--- | :--- |
+| **Frontend UI** | Refactoring `renderSegmentCard` layout and implementing new boxes. | 4h |
+| **Logic** | Implementing cascading range updates and name initialization. | 2h |
+| **Styling** | SCSS updates for IDC branding and alignment. | 2h |
+| **Total** | | **8h (~1 Story Point)** |
+
+## Proposed Changes
+
+### [Frontend]
+
+#### [MODIFY] [DataUploadSegmentForm.js](file:///Users/galihpratama/Sites/IDH-IDC/frontend/src/pages/cases/components/DataUploadSegmentForm.js)
+- Update `renderSegmentCard` to include Min/Max threshold boxes with "Adjust" button.
+
+#### [MODIFY] [SegmentConfigurationForm.js](file:///Users/galihpratama/Sites/IDH-IDC/frontend/src/pages/cases/components/SegmentConfigurationForm.js)
+- Update `handleOnChangeFieldValue` to implement bi-directional cascading adjustment logic (Min updates previous Max).
+
+#### [MODIFY] [steps.scss](file:///Users/galihpratama/Sites/IDH-IDC/frontend/src/pages/cases/styles/steps.scss)
+- Add styles for `.threshold-box-container` and `.threshold-adjust-btn`.
+
+## Verification Plan
+
+### Automated Tests
+- Run existing segmentation tests to ensure no regressions:
+  `./dc.sh test backend/tests/test_unit_segmentation_strategy.py`
+
+### Manual Verification
+1.  Open segment configuration in Step 1.
+2.  Select a numerical variable and set 3 segments.
+3.  Verify the "Number of Farmers" is clearly visible in green.
+4.  Adjust the Maximum of Segment 1.
+5.  Verify Segment 2's Minimum updates automatically.
+6.  Verify segment names are blank with the correct placeholder.

frontend/src/pages/cases/components/CaseSettings.js

@@ -517,18 +517,22 @@ const CaseSettings = ({ open = false, handleCancel = () => {} }) => {
               const findSegment = data.segments.find(
                 (s) => s.name === seg.name
               );
+              const sVarType =
+                seg.variable_type || values.data_upload_variable_type;
+              const isNumerical = sVarType === "numerical";
               return {
                 id: findSegment ? findSegment.id : null,
                 index: idx,
                 name: seg.name,
                 value: seg.value || 0,
+                min: isNumerical ? seg.min : null,
+                max: isNumerical ? seg.value : null,
                 number_of_farmers: seg.number_of_farmers || 0,
                 is_manual: !!seg.is_manual,
                 segmentation_variable:
                   seg.segmentation_variable ||
                   values.data_upload_segmentation_variable,
-                variable_type:
-                  seg.variable_type || values.data_upload_variable_type,
+                variable_type: sVarType,
               };
             }),
           };
@@ -560,12 +564,19 @@ const CaseSettings = ({ open = false, handleCancel = () => {} }) => {
             })
             .catch((e) => {
               console.error(e);
-              const detail =
-                e?.response?.data?.detail ||
-                "Failed to generate segment values from import data.";
+              const detail = e?.response?.data?.detail;
+              const errorMessage =
+                typeof detail === "string"
+                  ? detail
+                  : Array.isArray(detail)
+                  ? detail.map((d) => d.msg || JSON.stringify(d)).join(", ")
+                  : typeof detail === "object" && detail !== null
+                  ? JSON.stringify(detail)
+                  : "Failed to generate segment values from import data.";
+
               messageApi.open({
                 type: "error",
-                content: detail,
+                content: errorMessage,
                 duration: 5,
               });
             });