Lint fixes #338
Lint fixes #338
Conversation
matheusysd
requested review from
Hercilio1,
nossila and
vitorguima
as code owners
WalkthroughAdds a Django migration that removes two pgtrigger triggers, tightens a GraphQL resolver to require the requesting user equal the target user before counting unread notifications, reformats a test’s multi-context with-statement (stylistic), and exposes Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant GraphQL as Notifications Resolver
participant App as Application / DB
Client->>GraphQL: query notificationsUnreadCount(targetUserId)
GraphQL->>GraphQL: check info.context.user.is_authenticated
alt authenticated
GraphQL->>GraphQL: check info.context.user == self (target user)
alt same user
GraphQL->>App: count unread notifications for target user
App-->>GraphQL: unreadCount
GraphQL-->>Client: unreadCount
else not same user
GraphQL-->>Client: 0 (or no access)
end
else not authenticated
GraphQL-->>Client: 0 (or no access)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Files/areas to pay extra attention to:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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). (2)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
|
Wait to merge this PR we will add more issues we found on our project
|
![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)
baseapp_chats/migrations/0011_remove_message_create_message_status_and_more.py(1 hunks)baseapp_notifications/graphql/object_types.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
baseapp_chats/migrations/0011_remove_message_create_message_status_and_more.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-22: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (2)
- GitHub Check: tests (3.12, 4.2.15)
- GitHub Check: tests (3.12, 5.2.1)
🔇 Additional comments (1)
baseapp_chats/migrations/0011_remove_message_create_message_status_and_more.py (1)
7-22: Verify that trigger functionality is preserved in application code.The migration correctly removes two database triggers, but this is a significant functional change—not a lint fix. The removed triggers likely automated:
create_message_status: Auto-creation of message status recordsincrement_unread_count: Maintaining unread message countsEnsure this logic is now handled in application code (e.g., model signals, GraphQL mutations, or service layer).
Run the following script to verify trigger definitions and find replacement logic:
#!/bin/bash # Find the original trigger definitions and verify replacement logic exists echo "=== Searching for trigger definitions ===" rg -n "create_message_status|increment_unread_count" --type py -C 3 echo -e "\n=== Searching for message status creation logic ===" rg -n "MessageStatus.*create|MessageStatus.*save" --type py -C 2 echo -e "\n=== Searching for unread count logic ===" rg -n "unread.*count|increment.*unread" --type py -C 2 echo -e "\n=== Checking for pgtrigger decorators in models ===" ast-grep --pattern $'@pgtrigger.$$$ class $_ { $$$ }'
Note on static analysis warnings: The Ruff RUF012 warnings about
ClassVarannotations are false positives. Django's migration framework intentionally usesdependenciesandoperationsas plain class attributes—this is the standard pattern across all Django projects.
| def resolve_notifications_unread_count(self, info): | ||
| if self.is_authenticated: | ||
| if info.context.user.is_authenticated and info.context.user == self: | ||
| return Notification.objects.filter(recipient=self, unread=True).count() | ||
| return 0 |
There was a problem hiding this comment.
Good security improvement, but PR title is misleading.
This authorization check properly restricts unread notification count queries to authenticated users viewing their own data, consistent with the other resolvers in this interface (lines 36, 43, 48).
However, the PR title "Lint fixes" is misleading. This is a behavioral change that tightens security by adding an authorization guard that was previously missing. Consider updating the PR description to explicitly document this security improvement rather than categorizing it as just a lint fix.
🤖 Prompt for AI Agents
In baseapp_notifications/graphql/object_types.py around lines 30 to 33, the
change adds an authorization guard to restrict unread notification count queries
to authenticated users viewing their own data (a behavioral/security change) but
the PR title and description still say "Lint fixes"; update the PR title to
something like "Add authorization guard for unread notifications count" and
update the PR description to explicitly document the behavioral change and
security rationale (what was changed, why it tightens access, and any related
lines/resolvers), and mention that tests or changelog entries were updated if
applicable.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def resolve_notifications_unread_count(self, info): | ||
| if self.is_authenticated: | ||
| if info.context.user.is_authenticated and info.context.user == self: | ||
| return Notification.objects.filter(recipient=self, unread=True).count() | ||
| return 0 |
During our latest run on jenkins https://jenkins.tsl.io/job/The%20Silverlogic/job/jeteye-backend/job/JETEYE-73-rebase-with-ba/2/console we saw a lint error inside the package
`➜ jeteye-backend git:(JETEYE-73-rebase-with-ba) ✗ tsl-bac lint
Skipped 3 files
reformatted /usr/src/app/baseapp-backend/baseapp_cloudflare_stream_field/tests/test_refresh_from_cloudfare.py
All done! ✨ 🍰 ✨
1 file reformatted, 1069 files left unchanged.`
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.