Skip to content

Conversation

@helbashandy
Copy link
Collaborator

PR Description

Description

This PR refactors the project-specific join request tracker into a generic, reusable tracking framework while maintaining complete backward compatibility. The motivation is to enable tracking functionality across different areas of ColdFront (allocation renewals, hardware procurements, secure directory requests, etc.) without duplicating tracking logic.

Key Changes:

Generic Tracking Framework (coldfront/core/utils/tracking/):

  • BaseTracker - Abstract base class for all tracking implementations
  • ModelBasedTracker - Extended base for Django model-based tracking
  • TrackingStep and TrackingResult - Standardized data structures
  • Helper functions for common tracking patterns

Refactored Implementation:

  • Updated JoinRequestTracker to inherit from ModelBasedTracker
  • Maintains identical public API (backward compatible)
  • All existing functionality preserved with better organization

Comprehensive Test Suite:

  • 65 test methods across unit, whitebox, and blackbox categories
  • Django-pytest best practices with factories, parameterized tests, and proper fixtures
  • Established testing protocol and directory structure for future use

Documentation and Tooling:

  • Complete implementation guide with step-by-step instructions
  • Test runner script and configuration files
  • Examples for creating new trackers

No issues fixed - this is a proactive refactoring to improve code reusability and maintainability.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Comprehensive Test Coverage
  • Created seed data to test on the UI

Test Commands to use - I had an issue with my local env (python version incompatibility that prevented me from installing all the dependencies. @matthew-li - Would you be able to run it on your end? If not, lmk so that I downgrade my version

 pytest --cov=coldfront coldfront/tests/

PR Self Evaluation

  • My code follows the agreed upon best practices
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if needed)
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in the appropriate modules
  • I have performed a self-review of my own code

Additional Notes:

  • Zero breaking changes - all existing code continues to work unchanged
  • Framework designed for extensibility without over-engineering
  • Test suite establishes best practices for future ColdFront testing efforts

@helbashandy helbashandy requested a review from matthew-li July 28, 2025 20:01
Copy link
Collaborator

@matthew-li matthew-li left a comment

Choose a reason for hiding this comment

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

Thanks! Took an initial glance and did some light testing in the UI, which looked good.

I made a few changes in a different branch (see one of the comments) to get pytest running within Docker, at least.

We should discuss/align on the testing approach + integrate with what I'd set up previously.

pytest.ini Outdated
@@ -0,0 +1,25 @@
[tool:pytest]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the tests I'd introduced (for hardware procurements), I configured pytest in pyproject.toml. Can these options be incorporated there, or does it make sense to configure it in a standalone file?

(Also, I don't think this file exists: DJANGO_SETTINGS_MODULE = coldfront.config.settings.test.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've branched off of this in issue_668_progress_tracker_tests_protocol_tmp to address this so that I could get pytest to run (the new tests are error/failing).

Feel free to merge that here if it makes sense, and add any pytest config to pyproject.toml.

@@ -0,0 +1,23 @@
{% extends "common/base.html" %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks to be a stale file from a previous PR.

# Test Markers
# ============================================================================

def pytest_configure(config):
Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of the contents of this file are duplicated in tests/conftest.py.

@matthew-li
Copy link
Collaborator

@helbashandy Summarizing our conversation yesterday:

  • Let's go with your pytest markers: whitebox, blackbox, etc. Let's integrate those into the existing pyproject.toml, and then update the existing pytests to use the new markers.
  • We'll stick with the top-level tests directory.
    • It could mirror the structure of the application code (e.g., tests.core.project..., tests.plugins.hardware_procurements...).
    • Common utilities (e.g., shared conftest.py files) could go in a directory like common or resources.
    • Blackbox / UI tests might not go in an app-specific directory, since they could span multiple apps. Consider a workflows directory.

@helbashandy helbashandy force-pushed the issue_668_progress_tracker_tests_protocol branch 2 times, most recently from c8699fd to fc292f8 Compare September 16, 2025 18:18
- Create reusable tracking framework under coldfront/core/utils/tracking/
- Refactor JoinRequestTracker to inherit from new ModelBasedTracker base class
- Maintain complete backward compatibility with existing API and templates
- Add comprehensive test suite with 65 test methods covering unit, whitebox, and blackbox scenarios
- Establish testing protocol with proper directory structure and configuration
- Enable tracking functionality for future use cases beyond project join requests

The generic framework provides BaseTracker and ModelBasedTracker abstract classes
that can be extended for various request/process tracking needs while ensuring
consistent behavior, error handling, and progress visualization across the application.
@helbashandy helbashandy force-pushed the issue_668_progress_tracker_tests_protocol branch from fc292f8 to 473ea6e Compare September 16, 2025 18:49
@helbashandy helbashandy changed the base branch from develop to upgrade/rocky8-python3.13-develop September 16, 2025 18:53
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.

3 participants