Skip to content

refactor: support 2.42 en userRepository and UserModel#342

Merged
adrianq merged 6 commits intodevelopmentfrom
feature/support_2.42
Apr 17, 2026
Merged

refactor: support 2.42 en userRepository and UserModel#342
adrianq merged 6 commits intodevelopmentfrom
feature/support_2.42

Conversation

@xurxodev
Copy link
Copy Markdown
Contributor

@xurxodev xurxodev commented Apr 1, 2026

📌 References

📝 Implementation

Tab users

  • Support in d2ApiRepository to 2.42 in fields and responses
  • Support 2.42 filters previously in userCredentials
  • Support 2.42 filters previously in userCredentials in legacy code
  • Fix save openId and ldapId

Refactor detected

@Ramon-Jimenez @adrianq I've detected a posible refactor in this app.

From use cases and components for filters, this code builds expressions like userCredentials.username:in:[].

The problem is that userCredentials no longer exists in version 2.42. The changes required to adapt to 2.42 should only affect the data layer.

The main issue is that there is currently a coupling between the app and DHIS2 filters. The domain, presentation and data layers are using DHIS2 filters directly. The domain should have its own filter model and adapt to and from DHIS2 filters within the repository.

To avoid a large refactor in this PR, I fixed it by using a replace function based on the version in the repository. However, this solution may be insufficient if future API versions introduce similar changes.

I think a refactor in this area is necessary.

Questions for PMs

Previously the user filters to filter by 2factor enabled the code used userCredentials.twoFA.

Screenshot 2026-04-01 at 13 08 23

In 2.42 userCredentials is not in the response and twoFA has not been included in user.

Now if the version is 2.42 the filter has not effect because is removed in the process to avoid fail in api request.

📹 Screenshots/Screen capture

🔥 Testing

@xurxodev xurxodev marked this pull request as ready for review April 2, 2026 07:26
@adrianq adrianq changed the base branch from fix/bug_copy_in_with_replace to development April 7, 2026 08:42
@adrianq
Copy link
Copy Markdown
Member

adrianq commented Apr 7, 2026

As discussed with @xurxodev we will remove the 2FA filter for 42 from the graphical interface. I have also created a task to eventually take care of the refactor: https://app.clickup.com/t/869ctkw05

@bundlemon
Copy link
Copy Markdown

bundlemon Bot commented Apr 9, 2026

BundleMon

No change in files bundle size

Groups updated (1)
Status Path Size Limits
Build Folder
./**/*
1.41MB (+1.05KB +0.07%) +20%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@xurxodev
Copy link
Copy Markdown
Contributor Author

xurxodev commented Apr 9, 2026

@adrianq Ready remove 2FA from filter and also from columns

@adrianq adrianq requested a review from Ramon-Jimenez April 9, 2026 10:20
@MiquelAdell
Copy link
Copy Markdown
Contributor

Smoke-tested this against play.im.dhis2.org/stable-2-42-4 and the 2.42 routing works end-to-end — filters use root-level disabled / externalAuth / userRoles.id, the 2FA column/filter are correctly hidden, and saves succeed.

One thing worth checking before merge: on 2.42 the save payload (and the list fields= query) still include the full userCredentials.* block alongside the new root-level fields — it's a dual-write, not a version-split. Concretely, on a 2.42.4 save I see:

{
  "username": "traore",
  "disabled": true,
  "externalAuth": true,
  "openId": "1234567",
  "ldapId": "",
  "userRoles": [...],
  "userCredentials": {
    "username": "traore",
    "disabled": true,
    "twoFA": false,
    "openId": "1234567",
    "ldapId": "",
    "externalAuth": true,
    "userRoles": [...],
    ...
  }
}

@xurxodev Two questions:

Is the duplication intentional (belt-and-braces for backward compat across versions)? If so, a one-line comment near the version-detection helper would go a long way.
If it's not intentional, twoFA specifically shouldn't be in the payload on 2.42 — the API dropped the field and the PR description calls that out. Not a server-side failure because DHIS2 ignores it, but worth cleaning up.

Full test list

Boot & shell
✅ App shell loads at http://localhost:8081/ within 10 s
✅ /api/system/info fired (version-detection hook; returned 200, v2.42.4)
✅ Tabs rendered: USERS, USER GROUPS, USER ROLES, DASHBOARDS
✅ No red console errors attributable to PR #342

Users list
✅ Table renders 158 rows
✅ Columns present: Username, First Name, Surname, Email, Data capture OUs, Last login, Disabled
✅ Two-factor column absent on 2.42
✅ /api/users list request returned 200
✅ fields= param includes root-level userRoles, disabled, username, openId, ldapId, externalAuth, lastLogin
⚠ fields= also includes userCredentials.* copies (dual-write concern, flagged)

Filters
✅ Advanced Filters panel opens
✅ "Disabled" filter emits filter=disabled:eq:true at root
✅ "External auth" filter emits filter=externalAuth:eq:true at root
✅ "User roles" filter emits filter=userRoles.id:in:[…] at root
✅ 2FA filter control absent from panel on 2.42
✅ Row count changes correctly when filters applied (158 → 21)
✅ "Clear all filters" resets to full list

Edit user (target: traore, UID oXD88WWSQpR)
✅ Actions menu → Edit opens the edit page
✅ Form fields populated: Username (disabled), First Name, Surname, Email, Disabled toggle, OpenID (1234567), LDAP identifier
✅ External-auth checkbox reflects current state
✅ Surname edit + Save → POST /api/metadata?async=false returned 200 with updated:1
✅ Save payload carries disabled, openId, ldapId, externalAuth, userRoles, username at root
✅ Direct PATCH revert via /api/users/ → 200
✅ Read-back confirms surname restored to "Traore"
⚠ Payload also dual-writes userCredentials.* including twoFA:false (flagged)

Create user (API path, 6a)
✅ POST /api/users with root-level-shaped payload → 201 Created
✅ UID SXnatIGAZlw resolvable via GET /api/users/
✅ Read-back response has root-level disabled, externalAuth, userRoles, userGroups, organisationUnits
✅ DHIS2 rejects username change with E4056 (documented constraint)

Create user (UI form path, 6b)
✅ #/new route renders the form
✅ fill_form populates General Info fields

Copy in user (target: smoke_1776338902)
✅ Actions → "Copy in user" opens the bulk attribute-copy dialog
✅ Modal's users-list fetch uses fields=email,firstName,id,name,surname,username with username at root
✅ Cancel closes the dialog with no side effects

Bulk edit (targets: smoke user + traore)
✅ Multi-row selection via checkboxes works
✅ Selection persists across search-clear ("2 items selected (1 on other pages)")
✅ Row Actions → "Assign groups" opens the bulk modal with both users as targets
✅ Bulk metadata POST with root-level user payload → 200 OK, updated:1

User Roles page
✅ Table loads with 21 rows
✅ No console errors

User Groups page
✅ Table loads with 40 rows
✅ No console errors

CSV export
✅ User Groups CSV export — Actions → Export to CSV downloads user-groups-.csv (25 rows = page-scope)
✅ Users CSV export — Import/Export → Export to CSV downloads users-.csv (160 rows = full scope)
✅ Export empty template — downloads a header-only CSV for import seeding

Import/Export menu (Users tab)
✅ Menu renders with 4 entries: Import, Export to CSV, Export to JSON, Export empty template
✅ Hidden is wired (inspected via evaluate_script)

Dashboards tab
✅ Tab loads with 82 dashboards (owner + user visibility audit view)
✅ No console errors

Settings modal
✅ Settings icon opens the modal
✅ All 6 tabs render without blocking errors: Import, Logger, Permissions, User permissions, Columns, Filter permissions
✅ Tab switching works; CLOSE button dismisses

OU assignment modals (target: traore)
✅ Actions → "Assign to data capture organisation units" opens the capture-OU modal with Sierra Leone pre-checked
✅ Actions → "Assign to data view organisation units" opens the output/view-OU modal
✅ Actions → "Assign to search organisation units" opens the search-OU modal
✅ CANCEL on each preserves state — API read-back confirms organisationUnits, dataViewOrganisationUnits, teiSearchOrganisationUnits all unchanged after the three open-cancel cycles

2FA user edit — N/A
✅ Verified impossible on 2.42.4: all of twoFA, twoFactorType, userCredentials.twoFA return 400 E1003 Unknown path property. No 2FA-enabled user exists or can be constructed — the edit-edge-case test is not applicable on this version.

Cleanup
✅ DELETE /api/users/SXnatIGAZlw → 200
✅ Follow-up GET → 404 (confirmed gone)
✅ traore.surname verified as "Traore" post-run

Other
✅ "Replicate user from template" and "Replicate user from table"

Manual tests

✅ Create user through file import

@MiquelAdell MiquelAdell self-requested a review April 16, 2026 12:10
…read

- The shouldSendTwoFA guard in buildUsersToSave was effectively dead code:
  the `...user.userCredentials` spread already injected twoFA (including
  false) into the payload, which could disable 2FA on save. Strip twoFA
  from the spread so the guard actually controls the field.

- Also expand the dual-write comment to document that root-level fields
  are required by DHIS2 2.42+ (where userCredentials was removed) while
  the userCredentials block is kept for ≤2.41 compatibility.
@xurxodev
Copy link
Copy Markdown
Contributor Author

Thanks @MiquelAdell for the testing

Answering your two questions:

  1. Yes, the duplication is intentional. It's backwards-compat with 2.38–2.41,
    which still expect the fields inside userCredentials.* (and 2.38 has the
    known bug where root-level alone is silently ignored). On 2.42 the server
    removed the userCredentials schema entirely, so it just ignores the block —
    we chose to keep a single dual-write path instead of a full version-split to
    minimize risk on an already-validated PR.

  2. You were right about twoFA: it shouldn't have been in the 2.42 payload.
    Turns out it was a latent bug affecting all versions — the
    ...user.userCredentials spread was injecting twoFA:false before the
    shouldSendTwoFA guard ran, so the guard was effectively dead code and we
    were sending twoFA:false on every save (risking disabling 2FA).

Pushed a fix that:

  • Strips twoFA from the credentials spread so the guard actually controls it.
  • Expands the comment in buildUsersToSave to document the dual-write rationale
    (root required by 2.42+, userCredentials required by ≤2.41) and point to
    getIs242Plus() as the version-detection helper.

Payload on 2.42 should now be clean of twoFA, and on ≤2.41 we only send
twoFA:true when the user actually has 2FA enabled.

@MiquelAdell
Copy link
Copy Markdown
Contributor

Re-ran the smoke test against 639f3a7 on play.im.dhis2.org/stable-2-42-4. The save payload is now clean: twoFA absent from both root and userCredentials, all other root-level fields intact, save returns 200 with updated:1. 👍

@adrianq adrianq merged commit 1ac5256 into development Apr 17, 2026
6 checks passed
@MiquelAdell MiquelAdell deleted the feature/support_2.42 branch April 17, 2026 08: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