feat(users): add read-only role and filter read-only users from user list#5600
feat(users): add read-only role and filter read-only users from user list#5600rajohnson90 wants to merge 5 commits intomainfrom
Conversation
josbell
left a comment
There was a problem hiding this comment.
I had two independent agents review this PR, followed by a deep investigation into the concerns raised. Here are the findings:
🚨 BLOCKING ISSUE: Role ID Mismatch
Critical data integrity bug - will cause FK constraint violations during data import.
Problem:
- SQL files insert READ_ONLY as role ID 7 (backend/data_tools/initial_data/003-role.sql line 7, 004-role_version.sql line 7)
- JSON5 file assigns user 529 to role ID 8 (backend/data_tools/data/user_data.json5 line 695)
Fix Required:
Change both SQL files to use ID 8 instead of ID 7:
INSERT INTO ops.role (id, name, permissions, ...) VALUES (8, 'READ_ONLY', ...
The existing role sequence is 1-6 (SYSTEM_OWNER through PROCUREMENT_TEAM), so READ_ONLY should be ID 8, not 7.
- User Management Visibility (Can address in follow-up)
READ_ONLY users are filtered from GET /users list endpoint, making them "invisible" in the UI. However:
- They're still accessible via GET /users/{id} ✅
- They can still be updated via PUT/PATCH /users/{id} ✅
- This is a UX gap, not a data integrity issue
Recommendation: Implement the include_read_only parameter mentioned in the PR description in a follow-up PR. Consider adding it to the QueryParameters schema to allow admins to toggle READ_ONLY user visibility.
- API Design (Working as intended)
GET /users filters READ_ONLY users, but GET /users/{id} does not. This is intentional and correct REST API design:
- List endpoints apply business logic filtering
- Item endpoints return the resource if it exists and user has permission
Recommendation: Consider adding a test (test_get_read_only_user_by_id) to document that direct access works even though list filtering is applied.
✅ Action Items
Must fix before merge:
- Change role ID from 7 to 8 in 003-role.sql and 004-role_version.sql
Recommended before merge:
- Test database import after fix: docker compose down -v && docker compose up db data-import --build
Follow-up PR (not blocking):
- Implement include_read_only query parameter for better admin UX
Summary: One critical bug to fix (2-line change), then good to merge. The other concerns are either design-as-intended or nice-to-have enhancements for follow-up work.
|
The JSON role's listing is correct. Read-Only should be id 8, not 7. I removed the SQL code because as I remember it, this code isn't used anymore. It was initially used to seed databases early on but has no effect anymore |
What changed
This PR adds a new READ_ONLY role to the system and implements filtering logic to exclude users with this role from the user list API response. Important note: This doesn't close the task OPS-2728 yet because we have to choose an implementation for the frontend still.
backend/data_tools/data/user_data.json5
backend/data_tools/initial_data/003-role.sql & 004-role_version.sql
backend/ops_api/ops/services/users.py
get_users()service to filter out users with READ_ONLY role from the responsebackend/ops_api/tests/ops/users/test_users_get.py
Issue
#2728
How to test
Run automated tests:
cd backend/ops_api pipenv run pytest tests/ops/users/test_users_get.py::test_get_all_users -vManual verification:
docker compose up --buildA11y impact
A11Y-SUPPRESSIONmetadata (owner, expires, rationale)Screenshots
N/A — backend changes only
Definition of Done Checklist
Links
N/A