fix: Long DELETE project call
#3360
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
khvn26
force-pushed
the
fix/long-large-project-deletions
branch
from
8afd597 to
11f24e0
Compare
khvn26
force-pushed
the
fix/long-large-project-deletions
branch
from
ce0b69a to
c9d5564
Compare
khvn26
force-pushed
the
fix/long-large-project-deletions
branch
from
c9d5564 to
5188323
Compare
khvn26
force-pushed
the
fix/long-large-project-deletions
branch
from
5188323 to
6eebc02
Compare
khvn26
force-pushed
the
fix/long-large-project-deletions
branch
from
6eebc02 to
1fa3ac5
Compare
Yeah, it was a tricky one. The issue was here. The signals weren't getting added, so no AuditLog record was being created. |
# Conflicts: # .github/workflows/api-pull-request.yml
| environment, project = history_instance.instance.get_environment_and_project() | ||
| if project != history_instance.instance and ( | ||
| (project and project.deleted_at) | ||
| or (environment and environment.project.deleted_at) | ||
| ): | ||
| # don't trigger audit log records in deleted projects | ||
| return |
There was a problem hiding this comment.
This is arguably unrelated to this PR but I think it's a good improvement here to prevent unnecessary data in the AuditLog table.
There was a problem hiding this comment.
Yeah it's sorta unrelated but it's a good addition.
| from environments.tasks import delete_environment | ||
| from features.tasks import delete_feature | ||
| from projects.models import Project | ||
| from segments.tasks import delete_segment |
There was a problem hiding this comment.
Ugh, yeah, I tried to resolve this but I ended up just diving into a hell hole of circular dependencies. It annoyingly made testing a little harder because I can't mock the imports, but I do feel like it's not a major issue for tasks to have local imports given their place in the stack.
| @@ -0,0 +1,20 @@ | |||
| # Generated by Django 3.2.23 on 2024-02-01 12:12 | |||
|
|
|||
| from django.db import migrations, models | |||
There was a problem hiding this comment.
Can we add the sqlmigrate output for these migrations
There was a problem hiding this comment.
❯ python manage.py sqlmigrate features 0063
BEGIN;
--
-- Alter field project on feature
--
COMMIT;
❯ python manage.py sqlmigrate features 0064
BEGIN;
--
-- Alter field project on feature
--
--
-- Alter field project on historicalfeature
--
COMMIT;
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( |
There was a problem hiding this comment.
sqlmigrate output:
❯ python manage.py sqlmigrate environments 0034
BEGIN;
--
-- Alter field project on environment
--
COMMIT;
There was a problem hiding this comment.
Django migrations for this silly stuff is always a bit of an eye roll
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( |
There was a problem hiding this comment.
sqlmigrate output:
❯ python manage.py sqlmigrate segments 0020
BEGIN;
--
-- Alter field project on segment
--
COMMIT;
Well it's great that the test suite caught it. |
| environment, project = history_instance.instance.get_environment_and_project() | ||
| if project != history_instance.instance and ( | ||
| (project and project.deleted_at) | ||
| or (environment and environment.project.deleted_at) | ||
| ): | ||
| # don't trigger audit log records in deleted projects | ||
| return |
There was a problem hiding this comment.
Yeah it's sorta unrelated but it's a good addition.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( |
There was a problem hiding this comment.
Django migrations for this silly stuff is always a bit of an eye roll
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
Closes #3302.
This introduces the following changes:
test_recurring_task_run_should_execute_first_run_atfailing whenone_hour_from_nowparameter is next day.How did you test this code?
Added unit tests; created and deleted large projects locally.