diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md index 4889c68..91de57d 100644 --- a/.specify/memory/constitution.md +++ b/.specify/memory/constitution.md @@ -106,6 +106,30 @@ Documentation MUST live alongside code, be version-controlled, and follow the sa 5. **Implement** (`/speckit.implement`): Execute tasks following test-first discipline 6. **Analyze** (`/speckit.analyze`): Verify cross-artifact consistency +### Bug Fix Protocol + +Every bug fix MUST follow this protocol: + +1. **Write a Failing Test**: Create a test that reproduces the bug and currently fails +2. **Verify Failure**: Run the test to confirm it fails with the buggy code +3. **Fix the Bug**: Implement the minimal fix to address the root cause +4. **Verify Success**: Run the test to confirm it now passes +5. **Document**: Add entry to `docs/TECHNICAL_DEBT.md` documenting: + - Bug description and symptoms + - Root cause analysis + - Test coverage added + - Lessons learned +6. **Commit Together**: Test and fix MUST be committed in the same commit or immediately sequential commits + +**Rationale**: Bug fixes without tests are incomplete. Tests serve as regression prevention and documentation of expected behavior. If a bug was found manually, it means our test coverage has a gap that must be filled. + +**Exceptions**: The only acceptable reason to skip adding a test is if: +- The bug is in test infrastructure itself +- The bug requires external dependencies unavailable in CI (document in TECHNICAL_DEBT.md) +- The bug is in a deprecated component being removed + +In all exception cases, document the rationale in the commit message and TECHNICAL_DEBT.md. + ### Branching & Integration - Feature branches named `###-feature-name` where ### is numeric identifier @@ -149,4 +173,8 @@ All feature specifications, plans, and implementations MUST be reviewed for cons This constitution supersedes all other development guidelines, practices, or conventions. When conflicts arise, this document governs. If this constitution is unclear or incomplete for a specific situation, propose an amendment rather than work around it. -**Version**: 1.0.0 | **Ratified**: 2025-10-14 | **Last Amended**: 2025-10-14 +**Version**: 1.1.0 | **Ratified**: 2025-10-14 | **Last Amended**: 2025-10-16 + +**Amendment History**: +- **1.1.0** (2025-10-16): Added Bug Fix Protocol requiring tests for all bug fixes and documentation in TECHNICAL_DEBT.md +- **1.0.0** (2025-10-14): Initial constitution ratified diff --git a/docs/TECHNICAL_DEBT.md b/docs/TECHNICAL_DEBT.md new file mode 100644 index 0000000..732e601 --- /dev/null +++ b/docs/TECHNICAL_DEBT.md @@ -0,0 +1,275 @@ +# Technical Debt + +This document tracks known technical debt in the Reklamator project. + +## Definition + +Technical debt refers to: +- Missing test coverage +- Known limitations or workarounds +- Deferred improvements +- Areas needing refactoring + +--- + +## Current Technical Debt + +### 1. Missing Authentication Route Tests + +**Severity**: Medium +**Phase Introduced**: Phase 3 (MVP) +**Status**: Open + +**Description**: +Authentication routes (login/logout) lack comprehensive test coverage. The routes work but were not covered by contract tests during initial implementation. + +**Missing Tests**: +- Contract tests for `/auth/login` (GET) +- Contract tests for `/auth/login` (POST) with valid credentials +- Contract tests for `/auth/login` (POST) with invalid credentials +- Contract tests for `/auth/logout` +- Integration tests for complete login/logout flow +- Tests for session management +- Tests for authenticated vs unauthenticated access + +**Impact**: +- Authentication bugs may go undetected until manual testing +- Risk of regression when modifying auth code + +**Mitigation**: +- User model has comprehensive unit tests (15 tests) added after MVP +- Manual testing verified login/logout functionality +- CSRF protection tested manually (disabled in test config by design) + +**Plan to Resolve**: +- Add authentication contract tests in Phase 5 when implementing dashboard (which requires authentication) +- Or address as standalone task before Phase 4 + +--- + +### 2. CSRF Testing Disabled in Test Environment + +**Severity**: Low +**Phase Introduced**: Phase 2 (Foundational) +**Status**: Accepted (By Design) + +**Description**: +CSRF protection is disabled in test configuration (`config/testing.py:17` - `WTF_CSRF_ENABLED = False`). This is a common testing practice but means CSRF bugs only appear in development/production. + +**Impact**: +- CSRF-related bugs require manual testing to catch +- Forms without CSRF tokens will pass tests but fail in dev/prod + +**Bugs Found**: +- Bug #1: Missing CSRF token in submission form (found manually) +- Bug #2: Missing CSRF token in login form (found manually) + +**Mitigation**: +- Both forms now include CSRF tokens +- Manual testing checklist includes form submission +- CSRF protection verified working in development environment + +**Plan to Resolve**: +- Consider adding integration tests with CSRF enabled +- Or document as accepted trade-off for simpler testing + +--- + +### 3. Dashboard Routes Not Implemented + +**Severity**: Low (Expected) +**Phase Introduced**: Phase 3 (MVP) +**Status**: Planned + +**Description**: +Login/logout routes reference dashboard endpoints that don't exist yet: +- `admin.dashboard` (Phase 6 - User Story 4) +- `dashboard.list` (Phase 5 - User Story 3) + +**Current Workaround**: +- All users redirect to index page after login +- Base template shows "Coming in Phase X" messages +- TODO comments in code mark areas for future implementation + +**Impact**: +- Users cannot access dashboards after login (expected for MVP) +- Navigation shows placeholder text instead of functional links + +**Plan to Resolve**: +- Implement in Phase 5 (Product Owner Dashboard) +- Implement in Phase 6 (Admin Dashboard) + +--- + +### 4. ClamAV Integration Not Fully Tested + +**Severity**: Low +**Phase Introduced**: Phase 3 (MVP) +**Status**: Open + +**Description**: +ClamAV virus scanning has graceful degradation but limited test coverage. Tests run with ClamAV unavailable (skips scanning). + +**Missing Tests**: +- Tests with actual ClamAV daemon running +- Tests for virus detection +- Tests for ClamAV connection failures +- Tests for scanning timeout + +**Impact**: +- ClamAV integration relies on manual testing +- Virus scanning behavior not verified in automated tests + +**Mitigation**: +- Code includes comprehensive error handling +- Logs warnings when ClamAV unavailable +- Falls back gracefully (allows upload, logs warning) + +**Plan to Resolve**: +- Add mock ClamAV tests using `unittest.mock` +- Or add optional integration tests requiring ClamAV installation +- Document ClamAV setup in deployment guide + +--- + +## Bug Fixes Without Tests + +All bugs found during manual testing should have regression tests added. Track them here: + +### Bug #1: Missing CSRF Token in Forms + +**Date Found**: 2025-10-16 +**Severity**: High +**Found By**: Manual testing +**Fixed In**: Commit b8d0d6d + +**Description**: +Submission and login forms were missing CSRF token fields, causing "Bad Request - The CSRF token is missing" errors. + +**Root Cause**: +- Forms created without `{{ csrf_token() }}` hidden input +- CSRF disabled in test config meant tests didn't catch it + +**Test Coverage**: +- ❌ No test added (CSRF disabled in test config by design) +- ✅ Manual testing verified fix + +**Lesson Learned**: +- Always test forms in development environment +- Consider manual testing checklist for CSRF-protected forms + +--- + +### Bug #2: User Model is_active AttributeError + +**Date Found**: 2025-10-16 +**Severity**: High +**Found By**: Manual testing (login attempt) +**Fixed In**: Commit 73a9a74 + +**Description**: +`AttributeError: can't set attribute 'is_active'` when loading users. Flask-Login's `UserMixin` provides `is_active` as read-only property, conflicting with instance attribute assignment. + +**Root Cause**: +- Direct attribute assignment conflicted with Flask-Login property +- No unit tests for User model during Phase 2/3 + +**Test Coverage**: +- ✅ Added 15 comprehensive unit tests in `tests/unit/test_user_model.py` +- ✅ Specifically tests `is_active` property (test_user_is_active_property) +- ✅ Tests Flask-Login integration (test_user_flask_login_properties) + +**Lesson Learned**: +- Test-First Discipline should apply to ALL models, not just user-facing features +- Flask-Login integration needs explicit testing + +--- + +### Bug #3: BuildError for Non-Existent Dashboard Routes + +**Date Found**: 2025-10-16 +**Severity**: Medium +**Found By**: Manual testing (successful login) +**Fixed In**: Commit d5fd7a7 + +**Description**: +`werkzeug.routing.exceptions.BuildError: Could not build url for endpoint 'admin.dashboard'` after successful login. Auth routes tried to redirect to unimplemented dashboard routes. + +**Root Cause**: +- Forward references to routes not yet implemented (Phase 5/6) +- No integration tests for login flow + +**Test Coverage**: +- ❌ No test added (dashboards not implemented yet) +- ✅ Manual testing verified fix +- 📝 TODO comments added for future implementation + +**Lesson Learned**: +- Avoid forward references to unimplemented routes +- Or use defensive checks (e.g., `url_for()` with try/except) +- Integration tests should verify redirect destinations + +--- + +## Resolution Priorities + +1. **High Priority**: Add authentication route tests (Phase 5) +2. **Medium Priority**: Add ClamAV mock tests +3. **Low Priority**: Consider CSRF-enabled integration tests +4. **Ongoing**: Add regression test for each bug fix + +--- + +## Test Coverage Goals + +### Current Coverage (Phase 3 - MVP) + +- **Contract Tests**: 8 tests (submission routes) +- **Integration Tests**: 2 tests (feedback submission) +- **Unit Tests**: 15 tests (User model) +- **Total**: 25 tests + +**Coverage by Component**: +- ✅ Submission routes: Excellent (8 contract + 2 integration tests) +- ✅ User model: Excellent (15 unit tests) +- ✅ Feedback model: Good (tested via integration tests) +- ✅ Product model: Good (tested via integration tests) +- ⚠️ Authentication routes: Poor (0 tests) +- ⚠️ File validation: Partial (tested via submission tests) +- ❌ Admin routes: None (not implemented) +- ❌ Dashboard routes: None (not implemented) + +### Target Coverage (End of MVP+) + +- All user-facing routes: Contract tests +- All models: Unit tests +- All services: Unit tests +- Critical flows: Integration tests +- **Minimum**: 80% code coverage + +--- + +## How to Add Tests for Bug Fixes + +When fixing a bug: + +1. **Write a failing test** that reproduces the bug +2. **Verify the test fails** with the buggy code +3. **Fix the bug** +4. **Verify the test passes** with the fixed code +5. **Document** the bug and test in this file +6. **Commit** test and fix together + +See: `.specify/memory/constitution.md` - Bug Fix Protocol + +--- + +## Review Schedule + +This document should be reviewed: +- After each phase completion +- When adding new features +- When fixing bugs +- Monthly during active development + +Last Updated: 2025-10-16 (Phase 3 - MVP Complete) diff --git a/tests/unit/test_user_model.py b/tests/unit/test_user_model.py new file mode 100644 index 0000000..b4b246f --- /dev/null +++ b/tests/unit/test_user_model.py @@ -0,0 +1,347 @@ +"""Unit tests for User model""" +import pytest +from app.models.user import User + + +@pytest.mark.unit +def test_user_creation(app): + """Test User model instantiation""" + with app.app_context(): + user = User( + user_id='usr_test', + username='testuser', + email='test@example.com', + password_hash='hash123', + role='administrator', + product_ids=['prod_001'], + is_active=True + ) + + assert user.user_id == 'usr_test' + assert user.username == 'testuser' + assert user.email == 'test@example.com' + assert user.password_hash == 'hash123' + assert user.role == 'administrator' + assert user.product_ids == ['prod_001'] + assert user.is_active == True + + +@pytest.mark.unit +def test_user_is_active_property(app): + """Test is_active property (Flask-Login integration) + + Bug: AttributeError: can't set attribute 'is_active' + Fix: Use private _is_active attribute with property decorator + """ + with app.app_context(): + # Test active user + active_user = User( + user_id='usr_001', + username='active', + email='active@example.com', + password_hash='hash', + role='administrator', + is_active=True + ) + assert active_user.is_active == True + + # Test inactive user + inactive_user = User( + user_id='usr_002', + username='inactive', + email='inactive@example.com', + password_hash='hash', + role='administrator', + is_active=False + ) + assert inactive_user.is_active == False + + +@pytest.mark.unit +def test_user_flask_login_properties(app): + """Test Flask-Login required properties""" + with app.app_context(): + user = User( + user_id='usr_001', + username='test', + email='test@example.com', + password_hash='hash', + role='administrator' + ) + + # Flask-Login required properties + assert user.is_authenticated == True + assert user.is_anonymous == False + assert user.is_active == True + assert user.get_id() == 'usr_001' + + +@pytest.mark.unit +def test_user_password_hashing(app): + """Test password hashing with bcrypt""" + with app.app_context(): + password = 'test_password_123' + hashed = User.hash_password(password) + + # Hash should be different from plain password + assert hashed != password + + # Hash should be bcrypt format + assert hashed.startswith('$2b$') + + # Same password should produce different hashes (salt) + hashed2 = User.hash_password(password) + assert hashed != hashed2 + + +@pytest.mark.unit +def test_user_password_verification(app): + """Test password verification""" + with app.app_context(): + password = 'correct_password' + wrong_password = 'wrong_password' + + user = User( + user_id='usr_001', + username='test', + email='test@example.com', + password_hash=User.hash_password(password), + role='administrator' + ) + + # Correct password should verify + assert user.check_password(password) == True + + # Wrong password should not verify + assert user.check_password(wrong_password) == False + + +@pytest.mark.unit +def test_user_to_dict(app): + """Test user serialization to dictionary""" + with app.app_context(): + user = User( + user_id='usr_001', + username='testuser', + email='test@example.com', + password_hash='hash123', + role='product_owner', + product_ids=['prod_001', 'prod_002'], + is_active=True + ) + + user_dict = user.to_dict() + + assert user_dict['user_id'] == 'usr_001' + assert user_dict['username'] == 'testuser' + assert user_dict['email'] == 'test@example.com' + assert user_dict['password_hash'] == 'hash123' + assert user_dict['role'] == 'product_owner' + assert user_dict['product_ids'] == ['prod_001', 'prod_002'] + assert user_dict['is_active'] == True + + +@pytest.mark.unit +def test_user_from_dict(app): + """Test user deserialization from dictionary""" + with app.app_context(): + user_data = { + 'user_id': 'usr_001', + 'username': 'testuser', + 'email': 'test@example.com', + 'password_hash': 'hash123', + 'role': 'administrator', + 'product_ids': ['prod_001'], + 'is_active': True + } + + user = User.from_dict(user_data) + + assert user.user_id == 'usr_001' + assert user.username == 'testuser' + assert user.email == 'test@example.com' + assert user.password_hash == 'hash123' + assert user.role == 'administrator' + assert user.product_ids == ['prod_001'] + assert user.is_active == True + + +@pytest.mark.unit +def test_user_from_dict_with_defaults(app): + """Test user deserialization with missing optional fields""" + with app.app_context(): + user_data = { + 'user_id': 'usr_001', + 'username': 'testuser', + 'email': 'test@example.com', + 'password_hash': 'hash123', + 'role': 'administrator' + # Missing product_ids and is_active + } + + user = User.from_dict(user_data) + + assert user.product_ids == [] # Default empty list + assert user.is_active == True # Default True + + +@pytest.mark.unit +def test_user_create(app): + """Test user creation with auto-generated ID""" + with app.app_context(): + user = User.create( + username='newuser', + email='new@example.com', + password='password123', + role='product_owner', + product_ids=['prod_001'] + ) + + # User should be created + assert user.user_id.startswith('usr_') + assert user.username == 'newuser' + assert user.email == 'new@example.com' + assert user.role == 'product_owner' + assert user.product_ids == ['prod_001'] + assert user.is_active == True + + # Password should be hashed + assert user.password_hash != 'password123' + assert user.check_password('password123') == True + + # Cleanup + user.delete() + + +@pytest.mark.unit +def test_user_create_duplicate_username(app): + """Test user creation with duplicate username raises error""" + with app.app_context(): + # Create first user + user1 = User.create( + username='duplicate', + email='user1@example.com', + password='password123', + role='administrator' + ) + + # Try to create second user with same username + with pytest.raises(ValueError, match="Username already exists"): + User.create( + username='duplicate', + email='user2@example.com', + password='password456', + role='administrator' + ) + + # Cleanup + user1.delete() + + +@pytest.mark.unit +def test_user_create_invalid_role(app): + """Test user creation with invalid role raises error""" + with app.app_context(): + with pytest.raises(ValueError, match="Invalid role"): + User.create( + username='testuser', + email='test@example.com', + password='password123', + role='invalid_role' + ) + + +@pytest.mark.unit +def test_user_save_and_load(app): + """Test user persistence (save and load)""" + with app.app_context(): + # Create and save user + user = User.create( + username='persistent', + email='persist@example.com', + password='password123', + role='administrator' + ) + user_id = user.user_id + + # Load user from storage + loaded_user = User.get_by_id(user_id) + + assert loaded_user is not None + assert loaded_user.user_id == user_id + assert loaded_user.username == 'persistent' + assert loaded_user.email == 'persist@example.com' + assert loaded_user.role == 'administrator' + assert loaded_user.check_password('password123') == True + + # Cleanup + user.delete() + + +@pytest.mark.unit +def test_user_get_by_username(app): + """Test loading user by username""" + with app.app_context(): + # Create user + user = User.create( + username='findme', + email='findme@example.com', + password='password123', + role='product_owner' + ) + + # Find by username + found_user = User.get_by_username('findme') + + assert found_user is not None + assert found_user.username == 'findme' + assert found_user.email == 'findme@example.com' + + # Non-existent username + not_found = User.get_by_username('doesnotexist') + assert not_found is None + + # Cleanup + user.delete() + + +@pytest.mark.unit +def test_user_get_all(app): + """Test getting all users""" + with app.app_context(): + # Create multiple users + user1 = User.create('user1', 'user1@example.com', 'pass1', 'administrator') + user2 = User.create('user2', 'user2@example.com', 'pass2', 'product_owner') + + # Get all users + all_users = User.get_all() + + # Should include at least our test users + usernames = [u.username for u in all_users] + assert 'user1' in usernames + assert 'user2' in usernames + + # Cleanup + user1.delete() + user2.delete() + + +@pytest.mark.unit +def test_user_delete(app): + """Test user deletion""" + with app.app_context(): + # Create user + user = User.create( + username='deleteme', + email='delete@example.com', + password='password123', + role='administrator' + ) + user_id = user.user_id + + # Delete user + user.delete() + + # User should no longer exist + deleted_user = User.get_by_id(user_id) + assert deleted_user is None