mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
128 stars 41 forks source link

[Task]: Migrate from `BlockVersion.soft` to `BlockVersion.block_type` #15111

Open diox opened 1 month ago

diox commented 1 month ago

Description

https://github.com/mozilla/addons/issues/15012 introduced soft-blocks by adding a soft BooleanField to BlockVersion.

We debated switching to a block_type IntegerField instead (or probably a PositiveSmallIntegerField) and initially resisted, in part because the migration is annoying/costly, but it has since become clear this will make our lives easier, by allowing us to have constants/choices that are simpler to deal with (we can't have Choices with booleans).

Acceptance Criteria

  ### Milestones/checkpoints
  - [ ] `BlockVersion` have a `block_type` integer replacing the `soft` boolean

Checks

┆Issue is synchronized with this Jira Task

diox commented 3 weeks ago

If there was a django field for TINYINT we could use that... Maybe worth writing our own ? That would make the migration slightly faster and take less space for the future.

diox commented 3 weeks ago

If there was a django field for TINYINT we could use that... Maybe worth writing our own ? That would make the migration slightly faster and take less space for the future.

With some luck it's just a couple lines of code to change since we already have a DatabaseWrapper / DatabaseOperations with custom data_types / integer_field_ranges / cast_data_types defined...

KevinMind commented 3 weeks ago

Question @diox would it be possible to migrate from PositiveSmallIntegerField to a custom TINYINT field in the future? I would prefer not to add a new field, especially a custom rolled one unless we have known/serious performance issues or if we think the migration from A -> B -> C is like wayyyy higher than that of A -> C.

Wdyt?

diox commented 3 weeks ago

I would prefer avoiding migrating a table that has millions of rows over and over. The custom Tinyint field should be easy to do, we already have the basic things set up to do this (src/olympia/core/db/mysql/base.py) as we already have custom db fields (src/olympia/amo/fields.py)

KevinMind commented 3 weeks ago

@diox looking at the bloom filter and it's kind of not very much fun to try continuing on that without this so I opened a PR just now.. I see you've assigned @bakulf maybe could be used as a reference but I think having the dango integer choices field is my main ask at this point.

class BlockType(models.IntegerChoices):
    BLOCKED = 0, _('Blocked')
    SOFT_BLOCKED = 1, _('Restricted')

If this class exists and is then used by both models, then I don't really care about any other details underlying as I can rely on that enum to build out the block_type based filtering logic. wdyt?

alexandruschek commented 2 weeks ago

Hello, can you please help me with some information, I don't understand exactly what I need to check. Thank you very much!

bakulf commented 2 weeks ago

@alexandruschek this task cannot be tested. This is just an internal code change.