feat: Create versioning for segments #4138

zachaysan · 2024-06-10T18:53:08Z

Thanks for submitting a PR! Please check the boxes below:

I have run pre-commit to check linting
I have added information to docs/ if required so people know about the feature!
I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?
I have used a Conventional Commit title for this Pull Request

Changes

This introduces versioning for segments by cloning their features into new objects and filtering out older versions of segment through a new manager class.

How did you test this code?

A few new unit tests around the models as well as updates to some existing tests to handle audit log cases.

vercel · 2024-06-10T18:53:11Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 27, 2024 3:14pm
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 27, 2024 3:14pm
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 27, 2024 3:14pm

github-actions · 2024-06-10T18:56:07Z

Uffizzi Preview deployment-52851 was deleted.

codecov · 2024-06-10T19:21:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (5162687) to head (70e974a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4138      +/-   ##
==========================================
+ Coverage   96.57%   96.60%   +0.02%     
==========================================
  Files        1191     1194       +3     
  Lines       38809    39071     +262     
==========================================
+ Hits        37481    37745     +264     
+ Misses       1328     1326       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

api/segments/migrations/sql/0023_add_versioning_to_segments.sql

api/segments/models.py

matthewelwell · 2024-06-11T13:56:40Z

api/segments/models.py

+        new_segment = Segment.objects.create(
+            name=self.name,
+            description=self.description,
+            project=self.project,
+            feature=self.feature,
+            version=self.version,
+            version_of=self,
+        )


We have clone functions in other models in the code base where we take a slightly different approach, e.g.:

def deep_clone(self) -> "Segment": new_segment = copy.deepcopy(self) new_segment.id = None # this is what forces django to think it's creating a new model object ...

The benefit of this approach is that it maintains itself when new fields are added to those model classes. With the approach that you're using here, if a new field is added to the segment, we'll need to remember to add it here I think?

Ok I'll implement that strategy. Just to double check, the timestamps will still be set to now for the new model, right?

Ok this is done.

Just to double check, the timestamps will still be set to now for the new model, right?

Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the created_at not to change?

Well it turns out that none of the models involved in this PR have created_at or updated_at timestamps, although they probably should, at least on the segments. Should I add them to this PR? If so, what should the default be for existing objects None or now()? Should I include conditions and rules in having timestamps as well?

Let's add these fields to all models in the segments app, yes, but let's do it in a separate PR. Regarding the default, we could be smart and look in the objects history but that might end up being a fairly mammoth migration.

Ok sounds good I've made a new ticket for this.

#4211

The PR is up for the timestamps:

#4236

api/segments/models.py

api/segments/serializers.py

matthewelwell

I've added a few more comments but I also noticed that none of the view tests actually verify that the clone is created (perhaps because they rely on the number of audit records instead, which I'm suggesting that we remove).

api/segments/models.py

matthewelwell · 2024-06-12T07:01:05Z

api/segments/models.py

+        new_segment = Segment.objects.create(
+            name=self.name,
+            description=self.description,
+            project=self.project,
+            feature=self.feature,
+            version=self.version,
+            version_of=self,
+        )


Just to double check, the timestamps will still be set to now for the new model, right?

Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the created_at not to change?

api/segments/serializers.py

api/tests/integration/audit/test_audit_logs.py

api/tests/unit/segments/test_unit_segments_models.py

api/tests/unit/segments/test_unit_segments_views.py

…_segments

zachaysan added 10 commits June 10, 2024 15:11

Create versioning for segments migration

2ad4381

Create versioning of segments

f6746e1

Create segments manager for versioned segments

8df7f91

Switch to nullable integer

a582253

Deep clone the default segment fixture

730d064

Deep clone during update

42f93a9

Create default for newly created segments

c7eb529

Test deep cloning of segments

f06cb3d

Update unit segments views

641c02d

Merge branch 'main' into feat/create_versioning_for_segments

d40d5ed

github-actions bot added api Issue related to the REST API feature New feature or request labels Jun 10, 2024

Update audit log tests for segments versioning

7341357

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 10, 2024

zachaysan added 2 commits June 10, 2024 19:39

Switch to assertion error, since it's closer to intent

41f086d

Add tests for edge cases

ed56775

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 10, 2024

zachaysan requested a review from matthewelwell June 10, 2024 19:49

matthewelwell reviewed Jun 11, 2024

View reviewed changes

zachaysan added 2 commits June 11, 2024 18:46

Remove historical from sql migration

922001d

Switch to deepcopy for deep cloned objects

4842576

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 11, 2024

zachaysan requested a review from matthewelwell June 11, 2024 18:50

matthewelwell requested changes Jun 12, 2024

View reviewed changes

zachaysan mentioned this pull request Jun 21, 2024

Add created_at and updated_at fields to all models in the segments app #4211

Closed

Create WIP code for Matt

4481e4b

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 21, 2024

zachaysan requested a review from matthewelwell June 21, 2024 18:14

matthewelwell added 2 commits June 24, 2024 13:55

Minor optimisation

669a079

Handle exception generating audit log

34e7e46

matthewelwell requested a review from a team as a code owner June 24, 2024 13:29

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 24, 2024

zachaysan added 2 commits June 25, 2024 18:50

Add test for audit log on segment not existing

8f38dcd

Fix conflicts and merge branch 'main' into feat/create_versioning_for…

e5c7b47

…_segments

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 25, 2024

vercel bot deployed to Preview – docs June 25, 2024 19:09 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview June 25, 2024 19:10 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging June 25, 2024 19:10 View deployment

matthewelwell approved these changes Jun 27, 2024

View reviewed changes

Merge branch 'main' into feat/create_versioning_for_segments

70e974a

github-actions bot added feature New feature or request and removed feature New feature or request labels Jun 27, 2024

vercel bot deployed to Preview – docs June 27, 2024 15:12 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging June 27, 2024 15:14 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview June 27, 2024 15:14 View deployment

zachaysan added this pull request to the merge queue Jun 27, 2024

Merged via the queue into main with commit bc9b340 Jun 27, 2024
28 checks passed

zachaysan deleted the feat/create_versioning_for_segments branch June 27, 2024 15:28

dabeeeenster mentioned this pull request Jun 27, 2024

chore(main): release 2.127.0 #4261

Merged

matthewelwell mentioned this pull request Jul 9, 2024

fix(segments): add migration to set version on existing segments #4315

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create versioning for segments #4138

feat: Create versioning for segments #4138

zachaysan commented Jun 10, 2024

vercel bot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

matthewelwell Jun 11, 2024

zachaysan Jun 11, 2024

zachaysan Jun 11, 2024

matthewelwell Jun 12, 2024

zachaysan Jun 12, 2024

matthewelwell Jun 20, 2024

zachaysan Jun 21, 2024

zachaysan Jun 27, 2024

matthewelwell left a comment

matthewelwell Jun 12, 2024

feat: Create versioning for segments #4138

feat: Create versioning for segments #4138

Conversation

zachaysan commented Jun 10, 2024

Changes

How did you test this code?

vercel bot commented Jun 10, 2024 • edited Loading

github-actions bot commented Jun 10, 2024 • edited Loading

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading