Feature/55 order of items on sidebar incorrect by zgypa · Pull Request #69 · open-ortho/edu.case.BFD9000

zgypa · 2026-03-02T23:35:45Z

Feature: Sidebar Order Fix & Role-Based API Permissions (Fixes #55, #61), #68

Summary

This PR addresses sidebar navigation ordering, introduces a robust role-based permission system for Subjects and Encounters, and documents both the policy and group setup procedures. This enhances both the UI organization and backend security. A set of new/expanded tests and documentation are included.

Changelog / Main Changes

1. Sidebar Navigation/UI

Reorders sidebar in base_index.html for intuitive workflow: Subjects, Encounters, Records, Scan.
"Subjects" is now the landing section, as per workflow priorities.
"Add Record" button moved for clarity.

2. Permission System Overhaul & Curator Role

(Also addresses #61 — disables subject and encounter creation for regular users)
Introduces CuratorOrSuperuserEditPermission and RecordPermission classes in archive/permissions.py:
- Curator: Can add/change (but not delete) Subject/Encounter.
- Superuser: All rights, including deletes and user/group management.
- Regular users: Can create/update records only (not Subjects/Encounters).
Command setup_curator_group: Bootstraps "Curator" group permissions. Removes delete/user-management from Curators automatically for auditability and safety.
Adds tests: test_role_permissions.py and expanded API permission tests for User, Curator, Superuser boundaries.

3. Documentation & Maintenance

Adds detailed permission reference and use-cases to bfd9000_web/docs/permissions.md as well as updates to docs/use_cases.md.
README and docker.md updated for troubleshooting Docker volume permissions (clarifies media handling for non-root containers).

Implementation Details

Permission checks leverage Django groups and DRF custom permission classes.
New management command safe to rerun and acts as source of truth.
All permission checks and policy reflected in expanded automated test suite for API boundaries.
UI and navigation closely match new roles and intended workflow.

How to Test

Run python manage.py setup_curator_group to update/create the Curator group.
Log in as users of each role; verify allowed/denied actions align with the permissions matrix in permissions.md.
Run the full test suite; all new and existing tests should pass.
For Docker: see docker.md if using named volumes for file uploads.

Notes for Reviewers

No legacy permissions or UI remain.
See bfd9000_web/docs/permissions.md for matrix and policy rationale.
If the Curator role needs expanded rights, update both the setup script and docs to ensure auditability.

Fixes #61

…egular-users' into develop

Fixes #55

zgypa

Reviewer: claude-sonnet-4.6

Overall the permission system is well-structured and the tests are thorough. A few issues worth addressing before merge.

zgypa · 2026-03-02T23:47:58Z

+
+def logout_view(request: HttpRequest) -> HttpResponse:
+    """Log out on POST, redirect on GET."""
+    if request.method == 'POST':


claude-sonnet-4.6 — Bug (Low): GET /logout/ silently leaves the user logged in

The custom logout_view only calls logout() on POST. A GET request redirects to login but the user's session remains active. Any bookmark, prefetch, or direct link to /logout/ will silently fail to log the user out.

The test test_logout_get_redirects_without_error passes because it only checks the redirect status code, not whether the session was terminated.

Since the template was changed to <form method="post">, the GET path is no longer reachable from the UI. But the endpoint is still accessible. Consider either calling logout() unconditionally, or returning 405 for GET.

Agreed, and fix tests to catch this.

claude-sonnet-4.6 — Fixed in commit 168becdab5e0427a07f1e481216b109c3bb329b0. logout() is now called unconditionally on all request methods. The test test_logout_get_redirects_without_error now also asserts _auth_user_id is absent from the session after a GET.

zgypa · 2026-03-02T23:47:58Z

+    """Require model add/change perms for writes and auth for reads."""
+
+    def has_permission(self, request: Request, view: Any):  # pyright: ignore[reportIncompatibleMethodOverride]
+        if not request.user or not request.user.is_authenticated:


claude-sonnet-4.6 — Note: view.queryset assumption silently denies writes when queryset is not a class attribute

CuratorOrSuperuserEditPermission resolves the model via getattr(view, 'queryset', None). If a ViewSet only defines get_queryset() without a class-level queryset, this returns None, model is None, and the permission returns False for any non-safe, non-DELETE method—blocking writes even for valid curators with no error.

This does not affect current consumers (SubjectViewSet, EncounterViewSet both have class-level queryset), but it is a silent footgun for any future ViewSet that only overrides get_queryset().

OK, if fix is easy, apply.

claude-sonnet-4.6 — Fixed in commit de1d5529f2a1a3ed0ffe4b2b7cd90a6cd9ee3ee7. The permission class now falls back to calling view.get_queryset() when no class-level queryset attribute is present, preventing silent write denials for future ViewSets.

zgypa · 2026-03-02T23:47:58Z

                    </svg>
-                    Home
+                    Subjects
                </a>


claude-sonnet-4.6 — Intentional or accidental: "Scan" sidebar entry removed

The Scan nav item (data-nav="scan") was removed. The scan workflow is still reachable via the "Add Record" button added to records.html, but there is no longer a direct top-level sidebar link. If a user navigates directly to /scan/, the active-nav highlight JS will find no data-nav="scan" element and silently produce no highlight.

If this removal is intentional (scan is only accessible via Records), that should be noted. If accidental, the item needs to be restored.

Its intentional. Make a note of it.

claude-sonnet-4.6 — Noted in commit c191375d1c7a1310742a5ec160b073b85d7c5a47. Added an HTML comment in the sidebar template confirming the Scan nav item removal is intentional and that scanning is accessible via the "Add Record" button in the Records view.

zgypa · 2026-03-02T23:47:58Z

+        for perm in required_perms:
+            group.permissions.add(perm)
+
+        forbidden_perms = Permission.objects.filter(


claude-sonnet-4.6 — Note: command enforces only a subset of the documented permission matrix

The command removes delete_* and auth.* permissions, but does not enforce the full documented matrix on reruns. For example, if someone manually grants the Curator group add_record via the admin, rerunning this command will not remove it (since only delete/auth perms are in forbidden_perms). According to permissions.md, Curators can create/update records, so this is currently correct behavior—but the command's docstring says "Safe to rerun" without clarifying that it only enforces the no-delete, no-auth-management constraints, not the complete matrix.

So you want to fix the docstring then? If this is compliant, to permissions.md, then we are good. You can fix docstring.

claude-sonnet-4.6 — Fixed in commit 2c0a3cb826076edc8485a29ca41aa76e7a4610f0. The help text now explicitly states that reruns enforce only the no-delete/no-auth-management constraints and do not remove other manually granted permissions, with a pointer to docs/permissions.md.

zgypa · 2026-03-02T23:47:59Z

+
+        response = self.client.get(reverse("logout"))
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, reverse("login"))


claude-sonnet-4.6 — Test gap: GET logout test does not assert session state

test_logout_get_redirects_without_error only asserts a 302 redirect. The test name implies "no error" but does not verify whether the user remains logged in after a GET. Adding self.assertIn('_auth_user_id', self.client.session) would document the intended behavior (user stays logged in on GET) and catch a regression if the GET path is later changed to call logout().

OK, assert the session state.

claude-sonnet-4.6 — Covered in commit 168becdab5e0427a07f1e481216b109c3bb329b0 (same as comment 1). The test now calls self.assertNotIn('_auth_user_id', self.client.session) after the GET request, asserting the session is cleared.

aspiringLich · 2026-03-03T00:21:30Z

                </a>
            </li>
-            <li>
-                <a href="{% url 'archive:subjects' %}" data-nav="subjects">
-                    <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="currentColor" class="size-6">
-                      <path stroke-linecap="round" stroke-linejoin="round" d="M15.75 6a3.75 3.75 0 1 1-7.5 0 3.75 3.75 0 0 1 7.5 0ZM4.501 20.118a7.5 7.5 0 0 1 14.998 0A17.933 17.933 0 0 1 12 21.75c-2.676 0-5.216-.584-7.499-1.632Z" />
-                    </svg>
-                    Subjects
-                </a>
-            </li>
-            <li>
-                <a href="{% url 'archive:scan' %}" data-nav="scan">
-                    <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="currentColor" class="size-6">
-                      <path stroke-linecap="round" stroke-linejoin="round" d="M6.429 9.75 2.25 12l4.179 2.25m0-4.5 5.571 3 5.571-3m-11.142 0L2.25 7.5 12 2.25l9.75 5.25-4.179 2.25m0 0L21.75 12l-4.179 2.25m0 0 4.179 2.25L12 21.75 2.25 16.5l4.179-2.25m11.142 0-5.571 3-5.571-3" />
-                    </svg>
-                    Scan
-                </a>
-            </li>


Removing scan as a sidebar item makes sense but I think the distinction between "creating a new subject / encounter in a form" and "actually having to scan it in" was valuable. Currently, scanning remains at /scan while creating a new subject / encounter are at /subject/create and /encounter/create.

I propose two changes (I'll put review comments):

path of scanning page: /scan → /records/scan

text on button link: "Create New Record" → "Scan New Record"

Also if its under /records/scan the sidebar item for /records will actually light up.

@aspiringLich : why not just make this the /records/create API then?

claude-sonnet-4.6 — Implemented in commit 1e6f185. The scan page has been moved from /scan/ to /records/create/ with URL name record_create, consistent with subject_create and encounter_create. The sidebar Records item now lights up automatically when on that page.

aspiringLich · 2026-03-03T00:22:10Z

        </table>
    </div>
    <div class="bg-base-300 p-2 flex items-center gap-3">
+        <a href="{% url 'archive:scan' %}" class="btn btn-primary rounded-full">Add Record</a>


Change to "Scan New Record" maybe?

claude-sonnet-4.6 — Done in commit 1e6f185. Button text updated from "Add Record" to "Scan New Record".

No, that's not a good idea. Revert to Add Record. This is an Add Record View, which happens to also have Scan and import file capabilities. Technically is should only have to let you select the path of the file, so add Record is fine. What would make sense, though, would be to separate the flow: first scan/import file above, then add the recrod details. This view shold be completely re-written: the original deisgn was workflow oriented, without giving the user options, step by step, wizard like. But that will be for another issue.

claude-sonnet-4.6 — Reverted to "Add Record" in commit 9652ecc. The wizard-like redesign of the Add Record view has been captured as issue #73.

aspiringLich · 2026-03-03T00:26:00Z

Everything else here makes sense to me. There's tests and everything so I'm going to assume it works as described.

…eared in test

…when queryset is not a class attribute

…cs accurately

…edentials_round_trip

zgypa · 2026-03-03T13:14:30Z

claude-sonnet-4.6 — Fixed pre-existing test failure test_endpoint_credentials_round_trip in commit fb4c52a. The test was failing with ImproperlyConfigured: ENDPOINT_CREDENTIALS_KEY must be set in settings because no Fernet key was provided during test execution. Fixed by decorating the test with @override_settings(ENDPOINT_CREDENTIALS_KEY=...) using a generated test-only Fernet key. All 70 archive tests now pass.

…r consistency with subject_create and encounter_create

zgypa · 2026-03-03T13:52:23Z

No, actually its merged with commits. I merged with gitflow.

zgypa added 5 commits March 2, 2026 09:13

Fix Disable Subject and Encounter creation for regular users

aa24529

Fixes #61

Fix Broken Logout

5373f3e

Merge branch 'feature/61-disable-subject-and-encounter-creation-for-r…

236e533

…egular-users' into develop

Fix Order of Items on sidebar incorrect

01db1cc

Fixes #55

Add Docker troubleshooting notes for media volume permissions

5da1a12

zgypa self-assigned this Mar 2, 2026

zgypa requested a review from aspiringLich March 2, 2026 23:43

This was linked to issues Mar 2, 2026

Order of Items on sidebar incorrect #55

Closed

Disable Subject and Encounter creation for regular users #61

Closed

zgypa commented Mar 2, 2026

View reviewed changes

zgypa linked an issue Mar 2, 2026 that may be closed by this pull request

Missing favicon.ico #68

Closed

aspiringLich reviewed Mar 3, 2026

View reviewed changes

zgypa added 5 commits March 3, 2026 08:08

fix: call logout() unconditionally on GET and POST; assert session cl…

168becd

…eared in test

fix: fall back to get_queryset() in CuratorOrSuperuserEditPermission …

de1d552

…when queryset is not a class attribute

docs: note intentional removal of Scan sidebar item in template comment

c191375

docs: clarify setup_curator_group help text to describe rerun semanti…

2c0a3cb

…cs accurately

fix: supply test Fernet key via override_settings in test_endpoint_cr…

fb4c52a

…edentials_round_trip

zgypa added 3 commits March 3, 2026 08:31

refactor: rename /scan/ to /records/create/ (name='record_create') fo…

1e6f185

…r consistency with subject_create and encounter_create

revert: restore 'Add Record' button text on records view

9652ecc

fix: reorder record detail URL path for consistency in routing

87ec389

zgypa closed this Mar 3, 2026

zgypa deleted the feature/55-order-of-items-on-sidebar-incorrect branch March 3, 2026 13:51

Conversation

zgypa commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Feature: Sidebar Order Fix & Role-Based API Permissions (Fixes #55, #61), #68

Summary

Changelog / Main Changes

1. Sidebar Navigation/UI

2. Permission System Overhaul & Curator Role

3. Documentation & Maintenance

Implementation Details

How to Test

Notes for Reviewers

Uh oh!

zgypa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zgypa Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aspiringLich commented Mar 3, 2026

Uh oh!

zgypa commented Mar 3, 2026

Uh oh!

zgypa commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zgypa commented Mar 2, 2026 •

edited

Loading

zgypa Mar 3, 2026 •

edited

Loading