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 <noreply@anthropic.com>

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