Conversation
| program = opportunity.managedopportunity.program if opportunity and opportunity.managed else None | ||
| organization = getattr(request, "org", None) | ||
|
|
||
| filters = models.Q(users=user) |
There was a problem hiding this comment.
Apart from filtering for the user, we'll also want to short-circuit to returning True if the flag is enabled for staff/superusers and the user is one of those two.
Also, as a separate question, do we want to filter on org/opp/program if the user filter matches? I feel like if we explicitly enable it for a user in the flag then it should be enabled across all orgs/opps/programs for that users. What do you think?
There was a problem hiding this comment.
Apart from filtering for the user, we'll also want to short-circuit to returning True if the flag is enabled for staff/superusers and the user is one of those two.
I'll update
Also, as a separate question, do we want to filter on org/opp/program if the user filter matches? I feel like if we explicitly enable it for a user in the flag then it should be enabled across all orgs/opps/programs for that users. What do you think?
Since it's an OR operation, does it matter? If it's enabled for a particular user that will still evaluate to True
Charl1996
requested review from
ajeety4,
hemant10yadav,
mkangia and
pxwxnvermx
WalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
commcare_connect/flags/tests/test_flag_models.py (1)
205-212: Consider adding a test for whenopportunity.managedisTruebutmanagedopportunityrelation is missing.This would exercise the defensive path (or expose the crash) discussed in the model review. A test like this would confirm the method handles corrupt/inconsistent data gracefully:
def test_managed_opportunity_with_missing_relation(self): user = UserFactory() opportunity = OpportunityFactory(managed=True) # No ManagedOpportunity created for this opportunity flag = FlagFactory() request = self._make_request(user=user, opportunity=opportunity) # Should not raise; should return False assert Flag.is_flag_active_for_request(request, flag.name) is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/flags/tests/test_flag_models.py` around lines 205 - 212, Add a test that covers the case where an Opportunity has managed=True but lacks a ManagedOpportunity relation to ensure Flag.is_flag_active_for_request handles this corrupt/inconsistent state without raising: create a user and an Opportunity via OpportunityFactory(managed=True) without creating a ManagedOpportunity, create a Flag via FlagFactory (optionally attach programs if needed), build the request with the existing _make_request(user=..., opportunity=...), and assert that Flag.is_flag_active_for_request(request, flag.name) returns False and does not raise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commcare_connect/flags/models.py`:
- Around line 66-83: Replace the direct reverse-relation access
opportunity.managedopportunity with a defensive getattr call: obtain opportunity
via getattr(request, "opportunity", None) (already present), then get
managed_opportunity = getattr(opportunity, "managedopportunity", None) and set
program = managed_opportunity.program if opportunity and opportunity.managed and
managed_opportunity else None; update the symbol references in this method
(opportunity, managedopportunity, program) so missing ManagedOpportunity
instances do not raise RelatedObjectDoesNotExist.
---
Nitpick comments:
In `@commcare_connect/flags/tests/test_flag_models.py`:
- Around line 205-212: Add a test that covers the case where an Opportunity has
managed=True but lacks a ManagedOpportunity relation to ensure
Flag.is_flag_active_for_request handles this corrupt/inconsistent state without
raising: create a user and an Opportunity via OpportunityFactory(managed=True)
without creating a ManagedOpportunity, create a Flag via FlagFactory (optionally
attach programs if needed), build the request with the existing
_make_request(user=..., opportunity=...), and assert that
Flag.is_flag_active_for_request(request, flag.name) returns False and does not
raise.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@commcare_connect/flags/models.py`:
- Around line 67-73: The code correctly uses getattr to defensively access the
reverse OneToOne relation: keep the pattern in the block that reads opportunity
= getattr(request, "opportunity", None) and then uses managed_opp =
getattr(opportunity, "managedopportunity", None) to set program =
managed_opp.program only when managed_opp is truthy; no change required other
than ensuring this exact guarded access remains in the opportunity → managed_opp
→ program flow (symbols: opportunity, managed_opp, program, request).
There was a problem hiding this comment.
Thanks @Charl1996
I am surprised we had this setup incorrectly.
I don't fully understand the models being used so I can share an approval but had a couple of queries for my own understanding.
I see you have an approval already, considering it's related to authorization, would be good to have at least one more.
| if user.is_superuser: | ||
| filters |= models.Q(superusers=True) | ||
| if organization: | ||
| filters |= models.Q(organizations__id=organization.id) |
There was a problem hiding this comment.
so this is confirming if the ID of the "organization" from the request is one of the organization IDs added for the flag?
| if opportunity and opportunity.managed: | ||
| managed_opp = getattr(opportunity, "managedopportunity", None) | ||
| if managed_opp: | ||
| program = managed_opp.program |
There was a problem hiding this comment.
not sure I fully understand the path from opportunity to program.
During this block, can we assume that the user has access to the managed opp and the program if they have opportunity on the request object?
5 participants
Technical Summary
This PR improves the
is_flag_active_for_requestmethod on Flag model by considering the objects from the request instead of what flags the user has enabled.Currently
is_flag_active_for_requestis looking at all the flags active for the user (which will be a constant list regardless of where the user navigates on the platform) and simply checking if the specified flag is in this list - this means it's not request-specific (apart from pulling the user out of the request), so it's not considering whether the user is now looking at another program for instance which the flag is not active for (but currently it will think it is because theactive_flags_for_userreturns a constant list).What I think should happen is that we look at the request to get the context of which org/program/opportunity we're looking at and then checking whether the flag is active for those objects.
Safety Assurance
Safety story
Tested locally
Automated test coverage
Need to add some tests for this.
QA Plan
N.A
Labels & Review