webkom / lego

Backend for abakus.no
https://lego.abakus.no
MIT License
54 stars 20 forks source link

Remove contact tracing from event model #3559

Closed AdrianAndersen closed 7 months ago

AdrianAndersen commented 7 months ago

This PR removes the contact tracing model and views. Depends on https://github.com/webkom/lego-webapp/pull/4476

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AdrianAndersen commented 7 months ago

Resolves ABA-183

christiangryt commented 7 months ago

What is the purpose of the Migrate event you created?

norbye commented 7 months ago

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 commented 7 months ago

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!