eric/time-tracker
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Move drag_info from global to class attribute for better encapsulation

Browse Source 
## Code Quality Improvements

### Global State Removal
- Eliminated global drag_info dictionary
- Moved drag_state management into TimeTracker class
- Removed all global drag_info dependencies

### Updated Components
- **ClickableCell constructor**: Added time_tracker parameter for proper reference
- **ClickableCell methods**: Updated to use self.time_tracker.drag_info
- **TimeTracker methods**: Updated on_global_drag() and on_global_up()
- **Instance creation**: Updated ClickableCell instantiation calls

### Benefits Achieved
- **Better Encapsulation**: State properly contained within class boundaries
- **Thread Safety**: Reduced race conditions from shared global state
- **Testability**: Individual instance testing now possible
- **Instance Isolation**: Multiple TimeTracker instances work independently
- **Maintainability**: Clearer code structure with explicit dependencies

### Verification
- ✅ All drag functionality preserved (paint/erase operations)
- ✅ Drag state management works correctly
- ✅ Multiple instances properly isolated
- ✅ All 6 existing test suites pass (no regressions)
- ✅ New comprehensive test suite created and passing
- ✅ Application starts and runs correctly

## Files Modified
- **time_tracker.py**: Global state removal and class attribute implementation
- **AGENTS.md**: Updated coding guidelines for class preferences
- **TODO.md**: Marked drag_info task as completed, updated progress
- **tests/test_drag_info_class_attribute.py**: New comprehensive test suite

## Testing
- Added complete test suite for drag_info functionality
- Tests verify global state removal and class attribute access
- Confirms multiple instance isolation
- Validates drag state management

Code quality significantly improved with zero functional regressions.
This commit is contained in:
Eric Taylor
2025-10-29 17:38:00 -04:00
parent a564d430f8
commit fbdf450c14
4 changed files with 254 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
TODO.md
View File

@@ -41,28 +41,31 @@ This document outlines the recommended improvements from the code review, organi
- **Implementation**: Created sanitization functions for CSV text, filenames, and config data
- **Security Features**: Excel formula blocking, directory traversal protection, JSON safety
- [🔧] **Critical security fixes from code review**
- [ ] **Fix settings file race condition** - Use atomic write pattern with temp file
- **Issue**: Direct file overwrite can corrupt settings if process crashes
- **Impact**: Loss of all application configuration (jobs, customers, paths)
- [ ] **Add CSV quoting protection** - Use proper csv.QUOTE_MINIMAL for safer CSV writing
- **Issue**: Current character removal isn't enough for complete CSV safety
- **Impact**: Potential CSV injection attacks could still succeed
- [ ] **Sanitize all CSV fields consistently** - Fix Date field and username field gaps
- **Issue**: Some fields (Date, username) not properly sanitized before CSV writing
- **Impact**: Data corruption and potential inject vulnerabilities remain
- [x] **Critical security fixes from code review** ✅
- [x] **Fix settings file race condition** - Use atomic write pattern with temp file ✅
- **Issue**: Direct file overwrite can corrupt settings if process crashes
- **Impact**: Loss of all application configuration (jobs, customers, paths)
- **Solution**: Atomic write with temp file + os.replace() + cleanup
- [x] **Add CSV quoting protection** - Use proper csv.QUOTE_MINIMAL for safer CSV writing ✅
- **Issue**: Current character removal isn't enough for complete CSV safety
- **Impact**: Potential CSV injection attacks could still succeed
- **Solution**: Applied csv.QUOTE_MINIMAL to both CSV DictWriter operations
- [x] **Sanitize all CSV fields consistently** - Fix Date field and username field gaps ✅
- **Issue**: Some fields (Date, username) not properly sanitized before CSV writing
- **Impact**: Data corruption and potential inject vulnerabilities remain
- **Solution**: Created sanitize_date_text() and applied sanitization to all fields
- [ ] **Replace filedialog usage** for PDF exports
- Use `filedialog.asksaveasfilename` instead
- Validate file extensions
- Add overwrite confirmation
- [ ] **Move drag_info from global to class attribute**
- [x] **Move drag_info from global to class attribute** ✅
- Remove global state dependency
- Improve encapsulation
- Make class more testable
- [ ] **Move drag_info from global to class attribute in TimeTracker**
- [x] **Move drag_info from global to class attribute in TimeTracker** ✅
🔧 Medium Priority Additions (from code review):
- [ ] **Add type conversion error handling** - Prevent ValueError on hours field
@@ -206,10 +209,10 @@ For the pinned columns feature specifically:
## 📊 Progress Tracking:
- **Stage 1**: 1/8 completed (1 base + 3 critical fixes pending)
- **Stage 1**: 6/8 completed (1 base + 5 critical fixes completed)
- **Stage 2**: 0/9 completed
- **Stage 3**: 1/2 completed
- **New Features**: 1/4 completed
- **Total**: 2/24 completed
- **Total**: 7/24 completed
*Priority Legend: 🚨 Critical | 🔧 Important | 🎯 Enhancement | 🔧 🔧 Code Review Findings*