Skip to content
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

feat(versioning): add logic to create version in single endpoint #3991

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

matthewelwell
Copy link
Contributor

@matthewelwell matthewelwell commented May 21, 2024

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 PR updates the endpoint to create a new version to add new attributes that allow you to create, update and delete feature states when creating the new version and optionally publish it immediately. This will avoid the FE having to create the version, retrieve the feature states, update any necessary feature states and then publish it.

I'm keen to get feedback on the naming of the new fields as I'm not 100% pleased with them. The endpoint looks like this:

image

How did you test this code?

Added tests to cover all the use cases.

Copy link

vercel bot commented May 21, 2024

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 3, 2024 6:49pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 6:49pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 6:49pm

@github-actions github-actions bot added the api Issue related to the REST API label May 21, 2024
Copy link
Contributor

github-actions bot commented May 21, 2024

Uffizzi Ephemeral Environment deployment-52072

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/3991

📄 View Application Logs etc.

What is Uffizzi? Learn more!

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.45%. Comparing base (22d371b) to head (3dfa2a6).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3991      +/-   ##
==========================================
+ Coverage   96.44%   96.45%   +0.01%     
==========================================
  Files        1149     1149              
  Lines       37575    37728     +153     
==========================================
+ Hits        36238    36391     +153     
  Misses       1337     1337              

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

Copy link
Contributor

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mess of small suggestions or nits but overall the code looks great. I understand you want better naming but I couldn't think of better ideas than the ones you've already put in place.

class Meta:
model = FeatureSegment
fields = ("id", "segment", "priority", "uuid")
read_only_fields = ("priority",)

def save(self, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

"publish_immediately",
)

def create(self, validated_data):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


def create(self, validated_data):
for field_name in self.Meta.non_model_fields:
validated_data.pop(field_name, None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused how these popped fields are actually used, but I'll keep reading the PR for more info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I understand it now. Maybe the comment below that explains it could go into more detail why we want raw data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I certainly see your point, but I'm struggling to go into too much more detail without writing an essay. The main reason is just that we're utilising the serializers to create the feature states and the serializers expect raw data, not ORM models. I'm obviously keen to make the comment as useful as possible, so let me know if you have any suggestions on how to improve.

Comment on lines 180 to 184
fs_serializer.save(
environment_feature_version=version,
environment=version.environment,
feature=version.feature,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a DRYer way of doing this? The context was already set in the constructor, it seems weird to have to set them again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really too sure why we need both the context and the save kwargs. I tried to follow it down the rabbit hole (of my own making), but couldn't see anything obvious. I've tidied it up a little bit to use a single dict definition in both cases, and I'll try to tackle a better refactor in a separate piece of work.

Comment on lines 186 to 188
def _update_feature_state(
self, feature_state: dict, version: EnvironmentFeatureVersion
) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if the typing on the dict had more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added as much information as I think I can without it being beneficial to just create e.g. a pydantic model.

feature: Feature,
segment: Segment,
segment_featurestate: FeatureState,
admin_client: APIClient,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional use of admin_client instead of admin_client_new? Also, many of the tests use the admin_client_new which is great, but are we missing out on testing the ACL with staff_client too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've updated an older test to use staff client and standardised to using admin_client_new on these new tests.

@matthewelwell matthewelwell changed the title feat(versioning): add logic to create version in single endpoint (WIP) feat(versioning): add logic to create version in single endpoint May 23, 2024
…ate-in-single-endpoint

# Conflicts:
#	api/features/versioning/serializers.py
#	api/features/versioning/views.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants