Skip to content

fix: ebios workshop 3 sort by entity name is broken#4081

Draft
Axxiar wants to merge 20 commits into
mainfrom
CA-1207-invalid-entity-sort-workshop3
Draft

fix: ebios workshop 3 sort by entity name is broken#4081
Axxiar wants to merge 20 commits into
mainfrom
CA-1207-invalid-entity-sort-workshop3

Conversation

@Axxiar
Copy link
Copy Markdown
Contributor

@Axxiar Axxiar commented May 5, 2026

Fixes the sort by entity in Ebios RM, workshop 3, activity 1
Introduces a new CustomOrderingFilter that goes with an ordering_mapping field that can map the api call name to it's proper model field before applying the backend ordering. Useful for ordering on more complex fields.

Summary by CodeRabbit

  • New Features

    • Enhanced search functionality for stakeholders by entity and category names
    • Added support for sorting/ordering by mapped fields (e.g., folder names)
  • Improvements

    • User group names now display complete folder paths without truncation

@Axxiar Axxiar marked this pull request as draft May 5, 2026 17:51
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 5, 2026

📝 Walkthrough

Walkthrough

This PR refactors the ordering filter mechanism across the backend, replacing a specialized folder filter with a generalized CustomOrderingFilter that translates requested ordering fields to ORM paths via a configurable ordering_mapping dictionary. The frontend simplifies folder path display by removing truncation logic.

Changes

Ordering Filter Refactoring & Display Simplification

Layer / File(s) Summary
Core Infrastructure
backend/core/views.py
Introduces CustomOrderingFilter class that remaps ordering fields using ordering_mapping (merged across view MRO). Adds Optional to typing imports. Replaces FolderOrderingFilter in BaseModelViewSet.filter_backends and defines BaseModelViewSet.ordering_mapping = {"folder": "folder__name"}.
Viewset Configuration
backend/ebios_rm/views.py
Updates StakeholderViewSet with search_fields = ["entity__name", "category__name"] and ordering_mapping = {"entity": "entity__name"} to support searching and ordering by related fields. Reformats core.views import to multi-line parenthesized style.
Frontend Display
frontend/src/lib/components/ModelTable/field/UserGroupNameDisplay.svelte
Removes path truncation logic (MAX_VISIBLE, LAST_VISIBLE, "..." insertion) from fullPath computation. Now always joins the entire folder path sequence with " / " to display complete paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A filter so clever, now reusable and free,
Maps ordering fields with such flexibility!
No more truncated paths—the full chain we see,
Stakeholders ordered by entity, naturally! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title specifically describes fixing a broken sort-by-entity-name feature in workshop 3, which directly matches the core changes addressing ordering mapping for StakeholderViewSet and the new CustomOrderingFilter.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch CA-1207-invalid-entity-sort-workshop3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
backend/core/views.py (1)

743-743: 💤 Low value

lstrip("-") strips all leading dashes; prefer removeprefix("-").

order.lstrip("-") removes every leading "-" character, so a hypothetical "--field" becomes "field" instead of "-field". While DRF only produces single-dash prefixes in practice, str.removeprefix("-") (Python 3.9+, available on this project's 3.14 baseline) is semantically exact.

♻️ Proposed refactor
-            field_name = order.lstrip("-")
+            field_name = order.removeprefix("-")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/core/views.py` at line 743, Replace the use of str.lstrip in the
sorting/ordering logic so only a single leading dash is removed; in the code
that sets field_name = order.lstrip("-") (in backend/core/views.py), change to
use order.removeprefix("-") to remove exactly one leading "-" and preserve
strings like "--field" as "-field"; ensure the surrounding logic that checks for
descending order still inspects the original order string and that this change
is compatible with the project's Python 3.14 baseline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/core/views.py`:
- Around line 714-723: The docstring and class attribute are misleading because
CustomOrderingFilter.get_ordering inspects the view class MRO
(type(view).__mro__), so ordering_mapping must be defined on the view, not on a
filter subclass; update the docstring example to show placing ordering_mapping
on the view class (not on CustomOrderingFilter), remove or deprecate the
ordering_mapping: dict[str, str] = {} attribute from CustomOrderingFilter, and
ensure any tests/examples reference the view's ordering_mapping rather than a
filter subclass; keep references to CustomOrderingFilter and get_ordering so
reviewers can locate the change.

---

Nitpick comments:
In `@backend/core/views.py`:
- Line 743: Replace the use of str.lstrip in the sorting/ordering logic so only
a single leading dash is removed; in the code that sets field_name =
order.lstrip("-") (in backend/core/views.py), change to use
order.removeprefix("-") to remove exactly one leading "-" and preserve strings
like "--field" as "-field"; ensure the surrounding logic that checks for
descending order still inspects the original order string and that this change
is compatible with the project's Python 3.14 baseline.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5c449377-fab9-4514-a19b-276af3c13efc

📥 Commits

Reviewing files that changed from the base of the PR and between abe2fb9 and a0a9b98.

📒 Files selected for processing (3)
  • backend/core/views.py
  • backend/ebios_rm/views.py
  • frontend/src/lib/components/ModelTable/field/UserGroupNameDisplay.svelte

Comment thread backend/core/views.py
@Axxiar
Copy link
Copy Markdown
Contributor Author

Axxiar commented May 6, 2026

This approach will be abandoned, traceability problem. The suggested approach would be to keep a custom OrderingFilter that checks for foreign keys if a field name exists (fallback to email since User() doesn't have a name field). If so, apply ordering on foreignkey__name.

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