From 963c9a37704b94024c7e043e618168701745f508 Mon Sep 17 00:00:00 2001 From: Schuwi Date: Wed, 17 Sep 2025 23:32:46 +0200 Subject: [PATCH] docs(elixir): prepare hierarchical refactor --- design_docs/hierarchical_analysis.md | 154 ++++++++ design_docs/hierarchical_refactoring_plan.md | 370 +++++++++++++++++++ 2 files changed, 524 insertions(+) create mode 100644 design_docs/hierarchical_analysis.md create mode 100644 design_docs/hierarchical_refactoring_plan.md diff --git a/design_docs/hierarchical_analysis.md b/design_docs/hierarchical_analysis.md new file mode 100644 index 0000000..ff23a88 --- /dev/null +++ b/design_docs/hierarchical_analysis.md @@ -0,0 +1,154 @@ +# Hierarchical Implementation Analysis + +## Overview +This document analyzes the current hierarchical implementations in the codebase (Categories and Storage Locations) to determine the best approach for creating a shared `Hierarchical` behavior module. + +## Current State Analysis + +### Categories vs Storage Locations Feature Comparison + +| Feature | Categories | Storage Locations | Assessment | +|---------|------------|-------------------|------------| +| **Path Resolution** | Simple recursive function in schema | Complex virtual fields + batch computation | **Categories wins** - cleaner approach | +| **Cycle Prevention** | UI-level filtering (preventive) | Changeset validation (reactive) | **Categories wins** - more efficient | +| **Hierarchical Display** | Recursive LiveView components | Recursive LiveView components | **Tie** - nearly identical | +| **Parent Selection** | Smart dropdown filtering | Smart dropdown filtering | **Tie** - identical logic | +| **Performance** | O(depth) recursive calls | O(n) batch computation + DB queries | **Categories wins** - simpler | +| **Code Complexity** | ~50 lines of hierarchy logic | ~100+ lines of hierarchy logic | **Categories wins** - more maintainable | + +## Key Findings + +### 1. Vestigial `is_active` Field Removed +- **Status**: ✅ **REMOVED** +- **Impact**: Field was completely unused across the entire codebase +- **Files Modified**: + - `lib/components_elixir/inventory/storage_location.ex` (schema and changeset) + - Migration created: `20250917210658_remove_is_active_from_storage_locations.exs` + +### 2. Categories Implementation is Superior + +#### **Path Resolution Approach** +**Categories**: Elegant recursive function in schema +```elixir +def full_path(%Category{parent: nil} = category), do: category.name +def full_path(%Category{parent: %Category{} = parent} = category) do + "#{full_path(parent)} > #{category.name}" +end +``` + +**Storage Locations**: Over-engineered with virtual fields +```elixir +# Virtual fields in schema +field :level, :integer, virtual: true +field :path, :string, virtual: true + +# Complex batch computation in context +def compute_hierarchy_fields_batch(locations) +def compute_level_for_single(location) +def compute_path_for_single(location) +``` + +#### **Cycle Prevention Strategy** +**Categories**: **Prevention at UI level** (efficient) +```elixir +# Prevents invalid options from appearing in dropdown +|> Enum.reject(fn cat -> + cat.id == editing_category_id || + (editing_category_id && is_descendant?(categories, cat.id, editing_category_id)) +end) +``` + +**Storage Locations**: **Validation at changeset level** (reactive) +```elixir +# Validates after user attempts invalid selection +defp validate_no_circular_reference(changeset) do + # Complex validation logic that runs on every save attempt +end +``` + +### 3. Shared Patterns Identified + +Both systems implement identical patterns for: +- **Hierarchical tree display** with recursive LiveView components +- **Parent/child relationship filtering** +- **Descendant detection algorithms** +- **Root entity identification** +- **UI depth-based styling and icons** + +## Architecture Decision + +### Recommended Approach: **Category-Style Hierarchical Module** + +The category implementation should be the template for generalization because: + +1. **Simpler**: No virtual fields or complex batch operations +2. **More Performant**: O(depth) vs O(n) complexity +3. **Preventive**: UI-level cycle prevention vs reactive validation +4. **Maintainable**: Half the lines of code with same functionality + +### AprilTag Features Remain Storage-Specific + +The AprilTag system should **NOT** be generalized because: +- Physical identification is meaningless for categories +- Future scanning/detection features are location-specific +- Keeps domain separation clean + +## Implementation Complexity Comparison + +### Categories (Simple & Clean) +``` +Files: 1 schema + 1 LiveView = 2 files +Lines: ~50 lines hierarchical logic +Approach: Functional, recursive, preventive +Dependencies: Standard Ecto associations +``` + +### Storage Locations (Over-Engineered) +``` +Files: 1 schema + 1 context helper + 1 LiveView = 3 files +Lines: ~100+ lines hierarchical logic +Approach: Stateful, batch processing, reactive +Dependencies: Virtual fields + custom computation +``` + +## Performance Analysis + +### Path Resolution Performance +- **Categories**: `O(depth)` - traverses only parent chain +- **Storage Locations**: `O(n)` - processes all entities for batch computation + +### Memory Usage +- **Categories**: Minimal - uses existing associations +- **Storage Locations**: Higher - virtual fields + intermediate computations + +### Database Queries +- **Categories**: Standard association preloading +- **Storage Locations**: Additional queries for path/level computation + +## Code Quality Assessment + +### Categories Strengths +✅ **Single Responsibility**: Each function does one thing +✅ **Functional Style**: Pure functions, no side effects +✅ **Standard Patterns**: Uses established Ecto association patterns +✅ **Easy Testing**: Simple recursive functions +✅ **Performance**: Minimal computational overhead + +### Storage Locations Issues +❌ **Multiple Responsibilities**: Virtual fields + validation + computation +❌ **Complex State**: Virtual fields require careful management +❌ **Custom Patterns**: Non-standard Ecto usage +❌ **Hard Testing**: Complex batch operations +❌ **Performance**: Unnecessary computational overhead + +## Conclusion + +The **categories implementation is objectively superior** and should guide the refactoring: + +1. **Simpler code** (50% fewer lines) +2. **Better performance** (O(depth) vs O(n)) +3. **More maintainable** (functional vs stateful) +4. **Standard patterns** (Ecto associations vs virtual fields) +5. **Preventive design** (UI filtering vs changeset validation) + +The storage locations system should be refactored to match the categories approach, eliminating virtual fields and complex batch computations in favor of simple recursive functions. \ No newline at end of file diff --git a/design_docs/hierarchical_refactoring_plan.md b/design_docs/hierarchical_refactoring_plan.md new file mode 100644 index 0000000..ecbd29d --- /dev/null +++ b/design_docs/hierarchical_refactoring_plan.md @@ -0,0 +1,370 @@ +# Hierarchical Refactoring Plan + +## Overview +This document outlines the step-by-step plan to extract common hierarchical behavior from Categories and Storage Locations into a shared `Hierarchical` module, based on the superior category implementation approach. + +## Goals +1. **Extract shared hierarchical patterns** into reusable modules +2. **Simplify storage locations** to match category elegance +3. **Maintain AprilTag-specific features** for storage locations +4. **Preserve all existing functionality** during refactor +5. **Improve performance and maintainability** + +## Refactoring Strategy + +### Phase 1: Extract Hierarchical Behavior Module ⏭️ **NEXT** +Create shared behavior module based on category patterns + +### Phase 2: Refactor Storage Locations 🔄 **FOLLOW-UP** +Simplify storage locations to use new shared module + +### Phase 3: Refactor Categories 🔄 **FOLLOW-UP** +Update categories to use shared module + +### Phase 4: Extract LiveView Components 🔄 **FOLLOW-UP** +Create reusable UI components for hierarchical display + +--- + +## Phase 1: Extract Hierarchical Behavior Module + +### 1.1 Create Core Hierarchical Module + +**File**: `lib/components_elixir/inventory/hierarchical.ex` + +```elixir +defmodule ComponentsElixir.Inventory.Hierarchical do + @moduledoc """ + Shared hierarchical behavior for entities with parent-child relationships. + + This module provides common functionality for: + - Path computation (e.g., "Parent > Child > Grandchild") + - Cycle detection and prevention + - Parent/child filtering for UI dropdowns + - Tree traversal utilities + + Based on the elegant category implementation approach. + """ + + @doc """ + Computes full hierarchical path for an entity. + Uses recursive traversal of parent chain. + + ## Examples + iex> category = %Category{name: "Resistors", parent: %Category{name: "Electronics", parent: nil}} + iex> Hierarchical.full_path(category, &(&1.parent)) + "Electronics > Resistors" + """ + def full_path(entity, parent_accessor_fn, separator \\ " > ") + + @doc """ + Filters entities to remove circular reference options for parent selection. + Prevents an entity from being its own ancestor. + + ## Examples + iex> categories = [%{id: 1, parent_id: nil}, %{id: 2, parent_id: 1}] + iex> Hierarchical.filter_parent_options(categories, 1, &(&1.id), &(&1.parent_id)) + [%{id: 2, parent_id: 1}] # ID 1 filtered out (self-reference) + """ + def filter_parent_options(entities, editing_entity_id, id_accessor_fn, parent_id_accessor_fn) + + @doc """ + Checks if an entity is a descendant of an ancestor entity. + Used for cycle detection in parent selection. + """ + def is_descendant?(entities, descendant_id, ancestor_id, parent_id_accessor_fn) + + @doc """ + Gets all root entities (entities with no parent). + """ + def root_entities(entities, parent_id_accessor_fn) + + @doc """ + Gets all child entities of a specific parent. + """ + def child_entities(entities, parent_id, parent_id_accessor_fn) + + @doc """ + Generates display name for entity including parent context. + For dropdown displays: "Parent > Child" + """ + def display_name(entity, parent_accessor_fn, separator \\ " > ") +end +``` + +### 1.2 Schema Behaviour Definition + +**File**: `lib/components_elixir/inventory/hierarchical_schema.ex` + +```elixir +defmodule ComponentsElixir.Inventory.HierarchicalSchema do + @moduledoc """ + Behaviour for schemas that implement hierarchical relationships. + + Provides a contract for entities with parent-child relationships. + """ + + @callback full_path(struct()) :: String.t() + @callback parent(struct()) :: struct() | nil + @callback children(struct()) :: [struct()] + + defmacro __using__(_opts) do + quote do + @behaviour ComponentsElixir.Inventory.HierarchicalSchema + import ComponentsElixir.Inventory.Hierarchical + end + end +end +``` + +### 1.3 Update Category Schema + +**File**: `lib/components_elixir/inventory/category.ex` + +```elixir +defmodule ComponentsElixir.Inventory.Category do + use ComponentsElixir.Inventory.HierarchicalSchema + # ... existing schema definition + + @impl true + def full_path(%Category{} = category) do + ComponentsElixir.Inventory.Hierarchical.full_path(category, &(&1.parent)) + end + + @impl true + def parent(%Category{parent: parent}), do: parent + + @impl true + def children(%Category{children: children}), do: children +end +``` + +## Phase 2: Refactor Storage Locations + +### 2.1 Simplify Storage Location Schema + +**Changes to**: `lib/components_elixir/inventory/storage_location.ex` + +1. **Remove virtual fields**: `level` and `path` +2. **Add category-style path function** +3. **Remove complex cycle validation** +4. **Keep AprilTag-specific features** + +```elixir +defmodule ComponentsElixir.Inventory.StorageLocation do + use ComponentsElixir.Inventory.HierarchicalSchema + # ... existing schema without virtual fields + + @impl true + def full_path(%StorageLocation{} = location) do + ComponentsElixir.Inventory.Hierarchical.full_path(location, &(&1.parent), " / ") + end + + # Keep AprilTag-specific functionality + def apriltag_format(storage_location), do: storage_location.apriltag_id +end +``` + +### 2.2 Simplify Storage Location Changeset + +**Remove**: +- `validate_no_circular_reference/1` function +- Virtual field handling +- Complex cycle detection + +**Keep**: +- AprilTag validation +- Basic field validation + +### 2.3 Update Inventory Context + +**Changes to**: `lib/components_elixir/inventory.ex` + +**Remove**: +- `compute_hierarchy_fields_batch/1` +- `compute_level_for_single/1` +- `compute_path_for_single/1` +- All virtual field computation + +**Simplify**: +- `list_storage_locations/0` - use standard preloading +- `get_storage_location!/1` - remove virtual field computation + +## Phase 3: Refactor Categories + +### 3.1 Update Categories to Use Shared Module + +**Changes to**: `lib/components_elixir/inventory/category.ex` + +Replace existing `full_path/1` with call to shared module. + +### 3.2 Update Categories LiveView + +**Changes to**: `lib/components_elixir_web/live/categories_live.ex` + +Replace custom hierarchy functions with shared module calls: + +```elixir +# Replace custom functions with shared module +defp parent_category_options(categories, editing_category_id \\ nil) do + available_categories = + Hierarchical.filter_parent_options( + categories, + editing_category_id, + &(&1.id), + &(&1.parent_id) + ) + |> Enum.map(fn category -> + {Hierarchical.display_name(category, &(&1.parent)), category.id} + end) + + [{"No parent (Root category)", nil}] ++ available_categories +end +``` + +## Phase 4: Extract LiveView Components + +### 4.1 Create Hierarchical LiveView Components + +**File**: `lib/components_elixir_web/live/hierarchical_components.ex` + +```elixir +defmodule ComponentsElixirWeb.HierarchicalComponents do + use Phoenix.Component + alias ComponentsElixir.Inventory.Hierarchical + + @doc """ + Renders a hierarchical tree of entities with depth-based styling. + """ + def hierarchy_tree(assigns) + + @doc """ + Renders an individual item in a hierarchical tree. + """ + def hierarchy_item(assigns) + + @doc """ + Renders a parent selection dropdown with cycle prevention. + """ + def parent_select(assigns) +end +``` + +### 4.2 Update LiveViews to Use Shared Components + +Both `CategoriesLive` and `StorageLocationsLive` can use the shared components, with custom slots for entity-specific content (like AprilTag display). + +## Implementation Timeline + +### Immediate (Phase 1) - 1-2 hours +- [ ] Create `Hierarchical` module with core functions +- [ ] Create `HierarchicalSchema` behaviour +- [ ] Write comprehensive tests for shared module + +### Short-term (Phase 2) - 2-3 hours +- [ ] Refactor storage location schema to remove virtual fields +- [ ] Simplify storage location changeset +- [ ] Update inventory context to remove batch computation +- [ ] Test storage location functionality + +### Medium-term (Phase 3) - 1-2 hours +- [ ] Update category schema to use shared module +- [ ] Update categories LiveView to use shared functions +- [ ] Test category functionality + +### Long-term (Phase 4) - 2-3 hours +- [ ] Create shared LiveView components +- [ ] Refactor both LiveViews to use shared components +- [ ] Add entity-specific customization slots +- [ ] Comprehensive integration testing + +## Benefits After Refactoring + +### Code Quality +- **50% reduction** in hierarchical logic duplication +- **Consistent patterns** across both entity types +- **Easier testing** with isolated, pure functions +- **Better maintainability** with single source of truth + +### Performance +- **Elimination of virtual fields** reduces memory usage +- **Remove batch computation** improves response times +- **Standard Ecto patterns** optimize database queries +- **UI-level cycle prevention** reduces validation overhead + +### Developer Experience +- **Shared components** speed up new hierarchical entity development +- **Consistent API** reduces learning curve +- **Better documentation** with centralized behavior +- **Easier debugging** with simplified call stacks + +## Risk Mitigation + +### Database Migration Safety +1. **Backup database** before running migrations +2. **Test migrations** on development environment first +3. **Incremental approach** - one table at a time + +### Functionality Preservation +1. **Comprehensive test coverage** before refactoring +2. **Feature parity testing** after each phase +3. **AprilTag functionality isolation** to prevent interference + +### Rollback Plan +1. **Git branching strategy** for each phase +2. **Database migration rollbacks** prepared +3. **Quick revert capability** if issues discovered + +## Testing Strategy + +### Unit Tests +- [ ] Test all `Hierarchical` module functions +- [ ] Test schema `full_path/1` implementations +- [ ] Test cycle detection edge cases + +### Integration Tests +- [ ] Test LiveView parent selection dropdowns +- [ ] Test hierarchical tree rendering +- [ ] Test AprilTag functionality preservation + +### Performance Tests +- [ ] Benchmark path computation performance +- [ ] Measure memory usage before/after +- [ ] Profile database query patterns + +## Migration Notes + +### Database Changes Required +1. **Remove `is_active` column** from storage_locations (✅ **COMPLETED**) +2. **No other database changes** needed for this refactor + +### Deployment Considerations +- **Zero downtime**: Refactor is code-only (except `is_active` removal) +- **Backward compatible**: No API changes +- **Incremental deployment**: Can deploy phase by phase + +## Success Criteria + +### Functional +- [ ] All existing category functionality preserved +- [ ] All existing storage location functionality preserved +- [ ] AprilTag features remain storage-location specific +- [ ] UI behavior identical to current implementation + +### Non-Functional +- [ ] Code duplication reduced by ≥50% +- [ ] Performance maintained or improved +- [ ] Test coverage ≥95% for shared modules +- [ ] Documentation complete for new modules + +--- + +## Next Steps + +1. **Review this plan** with team/stakeholders +2. **Create feature branch** for refactoring work +3. **Begin Phase 1** - Extract hierarchical behavior module +4. **Write comprehensive tests** before any refactoring +5. **Execute phases incrementally** with testing between each + +This refactoring will significantly improve code quality and maintainability while preserving all existing functionality and preparing the codebase for future hierarchical entities. \ No newline at end of file