-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(project/serializer): limit edit to only fields that make sense #4846
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a minor comment
api/projects/serializers.py
Outdated
def update(self, instance: Project, validated_data: dict) -> Project: | ||
# Prevent updates to `organisation` field | ||
validated_data.pop("organisation", None) | ||
return super().update(instance, validated_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's perhaps better to just add a new serializer for update requests which adds organisation to the read only fields?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4846 +/- ##
=======================================
Coverage 97.35% 97.35%
=======================================
Files 1182 1183 +1
Lines 41287 41306 +19
=======================================
+ Hits 40194 40213 +19
Misses 1093 1093 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
Stop User from updating
organisation
,enable_dynamo_db
andedge_v2_migration_status
How did you test this code?
Covered by unit tests