Campaign #19
Campaign #19
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a many-to-many relationship between campaigns and surveys by adding a new association table, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FlaskApp
participant CampaignsService
participant CampaignRepository
participant Database
User->>FlaskApp: GET /campaigns
FlaskApp->>CampaignsService: get_campaigns(user_id)
CampaignsService->>CampaignRepository: query campaigns by user_id
CampaignRepository->>Database: fetch campaigns
Database-->>CampaignRepository: campaigns data
CampaignRepository-->>CampaignsService: campaigns data
CampaignsService-->>FlaskApp: campaigns data
FlaskApp-->>User: campaigns list
User->>FlaskApp: GET /campaigns/{id}/survey-responses-count
FlaskApp->>CampaignsService: get_survey_responses_count(campaign_id)
CampaignsService->>CampaignRepository: get survey responses count
CampaignRepository->>Database: count query
Database-->>CampaignRepository: count
CampaignRepository-->>CampaignsService: count
CampaignsService-->>FlaskApp: count
FlaskApp-->>User: count
User->>FlaskApp: GET /campaigns/{id}/participation
FlaskApp->>CampaignsService: get_campaign_participation(campaign_id)
CampaignsService->>CampaignRepository: get participation data
CampaignRepository->>Database: select participation
Database-->>CampaignRepository: participation data
CampaignRepository-->>CampaignsService: participation data
CampaignsService-->>FlaskApp: participation data
FlaskApp-->>User: participation data
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Associate multiple surveys with a campaign |
![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
🧹 Nitpick comments (2)
mind_matter_api/models/campaigns.py (2)
29-29: Great performance optimization for tracking responsesAdding a counter column is more efficient than counting related records every time you need this information. Consider adding validation to ensure this count doesn't exceed the required responses.
- survey_responses_count = db.Column(db.Integer, default=0) # Track number of survey responses + survey_responses_count = db.Column(db.Integer, default=0) # Track number of survey responses + + @property + def has_completed_surveys(self): + """Check if participant has completed the required number of survey responses""" + return self.survey_responses_count >= self.campaign.required_survey_responses
35-42: Well-structured association modelUsing a full model class for the association table rather than a simple
Tableconstruct is a good approach as it allows tracking creation timestamps and potentially adding more functionality later. The composite primary key is correctly defined.Consider adding methods to this model to handle association-specific logic if needed in the future, such as:
def is_active(self): """Check if this campaign-survey association is still active based on campaign dates""" now = datetime.utcnow() return self.campaign.start_date <= now <= self.campaign.end_date
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
migrations/versions/6d69432c2997_update_campaign_model.py(1 hunks)mind_matter_api/models/campaigns.py(2 hunks)mind_matter_api/models/surveys.py(1 hunks)
🔇 Additional comments (6)
mind_matter_api/models/surveys.py (1)
19-19: Well-implemented bidirectional relationship!The many-to-many relationship between
SurveyandCampaignmodels is properly defined with appropriate parameters. Thesecondaryparameter correctly references the association table, and theback_populatesparameter maintains the bidirectional relationship with thesurveysattribute in theCampaignmodel.migrations/versions/6d69432c2997_update_campaign_model.py (3)
21-28: Association table design looks goodThe
campaign_surveystable is correctly structured with composite primary keys and appropriate foreign key constraints. Including a timestamp will be helpful for auditing when associations were created.
29-31: Proper tracking of survey responsesAdding
survey_responses_countto track participant survey submissions is a good approach for performance optimization, as it avoids the need to count related records each time this information is needed.
38-47: Downgrade function is correctly implementedThe downgrade function correctly reverts all changes in the proper order: first dropping the added columns, then dropping the association table.
mind_matter_api/models/campaigns.py (2)
15-15: Good addition of required responses trackingThe
required_survey_responsescolumn with a default value of 1 provides a clear way to configure how many survey responses are needed for a campaign to be completed. The default value also ensures backward compatibility.
19-19: Properly defined many-to-many relationshipThe relationship to surveys is correctly implemented using the
secondaryparameter to reference the association table andback_populatesto maintain bidirectionality.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
mind_matter_api/api/campaigns.py (1)
1-7: Remove unused imports.Several imports are not used in this file and should be removed to improve code clarity.
- from flask import jsonify, request, current_app - from marshmallow import ValidationError + from flask import jsonify - from mind_matter_api.utils.decorators import require_auth, require_owner - from mind_matter_api.utils.auth import is_user_owner, is_user_admin + from mind_matter_api.utils.decorators import require_auth🧰 Tools
🪛 Ruff (0.11.9)
1-1:
flask.jsonifyimported but unusedRemove unused import
(F401)
1-1:
flask.requestimported but unusedRemove unused import
(F401)
1-1:
flask.current_appimported but unusedRemove unused import
(F401)
2-2:
marshmallow.ValidationErrorimported but unusedRemove unused import:
marshmallow.ValidationError(F401)
5-5:
mind_matter_api.utils.decorators.require_ownerimported but unusedRemove unused import:
mind_matter_api.utils.decorators.require_owner(F401)
6-6:
mind_matter_api.utils.auth.is_user_ownerimported but unusedRemove unused import
(F401)
6-6:
mind_matter_api.utils.auth.is_user_adminimported but unusedRemove unused import
(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
mind_matter_api/api/__init__.py(2 hunks)mind_matter_api/api/campaigns.py(1 hunks)mind_matter_api/api/emergency_contacts.py(5 hunks)mind_matter_api/repositories/campaigns.py(2 hunks)mind_matter_api/services/campaigns.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- mind_matter_api/api/emergency_contacts.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
mind_matter_api/api/__init__.py (3)
mind_matter_api/api/campaigns.py (1)
init_campaigns_routes(11-27)mind_matter_api/repositories/campaigns.py (1)
CampaignRepository(6-45)mind_matter_api/services/campaigns.py (1)
CampaignsService(5-32)
mind_matter_api/services/campaigns.py (3)
mind_matter_api/models/campaigns.py (1)
CampaignParticipation(21-33)mind_matter_api/api/campaigns.py (2)
get_survey_responses_count(21-22)get_campaign_participation(26-27)mind_matter_api/repositories/campaigns.py (2)
get_survey_responses_count(41-42)get_campaign_participation(44-45)
mind_matter_api/repositories/campaigns.py (4)
mind_matter_api/models/campaigns.py (2)
Campaign(4-19)CampaignParticipation(21-33)mind_matter_api/repositories/types.py (1)
Repository(7-56)mind_matter_api/api/campaigns.py (2)
get_survey_responses_count(21-22)get_campaign_participation(26-27)mind_matter_api/services/campaigns.py (2)
get_survey_responses_count(13-15)get_campaign_participation(17-19)
mind_matter_api/api/campaigns.py (3)
mind_matter_api/services/campaigns.py (4)
CampaignsService(5-32)get_campaigns(9-11)get_survey_responses_count(13-15)get_campaign_participation(17-19)mind_matter_api/utils/decorators.py (1)
require_auth(8-13)mind_matter_api/repositories/campaigns.py (2)
get_survey_responses_count(41-42)get_campaign_participation(44-45)
🪛 Ruff (0.11.9)
mind_matter_api/api/campaigns.py
1-1: flask.jsonify imported but unused
Remove unused import
(F401)
1-1: flask.request imported but unused
Remove unused import
(F401)
1-1: flask.current_app imported but unused
Remove unused import
(F401)
2-2: marshmallow.ValidationError imported but unused
Remove unused import: marshmallow.ValidationError
(F401)
5-5: mind_matter_api.utils.decorators.require_owner imported but unused
Remove unused import: mind_matter_api.utils.decorators.require_owner
(F401)
6-6: mind_matter_api.utils.auth.is_user_owner imported but unused
Remove unused import
(F401)
6-6: mind_matter_api.utils.auth.is_user_admin imported but unused
Remove unused import
(F401)
🔇 Additional comments (3)
mind_matter_api/api/__init__.py (1)
15-19: LGTM: Properly importing campaign-related modules.The imports follow the application's convention and include all necessary modules for the campaign feature.
mind_matter_api/services/campaigns.py (1)
2-2: LGTM: Proper model import.Correctly importing the required models.
mind_matter_api/repositories/campaigns.py (1)
2-3: LGTM: Proper imports.Imports are correct and include all necessary models and base classes.
| def get_survey_responses_count(self, campaign_id: str) -> int: | ||
| return self.session.query(Campaign).filter(Campaign.campaign_id == campaign_id).count() |
There was a problem hiding this comment.
Implementation doesn't match the method's intent.
This method is supposed to count survey responses for a campaign, but it's actually counting campaign records filtered by ID, which will always return 0 or 1. This doesn't align with the PR objective of tracking survey response counts.
The implementation should likely count entries in a survey responses table that are associated with surveys in this campaign:
- def get_survey_responses_count(self, campaign_id: str) -> int:
- return self.session.query(Campaign).filter(Campaign.campaign_id == campaign_id).count()
+ def get_survey_responses_count(self, campaign_id: int) -> int:
+ # Get the campaign
+ campaign = self.session.query(Campaign).get(campaign_id)
+ if not campaign:
+ return 0
+ # Count responses for all surveys in this campaign
+ from mind_matter_api.models.surveys import Survey, SurveyResponse
+ return self.session.query(SurveyResponse).\
+ join(Survey, SurveyResponse.survey_id == Survey.survey_id).\
+ join(campaign_surveys, Survey.survey_id == campaign_surveys.c.survey_id).\
+ filter(campaign_surveys.c.campaign_id == campaign_id).\
+ count()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mind_matter_api/repositories/campaigns.py around lines 41 to 42, the method
get_survey_responses_count incorrectly counts Campaign records instead of survey
responses. To fix this, update the query to count entries from the survey
responses table that are linked to surveys belonging to the specified
campaign_id, ensuring the count reflects actual survey responses associated with
that campaign.
| def get_campaign_participation(self, campaign_id: str) -> CampaignParticipation: | ||
| return self.session.query(CampaignParticipation).filter(CampaignParticipation.campaign_id == campaign_id).all() |
There was a problem hiding this comment.
Return type doesn't match the actual return value.
This method returns a list of CampaignParticipation objects with all(), but the return type is specified as a single CampaignParticipation object. Also, the campaign_id parameter is typed as str but used to filter an integer column.
- def get_campaign_participation(self, campaign_id: str) -> CampaignParticipation:
+ def get_campaign_participation(self, campaign_id: int) -> List[CampaignParticipation]:Also, add the missing import:
+ from typing import List📝 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.
| def get_campaign_participation(self, campaign_id: str) -> CampaignParticipation: | |
| return self.session.query(CampaignParticipation).filter(CampaignParticipation.campaign_id == campaign_id).all() | |
| # Add at the top of mind_matter_api/repositories/campaigns.py | |
| from typing import List | |
| def get_campaign_participation(self, campaign_id: int) -> List[CampaignParticipation]: | |
| return self.session.query(CampaignParticipation) \ | |
| .filter(CampaignParticipation.campaign_id == campaign_id) \ | |
| .all() |
🤖 Prompt for AI Agents
In mind_matter_api/repositories/campaigns.py around lines 44 to 45, the method
get_campaign_participation is declared to return a single CampaignParticipation
but actually returns a list due to using all(). Change the return type
annotation to List[CampaignParticipation] to match the returned list. Also,
update the campaign_id parameter type from str to int to match the integer
column used in the filter. Finally, add the missing import for List from typing
at the top of the file.
| @app.route('/campaigns/<int:campaign_id>/participation', methods=['GET']) | ||
| @require_auth | ||
| def get_campaign_participation(campaign_id): | ||
| return campaign_service.get_campaign_participation(campaign_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and owner verification.
Similar to the previous endpoint, this one should verify user permissions and include error handling.
@app.route('/campaigns/<int:campaign_id>/participation', methods=['GET'])
@require_auth
def get_campaign_participation(campaign_id):
- return campaign_service.get_campaign_participation(campaign_id)
+ try:
+ # Verify user has access to this campaign
+ user_campaigns = campaign_service.get_campaigns(user_id)
+ if not any(c.campaign_id == campaign_id for c in user_campaigns):
+ return jsonify({"error": "Access denied"}), 403
+
+ participations = campaign_service.get_campaign_participation(campaign_id)
+ return jsonify(participations)
+ except Exception as e:
+ logging.error(f"Error retrieving campaign participation: {str(e)}")
+ return jsonify({"error": "Failed to retrieve campaign participation"}), 500Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mind_matter_api/api/campaigns.py around lines 24 to 27, the
get_campaign_participation endpoint lacks error handling and user permission
verification. Update the function to check if the authenticated user is the
owner of the campaign before proceeding. Add try-except blocks to handle
potential errors from the service call and return appropriate error responses.
| @app.route('/campaigns/<int:campaign_id>/survey-responses-count', methods=['GET']) | ||
| @require_auth | ||
| def get_survey_responses_count(campaign_id): | ||
| return campaign_service.get_survey_responses_count(campaign_id) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and owner verification.
This endpoint should verify that the user has permission to view the campaign data and include error handling.
@app.route('/campaigns/<int:campaign_id>/survey-responses-count', methods=['GET'])
@require_auth
def get_survey_responses_count(campaign_id):
- return campaign_service.get_survey_responses_count(campaign_id)
+ try:
+ # Verify user has access to this campaign
+ user_campaigns = campaign_service.get_campaigns(user_id)
+ if not any(c.campaign_id == campaign_id for c in user_campaigns):
+ return jsonify({"error": "Access denied"}), 403
+
+ count = campaign_service.get_survey_responses_count(campaign_id)
+ return jsonify({"survey_responses_count": count})
+ except Exception as e:
+ logging.error(f"Error retrieving survey responses count: {str(e)}")
+ return jsonify({"error": "Failed to retrieve survey responses count"}), 500Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mind_matter_api/api/campaigns.py around lines 19 to 23, the
get_survey_responses_count endpoint lacks error handling and does not verify if
the requesting user owns or has permission to access the campaign data. Add a
check to confirm the authenticated user's ownership or permission for the
campaign before returning the count. Also, wrap the service call in a try-except
block to handle potential errors gracefully, returning appropriate HTTP error
responses when permission is denied or other exceptions occur.
| @app.route('/campaigns', methods=['GET']) | ||
| @require_auth | ||
| def get_campaigns(user_id): | ||
| return campaign_service.get_campaigns(user_id) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and response formatting.
The endpoint lacks error handling and proper response formatting. If get_campaigns fails or returns None, it could result in a 500 error.
@app.route('/campaigns', methods=['GET'])
@require_auth
def get_campaigns(user_id):
- return campaign_service.get_campaigns(user_id)
+ try:
+ campaigns = campaign_service.get_campaigns(user_id)
+ return jsonify(campaigns)
+ except Exception as e:
+ logging.error(f"Error retrieving campaigns: {str(e)}")
+ return jsonify({"error": "Failed to retrieve campaigns"}), 500📝 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.
| @app.route('/campaigns', methods=['GET']) | |
| @require_auth | |
| def get_campaigns(user_id): | |
| return campaign_service.get_campaigns(user_id) | |
| @app.route('/campaigns', methods=['GET']) | |
| @require_auth | |
| def get_campaigns(user_id): | |
| try: | |
| campaigns = campaign_service.get_campaigns(user_id) | |
| return jsonify(campaigns) | |
| except Exception as e: | |
| logging.error(f"Error retrieving campaigns: {str(e)}") | |
| return jsonify({"error": "Failed to retrieve campaigns"}), 500 |
🤖 Prompt for AI Agents
In mind_matter_api/api/campaigns.py around lines 14 to 18, the get_campaigns
endpoint lacks error handling and response formatting. Modify the function to
catch exceptions from campaign_service.get_campaigns and return a proper JSON
response with an appropriate HTTP status code. Also, handle the case where
get_campaigns returns None by returning a 404 or an empty list with a 200
status, ensuring the endpoint never returns an unhandled 500 error.
Description
Brief summary of the changes and any relevant context (design doc, screenshots of bugs...).
Fixes # (issue)
Changes
Testing
Describe tests run to verify changes. If manual testing has been done, provide instructions to reproduce.
Checklist:
Summary by CodeRabbit