Conversation
Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to make the project more “production-grade OSS” by adding contributor/community scaffolding, introducing CI via GitHub Actions, adding an initial pytest suite, and hardening/standardizing API behavior (rate limiting) alongside a small user-agent related bug fix.
Changes:
- Added project community/governance docs and GitHub templates (Contributing, Security, CoC, issue/PR templates).
- Added GitHub Actions CI (ruff lint + pytest matrix + Docker build) and introduced pytest/ruff dev tooling & configuration.
- Added DRF rate limiting configuration (env-configurable) and tests covering key utilities, models, web views, and API endpoints.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_web_views.py |
Adds view-level tests for dashboard, submit logs, and global_manage AJAX behavior. |
tests/test_core_utils.py |
Adds unit tests for core boolean/common utility helpers. |
tests/test_core_models.py |
Adds model tests for SubmitLog, FiltersModel, and GroupsModel. |
tests/test_api.py |
Adds API tests for health check and API root auth behavior. |
tests/conftest.py |
Provides global test settings overrides (LocMemCache + DRF settings) to avoid external services. |
tests/__init__.py |
Introduces the tests package marker file. |
sample.env |
Documents new env vars for API throttling defaults. |
pyproject.toml |
Adds dev dependencies (pytest/pytest-django/ruff) plus pytest + ruff tool configuration. |
main/core/utils/user_agent.py |
Refactors cache selection to be lazy per call (intended to respect settings overrides). |
main/core/middleware.py |
Prevents KeyError when HTTP_USER_AGENT is missing by using safe lookup. |
config/settings/com.py |
Enables DRF throttling with env-configurable rates. |
SECURITY.md |
Adds a security policy and deployment hardening checklist. |
README.md |
Documents new throttle env vars and updates contributing section to point to guides/templates. |
CONTRIBUTING.md |
Adds contributor onboarding, standards, testing instructions, and PR guidelines. |
CODE_OF_CONDUCT.md |
Adds Contributor Covenant 2.1 code of conduct. |
CHANGELOG.md |
Records a new release entry summarizing the OSS/CI/testing/security changes. |
.github/workflows/ci.yml |
Adds CI workflow (ruff, pytest matrix, docker build). |
.github/PULL_REQUEST_TEMPLATE.md |
Adds a PR template/checklist. |
.github/ISSUE_TEMPLATE/question.md |
Adds a support/question issue template. |
.github/ISSUE_TEMPLATE/feature_request.md |
Adds a feature request template. |
.github/ISSUE_TEMPLATE/bug_report.md |
Adds a bug report template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
main/core/utils/user_agent.py
Outdated
| USER_AGENTS_CACHE = getattr(settings, 'USER_AGENTS_CACHE', DEFAULT_CACHE_ALIAS) | ||
|
|
||
| if USER_AGENTS_CACHE: | ||
| cache = get_cache(USER_AGENTS_CACHE) | ||
| else: | ||
| cache = None | ||
|
|
||
| def _get_cache(): | ||
| """Return the cache backend lazily, respecting runtime settings overrides.""" | ||
| if USER_AGENTS_CACHE: | ||
| return caches[USER_AGENTS_CACHE] | ||
| return None |
There was a problem hiding this comment.
USER_AGENTS_CACHE is still read from settings at import time, so override_settings(USER_AGENTS_CACHE=...) or any runtime change won’t be picked up. This means the new lazy _get_cache() does not actually “respect runtime settings overrides” as the docstring/PR description claims. Consider resolving the cache alias inside _get_cache() (read from settings on each call) instead of using a module-level constant.
There was a problem hiding this comment.
Fixed in 26c1611. USER_AGENTS_CACHE is now resolved inside _get_cache() on every call via getattr(settings, 'USER_AGENTS_CACHE', DEFAULT_CACHE_ALIAS), so override_settings and any runtime settings changes are always picked up correctly.
tests/test_core_utils.py
Outdated
| """ | ||
| Tests for core utility functions. | ||
| """ | ||
| import pytest |
There was a problem hiding this comment.
pytest is imported but not used anywhere in this module. This will fail the ruff F401 check in CI; please remove the unused import.
| import pytest |
tests/test_core_models.py
Outdated
| assert log.decoded_destination_addr == '255700000001' | ||
|
|
||
| def test_decoded_destination_addr_bytes(self): | ||
| log = self._make_submit_log(destination_addr=b'255700000001') |
There was a problem hiding this comment.
This test likely fails under Django because destination_addr is a CharField: passing bytes to objects.create() will typically be coerced to a string like "b'255700000001'" before saving, so decoded_destination_addr won’t return "255700000001". If you want to exercise the bytes-handling branch, set the attribute on an unsaved instance (or use a DB representation your field actually returns, e.g. a "\\x..." hex string / memoryview).
| log = self._make_submit_log(destination_addr=b'255700000001') | |
| # Create a standard log entry, then set destination_addr to bytes on the instance | |
| log = self._make_submit_log() | |
| log.destination_addr = b'255700000001' |
There was a problem hiding this comment.
Fixed in 26c1611. The test now creates a standard DB instance, then assigns b'255700000001' directly to log.destination_addr on the unsaved-for-this-purpose instance, so the bytes-handling branch in decoded_destination_addr is actually exercised.
| FiltersModel.objects.create(type='TransparentFilter', fid='unique-fid', parameters='') | ||
| with pytest.raises(Exception): | ||
| FiltersModel.objects.create(type='ConnectorFilter', fid='unique-fid', parameters='') | ||
|
|
There was a problem hiding this comment.
Catching a generic Exception here can mask unexpected failures (e.g., validation errors unrelated to uniqueness). Since fid is unique=True, assert the specific DB exception (typically django.db.utils.IntegrityError) instead.
There was a problem hiding this comment.
Fixed in 26c1611. Now uses pytest.raises(django.db.IntegrityError) instead of the generic Exception.
.github/workflows/ci.yml
Outdated
| - name: Install test dependencies | ||
| run: pip install pytest pytest-django | ||
|
|
There was a problem hiding this comment.
The workflow installs dev extras (which already include pytest/pytest-django) and then installs pytest/pytest-django again. This is redundant and can lead to version skew if resolution differs; consider relying on the [project.optional-dependencies].dev install only.
| - name: Install test dependencies | |
| run: pip install pytest pytest-django |
There was a problem hiding this comment.
Removed the redundant pip install pytest pytest-django step in 26c1611. The --extra=dev install already brings in both packages.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…ed imports, fix tests, clean CI Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
All 6 items from that review thread were already applied in commit
55 tests pass, ruff is clean. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
main/core/middleware.py
Outdated
| user_agent_header = request.META.get('HTTP_USER_AGENT', '') | ||
| if not user_agent_header: | ||
| return | ||
| cached_user_agent = cache.get('user_agent_' + user_agent_header) | ||
| if not cached_user_agent: | ||
| user_agent = get_user_agent(request) | ||
| cache.set('user_agent_' + request.META['HTTP_USER_AGENT'], user_agent, 120) # Cache for 2 minutes | ||
| cache.set('user_agent_' + user_agent_header, user_agent, 120) # Cache for 2 minutes |
There was a problem hiding this comment.
process_request() now returns early when HTTP_USER_AGENT is missing/empty, which avoids the KeyError but also skips _enqueue_activity_log_creation() entirely. That’s a behavior change: manage requests without a User-Agent will no longer be logged. Consider continuing with an empty UA (and optionally skipping caching when the header is missing) so activity logging still occurs while preventing the crash.
CHANGELOG.md
Outdated
| @@ -1,5 +1,27 @@ | |||
| # Change history | |||
|
|
|||
| ## 3.1.0 | |||
There was a problem hiding this comment.
The changelog introduces a 3.1.0 section, but the codebase still reports version 3.0.3 (e.g., config/version.py and pyproject.toml). If this entry is for an unreleased version, consider labeling it as Unreleased; otherwise bump the project/version constants to match.
| ## 3.1.0 | |
| ## Unreleased |
CHANGELOG.md
Outdated
| - Added `CODE_OF_CONDUCT.md` (Contributor Covenant 2.1) | ||
| - Added GitHub issue templates (bug report, feature request, question) | ||
| - Added GitHub pull request template | ||
| - Added GitHub Actions CI workflow replacing outdated Travis CI configuration |
There was a problem hiding this comment.
This entry says GitHub Actions CI is “replacing outdated Travis CI configuration”, but .travis.yml is still present in the repository. Either remove/disable Travis config (if that’s the intent) or adjust the changelog wording to avoid implying it’s been replaced already.
| - Added GitHub Actions CI workflow replacing outdated Travis CI configuration | |
| - Added GitHub Actions CI workflow alongside existing Travis CI configuration |
.github/workflows/ci.yml
Outdated
| cache: pip | ||
|
|
||
| - name: Install linting tools | ||
| run: pip install ruff |
There was a problem hiding this comment.
The lint job installs ruff without a version constraint, which can drift from the version range declared in pyproject.toml and cause inconsistent lint results over time. Consider installing from the project’s dev dependencies (or pinning to the same version range) to keep CI and local runs aligned.
| run: pip install ruff | |
| run: | | |
| pip install --upgrade pip wheel uv | |
| uv pip install -r pyproject.toml --extra=dev |
.github/workflows/ci.yml
Outdated
| services: | ||
| redis: | ||
| image: redis:7-alpine | ||
| ports: | ||
| - 6379:6379 | ||
| options: >- | ||
| --health-cmd "redis-cli ping" | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
|
|
||
| env: | ||
| DJANGO_SETTINGS_MODULE: config.settings.dev | ||
| SECRET_KEY: ci-test-secret-key-not-for-production | ||
| DEVDB_URL: sqlite:///test_db.sqlite3 | ||
| REDIS_HOST: localhost | ||
| REDIS_PORT: 6379 |
There was a problem hiding this comment.
The test job provisions a Redis service, but the test suite overrides settings.CACHES to LocMemCache and disables DRF throttling in tests/conftest.py, so Redis should no longer be required to run tests. Consider removing the Redis service/env wiring to keep CI faster and consistent with the “no external services required” goal.
| services: | |
| redis: | |
| image: redis:7-alpine | |
| ports: | |
| - 6379:6379 | |
| options: >- | |
| --health-cmd "redis-cli ping" | |
| --health-interval 10s | |
| --health-timeout 5s | |
| --health-retries 5 | |
| env: | |
| DJANGO_SETTINGS_MODULE: config.settings.dev | |
| SECRET_KEY: ci-test-secret-key-not-for-production | |
| DEVDB_URL: sqlite:///test_db.sqlite3 | |
| REDIS_HOST: localhost | |
| REDIS_PORT: 6379 | |
| env: | |
| DJANGO_SETTINGS_MODULE: config.settings.dev | |
| SECRET_KEY: ci-test-secret-key-not-for-production | |
| DEVDB_URL: sqlite:///test_db.sqlite3 |
|
@copilot apply changes based on the comments in this thread |
… CI lint job, remove Redis from CI tests Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
All 5 items from that review applied in commit
55 tests still pass, CodeQL clean. |
|
fix that issue on CI building 2s Successfully installed packaging-26.0 uv-0.10.7 wheel-0.46.3 |
Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
Fixed in |
|
@copilot Run ruff check . --select E,W,F --ignore E501 E402 Module level import not at top of file E402 Module level import not at top of file E402 Module level import not at top of file |
…ignores Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
... Fixed in
|
|
@copilot apply changes based on the comments in this thread |
All 5 changes from that review thread are already applied and verified in the current branch:
|
|
@claude[agent] ensure ruff runs properly for ci , do the required modifications |
… add per-file-ignores Co-authored-by: 101t <4958477+101t@users.noreply.github.com>
Fixed in 88ed114. The CI workflow now checks only |
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.