fix: Audit log not displated correctly #2693
Conversation
WalkthroughAdds frontend support for many-to-many "add" audit-log entries by introducing a new M2MAdd Svelte component and updating RecursiveLogEntryChanges to emit and render 'm2m_add' entries. Adjusts backend LogEntryViewSet.get_queryset with a redundant local assignment; behavior is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AuditLogPage as AuditLog Page
participant Recursive as RecursiveLogEntryChanges.svelte
participant M2MAdd as M2MAdd.svelte
User->>AuditLogPage: Open audit-log entry
AuditLogPage->>Recursive: Provide raw change records
Note right of Recursive: Normalize changes into LogEntry union<br/>(type: 'change' or 'm2m_add')
alt Standard field change
Recursive-->>AuditLogPage: Render LogEntryChange component
else Many-to-many add
Recursive->>M2MAdd: Render with { action, field, objects }
M2MAdd-->>AuditLogPage: Display labeled list of added objects
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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). (6)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enterprise/backend/enterprise_core/views.py (1)
462-482: Remove redundant code and fix formatting.The variable
queryis assigned but never used, and the exact same queryset construction is duplicated in the return statement. This redundancy should be removed.Apply this diff to fix the issue:
def get_queryset(self): - query = LogEntry.objects.all().annotate( - folder=Lower( - Case( - When(additional_data__isnull=True, then=Value("")), - When(additional_data__folder=None, then=Value("")), - default=Cast("additional_data__folder", CharField()), - output_field=CharField(), - ) - ), - ) - return LogEntry.objects.all().annotate( folder=Lower( Case( When(additional_data__isnull=True, then=Value("")), When(additional_data__folder=None, then=Value("")), default=Cast("additional_data__folder", CharField()), output_field=CharField(), ) ), )
🧹 Nitpick comments (1)
enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/M2MAdd.svelte (1)
20-24: Handle empty objects array.The component doesn't handle the case where the
objectsarray is empty, which could result in rendering an empty list with just a plus icon.Consider adding a check:
<ul class="col-span-2 list-none"> + {#if objects.length === 0} + <li class="text-gray-500 italic">No items</li> + {:else} {#each objects as object} <li key={JSON.stringify(object)}> {object} </li> {/each} + {/if} </ul>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
enterprise/backend/enterprise_core/views.py(2 hunks)enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/M2MAdd.svelte(1 hunks)enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/RecursiveLogEntryChanges.svelte(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: Backend Linters
enterprise/backend/enterprise_core/views.py
[error] 1-1: Command failed: ruff format --check . -> 1 file would be reformatted (enterprise_core/views.py); run 'ruff format' to fix code style issues.
⏰ 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). (6)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
🔇 Additional comments (2)
enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/RecursiveLogEntryChanges.svelte (2)
164-188: LGTM: Type narrowing handled correctly.The
isNestedObjectChangecheck ensures thatchange.type === 'change'before accessingchange.beforeandchange.after, providing proper type narrowing. The nested object processing logic is sound.
189-190: LGTM: M2M add rendering path is correct.The conditional rendering for
m2m_addtype entries is properly implemented and passes the required props to the M2MAdd component.
| interface Props { | ||
| action: string; | ||
| field: string; | ||
| objects: any; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any type with proper type definition.
Using any defeats TypeScript's type safety. Based on the usage in RecursiveLogEntryChanges.svelte (line 146: objects: value.objects), define a proper type for the objects array.
Apply this diff to improve type safety:
interface Props {
action: string;
field: string;
- objects: any;
+ objects: Array<Record<string, any>>;
}📝 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.
| interface Props { | |
| action: string; | |
| field: string; | |
| objects: any; | |
| } | |
| interface Props { | |
| action: string; | |
| field: string; | |
| objects: Array<Record<string, any>>; | |
| } |
🤖 Prompt for AI Agents
In
enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/M2MAdd.svelte
around lines 4 to 8, the Props interface uses objects: any which breaks type
safety; inspect RecursiveLogEntryChanges.svelte (line 146 where objects:
value.objects is passed) to determine the actual shape (e.g., array of objects
with the specific fields used: id, name/label, maybe type or meta), declare a
proper interface/type for those object entries (or reuse an existing exported
type from the module that provides value.objects), and replace objects: any with
objects: YourObjectType[] (or the correct existing type). Export or import the
new type as needed so the component props are strongly typed.
| <dd class="text-gray-700 whitespace-pre-line grid grid-cols-4"> | ||
| <i class="fa-solid fa-plus col-span-1 pt-1" aria-hidden="true"></i> | ||
| <span class="col-span-2"> | ||
| {#each objects as object} | ||
| <li> | ||
| {object} | ||
| </li> | ||
| {/each} | ||
| </span> | ||
| </dd> |
There was a problem hiding this comment.
Fix semantic HTML and add key attribute.
The <li> elements should be wrapped in a <ul> or <ol> element for semantic correctness. Additionally, list items in Svelte loops should have a unique key for efficient rendering and to avoid potential issues.
Apply this diff to fix the issues:
<dd class="text-gray-700 whitespace-pre-line grid grid-cols-4">
<i class="fa-solid fa-plus col-span-1 pt-1" aria-hidden="true"></i>
- <span class="col-span-2">
+ <ul class="col-span-2 list-none">
{#each objects as object}
- <li>
+ <li key={JSON.stringify(object)}>
{object}
</li>
{/each}
- </span>
+ </ul>
</dd>Note: If objects have a unique identifier field (e.g., id), use that instead of JSON.stringify(object) for better performance.
📝 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.
| <dd class="text-gray-700 whitespace-pre-line grid grid-cols-4"> | |
| <i class="fa-solid fa-plus col-span-1 pt-1" aria-hidden="true"></i> | |
| <span class="col-span-2"> | |
| {#each objects as object} | |
| <li> | |
| {object} | |
| </li> | |
| {/each} | |
| </span> | |
| </dd> | |
| <dd class="text-gray-700 whitespace-pre-line grid grid-cols-4"> | |
| <i class="fa-solid fa-plus col-span-1 pt-1" aria-hidden="true"></i> | |
| <ul class="col-span-2 list-none"> | |
| {#each objects as object} | |
| <li key={JSON.stringify(object)}> | |
| {object} | |
| </li> | |
| {/each} | |
| </ul> | |
| </dd> |
🤖 Prompt for AI Agents
In
enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/M2MAdd.svelte
around lines 17 to 26, the <li> elements are not wrapped in a semantic list
container and the Svelte {#each} loop lacks a key; change the markup to wrap the
looped <li> items in a <ul> (or <ol>) and add a key to the {#each} block (e.g.,
{#each objects as object (object.id)} if objects have an id, otherwise use a
stable fallback such as (JSON.stringify(object))); ensure the list container
retains the present classes and layout and prefer a unique identifier field
instead of JSON.stringify for best performance.
| } else if (value.type === 'm2m' && value.operation === 'add') { | ||
| // objects | ||
| acc[key] = { | ||
| type: 'm2m_add', | ||
| objects: value.objects | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add runtime validation for M2M add entries.
The code assumes the backend will send data with value.type === 'm2m', value.operation === 'add', and value.objects array, but there's no validation that these properties exist or have the expected types. This could lead to runtime errors if the backend sends unexpected data.
Apply this diff to add validation:
-} else if (value.type === 'm2m' && value.operation === 'add') {
+} else if (
+ typeof value === 'object' &&
+ value !== null &&
+ value.type === 'm2m' &&
+ value.operation === 'add' &&
+ Array.isArray(value.objects)
+) {
// objects
acc[key] = {
type: 'm2m_add',
objects: value.objects
};📝 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.
| } else if (value.type === 'm2m' && value.operation === 'add') { | |
| // objects | |
| acc[key] = { | |
| type: 'm2m_add', | |
| objects: value.objects | |
| }; | |
| } else if ( | |
| typeof value === 'object' && | |
| value !== null && | |
| value.type === 'm2m' && | |
| value.operation === 'add' && | |
| Array.isArray(value.objects) | |
| ) { | |
| // objects | |
| acc[key] = { | |
| type: 'm2m_add', | |
| objects: value.objects | |
| }; |
🤖 Prompt for AI Agents
In
enterprise/frontend/src/routes/(app)/(internal)/audit-log/[id]/components/RecursiveLogEntryChanges.svelte
around lines 142 to 147, the branch that handles value.type === 'm2m' &&
value.operation === 'add' assumes value.objects exists and is an array; add
runtime validation to guard against malformed backend data by checking that
value is an object, value.objects is defined and Array.isArray(value.objects),
and that each object in the array has the expected shape (e.g., id or required
fields) before assigning acc[key]; if validation fails, set acc[key] to a safe
fallback (e.g., { type: 'm2m_add', objects: [] } or { type: 'm2m_add', objects:
null, invalid: true }) and optionally log a warning to aid debugging.
1 participant
Summary by CodeRabbit
New Features
Refactor