Conversation
Summary by CodeRabbit
WalkthroughThis change introduces a new statistic for the Slack workspace total members count, visible on the main page alongside existing counters. It adds a new field and logic to the backend to store and expose this count, updates the frontend to display it, and includes new tests and mock data to support and verify the feature. Changes
Assessment against linked issues
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. ✨ 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
Documentation and Community
|
github-actions
bot
added
docs
|
What? How did the rebase happen? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/apps/slack/models/workspace.py (1)
19-19: Consider adding a docstring to the new fieldThe
total_members_countfield looks good and uses an appropriate type (PositiveIntegerField) with a sensible default value. Consider adding a docstring to document its purpose and update behavior, similar to other fields in this model.- total_members_count = models.PositiveIntegerField(default=0) + total_members_count = models.PositiveIntegerField( + verbose_name="Total Members Count", + default=0, + help_text="The total number of members in the Slack workspace", + )backend/apps/slack/management/commands/slack_sync_data.py (1)
105-107: Consider optimizing database writes during sync.The implementation correctly updates the workspace's total members count during the pagination loop. However, calling
workspace.save()after every batch might lead to excessive database writes when processing large workspaces with many member batches.Consider saving the workspace only once after all batches are processed, unless there's a specific requirement to have real-time updates during sync:
- # Set workspace total members count - workspace.total_members_count = total_members - workspace.save() + # Update workspace total members count in memory + workspace.total_members_count = total_membersThen add after line 110 (after the pagination loop):
# Save the final total members count workspace.save()backend/apps/owasp/graphql/nodes/stats.py (1)
10-10: Note field reordering.The order of field declarations has changed, moving
active_projects_statsto the second position. While this has no functional impact (GraphQL doesn't rely on field definition order), it's worth noting in case there was a specific organizational intent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/apps/owasp/graphql/nodes/stats.py(1 hunks)backend/apps/owasp/graphql/queries/stats.py(2 hunks)backend/apps/slack/management/commands/slack_sync_data.py(1 hunks)backend/apps/slack/migrations/0010_workspace_total_members_count.py(1 hunks)backend/apps/slack/models/workspace.py(1 hunks)frontend/__tests__/e2e/data/mockHomeData.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(2 hunks)frontend/__tests__/unit/data/mockHomeData.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(2 hunks)frontend/src/app/page.tsx(2 hunks)frontend/src/server/queries/homeQueries.ts(1 hunks)frontend/src/types/home.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/app/page.tsx (1)
frontend/src/components/AnimatedCounter.tsx (1)
AnimatedCounter(10-35)
frontend/__tests__/e2e/pages/Home.spec.ts (1)
frontend/__tests__/e2e/data/mockHomeData.ts (1)
mockHomeData(1-286)
🪛 GitHub Actions: Run CI/CD
backend/apps/owasp/graphql/queries/stats.py
[error] 13-13: CSpell: Unknown word 'MINROUNDED' found.
[error] 33-33: CSpell: Unknown word 'MINROUNDED' found.
🔇 Additional comments (14)
frontend/src/types/home.ts (1)
41-41: LGTM! Type definition for Slack member countThe addition of the
workspaceStatsfield to thestatsOverviewtype is correctly implemented as a number type, following the naming pattern of other stats fields.frontend/__tests__/e2e/data/mockHomeData.ts (1)
245-245: LGTM! Mock data for Slack member countThe mock value for
workspaceStatsis appropriately added to support testing of the new Slack members count feature.backend/apps/slack/migrations/0010_workspace_total_members_count.py (1)
1-17: LGTM! Migration for adding Slack members count fieldThe migration is properly structured with the correct dependency and field definition. It follows Django migration best practices by focusing on a single, well-defined change.
frontend/src/server/queries/homeQueries.ts (1)
83-83: LGTM! Added workspaceStats field to GraphQL query.The addition of
workspaceStatsto thestatsOverviewselection set aligns with the PR objective to add Slack total members count. This change will enable the frontend to fetch the Slack workspace member count data from the backend.frontend/__tests__/unit/data/mockHomeData.ts (1)
120-124: LGTM! Updated mock data with new workspaceStats field.The mock data has been properly updated with the new
workspaceStatsfield to support testing the Slack members count feature. The values for other statistics have also been adjusted to more realistic figures.backend/apps/owasp/graphql/nodes/stats.py (1)
13-13: LGTM! Added workspace_stats field to StatsNode.The addition of the
workspace_statsfield to theStatsNodeGraphQL object type correctly exposes the Slack workspace member count through the API. The integer type is appropriate for this numeric statistic.frontend/src/app/page.tsx (3)
115-132: Added Slack Members statistics to counter data.The code adds a new statistic "Slack Members" to the existing stats display, which aligns with the PR objective to track Slack membership counts.
359-359: Grid layout adjusted to accommodate new statistics.The layout has been appropriately changed from
md:grid-cols-4tolg:grid-cols-5to fit the new Slack Members counter.
364-364: Simplified AnimatedCounter implementation.The code now passes raw numeric values directly to the AnimatedCounter component instead of using parseInt, which is cleaner and more efficient.
frontend/__tests__/unit/pages/Home.test.tsx (1)
5-5: Added millify import for number formatting.The millify library is correctly imported for number formatting in the test.
frontend/__tests__/e2e/pages/Home.spec.ts (1)
3-3: Added millify import for E2E tests.The millify library is correctly imported for formatting numbers in the E2E test.
backend/apps/owasp/graphql/queries/stats.py (3)
3-4: Added required imports for new functionality.The imports for
floorfrommathmodule andWorkspacemodel are necessary for the new functionality.Also applies to: 11-12
21-36: Good refactoring with general_round_down method.The new static method centralizes the rounding logic, which improves code maintainability and consistency.
🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 33-33: CSpell: Unknown word 'MINROUNDED' found.
61-65: Consistent use of the new rounding method.All statistics are now consistently rounded using the new
general_round_downmethod, which improves code maintainability.
| test('renders stats correctly', async () => { | ||
| render(<Home />) | ||
|
|
||
| const headers = [ | ||
| 'Active Projects', | ||
| 'Local Chapters', | ||
| 'Contributors', | ||
| 'Countries', | ||
| 'Slack Members', | ||
| ] | ||
| const stats = mockGraphQLData.statsOverview | ||
|
|
||
| await waitFor(() => { | ||
| headers.map((header) => expect(screen.getByText(header)).toBeInTheDocument()) | ||
| // Wait for 2 seconds | ||
| setTimeout(() => { | ||
| Object.values(stats).forEach((value) => | ||
| expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument() | ||
| ) | ||
| }, 2000) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test implementation for stats needs improvement.
While the test correctly verifies the presence of all statistics including the new "Slack Members" stat, the setTimeout approach can lead to flaky tests.
Replace the setTimeout with a proper waitFor implementation:
- // Wait for 2 seconds
- setTimeout(() => {
- Object.values(stats).forEach((value) =>
- expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument()
- )
- }, 2000)
+ // Wait for all stats to be rendered
+ await Promise.all(
+ Object.values(stats).map(async (value) => {
+ await waitFor(() => {
+ expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument()
+ })
+ })
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/__tests__/unit/pages/Home.test.tsx around lines 261 to 282, the test
uses setTimeout inside waitFor to delay assertions, which can cause flaky tests.
Remove the setTimeout and instead use waitFor to directly wait for the expected
elements with the stats values to appear. This means inside waitFor, assert that
each stat value with millify formatting plus '+' is present in the document
without any manual delay.
ahmedxgouda
force-pushed
the
feature/slack-total-members-count
branch
from
8cc068f to
7e2c036
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/common/utils.py (1)
104-115: Function implementation looks good, but consider adding input validation.The
round_downfunction is well-implemented with clear documentation and correct mathematical logic. However, consider adding input validation to handle edge cases.Consider adding input validation to handle edge cases:
def round_down(value: int, base: int) -> int: """Round down the stats to the nearest base. Args: value: The value to round down. base: The base to round down to. Returns: int: The rounded down value. + Raises: + ValueError: If base is zero or negative. """ + if base <= 0: + raise ValueError("Base must be a positive integer") return value - (value % base)backend/tests/apps/common/utils_test.py (1)
84-101: Comprehensive test coverage with excellent parametrization.The test cases are well-designed and cover various rounding scenarios effectively. All mathematical calculations are correct and the parametrized approach ensures good coverage.
Consider adding edge case tests to complement the existing coverage:
+ @pytest.mark.parametrize( + ("value", "base", "expected_exception"), + [ + (100, 0, ValueError), + (100, -1, ValueError), + ], + ) + def test_round_down_edge_cases(self, value, base, expected_exception): + with pytest.raises(expected_exception): + round_down(value, base) + + def test_round_down_negative_value(self): + # Test behavior with negative values + assert round_down(-15, 10) == -20
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/common/utils.py(1 hunks)backend/apps/owasp/graphql/nodes/stats.py(1 hunks)backend/apps/owasp/graphql/queries/stats.py(2 hunks)backend/apps/slack/management/commands/slack_sync_data.py(1 hunks)backend/apps/slack/models/workspace.py(1 hunks)backend/tests/apps/common/utils_test.py(2 hunks)frontend/__tests__/e2e/data/mockHomeData.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/data/mockHomeData.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(2 hunks)frontend/src/app/page.tsx(2 hunks)frontend/src/server/queries/homeQueries.ts(1 hunks)frontend/src/types/home.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/server/queries/homeQueries.ts
- frontend/src/types/home.ts
- frontend/tests/unit/data/mockHomeData.ts
- backend/apps/slack/management/commands/slack_sync_data.py
- backend/apps/owasp/graphql/queries/stats.py
- frontend/tests/e2e/data/mockHomeData.ts
- frontend/tests/e2e/pages/Home.spec.ts
- backend/apps/slack/models/workspace.py
- frontend/tests/unit/pages/Home.test.tsx
- backend/apps/owasp/graphql/nodes/stats.py
- frontend/src/app/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/common/utils_test.py (1)
backend/apps/common/utils.py (1)
round_down(104-115)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/common/utils_test.py (1)
13-13: Import addition looks good.The import for
round_downis correctly added to the test file.
| await waitFor(() => { | ||
| headers.forEach((header) => expect(screen.getByText(header)).toBeInTheDocument()) | ||
| // Wait for 2 seconds | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This doesn't look right but I'm not sure how to make that better 🤷♂️
* Add total_members_count field to Workspace model and update Slack sync command * Update pnpm-lock.yaml * Update member details page title * Update member details page layout * Update CONTRIBUTING.md * Update CONTRIBUTING.md formatting * Add workspace stats to the stats node and query * Update frontend * Refactore conterData * Update frontend tests * Fix spelling * Apply make-check * Update frontend tests * Use forEach instead of map * Apply coderabbitai suggestion * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
* Add body to the milestone node * Add progress to the milestone node * Update frontend * Update backend tests * Add slack total members count (#1508) * Add total_members_count field to Workspace model and update Slack sync command * Update pnpm-lock.yaml * Update member details page title * Update member details page layout * Update CONTRIBUTING.md * Update CONTRIBUTING.md formatting * Add workspace stats to the stats node and query * Update frontend * Refactore conterData * Update frontend tests * Fix spelling * Apply make-check * Update frontend tests * Use forEach instead of map * Apply coderabbitai suggestion * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> * Update Frontend tests * Apply coderabbitai suggestion * Update frontend mock data * Refactor frontend * Apply suggestions * Revert docker change * Add tooltip * Update frontend * Fix bug * Update progress icon location and color * Fix tests --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Kate Golovanova <kate@kgthreads.com>
2 participants
Resolves #1502