Attempt to move Pageview deletion to a background task #12386
Conversation
The test was failing locally with IntegrityErrors, so I'm not sure if this is a good approach or not.
|
Fixed a dumb mistake with the tests, and it should be ready to review now. |
| else: | ||
| project.imported_files.all().delete() |
There was a problem hiding this comment.
I'm curious why we're deleting these here explicitly -- was this a similar issue and we're trying to be more efficient with the delete? Should they also be done in a task in a similar way?
There was a problem hiding this comment.
This code probably does nothing, since we have CASCADE in the definition of the model: https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/projects/models.py#L1549-L1561
There was a problem hiding this comment.
When a version is de-activated, this function is also called.
| @@ -59,7 +59,7 @@ class PageView(models.Model): | |||
| project = models.ForeignKey( | |||
| Project, | |||
| related_name="page_views", | |||
| on_delete=models.CASCADE, | |||
| on_delete=models.DO_NOTHING, | |||
There was a problem hiding this comment.
I suppose you need SET_NULL here, otherwise there will be db integrity issues.
There was a problem hiding this comment.
we already have a analytics/tasks.py file -- there is no need to create a new directory for this.
| # Remove PageViews for this project async, | ||
| # since they can be very slow to delete. |
There was a problem hiding this comment.
It would be good to update the docstring of this function to reflect this.
| else: | ||
| project.imported_files.all().delete() |
There was a problem hiding this comment.
This code probably does nothing, since we have CASCADE in the definition of the model: https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/projects/models.py#L1549-L1561
| # The PageViews should be deleted by the background task | ||
| self.assertEqual(PageView.objects.filter(project__slug='test-project').count(), 0) |
There was a problem hiding this comment.
You are testing the pageviews were deleted, but we don't know if they were deleted because of the task or the DB cascade. I think you want to assert that your task was called here.
|
I think David already pointed out this, but you can try using |
|
@stsewd instead of the existing delete on ImportedFiles, or on PageViews? |
on both :D |
This is similar to #12386, but trying to do a simpler version first.
The test was failing locally with IntegrityErrors,
so I'm not sure if this is a good approach or not.