johnsturgeon/tgfp-web

Watch

Fork 0

Fix TypeError in weekly_planner scheduled job #325

Merged

Ghost merged 1 commit from refs/pull/325/head into main

2026-01-14 22:46:43 +01:00

Ghost commented

2026-01-14 18:27:05 +01:00

(Migrated from codeberg.org)

The schedule_jobs function requires a week_info parameter, but APScheduler calls scheduled jobs without arguments. Created a wrapper function schedule_jobs_current_week() that fetches current week info when called, ensuring the job always has up-to-date week information.

Ghost commented

2026-01-14 18:28:23 +01:00

(Migrated from codeberg.org)

PR Review: Fix TypeError in weekly_planner scheduled job

Summary

This PR successfully fixes a critical bug where the weekly_planner scheduled job was failing with a TypeError because schedule_jobs() requires a week_info parameter but APScheduler calls jobs without arguments. The solution introduces a wrapper function schedule_jobs_current_week() that fetches current week info when invoked.

✅ Positives

Clean Solution: The wrapper function approach is elegant and minimal, avoiding changes to the existing schedule_jobs() function signature.
Correct Fix: Properly addresses the root cause - APScheduler cannot pass arguments to scheduled jobs, so the wrapper fetches the required data at runtime.
Version Bump: Appropriately increments the PATCH version (2.4.0 → 2.4.1) following semantic versioning.
Documentation Updates: Includes minor typo fixes in README.md ("web site" → "website").

🔍 Code Quality Observations

scheduler.py:160-164

Issue: Circular Import Pattern

The wrapper function uses a local import to avoid circular dependencies:

def schedule_jobs_current_week():
    """Wrapper function that can be scheduled without arguments"""
    from models.model_helpers import current_week_info  # ⚠️ Local import
    week_info = current_week_info()
    schedule_jobs(week_info=week_info)

Concern: The file already imports WeekInfo from models.model_helpers at line 11, but not the current_week_info function. This suggests either:

There was/could be a circular import issue
The import structure could be cleaner

Recommendation: Consider adding current_week_info to the top-level imports if no circular dependency exists:

from models.model_helpers import WeekInfo, current_week_info

If there IS a circular dependency, this is acceptable but deserves a comment explaining why.

🐛 Potential Issues

1. No Error Handling

The wrapper function doesn't handle potential failures from current_week_info():

def schedule_jobs_current_week():
    """Wrapper function that can be scheduled without arguments"""
    from models.model_helpers import current_week_info
    week_info = current_week_info()  # ⚠️ What if this fails?
    schedule_jobs(week_info=week_info)

Risk: If current_week_info() raises an exception (ESPN API down, network issues, etc.), the scheduled job will crash and potentially stop rescheduling.

Recommendation: Add try/except with logging:

def schedule_jobs_current_week():
    """Wrapper function that can be scheduled without arguments"""
    from models.model_helpers import current_week_info
    import logging
    
    try:
        week_info = current_week_info()
        schedule_jobs(week_info=week_info)
    except Exception as e:
        logging.error(f"Failed to schedule jobs for current week: {e}")
        # Optionally: send alert, use cached week info, etc.

2. Inconsistent Usage Pattern

In app/main.py, the startup code calls schedule_jobs() directly with current_week_info():

# main.py:69
schedule_jobs(week_info=current_week_info())

But the scheduled job uses the wrapper. This creates two different code paths that could diverge. Consider using the wrapper in both places for consistency:

# main.py:69 (suggested)
from jobs.scheduler import schedule_jobs_current_week
schedule_jobs_current_week()

🧪 Test Coverage

Missing: No test coverage for the new wrapper function or the fix itself.

Recommendations:

Add a unit test verifying schedule_jobs_current_week() correctly fetches week info and calls schedule_jobs()
Add integration test ensuring the scheduled job executes without errors
Mock current_week_info() to test error handling (if implemented)

🔒 Security Concerns

None identified. This is an internal scheduling fix with no user input or external data exposure.

⚡ Performance Considerations

Minor overhead: The wrapper adds one extra function call and re-fetches current_week_info() each time the job runs (Wednesday 7am PT). This is negligible since:

Job runs once weekly
current_week_info() is a lightweight ESPN API call
No noticeable performance impact

📝 Documentation

Good: The docstring clearly explains the wrapper's purpose.

Suggestion: Add a comment in main.py explaining why the wrapper is needed:

# Use wrapper function since APScheduler cannot pass arguments to scheduled jobs
job_scheduler.add_job(
    "app.jobs.scheduler:schedule_jobs_current_week",
    trigger=trigger,
    id="weekly_planner",
)

🎯 Final Verdict

APPROVE with minor suggestions

This PR successfully fixes the reported bug with a clean, minimal solution. The suggestions above are enhancements for robustness and consistency but are not blockers.

Recommended Follow-ups (can be separate PRs):

Add error handling to schedule_jobs_current_week()
Add test coverage
Consider using the wrapper consistently in startup code
Document the circular import if that's why it's local

Great work fixing this! 🚀

Generated with Claude Code

## PR Review: Fix TypeError in weekly_planner scheduled job ### Summary This PR successfully fixes a critical bug where the `weekly_planner` scheduled job was failing with a `TypeError` because `schedule_jobs()` requires a `week_info` parameter but APScheduler calls jobs without arguments. The solution introduces a wrapper function `schedule_jobs_current_week()` that fetches current week info when invoked. --- ### ✅ Positives 1. **Clean Solution**: The wrapper function approach is elegant and minimal, avoiding changes to the existing `schedule_jobs()` function signature. 2. **Correct Fix**: Properly addresses the root cause - APScheduler cannot pass arguments to scheduled jobs, so the wrapper fetches the required data at runtime. 3. **Version Bump**: Appropriately increments the PATCH version (2.4.0 → 2.4.1) following semantic versioning. 4. **Documentation Updates**: Includes minor typo fixes in README.md ("web site" → "website"). --- ### 🔍 Code Quality Observations #### scheduler.py:160-164 **Issue: Circular Import Pattern** The wrapper function uses a local import to avoid circular dependencies: ```python def schedule_jobs_current_week(): """Wrapper function that can be scheduled without arguments""" from models.model_helpers import current_week_info # ⚠️ Local import week_info = current_week_info() schedule_jobs(week_info=week_info) ``` **Concern**: The file already imports `WeekInfo` from `models.model_helpers` at line 11, but not the `current_week_info` function. This suggests either: - There was/could be a circular import issue - The import structure could be cleaner **Recommendation**: Consider adding `current_week_info` to the top-level imports if no circular dependency exists: ```python from models.model_helpers import WeekInfo, current_week_info ``` If there IS a circular dependency, this is acceptable but deserves a comment explaining why. --- ### 🐛 Potential Issues #### 1. **No Error Handling** The wrapper function doesn't handle potential failures from `current_week_info()`: ```python def schedule_jobs_current_week(): """Wrapper function that can be scheduled without arguments""" from models.model_helpers import current_week_info week_info = current_week_info() # ⚠️ What if this fails? schedule_jobs(week_info=week_info) ``` **Risk**: If `current_week_info()` raises an exception (ESPN API down, network issues, etc.), the scheduled job will crash and potentially stop rescheduling. **Recommendation**: Add try/except with logging: ```python def schedule_jobs_current_week(): """Wrapper function that can be scheduled without arguments""" from models.model_helpers import current_week_info import logging try: week_info = current_week_info() schedule_jobs(week_info=week_info) except Exception as e: logging.error(f"Failed to schedule jobs for current week: {e}") # Optionally: send alert, use cached week info, etc. ``` #### 2. **Inconsistent Usage Pattern** In `app/main.py`, the startup code calls `schedule_jobs()` directly with `current_week_info()`: ```python # main.py:69 schedule_jobs(week_info=current_week_info()) ``` But the scheduled job uses the wrapper. This creates two different code paths that could diverge. Consider using the wrapper in both places for consistency: ```python # main.py:69 (suggested) from jobs.scheduler import schedule_jobs_current_week schedule_jobs_current_week() ``` --- ### 🧪 Test Coverage **Missing**: No test coverage for the new wrapper function or the fix itself. **Recommendations**: 1. Add a unit test verifying `schedule_jobs_current_week()` correctly fetches week info and calls `schedule_jobs()` 2. Add integration test ensuring the scheduled job executes without errors 3. Mock `current_week_info()` to test error handling (if implemented) --- ### 🔒 Security Concerns None identified. This is an internal scheduling fix with no user input or external data exposure. --- ### ⚡ Performance Considerations **Minor overhead**: The wrapper adds one extra function call and re-fetches `current_week_info()` each time the job runs (Wednesday 7am PT). This is negligible since: - Job runs once weekly - `current_week_info()` is a lightweight ESPN API call - No noticeable performance impact --- ### 📝 Documentation **Good**: The docstring clearly explains the wrapper's purpose. **Suggestion**: Add a comment in `main.py` explaining why the wrapper is needed: ```python # Use wrapper function since APScheduler cannot pass arguments to scheduled jobs job_scheduler.add_job( "app.jobs.scheduler:schedule_jobs_current_week", trigger=trigger, id="weekly_planner", ) ``` --- ### 🎯 Final Verdict **APPROVE with minor suggestions** This PR successfully fixes the reported bug with a clean, minimal solution. The suggestions above are enhancements for robustness and consistency but are not blockers. **Recommended Follow-ups** (can be separate PRs): 1. Add error handling to `schedule_jobs_current_week()` 2. Add test coverage 3. Consider using the wrapper consistently in startup code 4. Document the circular import if that's why it's local Great work fixing this! 🚀 --- **Generated with Claude Code**