From cd851b5170592b8884860b07b3d8bb7fb104756d Mon Sep 17 00:00:00 2001 From: Misaka_Company Date: Fri, 27 Mar 2026 12:22:20 +0800 Subject: [PATCH] Add auth module refactoring design document - Document decoupling approach for utils/auth.py - Remove all environment variable dependencies - Make login() a pure function with required parameters - Remove close_session() function - Provide migration guide for test scripts Co-Authored-By: Claude Sonnet 4.6 --- .../2026-03-27-auth-refactoring-design.md | 299 ++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 docs/plans/2026-03-27-auth-refactoring-design.md diff --git a/docs/plans/2026-03-27-auth-refactoring-design.md b/docs/plans/2026-03-27-auth-refactoring-design.md new file mode 100644 index 0000000..258cc99 --- /dev/null +++ b/docs/plans/2026-03-27-auth-refactoring-design.md @@ -0,0 +1,299 @@ +# Auth Module Refactoring Design + +**Date:** 2026-03-27 +**Status:** Approved +**Author:** Claude Code + +## Overview + +Refactor the `utils/auth.py` module to follow decoupling principles by removing all environment variable dependencies and making it a pure, stateless authentication component. + +## Problem Statement + +### Current Issues + +The `utils/auth.py` module has tight coupling to environment configuration: + +1. **Module-level side effects:** Loads environment variables on import (lines 10-22) +2. **Implicit defaults:** `login()` function reads from environment variables when parameters are `None` +3. **Hidden behavior:** URL construction and browser path configuration embedded in the module +4. **Mixed responsibilities:** Authentication logic mixed with configuration management + +### Design Principles Violated + +- **Separation of Concerns:** Configuration and business logic are mixed +- **Dependency Inversion:** Module depends on concrete environment implementation +- **Single Responsibility:** Module handles both auth and configuration loading + +## Proposed Solution + +### Architecture + +Transform `utils/auth.py` into a **pure, stateless authentication module** with: + +- Zero environment variable dependencies +- All parameters required (no defaults from environment) +- No module-level initialization or side effects +- Explicit parameter passing only + +### Module Structure + +```python +# utils/auth.py - Pure Authentication Module + +# Removed: +# - Environment variable loading (load_dotenv) +# - BASE_DIR and path calculation +# - os.getenv() calls +# - close_session() function + +# Functions: +# login(playwright, username, password, url, headless, ignore_https_errors, verbose) +# logout(main_frame, verbose) +``` + +### API Changes + +#### `login()` Function + +**Before:** +```python +def login( + playwright: Playwright, + username: str = None, # Defaults to os.getenv("ERP_USERNAME") + password: str = None, # Defaults to os.getenv("ERP_PASSWORD") + url: str = None, # Defaults to os.getenv("ERP_URL") + path + headless: bool = None, # Defaults to os.getenv("ERP_HEADLESS") + ignore_https_errors: bool = None, # Defaults to os.getenv("ERP_IGNORE_HTTPS_ERRORS") + verbose: bool = True, +) -> tuple[Browser, BrowserContext, Page, Frame]: +``` + +**After:** +```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]: + """ + 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) + """ +``` + +**Changes:** +- All parameters become required (no defaults) +- URL parameter expects complete URL (no automatic path appending) +- Removed lines 59-76 (environment variable reading) +- Removed lines 62-63 (URL construction logic) + +#### `logout()` Function + +No changes - already a pure function. + +#### `close_session()` Function + +**REMOVED ENTIRELY** + +Callers now manage browser lifecycle directly: +```python +# Caller code +context.close() +browser.close() +``` + +### Data Flow + +**Before:** +``` +Test Script → login() → [reads .env internally] → Browser + ↑ + (implicit config) +``` + +**After:** +``` +Test Script → [loads .env] → [constructs URL] → login() → Browser + ↓ ↓ + (explicit config) (explicit params) +``` + +### Implementation Details + +#### Imports to Remove + +```python +# Remove these imports: +import os # Only used for os.getenv() +from dotenv import load_dotenv # No longer needed +from pathlib import Path # Only used for BASE_DIR calculation +``` + +#### Code to Remove + +Lines 10-22: Environment setup +```python +# DELETE THESE LINES: +# Load environment variables +from dotenv import load_dotenv + +# Get project root directory +BASE_DIR = Path(__file__).resolve().parent.parent + +# Load .env file +env_path = BASE_DIR / ".env" +load_dotenv(env_path) + +# Configure Playwright browser path +browsers_path = os.getenv("PLAYWRIGHT_BROWSERS_PATH") +if browsers_path: + os.environ["PLAYWRIGHT_BROWSERS_PATH"] = browsers_path +``` + +Lines 59-76: Environment variable defaults in `login()` +```python +# DELETE THESE LINES: +# URL handling: read from environment variable or parameter +if not url: + url = os.getenv("ERP_URL") + if url and not url.endswith("login/main/index.html"): + url = url.rstrip("/") + "/yonbip/resources/uap/rbac/login/main/index.html" + +if not url: + raise ValueError("URL must be provided either as parameter or through ERP_URL environment variable") + +# headless parameter handling +if headless is None: + headless_str = os.getenv("ERP_HEADLESS", "false").lower() + headless = headless_str in ("true", "1", "yes") + +# ignore_https_errors parameter handling +if ignore_https_errors is None: + ignore_https_errors_str = os.getenv("ERP_IGNORE_HTTPS_ERRORS", "true").lower() + ignore_https_errors = ignore_https_errors_str in ("true", "1", "yes") +``` + +Lines 161-177: `close_session()` function +```python +# DELETE THIS ENTIRE FUNCTION +def close_session(browser: Browser, context: BrowserContext) -> None: + ... +``` + +#### Code to Modify + +`login()` signature - remove defaults: +```python +# CHANGE FROM: +def login(playwright, username=None, password=None, url=None, headless=None, ignore_https_errors=None, verbose=True) + +# CHANGE TO: +def login(playwright, username, password, url, headless, ignore_https_errors, verbose=True) +``` + +Add parameter validation: +```python +if not username: + raise ValueError("username is required") +if not password: + raise ValueError("password is required") +if not url: + raise ValueError("url is required") +``` + +## Testing Impact + +### Test Script Changes + +All test scripts must be updated to: + +1. **Load environment variables explicitly** (already done) +2. **Construct complete URLs** before calling `login()` +3. **Pass all parameters explicitly** to `login()` +4. **Replace `close_session()` calls** with direct `context.close()` and `browser.close()` + +### Example Migration + +**Before (test_login.py):** +```python +with sync_playwright() as p: + browser, context, page, main_frame = login( + playwright=p, + verbose=True + ) + # ... use browser ... + close_session(browser, context) +``` + +**After:** +```python +# Load and prepare config +url = f"{os.getenv('ERP_URL').rstrip('/')}/yonbip/resources/uap/rbac/login/main/index.html" + +with sync_playwright() as p: + 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 + ) + # ... use browser ... + context.close() + browser.close() +``` + +### Test Files to Update + +- `tests/test_login.py` - Update login() calls and close_session() usage +- `tests/test_auth_config.py` - No changes (doesn't call login()) + +## Benefits + +1. **Decoupling:** Auth module independent of configuration source +2. **Testability:** Easier to test with mock data +3. **Clarity:** Explicit dependencies make data flow obvious +4. **Flexibility:** Can be used with any configuration source (env, config file, CLI args, etc.) +5. **Purity:** Functions have no hidden side effects + +## Migration Path + +1. Update `utils/auth.py` with all changes +2. Update `tests/test_login.py` to use new API +3. Run tests to verify functionality +4. Update documentation (CLAUDE.md) + +## Risks and Mitigations + +| Risk | Mitigation | +|------|-----------| +| Breaking existing scripts | Clear migration guide; tests updated first | +| Parameter verbosity | Tests already have env loading code | +| URL construction duplication | Document pattern in CLAUDE.md | + +## Acceptance Criteria + +- [ ] `utils/auth.py` has zero `os.getenv()` calls +- [ ] `utils/auth.py` has no `load_dotenv()` calls +- [ ] `login()` requires all parameters (no None defaults) +- [ ] `close_session()` function removed +- [ ] All tests pass with new API +- [ ] CLAUDE.md documentation updated