Add auth module refactoring implementation plan
- Create detailed step-by-step implementation plan - 9 tasks covering all refactoring aspects - TDD approach with explicit verification steps - Include test updates and documentation changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Misaka_Company
572
docs/plans/2026-03-27-auth-refactoring-implementation.md
Normal file
572
docs/plans/2026-03-27-auth-refactoring-implementation.md
Normal file
@@ -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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
**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)
|
||||
Reference in New Issue
Block a user