diff --git a/docs/plans/2026-03-27-auth-refactoring-implementation.md b/docs/plans/2026-03-27-auth-refactoring-implementation.md new file mode 100644 index 0000000..60797f2 --- /dev/null +++ b/docs/plans/2026-03-27-auth-refactoring-implementation.md @@ -0,0 +1,572 @@ +# Auth Module Refactoring Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Refactor `utils/auth.py` into a pure, decoupled authentication module with zero environment variable dependencies. + +**Architecture:** Remove all environment variable reading from `auth.py`, make all parameters required, remove the `close_session()` wrapper function. Test scripts will explicitly load configuration and pass all parameters. + +**Tech Stack:** Python 3.x, Playwright, pytest + +--- + +## Task 1: Update utils/auth.py imports + +**Files:** +- Modify: `utils/auth.py:1-23` + +**Step 1: Remove unused imports** + +Remove the following imports that are only used for environment variable access: +- `import os` (line 5) +- `from dotenv import load_dotenv` (line 10) +- `from pathlib import Path` (line 6) + +The final imports should be: +```python +""" +Authentication module - Responsible for Yonyou BIP system login and logout operations +""" + +from playwright.sync_api import Playwright, Browser, BrowserContext, Page, Frame +``` + +**Step 2: Remove module-level environment setup** + +Delete lines 10-22 (BASE_DIR, load_dotenv, browsers_path configuration). + +**Step 3: Verify syntax** + +Run: `python -m py_compile utils/auth.py` +Expected: No syntax errors (function definitions will fail later steps, that's OK) + +**Step 4: Commit** + +```bash +git add utils/auth.py +git commit -m "refactor: remove environment variable imports from auth module + +- Remove os, dotenv, and pathlib imports +- Remove module-level environment setup code +- Prepare for pure function implementation + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 2: Refactor login() function signature + +**Files:** +- Modify: `utils/auth.py:25-52` + +**Step 1: Update function signature to remove default parameters** + +Change the `login()` function signature from: +```python +def login( + playwright: Playwright, + username: str = None, + password: str = None, + url: str = None, + headless: bool = None, + ignore_https_errors: bool = None, + verbose: bool = True, +) -> tuple[Browser, BrowserContext, Page, Frame]: +``` + +To: +```python +def login( + playwright: Playwright, + username: str, + password: str, + url: str, + headless: bool, + ignore_https_errors: bool, + verbose: bool = True, +) -> tuple[Browser, BrowserContext, Page, Frame]: +``` + +**Step 2: Update docstring** + +Replace the docstring (lines 34-52) with: +```python + """ + Login to Yonyou BIP system + + Args: + playwright: Playwright instance + username: Username (required) + password: Password (required) + url: Complete login page URL (required) + headless: Whether to use headless mode (required) + ignore_https_errors: Whether to ignore HTTPS errors (required) + verbose: Whether to print detailed logs (default: True) + + Returns: + tuple: (browser, context, page, main_frame) + - browser: Browser instance + - context: Browser context + - page: Page object + - main_frame: Main iframe after login (forwardFrame) + """ +``` + +**Step 3: Verify syntax** + +Run: `python -m py_compile utils/auth.py` +Expected: No syntax errors + +**Step 4: Commit** + +```bash +git add utils/auth.py +git commit -m "refactor: update login() signature to require all parameters + +- Remove default values for username, password, url, headless, ignore_https_errors +- Update docstring to reflect required parameters +- Maintain backward compatibility with 4-tuple return value + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 3: Remove environment variable logic from login() body + +**Files:** +- Modify: `utils/auth.py:53-77` + +**Step 1: Remove environment variable reading code** + +Delete lines 56-76 that contain: +- `username = username or os.getenv("ERP_USERNAME")` +- `password = password or os.getenv("ERP_PASSWORD")` +- URL construction logic with `os.getenv("ERP_URL")` +- headless default handling with `os.getenv("ERP_HEADLESS")` +- ignore_https_errors default handling with `os.getenv("ERP_IGNORE_HTTPS_ERRORS")` + +**Step 2: Add parameter validation** + +After the imports section (after the docstring), add validation: +```python + # Validate required parameters + if not username: + raise ValueError("username is required") + if not password: + raise ValueError("password is required") + if not url: + raise ValueError("url is required") +``` + +**Step 3: Verify the login() function body** + +The function body should now start directly with: +```python + import time + + # Validate required parameters + if not username: + raise ValueError("username is required") + if not password: + raise ValueError("password is required") + if not url: + raise ValueError("url is required") + + # Launch browser + browser = playwright.chromium.launch(headless=headless) + # ... rest of function unchanged +``` + +**Step 4: Verify syntax** + +Run: `python -m py_compile utils/auth.py` +Expected: No syntax errors + +**Step 5: Commit** + +```bash +git add utils/auth.py +git commit -m "refactor: remove environment variable reading from login() + +- Remove all os.getenv() calls from login() function +- Remove URL construction logic (caller provides complete URL) +- Add parameter validation with clear error messages +- Function is now pure with no side effects + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 4: Remove close_session() function + +**Files:** +- Modify: `utils/auth.py:161-177` + +**Step 1: Delete the close_session() function** + +Remove lines 161-177, the entire `close_session()` function: +```python +def close_session(browser: Browser, context: BrowserContext) -> None: + """ + Close browser session + + Args: + browser: Browser instance + context: Browser context + """ + # Check if auto-close browser is enabled + auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes") + + if auto_close: + context.close() + browser.close() + else: + print("Note: Browser not auto-closed (ERP_AUTO_CLOSE_BROWSER=false)") +``` + +**Step 2: Verify auth.py still has only two functions** + +Run: `grep "^def " utils/auth.py` +Expected output: +``` +def login( +def logout( +``` + +**Step 3: Verify syntax** + +Run: `python -m py_compile utils/auth.py` +Expected: No syntax errors + +**Step 4: Commit** + +```bash +git add utils/auth.py +git commit -m "refactor: remove close_session() function + +- Remove close_session() wrapper function entirely +- Callers now directly manage browser/context lifecycle +- Reduces module to pure authentication functions only + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 5: Update tests/test_login.py to use new API + +**Files:** +- Modify: `tests/test_login.py:13,37-39,62-69` + +**Step 1: Update import line** + +The import on line 13 stays the same: +```python +from utils.auth import login, logout, close_session +``` + +Change to: +```python +from utils.auth import login, logout +``` + +**Step 2: Add URL construction before login() call** + +After line 31 (after displaying configuration), add URL construction: +```python +# Construct complete login URL +url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html" +``` + +**Step 3: Update login() call with all required parameters** + +Replace lines 37-40: +```python + browser, context, page, main_frame = login( + playwright=p, + verbose=True + ) +``` + +With: +```python + browser, context, page, main_frame = login( + playwright=p, + username=os.getenv('ERP_USERNAME'), + password=os.getenv('ERP_PASSWORD'), + url=url, + headless=os.getenv('ERP_HEADLESS', 'false').lower() in ('true', '1', 'yes'), + ignore_https_errors=os.getenv('ERP_IGNORE_HTTPS_ERRORS', 'true').lower() in ('true', '1', 'yes'), + verbose=True + ) +``` + +**Step 4: Replace close_session() with direct calls** + +Replace lines 62-69: +```python + # Check if auto-close browser + auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes") + if not auto_close: + print("\n[INFO] Browser will stay open for 10 seconds for manual observation...") + time.sleep(10) + + # Close browser + print("\n[CLEANUP] Closing browser session...") + close_session(browser, context) +``` + +With: +```python + # Check if auto-close browser + auto_close = os.getenv("ERP_AUTO_CLOSE_BROWSER", "true").lower() in ("true", "1", "yes") + if not auto_close: + print("\n[INFO] Browser will stay open for 10 seconds for manual observation...") + time.sleep(10) + + # Close browser + print("\n[CLEANUP] Closing browser session...") + context.close() + browser.close() +``` + +**Step 5: Verify syntax** + +Run: `python -m py_compile tests/test_login.py` +Expected: No syntax errors + +**Step 6: Commit** + +```bash +git add tests/test_login.py +git commit -m "refactor: update test_login.py to use new auth API + +- Import only login and logout (remove close_session) +- Add explicit URL construction before login call +- Pass all required parameters to login() +- Replace close_session() with direct context.close() and browser.close() + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 6: Run tests to verify functionality + +**Files:** +- Test: `tests/test_login.py`, `utils/auth.py` + +**Step 1: Activate virtual environment** + +Run: `source .venv/Scripts/activate` +Expected: Command prompt shows `(.venv)` + +**Step 2: Run login test** + +Run: `python tests/test_login.py` +Expected: Test completes successfully with login/logout operations + +**Step 3: Verify no regressions** + +Check that the test output shows: +- Configuration display +- Successful login +- Successful logout +- Browser closes cleanly + +**Step 4: Run auth config test** + +Run: `python tests/test_auth_config.py` +Expected: Configuration check completes (no changes needed in this file) + +**Step 5: Commit if any fixes needed** + +If tests revealed issues: +```bash +git add tests/test_login.py utils/auth.py +git commit -m "fix: address test failures in refactored auth module + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +If all tests pass, no commit needed for this step. + +--- + +## Task 7: Update CLAUDE.md documentation + +**Files:** +- Modify: `CLAUDE.md:24-35,41-46` + +**Step 1: Update Environment Configuration section** + +Replace lines 24-35 with: +```markdown +## Environment Configuration + +Configuration is loaded from `.env` file in the project root. Never commit `.env` to version control - use `.env.example` as a template. + +**Environment Variables:** +- `PLAYWRIGHT_BROWSERS_PATH` - Path to Playwright browser installation +- `ERP_URL` - Base URL for the ERP system +- `ERP_USERNAME` - Login username +- `ERP_PASSWORD` - Login password +- `ERP_HEADLESS` - Whether to run browser in headless mode (true/false) +- `ERP_IGNORE_HTTPS_ERRORS` - Whether to ignore HTTPS certificate errors (true/false) + +Note: Test scripts are responsible for loading environment variables and passing configuration to utility functions. +``` + +**Step 2: Update Module Structure section** + +Replace lines 41-46 with: +```markdown +**`utils/auth.py`** - Core authentication module (pure functions) +- `login()` - Handles Yonyou BIP login with automatic force-login popup detection. Requires all parameters (username, password, url, headless, ignore_https_errors). +- `logout()` - Performs logout with confirmation dialog handling. +- Returns tuple: `(browser, context, page, main_frame)` where `main_frame` is the forwardFrame iframe. +- Callers are responsible for browser lifecycle management (context.close(), browser.close()). +``` + +**Step 3: Add URL Construction Pattern** + +After line 52 (after Page Interaction Pattern section), add: +```markdown +### URL Construction Pattern + +Callers must construct the complete login URL before passing to `login()`: + +```python +url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html" +``` +``` + +**Step 4: Update Code Conventions section** + +Replace line 84 (Environment-First convention) with: +```markdown +2. **Environment-First**: Test scripts load environment variables and explicitly pass configuration to utility functions. No hardcoded values in code. +``` + +**Step 5: Verify documentation** + +Run: `grep -n "os.getenv" CLAUDE.md` +Expected: Only in code examples, not as recommendations for module internals + +**Step 6: Commit** + +```bash +git add CLAUDE.md +git commit -m "docs: update CLAUDE.md for refactored auth module + +- Clarify that auth module is pure with no environment access +- Document URL construction pattern for callers +- Update module structure documentation +- Emphasize caller responsibility for configuration + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 8: Create .env.example template + +**Files:** +- Create: `.env.example` + +**Step 1: Create environment variable template** + +Create `.env.example` with: +```bash +# Playwright Configuration +PLAYWRIGHT_BROWSERS_PATH=path/to/playwright/browsers + +# ERP System Configuration +ERP_URL=https://your-erp-system.com +ERP_USERNAME=your_username +ERP_PASSWORD=your_password + +# Browser Behavior +ERP_HEADLESS=false +ERP_IGNORE_HTTPS_ERRORS=true +``` + +**Step 2: Verify .env is in .gitignore** + +Run: `grep .env .gitignore` +Expected: `.env` is listed + +**Step 3: Commit** + +```bash +git add .env.example +git commit -m "docs: add .env.example template + +- Provide template for required environment variables +- Document all configuration options +- Help users set up their local environment + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +--- + +## Task 9: Final verification and cleanup + +**Files:** +- Verify: `utils/auth.py`, `tests/`, `CLAUDE.md` + +**Step 1: Verify no environment imports in auth.py** + +Run: `grep -E "(os\.getenv|load_dotenv)" utils/auth.py` +Expected: No matches + +**Step 2: Verify all parameters are required** + +Run: `grep "def login" utils/auth.py` +Expected: Function signature has no `= None` defaults except `verbose=True` + +**Step 3: Run all tests** + +Run: `python tests/test_login.py && python tests/test_auth_config.py` +Expected: All tests pass + +**Step 4: Check documentation consistency** + +Run: `grep -n "close_session" CLAUDE.md` +Expected: No mentions (function was removed) + +**Step 5: Final commit if needed** + +If any cleanup was done: +```bash +git add -A +git commit -m "chore: final cleanup for auth module refactoring + +- Verify no environment dependencies remain +- Confirm all tests pass +- Documentation is consistent + +Co-Authored-By: Claude Sonnet 4.6 " +``` + +**Step 6: Push to remote** + +Run: `git push origin master` +Expected: All commits pushed successfully + +--- + +## Summary + +This refactoring achieves: +1. **Pure Functions**: `auth.py` has zero side effects or environment dependencies +2. **Explicit Dependencies**: All parameters required, no hidden behavior +3. **Separation of Concerns**: Configuration managed by callers, auth module handles business logic only +4. **Backward Compatible**: 4-tuple return value maintained for existing code +5. **Tested**: All tests updated and passing + +**Migration Impact**: Low - only test scripts need updates, API changes are additive (making optional params required)