fetch toggles #213
fetch toggles #213
Conversation
calellowitz
force-pushed
the
ce/fetch-toggles
branch
from
46663cb to
7e1540b
Compare
calellowitz
force-pushed
the
ce/fetch-toggles
branch
from
7e1540b to
40883cc
Compare
calellowitz
requested review from
ajeety4,
hemant10yadav,
sravfeyn and
zandre-eng
WalkthroughAdds Connect toggles integration and surfaces merged toggle data in API responses. Introduces CONNECT_TOGGLES_URL constant in settings. Adds utils/connect.get_connect_toggles(...) to fetch toggles from Connect (uses timeout=60 and returns a mapping of toggle name → {active, created_at, modified_at}). Adds flags.utils.get_user_toggles(...) to combine local waffle Switch states with Connect toggles. flags.views and users.views are updated to call get_user_toggles and include a "toggles" field in JSON responses. Tests in flags and users add autouse fixtures that mock the new toggle-fetching calls. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
connectid/settings.py(1 hunks)flags/tests/test_views.py(2 hunks)flags/utils.py(1 hunks)flags/views.py(1 hunks)users/tests/test_views.py(2 hunks)users/views.py(2 hunks)utils/connect.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: zandre-eng
Repo: dimagi/connect-id PR: 206
File: flags/views.py:1-10
Timestamp: 2025-12-01T11:26:51.596Z
Learning: In the connect-id repository, the toggles endpoint (TogglesView in flags/views.py) is intentionally a public, unauthenticated endpoint that exposes all waffle switches. This is by design and does not contain sensitive user data.
📚 Learning: 2025-12-01T11:26:51.596Z
Learnt from: zandre-eng
Repo: dimagi/connect-id PR: 206
File: flags/views.py:1-10
Timestamp: 2025-12-01T11:26:51.596Z
Learning: In the connect-id repository, the toggles endpoint (TogglesView in flags/views.py) is intentionally a public, unauthenticated endpoint that exposes all waffle switches. This is by design and does not contain sensitive user data.
Applied to files:
connectid/settings.pyusers/views.pyflags/tests/test_views.pyflags/utils.pyflags/views.py
🧬 Code graph analysis (4)
users/views.py (1)
flags/utils.py (1)
get_user_toggles(6-11)
flags/utils.py (1)
utils/connect.py (1)
get_connect_toggles(24-34)
utils/connect.py (1)
flags/views.py (1)
get(8-13)
flags/views.py (1)
flags/utils.py (1)
get_user_toggles(6-11)
🪛 Ruff (0.14.8)
utils/connect.py
32-32: Probable use of requests call without timeout
(S113)
🔇 Additional comments (1)
flags/utils.py (1)
6-11: Good integration of local and remote toggles.The function cleanly merges local waffle switches with Connect toggles, with remote toggles taking precedence via the
update()call. This approach allows remote configuration to override local defaults.Note: This function's resilience depends on
get_connect_toggleshaving proper error handling (see comment on utils/connect.py). Once that's addressed, this will gracefully fall back to local-only toggles if the Connect service is unavailable.
utils/connect.py
Outdated
| def get_connect_toggles(username=None, phone_number=None): | ||
| url = settings.CONNECT_TOGGLES_URL | ||
| auth = (settings.COMMCARE_CONNECT_CLIENT_ID, settings.COMMCARE_CONNECT_CLIENT_SECRET) | ||
| params = {} | ||
| if username is not None: | ||
| params["username"] = username | ||
| elif phone_number is not None: | ||
| params["phone_number"] = phone_number | ||
| response = requests.get(url, auth=auth, params=params) | ||
| data = response.json() | ||
| return {toggle["name"]: toggle["active"] for toggle in data.get("toggles", {})} |
There was a problem hiding this comment.
Add timeout and error handling to prevent service disruption.
The requests.get call on line 32 lacks a timeout parameter, which can cause the application to hang indefinitely if the Connect service is slow or unresponsive. Additionally, there's no error handling for network failures or unexpected response formats.
If the Connect service is unavailable, this will break the toggles endpoint for all users. Consider adding:
- A timeout parameter (e.g., 5-10 seconds)
- Error handling with graceful fallback to return an empty dict
🔎 Proposed fix with timeout and error handling
def get_connect_toggles(username=None, phone_number=None):
url = settings.CONNECT_TOGGLES_URL
auth = (settings.COMMCARE_CONNECT_CLIENT_ID, settings.COMMCARE_CONNECT_CLIENT_SECRET)
params = {}
if username is not None:
params["username"] = username
elif phone_number is not None:
params["phone_number"] = phone_number
- response = requests.get(url, auth=auth, params=params)
- data = response.json()
- return {toggle["name"]: toggle["active"] for toggle in data.get("toggles", {})}
+ try:
+ response = requests.get(url, auth=auth, params=params, timeout=10)
+ response.raise_for_status()
+ data = response.json()
+ toggles = data.get("toggles", [])
+ if not isinstance(toggles, list):
+ return {}
+ return {toggle["name"]: toggle["active"] for toggle in toggles if "name" in toggle and "active" in toggle}
+ except (requests.RequestException, ValueError, KeyError) as e:
+ # Log the error but don't fail - return empty dict to allow local toggles to work
+ return {}📝 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.
| def get_connect_toggles(username=None, phone_number=None): | |
| url = settings.CONNECT_TOGGLES_URL | |
| auth = (settings.COMMCARE_CONNECT_CLIENT_ID, settings.COMMCARE_CONNECT_CLIENT_SECRET) | |
| params = {} | |
| if username is not None: | |
| params["username"] = username | |
| elif phone_number is not None: | |
| params["phone_number"] = phone_number | |
| response = requests.get(url, auth=auth, params=params) | |
| data = response.json() | |
| return {toggle["name"]: toggle["active"] for toggle in data.get("toggles", {})} | |
| def get_connect_toggles(username=None, phone_number=None): | |
| url = settings.CONNECT_TOGGLES_URL | |
| auth = (settings.COMMCARE_CONNECT_CLIENT_ID, settings.COMMCARE_CONNECT_CLIENT_SECRET) | |
| params = {} | |
| if username is not None: | |
| params["username"] = username | |
| elif phone_number is not None: | |
| params["phone_number"] = phone_number | |
| try: | |
| response = requests.get(url, auth=auth, params=params, timeout=10) | |
| response.raise_for_status() | |
| data = response.json() | |
| toggles = data.get("toggles", []) | |
| if not isinstance(toggles, list): | |
| return {} | |
| return {toggle["name"]: toggle["active"] for toggle in toggles if "name" in toggle and "active" in toggle} | |
| except (requests.RequestException, ValueError, KeyError) as e: | |
| # Log the error but don't fail - return empty dict to allow local toggles to work | |
| return {} |
🧰 Tools
🪛 Ruff (0.14.8)
32-32: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In utils/connect.py around lines 24 to 34, the requests.get call lacks a timeout
and has no error handling, which can hang or raise on network failures or bad
responses; update the function to call requests.get with a timeout (e.g., 5–10s
or a CONNECT_TIMEOUT setting), wrap the HTTP call and JSON parsing in a
try/except that catches requests.RequestException and ValueError (for invalid
JSON), optionally check response.ok or call response.raise_for_status(), log the
error if a logger is available, and return an empty dict as a graceful fallback
when any network/HTTP/JSON error occurs.
There was a problem hiding this comment.
@calellowitz
Curious to hear your thoughts on this. I feel that having a timeout (with a conservative value like 60 seconds) as a safeguard is good to have for this and other Connect APIs, as it helps avoid indefinite hangs in worst-case scenarios.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
that seems reasonable to me
utils/connect.py
Outdated
| def get_connect_toggles(username=None, phone_number=None): | ||
| url = settings.CONNECT_TOGGLES_URL | ||
| auth = (settings.COMMCARE_CONNECT_CLIENT_ID, settings.COMMCARE_CONNECT_CLIENT_SECRET) | ||
| params = {} | ||
| if username is not None: | ||
| params["username"] = username | ||
| elif phone_number is not None: | ||
| params["phone_number"] = phone_number | ||
| response = requests.get(url, auth=auth, params=params) | ||
| data = response.json() | ||
| return {toggle["name"]: toggle["active"] for toggle in data.get("toggles", {})} |
There was a problem hiding this comment.
@calellowitz
Curious to hear your thoughts on this. I feel that having a timeout (with a conservative value like 60 seconds) as a safeguard is good to have for this and other Connect APIs, as it helps avoid indefinite hangs in worst-case scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
flags/tests/test_views.py (1)
14-18: Consider adding tests for Connect toggle integration.The mock fixture correctly isolates these tests from the external API, but consider adding separate test cases that verify:
- The merge logic when connect toggles are present
- Connect toggles correctly override local switches with the same name
- Error handling when
get_connect_togglesfails
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flags/tests/test_views.py(3 hunks)flags/utils.py(1 hunks)utils/connect.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/connect.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: zandre-eng
Repo: dimagi/connect-id PR: 206
File: flags/views.py:1-10
Timestamp: 2025-12-01T11:26:51.596Z
Learning: In the connect-id repository, the toggles endpoint (TogglesView in flags/views.py) is intentionally a public, unauthenticated endpoint that exposes all waffle switches. This is by design and does not contain sensitive user data.
📚 Learning: 2025-12-01T11:26:51.596Z
Learnt from: zandre-eng
Repo: dimagi/connect-id PR: 206
File: flags/views.py:1-10
Timestamp: 2025-12-01T11:26:51.596Z
Learning: In the connect-id repository, the toggles endpoint (TogglesView in flags/views.py) is intentionally a public, unauthenticated endpoint that exposes all waffle switches. This is by design and does not contain sensitive user data.
Applied to files:
flags/tests/test_views.pyflags/utils.py
🧬 Code graph analysis (1)
flags/utils.py (1)
utils/connect.py (1)
get_connect_toggles(24-41)
🔇 Additional comments (2)
flags/utils.py (1)
13-13: Verify that Connect toggles should override local switches.The
toggles.update(connect_switches)call means that toggles from Connect will silently override local switches with the same name. Please confirm this precedence is intentional and aligns with the expected behavior.flags/tests/test_views.py (1)
31-32: LGTM!The assertions correctly reflect the new nested data structure where each toggle is a dictionary containing an "active" field.
|
|
||
| def get_user_toggles(username=None, phone_number=None): | ||
| switches = Switch.objects.all() | ||
| connect_switches = get_connect_toggles(username=username, phone_number=phone_number) |
There was a problem hiding this comment.
Add error handling for the external Connect API call.
The call to get_connect_toggles can fail due to network errors, timeouts, or API errors. Without error handling, any failure will propagate and break the entire toggles endpoint. Since the toggles endpoint is public and affects all users, this creates a critical reliability risk.
Consider wrapping the call in a try-except block to gracefully handle failures and fall back to local switches only.
🔎 Proposed fix with error handling
def get_user_toggles(username=None, phone_number=None):
switches = Switch.objects.all()
- connect_switches = get_connect_toggles(username=username, phone_number=phone_number)
+ try:
+ connect_switches = get_connect_toggles(username=username, phone_number=phone_number)
+ except Exception:
+ # Log the error and fall back to local switches only
+ connect_switches = {}
toggles = {
switch.name: {"active": switch.active, "created_at": switch.created, "modified_at": switch.modified}
for switch in switches
}
toggles.update(connect_switches)
return toggles📝 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.
| connect_switches = get_connect_toggles(username=username, phone_number=phone_number) | |
| def get_user_toggles(username=None, phone_number=None): | |
| switches = Switch.objects.all() | |
| try: | |
| connect_switches = get_connect_toggles(username=username, phone_number=phone_number) | |
| except Exception: | |
| # Log the error and fall back to local switches only | |
| connect_switches = {} | |
| toggles = { | |
| switch.name: {"active": switch.active, "created_at": switch.created, "modified_at": switch.modified} | |
| for switch in switches | |
| } | |
| toggles.update(connect_switches) | |
| return toggles |
🤖 Prompt for AI Agents
In flags/utils.py around line 8, the call to get_connect_toggles can raise
network/API errors and should be wrapped in a try-except; catch broad exceptions
(or specific network/HTTP exceptions if available), log the error with context
(including username/phone_number and exception message), and on failure set
connect_switches to an empty dict (or the local-switches-only default used
elsewhere) so the function falls back to local switches only instead of
propagating the exception; do not re-raise the error so the public toggles
endpoint remains available.
|
Updated the APIs based on changes in the spec but should be good to go now. I don't think the coderabbit feedback is correct in this case, as I think we would rather it fail hard, when it does. It isn't blocking and getting partial flag info may cause issues |
Technical Summary
In order to sync toggles across both systems to the phone in one call, this PR updates the toggle view to fetch relevant toggles from connect. Additionally, it updates start configuration to fetch toggles the same way.
Linked connect pr dimagi/commcare-connect#901
Safety Assurance
Safety story
There is test coverage for the views that are already in use, which have been updated to reflect the changes. The full end to end workflow will be QAed with the new mobile release
Automated test coverage
QA Plan
Will be covered in the next mobile release which includes the mobile side of this functionality.
Labels & Review