-
Notifications
You must be signed in to change notification settings - Fork 0
feat: filter website communities data based on platform activation st… #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…atus and add corresponding tests
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Module
participant PlatformLoop
Caller->>Module: call get_learning_platforms(platforms)
Module->>+PlatformLoop: iterate over each platform
alt platform["metadata"]["activated"] is True
PlatformLoop-->>Module: append community data
else platform not activated
PlatformLoop-->>Module: skip community data
end
Module->>Caller: return filtered communities_data
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_module_website.py (1)
214-219: Small documentation improvementThe test docstring could be more precise about what's being tested.
- Test get_learning_platforms when a community has multiple platforms but one platform is disabled - Verifies that only website platform data is returned even when - other platform types exist. + Test get_learning_platforms when a community has multiple website platforms but one is disabled. + Verifies that only the activated website platform's data is returned, and the + deactivated platform is excluded from results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hivemind_etl/website/module.py(1 hunks)tests/integration/test_module_website.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/integration/test_module_website.py (1)
hivemind_etl/website/module.py (1) (1)
get_learning_platforms(11:74)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci / test / Test
- GitHub Check: ci / lint / Lint
🔇 Additional comments (2)
tests/integration/test_module_website.py (2)
51-53: LGTM - Good test updatesThe metadata structure has been properly updated in all relevant test cases to include the "activated" flag, which aligns with the changes made to the module implementation.
Also applies to: 107-109, 181-183, 188-190
214-288: Great negative test caseThis new test case properly validates that deactivated platforms are excluded from the results, which is an important part of the new filtering functionality.
One minor suggestion: consider adding a more specific assertion message to make test failures more descriptive:
- self.assertEqual(len(result), 1) + self.assertEqual(len(result), 1, "Only one activated platform should be returned")
…atus and add corresponding tests
Summary by CodeRabbit