fix: ebios workshop 3 invalid sort by entities #2646
Conversation
WalkthroughAdds a reusable CustomOrderingFilter that requires an ordering_mapping and remaps ordering terms (preserving direction); applies it as UserGroupOrderingFilter and StakeholderOrderingFilter, and wires StakeholderOrderingFilter plus DjangoFilterBackend and SearchFilter into StakeholderViewSet. Also removes path-truncation logic from UserGroupNameDisplay.svelte to always render full path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant ViewSet as StakeholderViewSet
participant DFB as DjangoFilterBackend
participant SFB as SearchFilter
participant OF as StakeholderOrderingFilter
participant ORM as QuerySet/DB
Client->>ViewSet: GET /stakeholders?search=...&ordering=entity
ViewSet->>DFB: apply filters (query params)
DFB-->>ViewSet: filtered queryset
ViewSet->>SFB: apply search
SFB-->>ViewSet: searched queryset
ViewSet->>OF: get_ordering(request, queryset, view)
note right of OF #D6EAF8: Map "entity" -> "entity__name"\nPreserve direction (e.g., -entity)
OF-->>ViewSet: ordering=["entity__name"]
ViewSet->>ORM: order_by(entity__name)
ORM-->>ViewSet: ordered results
ViewSet-->>Client: 200 OK (paginated list)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/views.py(2 hunks)backend/ebios_rm/views.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/ebios_rm/views.py (1)
backend/core/views.py (2)
GenericFilterSet(160-225)CustomOrderingFilter(228-261)
🔇 Additional comments (3)
backend/ebios_rm/views.py (3)
3-7: LGTM!The import of
CustomOrderingFilteris correctly added and necessary for the newStakeholderOrderingFilterimplementation.
27-28: LGTM!The imports for
filtersandDjangoFilterBackendare correctly added and necessary for the filter backends configuration.
214-216: LGTM!The
StakeholderOrderingFiltercorrectly extendsCustomOrderingFilterand defines the requiredordering_mappingattribute. The mapping of"entity"to"entity__name"directly addresses the sorting issue described in the PR, ensuring entities are sorted by name rather than ID.
| filter_backends = [ | ||
| DjangoFilterBackend, | ||
| filters.SearchFilter, | ||
| StakeholderOrderingFilter, | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Good fix for the entity sorting issue.
The filter_backends configuration correctly integrates the custom ordering filter to fix the entity sorting behavior.
Consider adding explicit configuration for better clarity:
filter_backends = [
DjangoFilterBackend,
filters.SearchFilter,
StakeholderOrderingFilter,
]
+ ordering_fields = ["entity", "created_at", "updated_at"]
+ search_fields = ["entity__name"] # If search functionality is desiredRun the following script to verify the entity field relationship and test the fix:
🏁 Script executed:
#!/bin/bash
# Description: Verify Stakeholder model has entity FK with name field
# Find the Stakeholder model definition
ast-grep --pattern $'class Stakeholder($$$):
$$$'
# Check for entity field usage in Stakeholder
rg -A 5 -B 2 "entity.*=" backend/ebios_rm/models.py
# Verify entity is used in related queries
rg "entity__name" backend/Length of output: 9180
🏁 Script executed:
#!/bin/bash
echo "=== StakeholderViewSet Definition ==="
rg -nP 'class\s+StakeholderViewSet' -C5 backend/ebios_rm/views.py
echo
echo "=== search_fields in StakeholderViewSet ==="
rg -nC3 'search_fields' backend/ebios_rm/views.py
echo
echo "=== filterset_fields or filterset_class in StakeholderViewSet ==="
rg -nC3 'filterset_' backend/ebios_rm/views.py
echo
echo "=== ordering_fields in StakeholderViewSet ==="
rg -nC3 'ordering_fields' backend/ebios_rm/views.pyLength of output: 2797
Specify search_fields for SearchFilter and restrict ordering_fields
The filters.SearchFilter will error without search_fields, so add:
class StakeholderViewSet(BaseModelViewSet):
filter_backends = [
DjangoFilterBackend,
filters.SearchFilter,
StakeholderOrderingFilter,
]
+ search_fields = ['entity__name']
+ ordering_fields = ['entity', 'created_at', 'updated_at']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter_backends = [ | |
| DjangoFilterBackend, | |
| filters.SearchFilter, | |
| StakeholderOrderingFilter, | |
| ] | |
| filter_backends = [ | |
| DjangoFilterBackend, | |
| filters.SearchFilter, | |
| StakeholderOrderingFilter, | |
| ] | |
| search_fields = ['entity__name'] | |
| ordering_fields = ['entity', 'created_at', 'updated_at'] |
🤖 Prompt for AI Agents
In backend/ebios_rm/views.py around lines 223 to 227, the SearchFilter is
configured but no search_fields are provided (which will raise an error) and
ordering is unconstrained; add a search_fields attribute listing the text fields
you want searchable (e.g., ['name', 'description', 'email'] or the actual model
field names used here) and add an ordering_fields attribute that restricts
allowed ordering keys (e.g., ['id', 'name', 'created_at'] or the specific fields
on the view's queryset). Place both attributes on the view class alongside
filter_backends.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
228-263: Solid ordering remap; harden subclass checkGreat reusable filter. Replace assert with a real exception and accept kwargs in init_subclass to be robust in all runtimes.
Apply this diff:
- def __init_subclass__(cls): - assert hasattr(cls, "ordering_mapping"), ( - f"Subclasses of {cls.__name__} must define the ordering_mapping attribute." - ) + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + if not hasattr(cls, "ordering_mapping"): + raise TypeError( + f"{cls.__name__} must define class attribute 'ordering_mapping'" + )Also consider extracting this class into a shared filters module for reuse across apps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
🔇 Additional comments (2)
backend/core/views.py (2)
16-16: Add Optional import — OKType annotation update looks good.
3213-3214: UserGroupOrderingFilter mapping looks correctMapping localization_dict → folder__name matches the view’s ordering_fields and preserves direction. LGTM.
This reverts commit afcccb0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/views.py (1)
228-263: Solid remapped ordering; replace assert in init_subclass with explicit errorLogic correctly preserves direction and falls back when unmapped. Prefer raising a concrete exception (and calling super) over assert, which can be disabled with -O.
Apply this diff:
- def __init_subclass__(cls): - assert hasattr(cls, "ordering_mapping"), ( - f"Subclasses of {cls.__name__} must define the ordering_mapping attribute." - ) + def __init_subclass__(cls) -> None: + super().__init_subclass__() + if not hasattr(cls, "ordering_mapping"): + raise TypeError( + f"{cls.__name__} must define class attribute 'ordering_mapping'." + )Optional: consider typing ordering_mapping as Mapping[str, str] for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/views.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
🔇 Additional comments (1)
backend/core/views.py (1)
16-16: Import update looks goodOptional is needed for the new get_ordering signature; no issues.
3 participants
In ebios/workshop 3/activity 1, sorting by entity sorts by entity.id, not entity.name
Summary by CodeRabbit
New Features
Bug Fixes / Behavior