Open
Conversation
closes #3349 sets up back end support for following/unfollowing all questions in a project add tests accordingly
Contributor
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Frontend
participant API as API View
participant Service as Subscription Service
participant DB as Database
participant PostSub as PostSubscription
User->>Client: Click "Follow" (also follow questions)
Client->>API: POST /projects/:id/subscribe { follow_questions: true }
API->>Service: subscribe_project(project, user, follow_questions=true)
Service->>DB: Upsert ProjectSubscription (follow_questions=true)
DB-->>Service: ProjectSubscription created/updated
Service->>DB: Query project question posts
DB-->>Service: List posts
loop for each question post without existing non-global subscription
Service->>PostSub: Create default PostSubscription(s)
PostSub-->>DB: Persisted
end
Service-->>API: Success
API-->>Client: 200 { is_subscribed: true, follow_questions: true }
Client-->>User: Update UI
sequenceDiagram
participant SystemEvent as NewPostEvent
participant Service as Subscription Service
participant DB as Database
participant PostSub as PostSubscription
SystemEvent->>Service: notify_post_added_to_project(post, project)
Service->>DB: Get subscribers where follow_questions=true
DB-->>Service: List users
loop for each user
Service->>DB: Check view permission & existing non-global PostSubscription
alt eligible & not subscribed
Service->>PostSub: Create default PostSubscription(s)
PostSub-->>DB: Persisted
end
end
Service-->>SystemEvent: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ 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)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
projects/views/common.py (2)
404-407: Consider validatingfollow_questionsinput.The
follow_questionsvalue is taken directly fromrequest.datawithout validation. While Python's truthiness will work, explicit boolean validation would ensure type safety and reject unexpected input values.♻️ Proposed validation
`@api_view`(["POST"]) def project_subscribe_api_view(request: Request, pk: str): qs = get_projects_qs(user=request.user) project = get_object_or_404(qs, pk=pk) - follow_questions = request.data.get("follow_questions", False) + follow_questions = serializers.BooleanField(default=False).run_validation( + request.data.get("follow_questions", False) + ) subscribe_project( project=project, user=request.user, follow_questions=follow_questions )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/views/common.py` around lines 404 - 407, The follow_questions value taken from request.data should be explicitly validated before calling subscribe_project; update the code that reads follow_questions to accept only clear boolean types or well-defined string/number equivalents (e.g., True/False, "true"/"false", "1"/"0") and either coerce them to a bool or raise a validation error (e.g., BadRequest) for invalid inputs, then pass the validated/coerced boolean into subscribe_project(project=project, user=request.user, follow_questions=follow_questions); ensure you reference the follow_questions variable and the subscribe_project call so the change is localized.
417-420: Same validation consideration applies tounfollow_questions.For consistency with
follow_questions, consider validating this input as well.♻️ Proposed validation
`@api_view`(["POST"]) def project_unsubscribe_api_view(request: Request, pk: str): qs = get_projects_qs(user=request.user) project = get_object_or_404(qs, pk=pk) - unfollow_questions = request.data.get("unfollow_questions", False) + unfollow_questions = serializers.BooleanField(default=False).run_validation( + request.data.get("unfollow_questions", False) + ) unsubscribe_project( project=project, user=request.user, unfollow_questions=unfollow_questions )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/views/common.py` around lines 417 - 420, Validate the incoming unfollow_questions value before passing it to unsubscribe_project the same way follow_questions is validated: ensure request.data.get("unfollow_questions", False) is coerced/checked to a boolean (or parsed from strings like "true"/"false") and raise/return a 400 or default to False on invalid input; then call unsubscribe_project(project=project, user=request.user, unfollow_questions=unfollow_questions) with the sanitized boolean.projects/services/subscriptions.py (3)
111-131: Consider skippingupdate_followers_countwhen only updating an existing subscription.When updating
follow_questionson an existing subscription (Lines 116-119), the follower count hasn't changed, butupdate_followers_count()andsave()are still called on Line 127-128.♻️ Proposed optimization
`@transaction.atomic` def subscribe_project(project: Project, user: User, follow_questions: bool = False): project_subscription = ProjectSubscription.objects.filter( project=project, user=user ).first() if project_subscription: # Update follow_questions if subscription already exists project_subscription.follow_questions = follow_questions project_subscription.save() + # No need to update followers_count since subscription already existed else: ProjectSubscription.objects.create( project=project, user=user, follow_questions=follow_questions, ) + project.update_followers_count() + project.save() - project.update_followers_count() - project.save() - if follow_questions: follow_all_project_questions(project, user)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/services/subscriptions.py` around lines 111 - 131, The function subscribe_project currently always calls project.update_followers_count() and project.save() even when only updating the existing ProjectSubscription's follow_questions flag; modify subscribe_project so you detect whether you created a new subscription (e.g. by checking if project_subscription existed before) and only call project.update_followers_count() and project.save() when a new subscription is created (or when follower status actually changes), leaving the existing-update path to just update and save the ProjectSubscription and optionally call follow_all_project_questions(project, user) as before; use the existing symbols subscribe_project, ProjectSubscription.objects.filter(...).first(), project_subscription.save(), ProjectSubscription.objects.create(...), update_followers_count(), project.save(), and follow_all_project_questions to locate and implement the change.
33-38: Unused return value.The function returns a list of created subscriptions, but neither
follow_all_project_questionsnorfollow_new_project_postuses this return value. Consider removing the return statement or documenting why it's needed for future use.♻️ Proposed fix
- return [ - create_subscription_new_comments(user=user, post=post, comments_frequency=1), - create_subscription_status_change(user=user, post=post), - create_subscription_milestone(user=user, post=post, milestone_step=0.2), - create_subscription_cp_change(user=user, post=post, cp_change_threshold=0.25), - ] + create_subscription_new_comments(user=user, post=post, comments_frequency=1) + create_subscription_status_change(user=user, post=post) + create_subscription_milestone(user=user, post=post, milestone_step=0.2) + create_subscription_cp_change(user=user, post=post, cp_change_threshold=0.25)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/services/subscriptions.py` around lines 33 - 38, The list of created subscription objects returned by the block calling create_subscription_new_comments, create_subscription_status_change, create_subscription_milestone, and create_subscription_cp_change is never used by follow_all_project_questions or follow_new_project_post; remove the unnecessary return and just invoke those create_subscription_* functions for side effects, or if you want to keep the values for future use, add a clear comment or docstring explaining why the list is returned and mark the variable (e.g., subscriptions) as intentionally unused; locate the call site in the function containing those create_subscription_* calls and either replace "return [ ... ]" with the calls on their own lines or add the explanatory comment/docstring.
87-108: Potential performance concern with N+1 queries for large subscriber bases.For each subscriber with
follow_questions=True, this function:
- Runs a permission check query (Line 101)
- Checks for existing subscriptions (Lines 104-106)
For tournaments with many subscribers, this could result in significant database load when a new post is added.
Consider batching:
- Pre-fetch all users who already have subscriptions on this post
- Use a single query to filter users with view permission
♻️ Proposed optimization
def follow_new_project_post(post: Post, project: Project): """ Auto-follow a newly added post for all project subscribers who have follow_questions enabled. """ subscriptions = ProjectSubscription.objects.filter( project=project, follow_questions=True, ).select_related("user") + # Pre-fetch users who already have subscriptions on this post + already_subscribed_user_ids = set( + PostSubscription.objects.filter( + post=post, + is_global=False, + ).values_list("user_id", flat=True) + ) + + # Get user IDs who can view the post + users_with_access = set( + post.default_project.get_users_for_permission(ObjectPermission.VIEWER) + .values_list("id", flat=True) + ) + for subscription in subscriptions: user = subscription.user - # Check user has permission to view the post - if not Post.objects.filter_permission(user=user).filter(pk=post.pk).exists(): + if user.id not in users_with_access: continue - # Skip if user already has subscriptions on this post - if PostSubscription.objects.filter( - user=user, post=post, is_global=False - ).exists(): + if user.id in already_subscribed_user_ids: continue _create_default_question_subscriptions(user, post)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/services/subscriptions.py` around lines 87 - 108, The loop in follow_new_project_post causes N+1 DB queries by checking view permission and existing subscriptions per user; fix by batching: from the ProjectSubscription queryset (in follow_new_project_post) collect the user ids, fetch all users who already have a PostSubscription for this post in one query via PostSubscription.objects.filter(post=post, is_global=False, user__in=users).values_list('user_id', flat=True), determine which users can view the post in bulk using Post.objects.filter_permission(user__in=users).filter(pk=post.pk) or the equivalent bulk permission check to get allowed user ids, compute the set difference (allowed_users minus users_with_existing_subs) and then iterate only over that reduced user list to call _create_default_question_subscriptions(user, post).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@projects/services/subscriptions.py`:
- Around line 111-131: The function subscribe_project currently always calls
project.update_followers_count() and project.save() even when only updating the
existing ProjectSubscription's follow_questions flag; modify subscribe_project
so you detect whether you created a new subscription (e.g. by checking if
project_subscription existed before) and only call
project.update_followers_count() and project.save() when a new subscription is
created (or when follower status actually changes), leaving the existing-update
path to just update and save the ProjectSubscription and optionally call
follow_all_project_questions(project, user) as before; use the existing symbols
subscribe_project, ProjectSubscription.objects.filter(...).first(),
project_subscription.save(), ProjectSubscription.objects.create(...),
update_followers_count(), project.save(), and follow_all_project_questions to
locate and implement the change.
- Around line 33-38: The list of created subscription objects returned by the
block calling create_subscription_new_comments,
create_subscription_status_change, create_subscription_milestone, and
create_subscription_cp_change is never used by follow_all_project_questions or
follow_new_project_post; remove the unnecessary return and just invoke those
create_subscription_* functions for side effects, or if you want to keep the
values for future use, add a clear comment or docstring explaining why the list
is returned and mark the variable (e.g., subscriptions) as intentionally unused;
locate the call site in the function containing those create_subscription_*
calls and either replace "return [ ... ]" with the calls on their own lines or
add the explanatory comment/docstring.
- Around line 87-108: The loop in follow_new_project_post causes N+1 DB queries
by checking view permission and existing subscriptions per user; fix by
batching: from the ProjectSubscription queryset (in follow_new_project_post)
collect the user ids, fetch all users who already have a PostSubscription for
this post in one query via PostSubscription.objects.filter(post=post,
is_global=False, user__in=users).values_list('user_id', flat=True), determine
which users can view the post in bulk using
Post.objects.filter_permission(user__in=users).filter(pk=post.pk) or the
equivalent bulk permission check to get allowed user ids, compute the set
difference (allowed_users minus users_with_existing_subs) and then iterate only
over that reduced user list to call _create_default_question_subscriptions(user,
post).
In `@projects/views/common.py`:
- Around line 404-407: The follow_questions value taken from request.data should
be explicitly validated before calling subscribe_project; update the code that
reads follow_questions to accept only clear boolean types or well-defined
string/number equivalents (e.g., True/False, "true"/"false", "1"/"0") and either
coerce them to a bool or raise a validation error (e.g., BadRequest) for invalid
inputs, then pass the validated/coerced boolean into
subscribe_project(project=project, user=request.user,
follow_questions=follow_questions); ensure you reference the follow_questions
variable and the subscribe_project call so the change is localized.
- Around line 417-420: Validate the incoming unfollow_questions value before
passing it to unsubscribe_project the same way follow_questions is validated:
ensure request.data.get("unfollow_questions", False) is coerced/checked to a
boolean (or parsed from strings like "true"/"false") and raise/return a 400 or
default to False on invalid input; then call
unsubscribe_project(project=project, user=request.user,
unfollow_questions=unfollow_questions) with the sanitized boolean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca4a0bbb-4af8-42ba-a22b-9efea0ee6703
📒 Files selected for processing (5)
projects/migrations/0022_projectsubscription_follow_questions.pyprojects/models.pyprojects/services/subscriptions.pyprojects/views/common.pytests/unit/test_projects/test_services/test_follow_questions.py
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
ncarazon
reviewed
Mar 17, 2026
front_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_button.tsx
Outdated
Show resolved
Hide resolved
front_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_modal.tsx
Show resolved
Hide resolved
…/follow-all-tournament-questions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
front_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_modal.tsx (1)
32-34:⚠️ Potential issue | 🟡 MinorCheckbox state may not reset when modal is reopened without prop change.
The
useEffectsyncs state whendefaultFollowQuestionschanges, but if the user opens the modal, toggles the checkbox, closes without submitting, and reopens the modal, the toggled state persists becausedefaultFollowQuestionshasn't changed. Consider also resetting when the modal opens.🛡️ Proposed fix to reset on modal open
- useEffect(() => { - setFollowQuestions(defaultFollowQuestions); - }, [defaultFollowQuestions]); + useEffect(() => { + if (isOpen) { + setFollowQuestions(defaultFollowQuestions); + } + }, [isOpen, defaultFollowQuestions]);This is related to a past review comment about modal state not resetting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/(tournaments)/tournament/components/tournament_subscribe_modal.tsx around lines 32 - 34, The checkbox state persists because useEffect only resets setFollowQuestions when defaultFollowQuestions changes; update the component to also reset when the modal is opened: add a useEffect (or extend the existing one) that listens for the modal open prop (e.g., open or isOpen) and calls setFollowQuestions(defaultFollowQuestions) whenever the modal becomes open, referencing the existing setFollowQuestions and defaultFollowQuestions to locate where to apply the change.
🧹 Nitpick comments (2)
projects/services/subscriptions.py (2)
64-67: Consider bulk creation for better performance.The loop creates subscriptions one post at a time, resulting in multiple database round-trips. For projects with many questions, this could be slow. Consider collecting posts first and using bulk operations.
♻️ Proposed bulk creation approach
+from itertools import chain def follow_all_project_questions(project: Project, user: User): ... for post in posts: if post.pk in already_followed_post_ids: continue - _create_default_question_subscriptions(user, post) + posts_to_follow.append(post) + + # Bulk create all subscriptions + all_subscriptions = [] + for post in posts_to_follow: + all_subscriptions.extend(_create_default_question_subscriptions(user, post, save=False)) + PostSubscription.objects.bulk_create(all_subscriptions, ignore_conflicts=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/services/subscriptions.py` around lines 64 - 67, The loop in subscriptions.py calls _create_default_question_subscriptions per post causing many DB round-trips; instead gather the posts that are not in already_followed_post_ids, build the corresponding Subscription objects (or the data needed by _create_default_question_subscriptions) in memory and write them in a single bulk operation (e.g., Django's bulk_create, optionally with ignore_conflicts=True or inside a transaction) to reduce queries; update the code that iterates over posts to collect new subscription instances and call bulk_create once rather than calling _create_default_question_subscriptions for each post.
111-125: Useupdate_or_createfor cleaner and safer upsert logic.The current pattern of
filter().first()followed by conditional create/update is verbose and could be simplified. Django'supdate_or_createhandles this atomically.♻️ Proposed simplification
`@transaction.atomic` def subscribe_project(project: Project, user: User, follow_questions: bool = False): - project_subscription = ProjectSubscription.objects.filter( - project=project, user=user - ).first() - if project_subscription: - # Update follow_questions if subscription already exists - project_subscription.follow_questions = follow_questions - project_subscription.save() - else: - ProjectSubscription.objects.create( - project=project, - user=user, - follow_questions=follow_questions, - ) + ProjectSubscription.objects.update_or_create( + project=project, + user=user, + defaults={"follow_questions": follow_questions}, + ) project.update_followers_count() project.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/services/subscriptions.py` around lines 111 - 125, Replace the manual filter().first() + conditional save/create in subscribe_project with Django's atomic upsert: call ProjectSubscription.objects.update_or_create with lookup keys project and user and pass defaults={'follow_questions': follow_questions} so the record is created or updated in one step; keep the `@transaction.atomic` decorator around subscribe_project and remove the existing conditional branch (the ProjectSubscription variable and explicit save/create).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front_end/src/app/`(main)/(tournaments)/tournament/[slug]/actions.tsx:
- Around line 56-68: subscribeProject and unsubscribeProject lack the same
try/catch error handling and cache revalidation used by
inviteProjectUsers/deleteProjectMember; wrap the ServerProjectsApi.subscribe and
ServerProjectsApi.unsubscribe calls in a try/catch, log or throw a descriptive
Error on failure, and call the same revalidation helper used elsewhere (e.g.,
revalidatePath or revalidateTag) after a successful response to refresh server
cache/state; ensure you reference the existing functions subscribeProject and
unsubscribeProject and mirror the error handling/revalidation pattern from
inviteProjectUsers and deleteProjectMember.
In
`@front_end/src/app/`(main)/(tournaments)/tournament/components/tournament_subscribe_button.tsx:
- Around line 52-84: Both submit callbacks lack error handling: update
handleFollowSubmit and handleUnfollowSubmit to catch API errors from
subscribeProject/unsubscribeProject, log the error, show a user-facing error
(e.g., toast or set an error state) so the user knows the action failed, and
only call setIsFollowing/setFollowQuestions/setModalMode on success; keep the
existing finally to call setIsLoading(false). Use the existing symbols
subscribeProject, unsubscribeProject, handleFollowSubmit, handleUnfollowSubmit,
setIsLoading, setIsFollowing, setFollowQuestions, and setModalMode to locate and
implement the changes.
In `@projects/services/subscriptions.py`:
- Around line 93-108: The loop causes N+1 queries; first compute the set of user
IDs who already have a non-global PostSubscription for this post by querying
PostSubscription.values_list("user_id"), then exclude those IDs from the
ProjectSubscription queryset (subscriptions =
subscriptions.exclude(user_id__in=users_with_subs)) before iterating; keep the
existing permission check against
Post.objects.filter_permission(user=user).filter(pk=post.pk).exists() and then
call _create_default_question_subscriptions(user, post) for remaining
subscriptions.
---
Duplicate comments:
In
`@front_end/src/app/`(main)/(tournaments)/tournament/components/tournament_subscribe_modal.tsx:
- Around line 32-34: The checkbox state persists because useEffect only resets
setFollowQuestions when defaultFollowQuestions changes; update the component to
also reset when the modal is opened: add a useEffect (or extend the existing
one) that listens for the modal open prop (e.g., open or isOpen) and calls
setFollowQuestions(defaultFollowQuestions) whenever the modal becomes open,
referencing the existing setFollowQuestions and defaultFollowQuestions to locate
where to apply the change.
---
Nitpick comments:
In `@projects/services/subscriptions.py`:
- Around line 64-67: The loop in subscriptions.py calls
_create_default_question_subscriptions per post causing many DB round-trips;
instead gather the posts that are not in already_followed_post_ids, build the
corresponding Subscription objects (or the data needed by
_create_default_question_subscriptions) in memory and write them in a single
bulk operation (e.g., Django's bulk_create, optionally with
ignore_conflicts=True or inside a transaction) to reduce queries; update the
code that iterates over posts to collect new subscription instances and call
bulk_create once rather than calling _create_default_question_subscriptions for
each post.
- Around line 111-125: Replace the manual filter().first() + conditional
save/create in subscribe_project with Django's atomic upsert: call
ProjectSubscription.objects.update_or_create with lookup keys project and user
and pass defaults={'follow_questions': follow_questions} so the record is
created or updated in one step; keep the `@transaction.atomic` decorator around
subscribe_project and remove the existing conditional branch (the
ProjectSubscription variable and explicit save/create).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8bbc049-8cca-401e-b043-26ba4604f3bf
📒 Files selected for processing (13)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/(tournaments)/tournament/[slug]/actions.tsxfront_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_button.tsxfront_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_modal.tsxfront_end/src/services/api/projects/projects.server.tsfront_end/src/types/projects.tsprojects/services/subscriptions.pytests/unit/test_projects/test_services/test_follow_questions.py
✅ Files skipped from review due to trivial changes (5)
- front_end/messages/en.json
- front_end/messages/es.json
- front_end/messages/pt.json
- front_end/messages/cs.json
- tests/unit/test_projects/test_services/test_follow_questions.py
front_end/src/app/(main)/(tournaments)/tournament/components/tournament_subscribe_button.tsx
Show resolved
Hide resolved
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.
closes #3349
sets up back end support for following/unfollowing all questions in a project add tests accordingly
Summary by CodeRabbit
New Features
Localization
Tests