Add proportional to results page #218
Conversation
Reviewer's Guide by SourceryThis pull request introduces proportional representation to the poll results page, displaying both the number of approval votes and the proportional representation of each choice. The changes include modifications to the results template to display the new data and updates to the view to calculate and provide the proportional vote data. Sequence diagram for calculating proportional votessequenceDiagram
participant User
participant ResultsView
participant Poll
participant Ballot
participant Choice
User->>ResultsView: View poll results
activate ResultsView
ResultsView->>Poll: get_context_data()
activate Poll
Poll->>Choice: annotate(vote_count)
Poll->>Ballot: prefetch_related(votes)
loop For each ballot
ResultsView->>Ballot: get approved choices
Ballot-->>ResultsView: approved_choices
ResultsView->>ResultsView: calculate proportional weight
Note right of ResultsView: weight = 1 / num_approved_choices
end
ResultsView->>ResultsView: calculate percentages
ResultsView-->>User: display results page
deactivate ResultsView
Class diagram showing poll results calculationclassDiagram
class ResultsView {
+get_context_data()
}
class Poll {
+question: str
+total_ballots: int
+choice_set: QuerySet
}
class Choice {
+choice_text: str
+vote_count: int
+proportional_votes: float
+proportional_percentage: float
}
class Ballot {
+vote_set: QuerySet
}
class Vote {
+choice: Choice
}
Poll "1" -- "*" Choice
Poll "1" -- "*" Ballot
Ballot "1" -- "*" Vote
Vote "*" -- "1" Choice
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: approval_polls/views.py
Did you find this useful? React with a 👍 or 👎 |
|
Warning Rate limit exceeded@fsargent has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a new proportional voting mechanism to the approval polls system. The changes span across three files: Changes
Sequence DiagramsequenceDiagram
participant User
participant ResultsView
participant Database
participant Template
User->>ResultsView: Request poll results
ResultsView->>Database: Fetch ballots and votes
Database-->>ResultsView: Return ballot data
ResultsView->>ResultsView: Calculate approval votes
ResultsView->>ResultsView: Calculate proportional votes
ResultsView->>Template: Pass results context
Template->>User: Render results with approval and proportional voting
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsargent - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The divisibleby filter is incorrect for percentage calculation (link)
Overall Comments:
- The PR removes some existing functionality (poll closure state, pluralization, winner badges) that should be preserved while adding the new proportional voting display
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
approval_polls/templates/results.html(1 hunks)approval_polls/views.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
approval_polls/views.py
291-291: Undefined name Vote
(F821)
🪛 GitHub Check: Trunk Check
approval_polls/views.py
[failure] 291-291: flake8(F821)
[new] undefined name 'Vote'
[failure] 291-291: ruff(F821)
[new] Undefined name Vote
🪛 GitHub Actions: Django Tests
approval_polls/views.py
[error] 291-291: NameError: Vote model is not defined. Missing import for Vote model in the views.py file.
[warning] 154-154: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: Poll QuerySet needs ordering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
approval_polls/views.py (1)
287-302: Consider optimizing the proportional voting calculation.While the implementation is correct, we can improve its performance and readability:
- The vote weight calculation can be moved to a separate method
- We can use Django's annotation to reduce database queries
# Proportional voting logic (separate) -ballots = poll.ballot_set.prefetch_related( - Prefetch("vote_set", queryset=Vote.objects.select_related("choice")) -) -proportional_votes = {choice.id: 0 for choice in poll.choice_set.all()} -total_proportional_votes = 0 - -for ballot in ballots: - approved_choices = ballot.vote_set.all().values_list("choice_id", flat=True) - num_approved = len(approved_choices) - if num_approved > 0: - weight = 1 / num_approved - for choice_id in approved_choices: - proportional_votes[choice_id] += weight - total_proportional_votes += weight +def calculate_vote_weight(num_choices): + return 1 / num_choices if num_choices > 0 else 0 + +from django.db.models import Count, F, ExpressionWrapper, FloatField + +ballots_with_counts = poll.ballot_set.annotate( + num_choices=Count('vote'), + vote_weight=ExpressionWrapper( + 1.0 / F('num_choices'), + output_field=FloatField() + ) +) + +proportional_votes = {} +total_proportional_votes = 0 + +for ballot in ballots_with_counts: + if ballot.num_choices > 0: + for choice in ballot.vote_set.all(): + proportional_votes[choice.choice_id] = ( + proportional_votes.get(choice.choice_id, 0) + ballot.vote_weight + ) + total_proportional_votes += ballot.vote_weight
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
approval_polls/templates/results.html(1 hunks)approval_polls/views.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- approval_polls/templates/results.html
🧰 Additional context used
🪛 Ruff (0.8.2)
approval_polls/views.py
269-269: Redefinition of unused Count from line 11
Remove definition: Count
(F811)
269-269: Redefinition of unused Prefetch from line 11
Remove definition: Prefetch
(F811)
🪛 GitHub Check: Trunk Check
approval_polls/views.py
[failure] 269-269: ruff(E402)
[new] Module level import not at top of file
[failure] 269-269: flake8(F811)
[new] redefinition of unused 'Prefetch' from line 11
[failure] 269-269: ruff(F811)
[new] Redefinition of unused Count from line 11
[failure] 269-269: ruff(F811)
[new] Redefinition of unused Prefetch from line 11
[failure] 269-269: flake8(F811)
[new] redefinition of unused 'Count' from line 11
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Trunk Check
🔇 Additional comments (3)
approval_polls/views.py (3)
282-285: LGTM: Approval voting logic is clear and concise.The original approval voting calculation is well-implemented, using appropriate database annotations for efficiency.
303-314: LGTM: Results formatting is well-structured.The code properly formats the proportional results with clear percentage calculations and null-safety checks.
317-324: LGTM: Context update is comprehensive.The context update properly includes both approval and proportional voting results, maintaining backward compatibility while adding new functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
approval_polls/templates/results.html (2)
119-126: Consider dynamic color generation for the pie chart.The current implementation only supports up to 6 colors. For polls with more choices, you'll need additional colors.
Consider using a color generation function. Here's an example implementation:
- backgroundColor: [ - 'rgba(75, 192, 192, 0.2)', - 'rgba(255, 99, 132, 0.2)', - 'rgba(255, 206, 86, 0.2)', - 'rgba(54, 162, 235, 0.2)', - 'rgba(153, 102, 255, 0.2)', - 'rgba(255, 159, 64, 0.2)' - ], + backgroundColor: Array.from( + { length: proportional_results.length }, + (_, i) => `hsla(${(i * 360) / proportional_results.length}, 70%, 60%, 0.2)` + ),Apply the same pattern to
borderColor.
107-143: Add error handling for chart initialization.The chart initialization should be wrapped in error handling to gracefully handle cases where the canvas element might not be available.
- const ctx = document.getElementById('proportionalChart').getContext('2d'); - const data = { + try { + const canvas = document.getElementById('proportionalChart'); + if (!canvas) { + console.warn('Proportional chart canvas not found'); + return; + } + const ctx = canvas.getContext('2d'); + const data = { // ... existing data configuration ... - }; - const config = { - type: 'pie', - data: data, - }; - const proportionalChart = new Chart(ctx, config); + }; + const config = { + type: 'pie', + data: data, + }; + const proportionalChart = new Chart(ctx, config); + } catch (error) { + console.error('Failed to initialize proportional chart:', error); + }approval_polls/views.py (3)
316-320: Simplify the percentage calculation with a ternary operator.The nested if condition can be simplified.
- "proportional_percentage": ( - proportional_votes[choice.id] / total_proportional_votes * 100 - if total_proportional_votes > 0 - else 0 - ), + "proportional_percentage": proportional_votes[choice.id] / total_proportional_votes * 100 if total_proportional_votes else 0,
296-324: Add docstring explaining the proportional voting algorithm.The proportional voting calculation logic should be documented for better maintainability.
+ # Calculate proportional votes where each ballot's weight is distributed equally among its choices. + # For example, if a ballot approves 2 choices, each choice gets 0.5 votes from that ballot. + # This ensures that each ballot has equal voting power regardless of how many choices it approves. # Proportional voting logic ballots = poll.ballot_set.prefetch_related(
303-310: Optimize the weight calculation using list comprehension.The current implementation can be made more efficient and readable.
- for ballot in ballots: - approved_choices = ballot.vote_set.all().values_list("choice_id", flat=True) - num_approved = len(approved_choices) - if num_approved > 0: - weight = 1 / num_approved - for choice_id in approved_choices: - proportional_votes[choice_id] += weight - total_proportional_votes += weight + for ballot in ballots: + approved_choices = list(ballot.vote_set.all().values_list("choice_id", flat=True)) + if approved_choices: + weight = 1 / len(approved_choices) + total_proportional_votes += weight * len(approved_choices) + for choice_id in approved_choices: + proportional_votes[choice_id] += weight
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
approval_polls/templates/results.html(4 hunks)approval_polls/views.py(3 hunks)
🔇 Additional comments (3)
approval_polls/templates/results.html (2)
Line range hint
12-48: LGTM! Clean implementation of the approval voting results section.The section is well-structured with clear visual indicators for leading choices and correct percentage calculations.
49-97: Great implementation of the proportional voting results section!The accordion design, informative description, and dual visualization (table + chart) provide an excellent user experience.
approval_polls/views.py (1)
287-295: LGTM! Efficient implementation of approval voting calculation.The use of Django's Count aggregation and the logic for determining leading choices is well implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
approval_polls/settings.py (1)
38-39: Consider using structured logging instead of print statements.While the print statements are helpful for debugging, consider using Python's logging framework for better control and consistency with the existing logging configuration.
Apply this diff to use structured logging:
- print("Allowed Hosts: ", ALLOWED_HOSTS) - print("APP_NAME: ", APP_NAME) + import logging + logger = logging.getLogger(__name__) + logger.debug("Allowed Hosts: %s", ALLOWED_HOSTS) + logger.debug("APP_NAME: %s", APP_NAME)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/fly-review.ymlis excluded by!**/*.ymlfly.pr-review.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
approval_polls/settings.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Trunk Check
approval_polls/settings.py
Outdated
| CSRF_TRUSTED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] | ||
| CSRF_ALLOWED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] | ||
| CORS_ORIGINS_WHITELIST = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] |
There was a problem hiding this comment.
Add HTTPS prefix to dynamically constructed URLs in security settings.
The security settings are missing the HTTPS prefix in the dynamically constructed URLs, which could lead to security vulnerabilities. The CSRF and CORS settings should consistently use HTTPS URLs like the existing configuration.
Apply this diff to fix the security configuration:
- CSRF_TRUSTED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"]
- CSRF_ALLOWED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"]
- CORS_ORIGINS_WHITELIST = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"]
+ CSRF_TRUSTED_ORIGINS = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"]
+ CSRF_ALLOWED_ORIGINS = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"]
+ CORS_ORIGINS_WHITELIST = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"]📝 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.
| CSRF_TRUSTED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] | |
| CSRF_ALLOWED_ORIGINS = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] | |
| CORS_ORIGINS_WHITELIST = ["https://vote.electionscience.org", f"{APP_NAME}.fly.dev"] | |
| CSRF_TRUSTED_ORIGINS = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"] | |
| CSRF_ALLOWED_ORIGINS = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"] | |
| CORS_ORIGINS_WHITELIST = ["https://vote.electionscience.org", f"https://{APP_NAME}.fly.dev"] |
approval_polls/settings.py
Outdated
| print("Allowed Hosts: ", ALLOWED_HOSTS) | ||
| print("APP_NAME: ", APP_NAME) |
There was a problem hiding this comment.
💡 Codebase verification
APP_NAME validation is required to prevent invalid security settings
Empty APP_NAME leads to invalid domains (".fly.dev") in multiple security-critical settings:
- ALLOWED_HOSTS configuration
- CSRF trusted origins
- CORS whitelist
Add validation at the start of settings.py to fail fast if APP_NAME is empty:
APP_NAME = env("FLY_APP_NAME", str, "")
if not APP_NAME:
raise ValueError("FLY_APP_NAME environment variable must not be empty")🔗 Analysis chain
Add validation for empty APP_NAME to prevent invalid URLs.
The APP_NAME is fetched with an empty string default, which could lead to invalid URLs in security settings. Consider adding validation or a fallback.
Let's verify the APP_NAME handling:
Also applies to: 54-56
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if APP_NAME validation exists in the codebase
# Search for APP_NAME validation or usage
rg "APP_NAME" -B 2 -A 2
# Check if there are any environment validation utilities
rg -l "environ|getenv|os\.environ" --type py
Length of output: 1391
Script:
#!/bin/bash
# Check env() function implementation and Django settings validation
rg -B 3 -A 3 "def env\(" --type py
rg -B 3 -A 3 "environ\.get" --type py
# Check for any Django settings validation
rg -B 3 -A 3 "check|validate" approval_polls/settings.py
Length of output: 438
Summary by Sourcery
New Features: