fix: metadata-feature-changes-erases-existing-value #5362
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
for more information, see https://pre-commit.ci
Docker builds report
|
|
Uffizzi Preview |
for more information, see https://pre-commit.ci
Zaimwa9
changed the title
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
In general, I'm a little confused by this PR (and the one in common). I'm not sure exactly what we're solving. Based on the changes that I See here, it feels like the metadata logic never would have previously worked for environments and features, but I don't think that's true, right?
Also, I don't see any additional tests added to this PR, or the one in the flagsmith-common repository. we should definitely add a test (or tests) somewhere.
api/environments/serializers.py
Outdated
| environment = super().update(instance, validated_data) | ||
| self.update_metadata(environment, metadata_items) # type: ignore[no-untyped-call] | ||
| environment.refresh_from_db() | ||
| assert isinstance(environment, Environment) |
There was a problem hiding this comment.
This assertion feels unnecessary?
There was a problem hiding this comment.
Actually as of it is "necessary" for mypy because super().update is not fully typed and returns a models.Model. I can type it in a separate PR because it might need to type a couple of used functions inside
There was a problem hiding this comment.
I'd at least hide it behind if typing.TYPE_CHECKING: to clearly state its purpose.
Ideally, i'd try annotating the var like so:
environment: Environment = super().update(instance, validated_data)and see if mypy complains about it.
api/features/serializers.py
Outdated
| feature = super().update(instance, validated_data) # type: ignore[no-untyped-call] | ||
| self.update_metadata(feature, metadata_items) | ||
| feature.refresh_from_db() | ||
| assert isinstance(feature, Feature) |
There was a problem hiding this comment.
As above, this seems unnecessary?
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5362 +/- ##
=======================================
Coverage 97.63% 97.63%
=======================================
Files 1239 1239
Lines 43065 43097 +32
=======================================
+ Hits 42046 42078 +32
Misses 1019 1019 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Added new test case to assert the correct number of metadata is created. |
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
updateto useupdate_metadatamethod fromSerializerWithMetadatainherited from commonHow did you test this code?
segments_viewsfailing on previousflagsmith-commonversion, passing nowhttps://www.loom.com/share/f980f034f6144c548a2f30efd809f828