feat: Add version_of and matches to rules and conditions
#4988
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
bot
added
api
Docker builds report
|
Uffizzi Ephemeral Environment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4988 +/- ##
==========================================
+ Coverage 97.40% 97.42% +0.01%
==========================================
Files 1193 1194 +1
Lines 41675 41958 +283
==========================================
+ Hits 40593 40876 +283
Misses 1082 1082 ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
feature
| @@ -170,7 +170,7 @@ hubspot-api-client = "^8.2.1" | |||
| djangorestframework-dataclasses = "^1.3.1" | |||
| pyotp = "^2.9.0" | |||
| flagsmith-task-processor = { git = "https://github.com/Flagsmith/flagsmith-task-processor", tag = "v1.1.1" } | |||
| flagsmith-common = { git = "https://github.com/Flagsmith/flagsmith-common", tag = "v1.4.2" } | |||
| flagsmith-common = { git = "https://github.com/Flagsmith/flagsmith-common", branch = "feat/add_version_of_to_rules_and_conditions" } | |||
There was a problem hiding this comment.
Let's remember to update these once we've got builds in those packages.
github-actions
bot
added
feature
…es_and_conditions
github-actions
bot
added
feature
api/segments/models.py
Outdated
| for self_rule in self_rules: | ||
| if sub_rule_matched: | ||
| break | ||
|
|
||
| if self_rule in matched_rules and self_rule.version_of != rule: | ||
| continue | ||
|
|
||
| if rule.type != self_rule.type: | ||
| continue | ||
| for self_sub_rule in self_rule.rules.all(): | ||
| if self_sub_rule in matched_sub_rules: | ||
| continue | ||
| if self_sub_rule.assign_conditions_if_matching_rule(sub_rule): | ||
| self_sub_rule.version_of = sub_rule | ||
| sub_rule_matched = True | ||
| matched_sub_rules.add(self_sub_rule) | ||
| self_rule.version_of = rule | ||
| matched_rules.add(self_rule) | ||
| break |
There was a problem hiding this comment.
Sorry @zachaysan , this code is still unreadable to me. I was hoping that the docstring and renaming of the variables would help, but it hasn't really. Can we add some inline comments on each of the conditional statements perhaps?
There was a problem hiding this comment.
It feels to me like this is perhaps evidence that we're thinking about this wrong? Could we instead just store a 'diff' object (probably in JSON), or set of objects, which look(s) something like:
class SegmentConditionChanges:
condition: Condition
new_operator: str
new_property: str
new_value: str
class SegmentRuleChanges:
rule: SegmentRule
new_type: str
new_rules: list[SegmentRule]
deleted_rule_ids: list[int]
updated_rules: list[SegmentRuleChanges]
new_conditions: list[Condition]
delete_condition_ids: list[int]
updated_conditions: list[SegmentConditionChanges]
class SegmentChangeSet:
segment: Segment
new_name: str
new_description: str
new_rules: list[SegmentRule]
deleted_rule_ids: list[int]
updated_rules: list[SegmentRuleChanges]... maybe it's a bit late in the day for this, but I feel like this might be more flexible?
There was a problem hiding this comment.
Ok I've added some inline comments as we discussed.
| assert condition2.version_of == condition1 | ||
|
|
||
|
|
||
| def test_assign_matching_rules_to_segment_simple(project: Project) -> None: |
There was a problem hiding this comment.
Can we be more explicit with the test names? What does _simple mean? Alternatively (or maybe as well as), let's add more comments / docstrings to these tests.
There was a problem hiding this comment.
Ok I've updated the test name and have added comments locally.
api/segments/models.py
Outdated
| if ( | ||
| condition.operator == self_condition.operator | ||
| and condition.property == self_condition.property | ||
| ): |
There was a problem hiding this comment.
Shouldn't we only care about the property here, rather than the operator as well? What if they want to change it from e.g. organisation_id EQUALS 123 to organisation_id IN 123,456 - that seems like a legitimate use case that we want to cater for, right?
There was a problem hiding this comment.
Ah, but there's perhaps an issue with operators like percentage split in that case? But I think the logic should be something like:
if not condition.property:
if condition.operator == self_condition.operator:
# do valid match logic
elif condition.property == self_condition.property:
# do valid match logicMy concern here as well is that I don't think there is anyone stopping someone from adding e.g. 2 percentage split conditions to a segment. It would be a stupid idea, but there's nothing stopping it from happening as far as I know, and there are likely other scenarios where this is the case.
For example, there are a lot of segments that look something like:
organisation_id IN 123,456,...
OR
organisation_id IN abc,def,...
This is to get around our limitation on the size of the value in a segment condition. I don't think your logic here (or mine suggested above for that matter) will handle this scenario very well, unless I'm missing something?
There was a problem hiding this comment.
I've updated the condition matching logic to now include operator differences as well as adding a section to the code to look for exact condition matches first.
github-actions
bot
added
feature
github-actions
bot
added
feature
api/segments/models.py
Outdated
| if self_condition in matched_conditions: | ||
| continue |
There was a problem hiding this comment.
I really can't see a scenario where this could be the case?
There was a problem hiding this comment.
If the condition had previously matched another condition, then when the outer loop starts again we don't want to match it again. This is useful for conditions that have the same property and could match different conditions.
api/segments/models.py
Outdated
| # Set a bool flag to be used to break out of the | ||
| # below self_rules iteration if a sub_rule matches. |
There was a problem hiding this comment.
Why? Please always try to phrase comments about the why, not the what. I can infer exactly what this comment says from a quick glance at the variable name, and its use below, but I'm still none the wiser really as to why.
There was a problem hiding this comment.
Ok I've tried to lean in with a Because to guide my writing, but it's hard not to explain code when writing about why a variable exists.
api/segments/models.py
Outdated
| # If a self_rule has already been matched then | ||
| # the only time we care about it is when we | ||
| # further iterate on the the same sub_rules | ||
| # that are present in the matching parent rule. |
There was a problem hiding this comment.
Again, why? This logic is super hard to follow and all these comments are doing is just rephrasing the code that I can read already. It's super hard to follow what self_rule, sub_rule and 'parent rule' here are. Can we reword this to talk about the business logic rather than the code?
There was a problem hiding this comment.
Ok I've tried to do this one as well.
| # To eliminate false matches we force the types | ||
| # to be the same for the rules. |
There was a problem hiding this comment.
What is a 'false match' ?
There was a problem hiding this comment.
When the match shouldn't go through.
api/segments/models.py
Outdated
| versioning changes. | ||
|
|
||
| Side Effects: | ||
| - Updates the `version_of` field for matched rules and sub-rules. |
There was a problem hiding this comment.
Which segment does it modify though?
There was a problem hiding this comment.
Ok I've updated this to explicitly say that it calls the segment self.
api/segments/models.py
Outdated
| def assign_matching_rules_to_segment(self, segment: "Segment") -> bool: | ||
| """ | ||
| Assign matching rules of the current object to the rules of the given segment. | ||
|
|
||
| This method iterates through the rules of the provided `Segment` and | ||
| attempts to match them to the current object's rules and sub-rules, | ||
| updating versioning where matches are found. | ||
|
|
||
| This is done in order to set the `version_of` field for matched rules | ||
| and conditions so that the frontend can more reliably diff rules and | ||
| conditions between a change request for a segment and the target segment | ||
| itself. |
There was a problem hiding this comment.
This still doesn't really talk about when it should be used? One of the biggest things I'm finding is that I'm consistently struggling to follow which segment is which. Like should we be more explicit here (and in all of the rule / condition variables) to say something like current_segment, modified_segment or something?
Maybe the method could then be more explicit like: update_segment_with_matches_from_current_segment(self, current_segment: "Segment")?
This feels like it's pigeon-hoing us a little bit into only using this functionality for change requests, but since we're modifying the segments, it feels like this functionality can only ever be used for that purpose anyway?
There was a problem hiding this comment.
This still doesn't really talk about when it should be used?
Ok I've updated the introductory paragraph to include that self is the target of the changes and that this is called in the context of a change request.
One of the biggest things I'm finding is that I'm consistently struggling to follow which segment is which. Like should we be more explicit here (and in all of the rule / condition variables) to say something like current_segment, modified_segment or something?
I've updated the code to reference current_[segment,rule,sub_rule,condition] and I've added references to which object is being updated by referencing them as modified_[rule,sub_rule,condition] to make it clear which objects are optionally being modified.
Maybe the method could then be more explicit like: update_segment_with_matches_from_current_segment(self, current_segment: "Segment")?
Ok I've updated the method name and args.
This feels like it's pigeon-hoing us a little bit into only using this functionality for change requests, but since we're modifying the segments, it feels like this functionality can only ever be used for that purpose anyway?
Yes this functionality is only going to be used for change requests.
github-actions
bot
added
feature
github-actions
bot
added
feature
Changes
When creating a new change request for segments we need a way of tracing which provided rules and conditions match the candidate change request so that the frontend can more visually associate which rules and conditions match between the change request and the targeted segment (the one that will be updated).
To do so I've reused the concept of
version_ofwhich was originally used for segments and I associate a rule to another rule (or sub-rule to sub-rule) and in that process also match up conditions at the same layer. If at least one condition matches, then the sub-rule and rule match and future matching for the subsequent sub-rule is guided by holding onto the match between the parent rule and its sister rule. This way matches are only ever tree like in nature.Remaining work
Since the matching code is ready in this PR another PR will need to be opened to tie in these changes into the relevant code. Probably both the Workflows and Commons will need to be updated.
How did you test this code?
Lots of new tests. Some are simple, like checking that
version_ofgets correctly set, which is necessary due to an audit log bug we had whenversion_ofwas setup for the segments work.