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

fix(model/featurestate): make environment not null #2708

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented Aug 30, 2023

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

  • I have run pre-commit to check linting
  • 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

Don't allow null value for environment field of feature state model.

Here is the sqlmigrate of the migration:

BEGIN;
--
-- Alter field environment on featurestate
--
SET CONSTRAINTS "features_featurestat_environment_id_c5882902_fk_environme" IMMEDIATE; ALTER TABLE "features_featurestate" DROP CONSTRAINT "features_featurestat_environment_id_c5882902_fk_environme";
ALTER TABLE "features_featurestate" ALTER COLUMN "environment_id" SET NOT NULL;
ALTER TABLE "features_featurestate" ADD CONSTRAINT "features_featurestat_environment_id_c5882902_fk_environme" FOREIGN KEY ("environment_id") REFERENCES "environments_environment" ("id") DEFERRABLE INITIALLY DEFERRED;
COMMIT;



How did you test this code?

Adds migration test

@vercel
Copy link

vercel bot commented Aug 30, 2023

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 Sep 4, 2023 11:59am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2023 11:59am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2023 11:59am

@github-actions github-actions bot added the api Issue related to the REST API label Aug 30, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

Uffizzi Preview deployment-34789 was deleted.

@gagantrivedi gagantrivedi marked this pull request as ready for review August 30, 2023 14:11
@gagantrivedi gagantrivedi requested review from a team and novakzaballa August 30, 2023 14:11
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5b46cc0) 95.47% compared to head (28ee9cd) 95.48%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2708    +/-   ##
========================================
  Coverage   95.47%   95.48%            
========================================
  Files         986      988     +2     
  Lines       27709    27810   +101     
========================================
+ Hits        26454    26553    +99     
- Misses       1255     1257     +2     
Files Changed Coverage Δ
api/features/models.py 93.60% <ø> (+0.01%) ⬆️
...ions/0060_set_featurestate_environment_not_null.py 100.00% <100.00%> (ø)
api/tests/unit/features/test_migrations.py 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

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

Copy link
Contributor

@novakzaballa novakzaballa left a comment

Choose a reason for hiding this comment

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

LGTM


def delete_feature_states_without_environment(apps, schema_editor):
FeatureState = apps.get_model("features", "FeatureState")
FeatureState.objects.filter(environment=None).delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it makes a difference, but the correct way to do this I think is: filter(environment__isnull=True). I think it's perhaps because in different database engines, it handles null differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, done

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

4 participants