Skip to content

Read login_name and display_name back for service users - #71

Open
jacodegroothydrab wants to merge 1 commit into
datacoves:mainfrom
jacodegroothydrab:fix/service-user-login-name
Open

Read login_name and display_name back for service users#71
jacodegroothydrab wants to merge 1 commit into
datacoves:mainfrom
jacodegroothydrab:fix/service-user-login-name

Conversation

@jacodegroothydrab

Copy link
Copy Markdown
Contributor

fetch_user discards login_name and display_name for TYPE = SERVICE:

if user_type != "SERVICE":
    display_name = data["display_name"]
    login_name = data["login_name"]
    must_change_password = data["must_change_password"] == "true"

SHOW USERS has already returned both. The provider reports None regardless, so a config declaring either gets a diff that can never converge — applying it changes nothing and the next plan proposes it again.

Only must_change_password is genuinely unreadable for a service user, so that stays blanked.

It also defeats the point of declaring login_name: ALTER USER ... RENAME TO leaves LOGIN_NAME behind, and a field always read as None cannot surface that drift.

6 unit tests, which fetch_user had none of. Reverting the fix fails one of them.

🤖 Generated with Claude Code

@jacodegroothydrab jacodegroothydrab changed the title fix(user): read login_name and display_name back for service users Read login_name and display_name back for service users Aug 26, 2026
@noel

noel commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

The fetch_user change looks correct, including SERVICE, LEGACY_SERVICE, empty-value normalization, and retaining must_change_password behavior.

The branch now conflicts with current main in tests/test_data_provider.py, where this PR and PR #69 appended tests at the same location. Please rebase onto main, retain both test blocks, and rerun the suite.

Please also shorten the seven-line rationale above the changed fetch_user fields to the invariant that matters—for example, # SERVICE users do not report must_change_password. The PR description and tests already preserve the rest of that history. The TestFetchUserServiceFields class docstring can likewise be removed.

fetch_user discards login_name and display_name for TYPE = SERVICE, so the provider
reports None whatever the account holds. A config declaring either gets a diff that
can never converge: applying it changes nothing, and the next plan proposes it
again.

SHOW USERS reports both for service users. Only must_change_password is genuinely
unreadable for TYPE = SERVICE, so that stays blanked.

It also defeats the point of declaring login_name at all. ALTER USER ... RENAME TO
leaves LOGIN_NAME behind, and a field the provider always reads as None can never
surface that drift.

Adds 6 unit tests over fetch_user, which had none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jacodegroothydrab
jacodegroothydrab force-pushed the fix/service-user-login-name branch from 1f78e5b to 1f49ca4 Compare August 28, 2026 12:30
@jacodegroothydrab

Copy link
Copy Markdown
Contributor Author

Rebased onto main — the conflict in tests/test_data_provider.py was #69's block and this one appended at the same spot, so both are retained. Suite reruns clean (2180 passed; the 6 remaining failures are the pre-existing Windows-only SecurePath/chmod ones, identical on a clean checkout).

Trimmed the rationale to # SERVICE users do not report must_change_password., sitting on the one line it explains, and dropped the TestFetchUserServiceFields docstring.

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