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

delete broadcasts PR#1 #4056

Merged
merged 7 commits into from
Oct 18, 2022
Merged

delete broadcasts PR#1 #4056

merged 7 commits into from
Oct 18, 2022

Conversation

susanm74
Copy link

No description provided.

1. add is_active column with null=True
2. ensure is being set to True for new scheduled broadcasts (adding default=True to model will achive this)
@susanm74 susanm74 marked this pull request as ready for review October 17, 2022 05:39
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #4056 (6aa706b) into main (1dea7f0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #4056   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          440       441    +1     
  Lines        26856     26861    +5     
=========================================
+ Hits         26856     26861    +5     
Impacted Files Coverage Δ
temba/msgs/migrations/0194_broadcast_is_active.py 100.00% <100.00%> (ø)
temba/msgs/models.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

migrations.AddField(
model_name="broadcast",
name="is_active",
field=models.BooleanField(default=True, null=True),
Copy link
Member

Choose a reason for hiding this comment

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

Can't add the default in this step otherwise django will try to update every existing row with is_active=True. If you run ./manage sqlmigrate msgs 0194 it'll show you the SQL that would be run for this migration and you'll see an UPDATE like that. One of our databases has 7 million rows in this table which would require a longer lock than we can do. So what we do is.. a

  1. Add it without the default..
is_active = models.BooleanField(null=True)
  1. Run makemigrations..
  2. Add the default
is_active = models.BooleanField(null=True, default=True)
  1. Run makemigrations..
  2. (Optional for tidyness) manually move the operation from the second migration file into the first migration file so we have a single migration with 2 operations - the AddField and the AlterField.

What that will give you is a migration which adds a nullable field and starts setting it for new rows. We can worry about the existing rows later in a data migration that updates them in batches.

Copy link
Author

Choose a reason for hiding this comment

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

@rowanseymour ah cool, makes sense, will update this PR with the above

@susanm74 susanm74 changed the title delete broadcasts - PR#1 delete broadcasts PR#1 Oct 17, 2022
@susanm74 susanm74 requested a review from rowanseymour October 18, 2022 07:16
@@ -1948,6 +1948,7 @@ def test_model(self):
schedule=Schedule.create_schedule(self.org, self.admin, timezone.now(), Schedule.REPEAT_MONTHLY),
)
self.assertEqual("I", broadcast1.status)
self.assertEqual(True, broadcast1.is_active)
Copy link
Member

Choose a reason for hiding this comment

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

don't forget there's a self.assertTrue(..)

@rowanseymour rowanseymour merged commit da44ca4 into main Oct 18, 2022
@rowanseymour rowanseymour deleted the sl-delete-broadcasts-1 branch October 18, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants