add api to fetch toggles #901
Conversation
calellowitz
requested review from
ajeety4,
hemant10yadav,
sravfeyn and
zandre-eng
WalkthroughAdds a test factory Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/users/tests/test_views.py (1)
222-252: LGTM! Tests provide good coverage for the new endpoint.The tests correctly verify both the empty state and the presence of toggles, with proper authentication mocking using
ClientProtectedResourceMixin.Note: The
data={"username": "abc"}parameter passed in line 225 and 241 aligns with the PR description that user arguments are accepted but currently ignored until non-global toggles are implemented.Optional: Consider adding tests for additional scenarios
For more comprehensive coverage, you might consider adding tests for:
- Multiple switches with different active states
- Inactive switches (by overriding
SwitchFactory(active=False))These are not critical for the current implementation but would provide better coverage as the feature evolves.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commcare_connect/flags/tests/factories.py(1 hunks)commcare_connect/users/tests/test_views.py(3 hunks)commcare_connect/users/urls.py(2 hunks)commcare_connect/users/views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
commcare_connect/users/urls.py (1)
commcare_connect/users/views.py (1)
UserToggleView(231-234)
commcare_connect/users/tests/test_views.py (2)
commcare_connect/flags/tests/factories.py (1)
SwitchFactory(5-10)commcare_connect/users/views.py (4)
UserToggleView(231-234)get(126-138)get(222-227)get(232-234)
🪛 Ruff (0.14.8)
commcare_connect/users/views.py
232-232: Unused method argument: request
(ARG002)
232-232: Unused method argument: args
(ARG002)
232-232: Unused method argument: kwargs
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (6)
commcare_connect/users/urls.py (2)
10-10: LGTM!The import is correctly added and follows the existing code style.
31-31: LGTM!The URL pattern is correctly configured and follows the existing conventions in this file.
commcare_connect/users/views.py (2)
24-24: LGTM!The Switch model import from django-waffle is correctly added and used by the new UserToggleView.
230-234: LGTM! View correctly returns all global feature toggles.The implementation is straightforward and aligns with the PR description that user-specific filtering will be added later when non-global toggles are implemented.
Note: The static analysis warning about unused method arguments (
request,args,kwargs) is a false positive—these are part of Django's standard view method signature.commcare_connect/flags/tests/factories.py (1)
1-10: LGTM! Well-structured test factory.The
SwitchFactoryis correctly implemented using factory_boy patterns. The name generation using Faker'sbothifycreates unique, easily identifiable test data, and defaultingactivetoTrueis sensible for the current test scenarios.commcare_connect/users/tests/test_views.py (1)
1-1: LGTM!The new imports (
json,SwitchFactory, andUserToggleView) are correctly added and necessary for the test implementation.Also applies to: 14-14, 19-19
commcare_connect/users/views.py
Outdated
| @method_decorator(csrf_exempt, name="dispatch") | ||
| class UserToggleView(ClientProtectedResourceMixin, View): | ||
| def get(self, request, *args, **kwargs): | ||
| toggles = list(Switch.objects.all().values("name", "active")) |
There was a problem hiding this comment.
Nit: Should we add a comment here that this view expects either a username or phone number, but it won't be used until we have segmented toggles?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
commcare_connect/users/tests/test_views.py(3 hunks)commcare_connect/users/views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
commcare_connect/users/tests/test_views.py (2)
commcare_connect/flags/tests/factories.py (1)
SwitchFactory(5-10)commcare_connect/users/views.py (4)
UserToggleView(231-236)get(126-138)get(222-227)get(234-236)
🪛 Ruff (0.14.8)
commcare_connect/users/views.py
234-234: Unused method argument: request
(ARG002)
234-234: Unused method argument: args
(ARG002)
234-234: Unused method argument: kwargs
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (4)
commcare_connect/users/tests/test_views.py (2)
1-1: LGTM!The imports are correctly added to support the new toggle view tests.
Also applies to: 14-14, 19-19
223-236: LGTM!The test correctly verifies that an empty toggles list is returned when no switches exist.
commcare_connect/users/views.py (2)
24-24: LGTM!The import is correctly added to support the new toggle view.
230-236: LGTM!The implementation correctly returns all feature toggles as JSON. The view is appropriately protected with OAuth2 authentication via
ClientProtectedResourceMixin, and the comment clearly explains that user-specific parameters are accepted but not yet used.Note: The static analysis warnings about unused method arguments are false positives—Django's view method signature requires these parameters even when not explicitly used in the method body.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/users/tests/test_views.py (1)
238-256: Nice fix! Previous datetime comparison issue resolved.The test now correctly validates field presence rather than comparing serialized datetime strings to datetime objects, which resolves the issue flagged in the previous review.
The test structure is clean and validates the core response format. Consider adding test cases for:
- Multiple toggles to ensure the endpoint returns all switches
- Inactive toggles (
active=False) to validate that the active field correctly reflects switch stateThese additions would improve coverage of the endpoint's behavior.
Example test case for multiple toggles
def test_multiple_toggles(self, mobile_user: User, rf: RequestFactory): SwitchFactory(name="TOGGLE_A", active=True) SwitchFactory(name="TOGGLE_B", active=False) user_toggle_view = UserToggleView.as_view() request = rf.get("/fake-url/", data={"username": "abc"}) request.user = mobile_user with mock.patch( "oauth2_provider.views.mixins.ClientProtectedResourceMixin.authenticate_client" ) as authenticate_client: authenticate_client.return_value = True response = user_toggle_view(request) data = json.loads(response.content) assert response.status_code == 200 assert len(data["toggles"]) == 2 toggle_names = {t["name"] for t in data["toggles"]} assert toggle_names == {"TOGGLE_A", "TOGGLE_B"}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commcare_connect/users/tests/test_views.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (2)
commcare_connect/users/tests/test_views.py (2)
1-1: LGTM! Clean import additions.The new imports are necessary for the toggle tests and properly organized with the existing imports.
Also applies to: 14-14, 19-19
223-236: LGTM! Good coverage of the empty state.The test properly validates the endpoint's behavior when no toggles are configured, including authentication and response structure.
Technical Summary
This adds an API for the personalid server to call to fetch a user's toggles, so that mobile only needs to make one call to get full toggle state. It accepts user arguments, but those are ignored until we implement toggles that are not global.
Linked connectid PR dimagi/connect-id#213
Safety Assurance
Safety story
Tests are added for the new view and it is not yet used.
Automated test coverage
Tests added
QA Plan
Will be QAed when the mobile functionality is complete.
Labels & Review