Skip to content

refactor: split large files with backwards-compatible re-exports#133

Open
iloveagent57 wants to merge 1 commit intomainfrom
adusenbery/large-file-refactoring
Open

refactor: split large files with backwards-compatible re-exports#133
iloveagent57 wants to merge 1 commit intomainfrom
adusenbery/large-file-refactoring

Conversation

@iloveagent57
Copy link
Copy Markdown
Member

  • Split bffs/handlers.py (749 → 71 lines) by extracting learner portal handlers to bffs/learner_portal/handlers.py
  • Split subsidy_access_policy/models.py (2276 → 2090 lines) by extracting PolicyGroupAssociation and ForcedPolicyRedemption to models_supporting.py
  • Remove pylint skip-file directive from models.py and fix all pylint issues
  • Fix bug: action variable used before assignment in assignment_request_redeem()
  • Add missing docstrings to clean_spend_limit, SubsidyAccessPolicyRequestAssignmentMixin, and assignment_request_redeem
  • Document module splitting pattern in docs/architecture-patterns.md

All existing imports continue to work via re-exports for backwards compatibility.

🤖 Generated with Claude Code

Merge checklist:

  • ./manage.py makemigrations has been run
    • Note: This must be run if you modified any models.
      • It may or may not make a migration depending on exactly what you modified, but it should still be run.

Post merge:

  • Ensure that your changes went out to the stage instance
  • Deploy to prod instance

@iloveagent57 iloveagent57 requested review from a team as code owners April 10, 2026 15:58
Copilot AI review requested due to automatic review settings April 10, 2026 15:58

This comment was marked as outdated.

@iloveagent57 iloveagent57 force-pushed the adusenbery/large-file-refactoring branch from cbfb603 to fee1aff Compare April 10, 2026 16:28
Copilot AI review requested due to automatic review settings April 10, 2026 16:29
@iloveagent57 iloveagent57 force-pushed the adusenbery/large-file-refactoring branch from fee1aff to 256d88c Compare April 10, 2026 16:29
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

@iloveagent57 iloveagent57 force-pushed the adusenbery/large-file-refactoring branch from 256d88c to c5a74d5 Compare April 10, 2026 17:10
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 85.75581% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.29%. Comparing base (0739e1a) to head (2c20f5b).

Files with missing lines Patch % Lines
...rprise_access/apps/bffs/learner_portal/handlers.py 86.69% 24 Missing and 9 partials ⚠️
...ss/apps/subsidy_access_policy/models_supporting.py 86.11% 6 Missing and 4 partials ⚠️
...rprise_access/apps/subsidy_access_policy/models.py 33.33% 6 Missing ⚠️

❌ Your patch check has failed because the patch coverage (85.75%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   84.28%   84.29%   +0.01%     
==========================================
  Files         144      147       +3     
  Lines       12209    12230      +21     
  Branches     1162     1163       +1     
==========================================
+ Hits        10290    10309      +19     
- Misses       1598     1600       +2     
  Partials      321      321              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Split bffs/handlers.py (749 → 71 lines) by extracting learner portal
  handlers to bffs/learner_portal/handlers.py
- Split subsidy_access_policy/models.py (2276 → 2090 lines) by extracting
  PolicyGroupAssociation and ForcedPolicyRedemption to models_supporting.py
- Remove pylint skip-file directive from models.py and fix all pylint issues
- Fix bug: action variable used before assignment in assignment_request_redeem()
- Add missing docstrings to clean_spend_limit, SubsidyAccessPolicyRequestAssignmentMixin,
  and assignment_request_redeem
- Document module splitting pattern in docs/architecture-patterns.md

All existing imports continue to work via re-exports for backwards compatibility.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 10, 2026 18:44
@iloveagent57 iloveagent57 force-pushed the adusenbery/large-file-refactoring branch from c5a74d5 to 2c20f5b Compare April 10, 2026 18:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

"""
A base handler class for learner-focused routes.

The `BaseLearnerHandler` extends `BaseHandler` and provides shared core functionality
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class docstring refers to BaseLearnerHandler, but the class name is BaseLearnerPortalHandler. Update the docstring to reference the correct class name to avoid confusion when navigating the codebase.

Suggested change
The `BaseLearnerHandler` extends `BaseHandler` and provides shared core functionality
The `BaseLearnerPortalHandler` extends `BaseHandler` and provides shared core functionality

Copilot uses AI. Check for mistakes.
Comment on lines 793 to 798
if not self.bnr_enabled:
return {
"error_reason": REASON_BNR_NOT_ENABLED
}
return self.assignment_request_can_allocate(learner_credit_requests)
return self.assignment_request_can_allocate(learner_credit_requests) # pylint: disable=no-member

Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can_approve() is defined on SubsidyAccessPolicy but calls assignment_request_can_allocate(), which is only provided by SubsidyAccessPolicyRequestAssignmentMixin (used by PerLearnerSpendCreditAccessPolicy). Instead of suppressing pylint with # pylint: disable=no-member, consider moving can_approve() onto the mixin / relevant policy subclass, or add a concrete implementation on the base class that fails fast with a clear error when BNR is enabled on an unsupported policy type.

Copilot uses AI. Check for mistakes.
Comment on lines +137 to +141
return self.subsidy_access_policy.uuid

def __str__(self):
return (
f'<{self.__class__.__name__} policy_uuid={self.subsidy_access_policy.uuid}, '
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ForcedPolicyRedemption.subsidy_access_policy is nullable (on_delete=SET_NULL), but both policy_uuid and __str__ unconditionally dereference self.subsidy_access_policy.uuid, which will raise if the related policy is deleted. Guard for a null policy (or make the FK non-nullable if null should be impossible in practice).

Suggested change
return self.subsidy_access_policy.uuid
def __str__(self):
return (
f'<{self.__class__.__name__} policy_uuid={self.subsidy_access_policy.uuid}, '
if self.subsidy_access_policy is None:
return None
return self.subsidy_access_policy.uuid
def __str__(self):
return (
f'<{self.__class__.__name__} policy_uuid={self.policy_uuid}, '

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants