Test/user account#6
Closed
TonyPythoneer wants to merge 40 commits into
Closed
Conversation
CLAUDE.md, README, and OBSERVABILITY.md still described the pre-flatten project structure and Make targets that no longer exist. Fix the unconditionally-stale facts: real `make` targets, flat repo layout, boto3 (not django-storages), valid file paths, and the docker-compose per-process OTEL_SERVICE_NAME wiring in OBSERVABILITY.md Symptom 5. Code-coupled doc fixes (storage.py rename, apps.py telemetry wiring, span processor choice) are deferred to their refinement tasks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…config - Change setdefault in manage.py/wsgi.py/asgi.py from empty package django_thumbnail.settings to django_thumbnail.settings.local so bare invocation without DJANGO_SETTINGS_MODULE set no longer crashes - Remove redundant DJANGO_SETTINGS_TEST prefix from make test; pyproject.toml [tool.pytest.ini_options] already pins the test settings module - Add tasks/refinement/ checklist files tracking the full refinement plan Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Only mark ImageTask as FAILED when retries are exhausted; keep PROCESSING during intermediate retries so pollers see accurate state - Add Outcome.FAIL to distinguish terminal failure from mid-retry in spans - Remove force_flush() and stale BSP comment — SimpleSpanProcessor exports synchronously on span.end(), making the flush a no-op - Update test to simulate exhausted retries via apply(retries=max_retries) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move login_required_json from utils.py to images/decorators.py; update views.py import — auth concern separated from storage concern - Replace class-attribute config pattern in _S3BaseClient with explicit __init__ parameters; InternalS3/PublicS3 pass their own endpoint/config at construction — no more declarative-field convention confusion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nces - Rename images/utils.py to images/storage.py — name now matches content - Update imports across models.py, views.py, tasks.py, test_api.py, test_smoke.py, and management/commands/test_image_upload.py - Update patch targets in test_api.py (images.utils.* → images.storage.*) - Update CLAUDE.md §3 project layout to reflect storage.py and decorators.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erializer - Add parse_json_body() to decorators.py; replace three inline try/except json.loads blocks in images_list, tasks_create, auth_login - Add functools.wraps to login_required_json so decorated views keep __name__ - Add _gallery_row() private function in views.py (not on the model) to serialize Image + task for the HTML gallery; keeps model free of presentation-only concerns - Remove json import and unused ImageStatus import from views.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove blank=True/default="" from thumbnail_key — key is always computed
at create time via create_with_key(); update 0001_initial migration in-place
- Drop all if thumbnail_key / if original_key None-guards in to_dict(),
ImageTask.to_dict(), and _gallery_row() — fields are always populated
- Fix latest_task() to use next(iter(self.tasks.all())) so prefetch cache
is used instead of bypassed by order_by()
- Add prefetch_related("tasks") to images_list GET queryset to eliminate N+1
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Delete AWS_S3_ADDRESSING_STYLE, AWS_DEFAULT_ACL, AWS_QUERYSTRING_AUTH from base.py — django-storages is not installed; nothing reads these settings - Extract _fmt_ts() helper to deduplicate created_at formatting in both Image.__str__ and ImageTask.__str__; add example output comments - Remove redundant if-key guard in delete_from_storage() — both keys required Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add tests for `Image.latest_task()` / `current_status()` and verify they use the prefetch cache (query count assertion). - Split S3-error task test: distinguish retry-in-progress (Retry raised, status PROCESSING) from retry exhaustion (FAILED). - Add HTML view tests covering owner isolation — gallery filters by user, image delete refuses cross-user requests. - Smoke test now uses `Image.format_thumbnail_key_from_original()` instead of duplicating the key-format logic. Drop dead `STORAGES` config from settings/test.py (project uses boto3 directly, not django-storages) and remove the `test_image_upload` management command — its scenarios are now covered by the new tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace pyright with pyrefly across config and IDE setup: - pyrefly.toml migrated from pyrightconfig.json - Drop pyright dev dep - Zed: pyrefly LSP via .venv/bin/pyrefly + format_on_save via ruff LSP - Makefile: consolidate lint/format into `make check` (CI-safe) + `make fix` - Models/views: add type annotations and casts to satisfy pyrefly Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add `[tool.ruff]` to pyproject.toml: target-version py314, line-length 100, deliberate lint select (E/W/F/I/UP/B/SIM), exclude .claude - Apply ruff auto-fixes + format across project (mostly import sorting / wrap) - Code fixes prompted by new rules: * Split long __str__ f-strings in models.py * `raise self.retry(...) from exc` in tasks.py (B904) * Narrow bare Exception → ConnectionError in test_tasks.py (B017) - CI workflows: both use `uv sync --group dev` + setup-uv cache; ci-app-integration-test runs `make check` + `make test` instead of inline - Makefile: add `make migrate` target - Update tasks/refinement/09 checklist + acceptance criteria Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
thumbnail() decodes the file already, so the prior verify() pass was redundant work on the same buffer. Collapse _validate_image_buffer + _create_thumbnail_buffer into one _make_thumbnail() that catches UnidentifiedImageError/OSError as InvalidImageError. Closes the last open box in tasks/refinement/06_tasks_cleanup.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom 1/3 claimed OTel was wired in apps.py:ready(); actual init lives in manage.py via setup_otel_for_django_web(). Rewrote samples to match. Dropped non-existent setup_telemetry() call. Reframed BatchSpanProcessor note as production consideration (task 06 kept SimpleSpanProcessor only). Task 01 acceptance: all doc links resolve; README Quick Start statically verified against Makefile + docker-compose; runtime clone-follow deferred to task 10. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Telemetry init no longer rides manage.py: - New django_thumbnail/apps.py with CoreConfig.ready() — gated on _is_web_server_process() (runserver / gunicorn / uvicorn) so it doesn't fire in the Celery worker parent (gRPC channel would inherit across fork) or in management commands. - INSTALLED_APPS gets CoreConfig as the first entry so OTel init precedes other apps' ready(). - manage.py reverts to stock Django boilerplate. - images/apps.py stays a pure domain app config — no instrumentation responsibility leaks into domain apps. Worker keeps its per-child worker_process_init handler (the celery- blessed fork-safe hook). Web vs worker init now mirror their framework-blessed lifecycle hooks. OBSERVABILITY.md Symptom 1 / 3 fix sections rewritten to describe the new wiring. Verified: - make test → 37 passed - make check → ruff/format/pyrefly clean - manage.py check (no DJANGO_SETTINGS_MODULE) → no issues - Process probe: runserver argv → TracerProvider; check/celery argv → ProxyTracerProvider (no-op) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
models.py top-level has no `tasks` dependency, so tasks.py can import `ImageTask` (and `Image`) at top level without re-introducing a circular import. Drops the in-function `from .models import ImageTask` and the TYPE_CHECKING block for `Image`. Net: one runtime lazy import remains in the models/tasks pair (`models.py:create_and_dispatch` → `generate_thumbnail`, intentional and commented). Satisfies task 07 acceptance. Verified: make test → 37 passed; make check → clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirrors the gallery query-count assertion: 5 images + tasks, then asserts the API list endpoint stays at 4 queries (session + user + images + prefetched tasks). Closes the last task 08 gap; bulk of the test work landed earlier in commit 9ac9b94. Test count 37 → 38; make check still clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- task 10: automated gates green (make test 38 passed, make check clean); runtime/docker gates noted as user-driven. - task 05: Debug Toolbar manual check marked superseded by task 08's programmatic django_assert_num_queries assertions. - task 00_overview: new "Outcome (2026-05-16)" section with the LOC delta vs merge-base d6ac5ce — production code -72 (-4.5%), tests +137 (task 08). Deferred items listed with rationale. - CLAUDE.md §0: updated to "Refinement closing" phase with LOC outcome and pointer to 00_overview.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- POSTGRES_DB: django_thumbnail_test → django_thumbnail. The previous value sat unused — Django connects to the maintenance DB and creates test_django_thumbnail itself. Matching settings/base.py defaults removes a reviewer's "wait, which DB?" pause. - Drop redundant DJANGO_SETTINGS_MODULE env on the Test step. pyproject.toml [tool.pytest.ini_options] already pins it. - Add "Dump postgres logs on failure" step mirroring the smoke workflow — gives diagnostic context if the DB connection breaks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Refinement plan complete — drop tasks/refinement checklists and the related CLAUDE.md sections (current-state header, tasks/ tree entry, session-start and per-task rules). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Refactor/simply
- Disable redundant uv@jamie-bitflight-skills (python-engineering:uv covers it). - Remove 6 ghost rows from .claude/skills/README.md (onboard, ticket, pr-review, pr-summary, worktree-commit-merge, htmx-patterns); celery-patterns kept for H1. - Update SKILLS_HIRE.md to record F1/F2 completion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace pyright/ty references with pyrefly across hooks, agents, skills, and commands to match actual dev dependency (pyrefly>=1.0.0) and CI usage (make check runs python -m pyrefly check). Six locations updated: .claude/settings.json hook, settings.md, code-reviewer agent, systematic-debugging skill, code-quality skill, and fix command. Mark P1 done in SKILLS_HIRE.md and record F3/F4/F5 type-checker portions resolved; remaining work in F3/F4/F5 is htmx-related, gated on P2. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Decide P2: never adopt htmx (no package installed, frontend uses plain JS fetch + JsonResponse). Apply consequences: - F4 systematic-debugging: drop Django Debug Toolbar and HTMX sections - F5 code-reviewer agent: drop HTMX checklist, partial-return branch, and htmx-alpine-patterns integration line - F6 django-forms/templates/pytest-django-patterns: insert "htmx not used in this project" note above htmx sections; preserve content for potential future adoption - F3 code-quality: remove stray HTMX HX-Request checklist row Add H1 celery-patterns skill grounded in actual project code (images/tasks.py, images/models.py, django_thumbnail/celery.py, django_thumbnail/settings/test.py): @shared_task convention, idempotency rule, exponential backoff retry, pass-IDs pattern, Image/ImageTask state flow with real field names, and CELERY_TASK_ALWAYS_EAGER testing mode. Mark wave 1 complete in SKILLS_HIRE.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…, policy Add three new local skills and a plugin enablement policy. Conclude all remaining SKILLS_HIRE.md tasks (H2, H3, P3, P4, C1). New skills (English, grounded in actual project code): - storage-s3 — boto3 singleton construction (internal_s3 / public_s3 in images/storage.py), MinIO endpoint split (in-cluster vs public-host), presign patterns, signature_version="s3v4", smoke-test verification via head_object, and patching-by-import-path guidance. - opentelemetry-patterns — span.set_attributes(dict) batch rule, web vs Celery worker telemetry separation (apps.CoreConfig.ready vs worker_process_init signal), SimpleSpanProcessor over BSP for fork safety (rationale from commit af308f6), per-process OTEL_SERVICE_NAME override in docker-compose, OTEL_SDK_DISABLED in settings/test.py. Policy: - .claude/SKILLS_POLICY.md catalogues the 36 python-engineering plugin skills: 12 approved (concept-only references), 24 disabled (CLI/TUI, packaging, orchestrator family, ty/prek-assumptions, stack-incompatible). Maps each project need to its local tool. Defines the quality-check chain (P4): /fix inner-loop, /code-quality + code-reviewer pre-PR, make check + make test in CI. P3 decision: django-extensions stays uninstalled. No usage signals (zero references in pyproject.toml, settings, Makefile, agents, commands, or user memory). The skill remains as a future-activation reference with a DORMANT banner and explicit activation steps. Also fix the leftover example "core.EmailAccount / metabox.Thread" to use the project's actual models. Post-audit fixes (factual accuracy): - storage-s3: rename cited Image methods to real names (upload_original, delete_from_storage) per images/models.py:86,89 - opentelemetry-patterns: document per-process OTEL_SERVICE_NAME override (web=django-thumbnail-app, worker=django-thumbnail-worker) required by Jaeger trace separation in test_smoke.py - SKILLS_POLICY: correct plugin skill count from 38 to 36 Convert F6 htmx notes and django-extensions dormant banner from Traditional Chinese to English to match the rest of the skill corpus. SKILLS_HIRE.md fully closed; all three waves are now ✅ DONE. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
All planned tasks (P1-P4, F1-F6, H1-H3, C1) are complete. Remove the internal planning docs and strip the dead "Companion docs" footer from SKILLS_POLICY.md. Operational policy lives in: - .claude/SKILLS_POLICY.md (plugin enablement + quality-check chain) - .claude/skills/*/SKILL.md (concrete skills) - .claude/skills/django-extensions/SKILL.md (DORMANT banner) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude/skills
Enable flake8-annotations (ANN) in ruff with ANN401 active so Any is rejected. Add per-file ignores for tests/migrations/manage.py/conftest. Annotate 32 previously untyped sites: - decorators: PEP 695 ParamSpec on login_required_json to preserve wrapped view signatures end-to-end - management commands: handle(*args: object, **options: object) -> None; s3 cached_property typed as S3Client (TYPE_CHECKING) - storage: 3 __init__ return None - tasks: generate_thumbnail self: Task (Celery bind=True) - views: 5 HTML views -> HttpResponse - models: _fmt_ts(dt: datetime | None)
Drop per-file ignores for manage.py, conftest.py, and tests/. Keep migrations ignored — Django regenerates them via makemigrations and would overwrite annotations. - manage.py: main() -> None - conftest.py: fixtures typed with Client, User, tuple[Client, User] - factories.py: post_generation password method fully typed - tests/: 38 test methods get -> None; fixtures typed via DjangoAssertNumQueries and Iterator[None] for yielding mock_s3
Collapse 6 duplicated test methods into 3 parametrized ones using @pytest.mark.parametrize with named ids: - test_login: matrix over (password, expected_status) - test_create (images): matrix over (payload, expected_status) - test_delete (images): matrix over (owned, expected_status, image_remains) - test_create_task: matrix over (payload_kind, expected_status, task_created) - test_get_task: matrix over (owned, expected_status) 18 test methods → 12 methods producing 17 cases. Tests with distinct assertion shapes (cancel_task, list_query_count_constant, unauthenticated) stay separate. ids=[...] keeps failure messages readable.
factory-boy class calls return the factory class to type checkers, breaking IDE attribute resolution on returned model fields. Wrap each factory in a helper that asserts the model return type via typing.cast, restoring IDE/pyrefly visibility on .id, .latest_task, refresh_from_db, etc. Pyrefly errors across tests drop from 40 to 8. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Promote generate_thumbnail max_retries to a module-level constant so tests reference the int directly instead of celery's untyped Task attr. - Wrap celery .apply call in a Callable cast (stubs misreport it as list). - factories.UserFactory.password: cast self to User (factory-boy injects the instance at runtime; pyrefly enforces LSP on self annotations). - test_models: null-check latest_task() before .id access. - pyrefly.toml: drop tests/ and management/ excludes; add manage.py. Result: 0 pyrefly errors across the full project (was 40 with tests included), no new type:ignore lines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DELETE /api/tasks/<id>/ let any owner revoke a running Celery task. Task lifecycle should be controlled internally (admin/management), not exposed to end users — removing the verb closes that surface. tasks_detail now accepts GET only; AsyncResult import and the test_cancel_task case are dropped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- test_tasks: split test_status_outcomes into test_direct_call (success and invalid_image) and test_resilient_to_download_failure (retry outcomes). Each method is now straight-line with no branching; the parametrize matrices align with their actual call mode. - test_views_html: hoist URL names into class-level *_viewname attributes, matching the convention already used in test_api.py. - test_smoke: rename infra-check helpers so each name announces the stack layer it covers (async worker, storage, observability) and drop the redundant docstrings. Add Jaeger query retry loop so the trace assertion no longer relies on a single fixed sleep. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wire a TracerProvider + OTLP gRPC exporter and RequestsInstrumentor in the smoke test process so outbound HTTP calls inject traceparent and emit client spans. Web/worker spans become children under a single trace, enabling end-to-end visibility in Jaeger. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move Jaeger HTTP polling, response schemas, and helpers into a single test-only module (images/tests/jaeger.py) with a JaegerClient singleton mirroring the storage.py pattern. Response shapes are typed via TypedDict, with a shared KeyedValue covering process/span/log fields and NotRequired warnings to reflect Jaeger's optional payloads. Switch smoke process to SimpleSpanProcessor so spans export synchronously on end, removing the need for force_flush() before the events assertion. The smoke root span now runs inline in the test method via tracer.start_as_current_span, with trace_id read from the span context. The trace is verified in two passes: service presence inside the active span, then events on the root span after it ends. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Chore/typing
Extract hardcoded test1/test2 credentials from create_test_users command and smoke test into shared settings._test_users module. Imported only by local.py/test.py; base.py defaults to empty list so prod inherits no preloaded accounts. Command no-ops with warning when TEST_USERS is empty (prod safety guard). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolves pyrefly untyped-import warning on requests usage in smoke tests (images/tests/test_smoke.py). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.