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

Remove contact tracing from event model #3559

Merged
merged 1 commit into from Feb 20, 2024

Conversation

AdrianAndersen
Copy link
Member

This PR removes the contact tracing model and views. Depends on webkom/lego-webapp#4476

@AdrianAndersen AdrianAndersen added review-needed Pull requests that need review small-fix Pull requests that fix something small labels Feb 20, 2024
@AdrianAndersen AdrianAndersen self-assigned this Feb 20, 2024
@ivarnakken ivarnakken added the approved Pull requests that have been approved label Feb 20, 2024
@AdrianAndersen AdrianAndersen requested a review from a team February 20, 2024 19:44
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3d64d9f) 88.24% compared to head (32a6be8) 88.24%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3559      +/-   ##
==========================================
- Coverage   88.24%   88.24%   -0.01%     
==========================================
  Files         669      670       +1     
  Lines       21118    21089      -29     
==========================================
- Hits        18636    18609      -27     
+ Misses       2482     2480       -2     

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

@AdrianAndersen
Copy link
Member Author

Resolves ABA-183

@AdrianAndersen AdrianAndersen merged commit cc3afbf into master Feb 20, 2024
3 checks passed
@AdrianAndersen AdrianAndersen deleted the remove-contact-tracing branch February 20, 2024 20:03
@christiangryt
Copy link

What is the purpose of the Migrate event you created?

@norbye
Copy link
Member

norbye commented Feb 21, 2024

What is the purpose of the Migrate event you created?

Absolutely love that you looked through it and asked a question! Extreme kudos for that 💯

I'll add a lil answer as im here anyways - but if anyone has anything to add feel free:) and let me know if I missed what you were actually asking about

Quick backstory;

In Django we have models, that we use to describe the database tables we want. This allows us to access nice features such as getting items from the database fairly simply in the code. We can change these models as we'd like, but then we need to ensure that our actual database matches the newest version of our models. That's where migrations come in - synchronizing the changes to the database structure so it's the same in the code and the actual database.

Whats happening here;

In our Event model, we had a boolean field use_contact_tracing = models.BooleanField(default=False). As contact tracing is no longer needed - we wanted to get rid of that field. If we just removed it from the code without making a migration, the data and field would still be in the database, which makes no sense to us as the code wouldn't be aware that it was there. Thus he added a migration (something Django can do for you if you run python manage.py makemigrations) so that that value will be deleted from the database as well.

In this specific case - we wouldn't really break anything by leaving the field there, as it had a default value of False - so if we kept it we would just continue to save a False value for every event created. But in a lot of cases the value might be required - and in that case it wouldn't be possible to create new events if the field wasn't also deleted from the database.

And there's literally no reason not to remove it since it's not used

Neat stuffs about migrations;

We have also setup tests to make sure that the appropriate migrations are created, so if you tried to make changes to the models and did not create the required migrations it would not pass CI:)

Another cool thing about migrations is that no matter what version of the code you are running (either on some prod server or locally), the database stores which migrations you have already applied - so when there are new ones you only have to run the ones that you're missing 🚀

This became a tad longer than I planned - but I hope it's not too much and that it's useful(:

@christiangryt
Copy link

What is the purpose of the Migrate event you created?

Absolutely love that you looked through it and asked a question! Extreme kudos for that 💯

I'll add a lil answer as im here anyways - but if anyone has anything to add feel free:) and let me know if I missed what you were actually asking about

Quick backstory;

In Django we have models, that we use to describe the database tables we want. This allows us to access nice features such as getting items from the database fairly simply in the code. We can change these models as we'd like, but then we need to ensure that our actual database matches the newest version of our models. That's where migrations come in - synchronizing the changes to the database structure so it's the same in the code and the actual database.

Whats happening here;

In our Event model, we had a boolean field use_contact_tracing = models.BooleanField(default=False). As contact tracing is no longer needed - we wanted to get rid of that field. If we just removed it from the code without making a migration, the data and field would still be in the database, which makes no sense to us as the code wouldn't be aware that it was there. Thus he added a migration (something Django can do for you if you run python manage.py makemigrations) so that that value will be deleted from the database as well.

In this specific case - we wouldn't really break anything by leaving the field there, as it had a default value of False - so if we kept it we would just continue to save a False value for every event created. But in a lot of cases the value might be required - and in that case it wouldn't be possible to create new events if the field wasn't also deleted from the database.

And there's literally no reason not to remove it since it's not used

Neat stuffs about migrations;

We have also setup tests to make sure that the appropriate migrations are created, so if you tried to make changes to the models and did not create the required migrations it would not pass CI:)

Another cool thing about migrations is that no matter what version of the code you are running (either on some prod server or locally), the database stores which migrations you have already applied - so when there are new ones you only have to run the ones that you're missing 🚀

This became a tad longer than I planned - but I hope it's not too much and that it's useful(:

Thanks for the comprehensive answer!

norbye pushed a commit that referenced this pull request Feb 23, 2024
Remove contact tracing from event model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved review-needed Pull requests that need review small-fix Pull requests that fix something small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants