last modified added#1321
Merged
siddhant3030 merged 5 commits intomainfrom May 4, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a nullable ReportSnapshot.last_modified_by FK and migration, updates ReportService.update_snapshot to accept an OrgUser and record last_modified_by when summary changes, threads the authenticated orguser from the API into the service call, and exposes the editor’s email in snapshot view metadata and tests. ChangesReport snapshot last-modifier and update flow
Sequence DiagramsequenceDiagram
participant Client
participant API as API Endpoint
participant Service as ReportService
participant DB as Database
Client->>API: PATCH /snapshot/{id} (payload with summary)
API->>API: Authenticate -> obtain orguser
API->>Service: update_snapshot(snapshot_id, payload, orguser)
Service->>DB: SELECT snapshot (select_related created_by, last_modified_by)
DB-->>Service: ReportSnapshot
alt summary changed
Service->>Service: snapshot.summary = payload.summary
Service->>Service: snapshot.last_modified_by = orguser
Service->>Service: snapshot.updated_at = now
Service->>DB: SAVE snapshot (summary, last_modified_by, updated_at)
DB-->>Service: Persisted
end
Service->>Service: Build view data (include last_modified_by.user.email or null)
Service-->>API: Updated snapshot + metadata
API-->>Client: 200 OK with snapshot and metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
=======================================
Coverage 58.42% 58.43%
=======================================
Files 132 132
Lines 15653 15654 +1
=======================================
+ Hits 9146 9147 +1
Misses 6507 6507 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/core/reports/report_service.py`:
- Around line 549-553: The code unconditionally sets snapshot.last_modified_by
whenever data.summary is present, which can overwrite attribution even if the
summary is unchanged; change the logic in report_service.py so you first compare
the incoming data.summary with snapshot.summary (handling None safely) and only
assign snapshot.summary, snapshot.last_modified_by and call
snapshot.save(update_fields=["summary","last_modified_by","updated_at"]) when
the values differ; keep the existing behavior of returning snapshot otherwise.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7df436c-6d8f-4996-8e27-083e0e9727f6
📒 Files selected for processing (6)
ddpui/api/report_api.pyddpui/core/reports/report_service.pyddpui/migrations/0157_add_last_modified_by_to_report_snapshot.pyddpui/migrations/0158_merge_20260415_1814.pyddpui/models/report.pyddpui/tests/core/reports/test_report_service.py
Comment on lines
549
to
553
| if data.summary is not None: | ||
| snapshot.summary = data.summary | ||
| snapshot.save(update_fields=["summary"]) | ||
| snapshot.last_modified_by = orguser | ||
| snapshot.save(update_fields=["summary", "last_modified_by", "updated_at"]) | ||
| return snapshot |
There was a problem hiding this comment.
Only set last_modified_by when summary actually changes.
Current logic updates attribution whenever summary is present, even if unchanged, which can corrupt audit intent.
Suggested fix
- if data.summary is not None:
+ if data.summary is not None and data.summary != snapshot.summary:
snapshot.summary = data.summary
snapshot.last_modified_by = orguser
snapshot.save(update_fields=["summary", "last_modified_by", "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.
Suggested change
| if data.summary is not None: | |
| snapshot.summary = data.summary | |
| snapshot.save(update_fields=["summary"]) | |
| snapshot.last_modified_by = orguser | |
| snapshot.save(update_fields=["summary", "last_modified_by", "updated_at"]) | |
| return snapshot | |
| if data.summary is not None and data.summary != snapshot.summary: | |
| snapshot.summary = data.summary | |
| snapshot.last_modified_by = orguser | |
| snapshot.save(update_fields=["summary", "last_modified_by", "updated_at"]) | |
| return snapshot |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddpui/core/reports/report_service.py` around lines 549 - 553, The code
unconditionally sets snapshot.last_modified_by whenever data.summary is present,
which can overwrite attribution even if the summary is unchanged; change the
logic in report_service.py so you first compare the incoming data.summary with
snapshot.summary (handling None safely) and only assign snapshot.summary,
snapshot.last_modified_by and call
snapshot.save(update_fields=["summary","last_modified_by","updated_at"]) when
the values differ; keep the existing behavior of returning snapshot otherwise.
siddhant3030
force-pushed
the
feature/last_modified_in_report
branch
from
8e26715 to
de105ad
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ddpui/core/reports/report_service.py (1)
546-550:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent audit attribution changes on no-op summary updates.
Line 546 still updates
last_modified_bywheneversummaryis present, even when the incoming text equals the existing summary. That makes idempotent requests look like real edits.Proposed fix
- if data.summary is not None: + if data.summary is not None and data.summary != snapshot.summary: snapshot.summary = data.summary snapshot.last_modified_by = orguser snapshot.save(update_fields=["summary", "last_modified_by", "updated_at"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/reports/report_service.py` around lines 546 - 550, The code unconditionally sets snapshot.last_modified_by and saves audit fields whenever data.summary is present, even if data.summary equals snapshot.summary; change the logic in report_service.py so you first compare snapshot.summary and data.summary and only assign snapshot.summary = data.summary, set snapshot.last_modified_by = orguser, and include "last_modified_by" in the save(update_fields=[...]) list when the summary actually changed; if the incoming summary is identical, do not modify last_modified_by or include it in update_fields (only touch "updated_at"/others as appropriate) and still return snapshot.
🧹 Nitpick comments (1)
ddpui/tests/core/reports/test_report_service.py (1)
513-560: ⚡ Quick winAdd coverage for unchanged-summary updates.
Current cases cover
summary=Noneand changed summaries, but notsummaryset to the same existing value. Add a test assertinglast_modified_byis not rewritten on no-op text updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/tests/core/reports/test_report_service.py` around lines 513 - 560, Add a test that verifies ReportService.update_snapshot does not update last_modified_by when the summary provided equals the existing summary: set sample_snapshot.summary to a specific string and save, ensure sample_snapshot.last_modified_by is None (or capture current value), call ReportService.update_snapshot(sample_snapshot.id, SnapshotUpdate(summary="same value"), orguser) with the exact same text, and assert that last_modified_by remains unchanged (still None or equal to the captured value); reference ReportService.update_snapshot, SnapshotUpdate, sample_snapshot and orguser to locate where to add this new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ddpui/core/reports/report_service.py`:
- Around line 546-550: The code unconditionally sets snapshot.last_modified_by
and saves audit fields whenever data.summary is present, even if data.summary
equals snapshot.summary; change the logic in report_service.py so you first
compare snapshot.summary and data.summary and only assign snapshot.summary =
data.summary, set snapshot.last_modified_by = orguser, and include
"last_modified_by" in the save(update_fields=[...]) list when the summary
actually changed; if the incoming summary is identical, do not modify
last_modified_by or include it in update_fields (only touch "updated_at"/others
as appropriate) and still return snapshot.
---
Nitpick comments:
In `@ddpui/tests/core/reports/test_report_service.py`:
- Around line 513-560: Add a test that verifies ReportService.update_snapshot
does not update last_modified_by when the summary provided equals the existing
summary: set sample_snapshot.summary to a specific string and save, ensure
sample_snapshot.last_modified_by is None (or capture current value), call
ReportService.update_snapshot(sample_snapshot.id, SnapshotUpdate(summary="same
value"), orguser) with the exact same text, and assert that last_modified_by
remains unchanged (still None or equal to the captured value); reference
ReportService.update_snapshot, SnapshotUpdate, sample_snapshot and orguser to
locate where to add this new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 795453c6-f0ce-4071-8879-44448ccf8e3e
📒 Files selected for processing (6)
ddpui/api/report_api.pyddpui/core/reports/report_service.pyddpui/migrations/0157_add_last_modified_by_to_report_snapshot.pyddpui/migrations/0158_merge_20260415_1814.pyddpui/models/report.pyddpui/tests/core/reports/test_report_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ddpui/api/report_api.py
- ddpui/models/report.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ddpui/core/reports/report_service.py (1)
547-550:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly stamp
last_modified_bywhen the summary actually changes.Line 547-550 still overwrites attribution for no-op updates, which muddies audit history. Compare against
snapshot.summarybefore assigning.Suggested fix
- if data.summary is not None: + if data.summary is not None and data.summary != snapshot.summary: snapshot.summary = data.summary snapshot.last_modified_by = orguser snapshot.save(update_fields=["summary", "last_modified_by", "updated_at"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/core/reports/report_service.py` around lines 547 - 550, The code currently sets snapshot.last_modified_by and saves even when data.summary equals snapshot.summary; change the logic in report_service.py to compare data.summary against snapshot.summary and only assign snapshot.summary = data.summary, set snapshot.last_modified_by = orguser, and call snapshot.save(update_fields=[...]) when the value actually differs (otherwise do nothing), ensuring update_fields still include "summary", "last_modified_by", and "updated_at" for the real-change branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ddpui/core/reports/report_service.py`:
- Around line 547-550: The code currently sets snapshot.last_modified_by and
saves even when data.summary equals snapshot.summary; change the logic in
report_service.py to compare data.summary against snapshot.summary and only
assign snapshot.summary = data.summary, set snapshot.last_modified_by = orguser,
and call snapshot.save(update_fields=[...]) when the value actually differs
(otherwise do nothing), ensuring update_fields still include "summary",
"last_modified_by", and "updated_at" for the real-change branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a625472d-74d9-4151-b0c4-61f4690bfb13
📒 Files selected for processing (3)
ddpui/api/report_api.pyddpui/core/reports/report_service.pyddpui/models/report.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ddpui/api/report_api.py
- ddpui/models/report.py
Resolves conflicting leaf nodes between 0158_allow_null_period_end_on_snapshot (from main) and 0158_merge_20260415_1814 (last_modified_by feature). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
siddhant3030
force-pushed
the
feature/last_modified_in_report
branch
from
7884260 to
60ef9cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit