Skip to content

feat(accounts): account profile view backend — contact_email + PATCH /v2/users/me/ (GUNDI-5347)#427

Merged
chrisdoehring merged 7 commits into
mainfrom
gundi_5347_account_add_contact_email
Jun 8, 2026
Merged

feat(accounts): account profile view backend — contact_email + PATCH /v2/users/me/ (GUNDI-5347)#427
chrisdoehring merged 7 commits into
mainfrom
gundi_5347_account_add_contact_email

Conversation

@amicavi

@amicavi amicavi commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Backend support for the Account Profile View (GUNDI-5347, sub-ticket of the epic GUNDI-5247 — see the backend implementation plan).

  • New field AccountProfile.contact_email (EmailField, nullable). Independent of User.email, which stays managed by the auth provider — needed for the upcoming Auth0 migration.
  • GET /v2/users/me/ now also returns first_name, last_name, contact_email, and workspaces[] (each with id, workspace_id, name, role) so the frontend can populate the whole Account Page with a single request.
  • New PATCH /v2/users/me/ to let users update first_name, last_name, contact_email — transactional so a partial failure can't leave the User and AccountProfile out of sync.
  • Refactor: Keycloak integration extracted from accounts/utils.py into accounts/keycloak.py (prep for the Auth0 swap). add_account now has an HTTP timeout, a single bool return, and proper network-error handling.

API changes

Method Endpoint Notes
GET /v2/users/me/ Response gains first_name, last_name, contact_email, workspaces[]. Existing fields unchanged.
PATCH /v2/users/me/ New. Body accepts any subset of {first_name, last_name, contact_email}. Returns the retrieve shape.

Frontend can populate the Account Page from a single GET /v2/users/me/:

```json
{
"id": ...,
"username": "...",
"first_name": "...",
"last_name": "...",
"contact_email": "user@example.com" | null,
"workspaces": [
{"id": 42, "workspace_id": "<org_uuid>", "name": "Workspace A", "role": "admin"}
]
}
```

workspaces[].id is the membership row id (integer, AccountProfileOrganization.id is a BigAutoField today). workspaces[].workspace_id is the organization UUID.

Out of scope (deferred to sibling sub-tickets of the epic)

  • `account_setup_complete` first-login flag
  • `POST /v2/users/me/leave-workspace/`
  • Auto-seeding `contact_email` from invite emails
  • Exposing `contact_email` (read-only) in the org members serializer
  • Renaming `AccountProfileOrganization` reverse manager (will land with the Organization → Workspace rename)

Test plan

  • `pytest cdip_admin/api/v2/tests/test_users_api.py` — covers GET shape (admin/viewer/superuser-without-profile), PATCH happy path, partial PATCH, invalid email, null contact_email, profile auto-create for superusers, unauthenticated, and N+1 regression on workspaces.
  • `pytest cdip_admin/api/v2/tests/test_organizations_api.py` — invite flow still passes after the Keycloak mock target moved from `accounts.utils.add_account` to `accounts.keycloak.add_account`.
  • `python3 manage.py makemigrations accounts --dry-run --check` — should report "No changes detected" (migration was hand-written; this confirms no drift, now that `help_text` is included on both model and migration).
  • Manual: hit `/v2/users/me/` via Swagger; PATCH a profile; confirm `contact_email` visible/searchable in Django admin.

🤖 Generated with Claude Code

amicavi and others added 4 commits June 2, 2026 13:31
- UserDetailsUpdateSerializer.update wrapped in transaction.atomic so a
  failed profile save reverts the user save.
- add_account: collapse to a single bool return, add HTTP timeout and
  RequestException handling; drop dead JsonResponse branch and the unused
  user param on get_password_reset_link.
- add_or_create_user_in_org: cache the org-members Group lookup and clean
  up control flow.
- Admin: rename shadowing ModelAdmin to AccountProfileAdmin and add
  list_select_related to avoid N+1 on the list view.
- AccountProfile.contact_email: add help_text documenting independence
  from the auth provider email.
- Docstrings on UserWorkspaceSerializer, UserDetailsUpdateSerializer and
  UsersView; comment the query-count budget.
- Tests: add a max_num_queries regression test for /v2/users/me/.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds backend support for an Account Profile view by introducing a user-controlled contact_email, expanding the /v2/users/me/ payload to include profile + workspace membership data, and adding a self-service PATCH /v2/users/me/ for updating profile fields. This also refactors Keycloak integration into a dedicated module in preparation for an Auth0 migration.

Changes:

  • Add AccountProfile.contact_email (nullable) plus Django admin/search support and a migration.
  • Expand GET /v2/users/me/ response to include first_name, last_name, contact_email, and workspaces[]; add PATCH /v2/users/me/ with atomic updates across User + AccountProfile.
  • Extract Keycloak logic into accounts/keycloak.py, update call sites, and adjust tests/mocks accordingly.

Reviewed changes

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

Show a summary per file
File Description
cdip_admin/emails/utils.py Updates Keycloak import path for password reset link helper.
cdip_admin/api/v2/views.py Enables user self-service PATCH via UpdateModelMixin and action-based serializer selection.
cdip_admin/api/v2/urls.py Routes PATCH /v2/users/me/ to partial_update.
cdip_admin/api/v2/serializers.py Adds workspace serializer; extends /me retrieve fields; adds atomic PATCH serializer with retrieve-shaped response.
cdip_admin/api/v2/tests/test_users_api.py Adds coverage for expanded /me shape, PATCH behaviors, auth requirement, and N+1 guard.
cdip_admin/api/v2/tests/test_organizations_api.py Updates Keycloak mock patch target to new module.
cdip_admin/accounts/utils.py Switches to new accounts.keycloak.add_account and refactors org-member group assignment.
cdip_admin/accounts/models.py Adds contact_email field to AccountProfile.
cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py Introduces DB schema change for contact_email.
cdip_admin/accounts/keycloak.py New Keycloak integration module with timeout and network-error handling.
cdip_admin/accounts/admin.py Exposes contact_email in admin list/search and edits fieldsets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cdip_admin/emails/utils.py
Comment thread cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py
Comment thread cdip_admin/accounts/keycloak.py Outdated
Comment thread cdip_admin/accounts/utils.py
Comment thread cdip_admin/api/v2/serializers.py
- emails/utils: drop unused get_password_reset_link import.
- migration 0017: include help_text on contact_email AddField so it
  matches the model and makemigrations --check stays clean.
- keycloak.add_account: replace response.ok with explicit 2xx range
  check (response.ok is True for 3xx too, contradicting the docstring).
- accounts.utils: use DjangoGroups.ORGANIZATION_MEMBER.value for the
  group lookup, matching conftest.py and core/permissions.py.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread cdip_admin/accounts/utils.py Outdated
Comment thread cdip_admin/accounts/keycloak.py Outdated
- accounts/utils: import the keycloak module instead of binding the
  function at import time. With `from .keycloak import add_account`,
  patching `accounts.keycloak.add_account` in tests no longer
  intercepted the call inside `add_or_create_user_in_org`, so the
  invite tests would issue real outbound HTTP to Keycloak. Switching
  to `from . import keycloak` and `keycloak.add_account(...)` makes
  the patch resolve at call-time, restoring test isolation.
- keycloak.add_account: use `logger.exception` (preserves traceback)
  in the RequestException handler, matching the convention used in
  event_consumers and integrations/tasks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chrisdoehring chrisdoehring self-requested a review June 5, 2026 16:39
@chrisdoehring

Copy link
Copy Markdown
Contributor

Possible dead code: keycloak.get_password_reset_link()

This PR removes the from accounts.utils import get_password_reset_link import from emails/utils.py, and a repo-wide grep now finds no callers of get_password_reset_link() — only its definition in accounts/keycloak.py:68. It looks like it was already an unused import before this change.

Suggest either wiring it up where it was intended to be used, or dropping it so we don't carry dead code into the Auth0 migration. (Worth a quick double-check that it isn't referenced from a template or other non-.py location first.)

🤖 Generated with Claude Code

@chrisdoehring

Copy link
Copy Markdown
Contributor

Nice work on two things in the Keycloak refactor that are easy to miss 👏

  1. Fixed a latent truthiness bug. The old add_account returned a JsonResponse(status=403) when no admin token was available, and the caller did if response:. A JsonResponse is truthy, so the old path would create the Django user even when Keycloak creation had effectively failed. Returning a plain bool makes if not keycloak.add_account(...): raise SuspiciousOperation behave correctly.

  2. Bounded the outbound Keycloak call. Adding KEYCLOAK_HTTP_TIMEOUT plus except requests.RequestException replaces a previously unbounded requests.post — a real robustness win that keeps a portal request from hanging on a slow/unreachable Keycloak.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 11 out of 11 changed files in this pull request and generated no new comments.

@chrisdoehring

chrisdoehring commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Data-model follow-up filed: GUNDI-5377

A design review of the User / AccountProfile split (prompted by adding contact_email here) concluded the separation is sound and idiomatic — this PR is safe to merge as-is and is a good citizen (proper select_related, N+1 regression test, defensive profile reads).

The one genuine, pre-existing risk surfaced is that the "every User has exactly one AccountProfile" invariant is unenforced (the OIDC login path creates a User with no profile), which is why the codebase carries scattered getattr/try-except guards — and one unguarded spot (integrations/views.py:829) that 500s instead of 403s. That's tracked separately in GUNDI-5377 (auto-create + backfill + the 403 fix) and does not block this PR.

🤖 Generated with Claude Code

After removing the unused import in emails/utils.py (commit 9359a51),
get_password_reset_link had no callers — and no templates reference a
`password_reset_link` context variable either. The invite email tells
the user to click "Forgot Password?" in the portal UI; there is no
email-embedded reset link.

Carrying a Keycloak-specific URL builder into the Auth0 migration is
exactly the kind of dead weight we want to leave behind. If a reset
link is ever needed again, it should be added via the AuthProvider
abstraction at that time.

Also drops the now-unused KEYCLOAK_CLIENT constant.

Addresses chrisdoehring's review comment on PR #427.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amicavi

amicavi commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — confirmed dead code. Verified there's no password_reset_link reference in emails/templates/invite_email.html or invite_email.txt either; the invite copy explicitly instructs the user to click "Forgot Password?" in the portal UI, so there's no email-embedded reset link.

Dropped get_password_reset_link and the now-unused KEYCLOAK_CLIENT constant in ddce5f9. If a reset link is ever needed again it should be added via the AuthProvider abstraction at that time, not carried into the Auth0 migration as Keycloak-specific glue.

@amicavi

amicavi commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for calling those out — appreciated. Both were genuinely latent: the if response: against a truthy JsonResponse would have created Django users orphaned from any Keycloak account on a missing-token path, and an unbounded requests.post is the kind of thing that only shows up as a portal-wide stall during a Keycloak outage. Glad they're closed off now.

@amicavi

amicavi commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Acknowledged and thanks for the design review. Agreed the User/AccountProfile split is the right shape — and the unenforced 1:1 invariant is the real source of the scattered defensive reads (including the get_or_create inside UserDetailsUpdateSerializer.update in this PR, which is precisely that pattern). Tracking the auto-create + backfill + the integrations/views.py:829 403-vs-500 fix under GUNDI-5377 is the right call; keeping it out of this PR keeps the scope honest.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 11 out of 11 changed files in this pull request and generated no new comments.

@chrisdoehring chrisdoehring left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks good.

@chrisdoehring chrisdoehring merged commit 26a50f0 into main Jun 8, 2026
2 checks passed
@chrisdoehring chrisdoehring deleted the gundi_5347_account_add_contact_email branch June 8, 2026 18:20
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