trending polls should show active polls first #117
Merged
Conversation
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
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PollService
participant Database
Client->>PollService: getPolls(query, worldID)
alt sortBy == PARTICIPANT_COUNT
PollService->>Database: Query active polls (order by participant count desc)
PollService->>Database: Query ended polls (order by participant count desc)
PollService->>PollService: Combine, paginate, map hasVoted, count total
else sortBy == END_DATE or CREATION_DATE
PollService->>Database: Query polls (order by sortBy and sortOrder)
PollService->>PollService: Map hasVoted, count total
end
PollService->>Client: Return polls and total count
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
![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 (1)
src/poll/poll.service.ts (1)
373-380: Extract duplicated poll mapping logic to reduce code duplication.The logic for mapping polls with vote status is repeated four times throughout the method. Consider extracting it into a helper method.
Add this helper method to the class:
private mapPollsWithVoteStatus(polls: any[], userId: number) { return polls.map(poll => { const { votes, ...pollWithoutVotes } = poll; return { ...pollWithoutVotes, hasVoted: votes.length > 0, }; }); }Then replace all occurrences with:
-const pollsWithVoteStatus = paginatedPolls.map(poll => { - const { votes, ...pollWithoutVotes } = poll - - return { - ...pollWithoutVotes, - hasVoted: votes.length > 0, - } -}) +const pollsWithVoteStatus = this.mapPollsWithVoteStatus(paginatedPolls, userId);Also applies to: 433-440, 470-477, 510-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/poll/Poll.dto.ts(2 hunks)src/poll/poll.service.ts(4 hunks)
🔇 Additional comments (2)
src/poll/Poll.dto.ts (2)
16-20: Good use of enum for type safety!The introduction of the
PollSortByenum improves type safety and maintainability by replacing string literals with strongly typed values.
124-125: Consistent use of enum for validation!The update to use
PollSortByenum with@IsEnumdecorator ensures type safety at both compile-time and runtime.
Comment on lines
388
to
445
| if (sortBy === PollSortBy.PARTICIPANT_COUNT) { | ||
| // For participantCount: show active polls first (by highest voter count), then ended polls | ||
| const activeFilters = { | ||
| ...filters, | ||
| startDate: { lte: now }, | ||
| endDate: { gt: now }, | ||
| } | ||
|
|
||
| const endedFilters = { | ||
| ...filters, | ||
| endDate: { lte: now }, | ||
| } | ||
|
|
||
| const [activePolls, endedPolls, total] = | ||
| await this.databaseService.$transaction([ | ||
| this.databaseService.poll.findMany({ | ||
| where: activeFilters, | ||
| include: { | ||
| author: true, | ||
| votes: { | ||
| where: { userId }, | ||
| select: { voteID: true }, | ||
| }, | ||
| }, | ||
| orderBy: { participantCount: 'desc' }, // Highest voter count first for active polls | ||
| take: Number(limit) + skip, | ||
| }), | ||
| this.databaseService.poll.findMany({ | ||
| where: endedFilters, | ||
| include: { | ||
| author: true, | ||
| votes: { | ||
| where: { userId }, | ||
| select: { voteID: true }, | ||
| }, | ||
| }, | ||
| orderBy: { participantCount: 'desc' }, // Highest voter count first for ended polls too | ||
| take: Number(limit) + skip, | ||
| }), | ||
| this.databaseService.poll.count({ where: filters }), | ||
| ]) | ||
|
|
||
| const combinedPolls = [...activePolls, ...endedPolls] | ||
| const paginatedPolls = combinedPolls.slice(skip, skip + Number(limit)) | ||
|
|
||
| const pollsWithVoteStatus = paginatedPolls.map(poll => { | ||
| const { votes, ...pollWithoutVotes } = poll | ||
|
|
||
| return { | ||
| ...pollWithoutVotes, | ||
| hasVoted: votes.length > 0, | ||
| } | ||
| }) | ||
|
|
||
| return { | ||
| polls: pollsWithVoteStatus, | ||
| total, | ||
| } |
There was a problem hiding this comment.
The participant count sorting has two issues to address.
- The
sortOrderparameter is ignored - both active and ended polls are always sorted by'desc'regardless of the user's preference. - Future polls (where
startDate > now) might be incorrectly included with ended polls.
Apply this diff to fix both issues:
// For participantCount: show active polls first (by highest voter count), then ended polls
const activeFilters = {
...filters,
startDate: { lte: now },
endDate: { gt: now },
}
const endedFilters = {
...filters,
endDate: { lte: now },
+ startDate: { lte: now }, // Exclude future polls
}
const [activePolls, endedPolls, total] =
await this.databaseService.$transaction([
this.databaseService.poll.findMany({
where: activeFilters,
include: {
author: true,
votes: {
where: { userId },
select: { voteID: true },
},
},
- orderBy: { participantCount: 'desc' }, // Highest voter count first for active polls
+ orderBy: { participantCount: sortOrder },
take: Number(limit) + skip,
}),
this.databaseService.poll.findMany({
where: endedFilters,
include: {
author: true,
votes: {
where: { userId },
select: { voteID: true },
},
},
- orderBy: { participantCount: 'desc' }, // Highest voter count first for ended polls too
+ orderBy: { participantCount: sortOrder },
take: Number(limit) + skip,
}),
this.databaseService.poll.count({ where: filters }),
])📝 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 (sortBy === PollSortBy.PARTICIPANT_COUNT) { | |
| // For participantCount: show active polls first (by highest voter count), then ended polls | |
| const activeFilters = { | |
| ...filters, | |
| startDate: { lte: now }, | |
| endDate: { gt: now }, | |
| } | |
| const endedFilters = { | |
| ...filters, | |
| endDate: { lte: now }, | |
| } | |
| const [activePolls, endedPolls, total] = | |
| await this.databaseService.$transaction([ | |
| this.databaseService.poll.findMany({ | |
| where: activeFilters, | |
| include: { | |
| author: true, | |
| votes: { | |
| where: { userId }, | |
| select: { voteID: true }, | |
| }, | |
| }, | |
| orderBy: { participantCount: 'desc' }, // Highest voter count first for active polls | |
| take: Number(limit) + skip, | |
| }), | |
| this.databaseService.poll.findMany({ | |
| where: endedFilters, | |
| include: { | |
| author: true, | |
| votes: { | |
| where: { userId }, | |
| select: { voteID: true }, | |
| }, | |
| }, | |
| orderBy: { participantCount: 'desc' }, // Highest voter count first for ended polls too | |
| take: Number(limit) + skip, | |
| }), | |
| this.databaseService.poll.count({ where: filters }), | |
| ]) | |
| const combinedPolls = [...activePolls, ...endedPolls] | |
| const paginatedPolls = combinedPolls.slice(skip, skip + Number(limit)) | |
| const pollsWithVoteStatus = paginatedPolls.map(poll => { | |
| const { votes, ...pollWithoutVotes } = poll | |
| return { | |
| ...pollWithoutVotes, | |
| hasVoted: votes.length > 0, | |
| } | |
| }) | |
| return { | |
| polls: pollsWithVoteStatus, | |
| total, | |
| } | |
| if (sortBy === PollSortBy.PARTICIPANT_COUNT) { | |
| // For participantCount: show active polls first (by highest voter count), then ended polls | |
| const activeFilters = { | |
| ...filters, | |
| startDate: { lte: now }, | |
| endDate: { gt: now }, | |
| } | |
| const endedFilters = { | |
| ...filters, | |
| endDate: { lte: now }, | |
| startDate: { lte: now }, // Exclude future polls | |
| } | |
| const [activePolls, endedPolls, total] = | |
| await this.databaseService.$transaction([ | |
| this.databaseService.poll.findMany({ | |
| where: activeFilters, | |
| include: { | |
| author: true, | |
| votes: { | |
| where: { userId }, | |
| select: { voteID: true }, | |
| }, | |
| }, | |
| orderBy: { participantCount: sortOrder }, | |
| take: Number(limit) + skip, | |
| }), | |
| this.databaseService.poll.findMany({ | |
| where: endedFilters, | |
| include: { | |
| author: true, | |
| votes: { | |
| where: { userId }, | |
| select: { voteID: true }, | |
| }, | |
| }, | |
| orderBy: { participantCount: sortOrder }, | |
| take: Number(limit) + skip, | |
| }), | |
| this.databaseService.poll.count({ where: filters }), | |
| ]) | |
| const combinedPolls = [...activePolls, ...endedPolls] | |
| const paginatedPolls = combinedPolls.slice(skip, skip + Number(limit)) | |
| const pollsWithVoteStatus = paginatedPolls.map(poll => { | |
| const { votes, ...pollWithoutVotes } = poll | |
| return { | |
| ...pollWithoutVotes, | |
| hasVoted: votes.length > 0, | |
| } | |
| }) | |
| return { | |
| polls: pollsWithVoteStatus, | |
| total, | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/poll/poll.service.ts around lines 388 to 445, the participant count
sorting ignores the sortOrder parameter and always sorts descending, and future
polls are incorrectly included with ended polls. Fix this by using the provided
sortOrder value instead of hardcoding 'desc' in both active and ended polls
queries. Also, exclude future polls from the ended polls query by adding a
condition to ensure startDate is less than or equal to now, so only truly ended
polls are included.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
New Features
Improvements