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

[Fix #4576] Do not delete projects which have multiple users #4577

Merged
merged 4 commits into from Aug 29, 2018

Conversation

Projects
None yet
5 participants
@safwanrahman
Member

safwanrahman commented Aug 28, 2018

Fixes #4576
The query was limiting the users before annotation because of django's backward relation query. the Documentation of django does not provide much information about it, but I have gone through the differentiate of the sql query and found the root cause.
I have also added tests for this so it should not happen again. I should have written the tests when I implemented the feature in #3214.
@agjohnson @humitos r?

@humitos

These changes look good.

I test these queries (old and new ones) in production DB and the new ones return what is expected.

Although, by checking the underline SQL code executed I wasn't able to fully understand the difference

Show outdated Hide outdated readthedocs/core/signals.py Outdated
obj.refresh_from_db()
assert obj.id
obj_users = obj.users.all()
assert len(obj_users) == 1

This comment has been minimized.

@humitos

humitos Aug 28, 2018

Member

nit: use .count() here.

@humitos

humitos Aug 28, 2018

Member

nit: use .count() here.

This comment has been minimized.

@safwanrahman

safwanrahman Aug 28, 2018

Member

We need to take the queryset value in memory eventually. So better to use len() to reduce single COUNT sql query

@safwanrahman

safwanrahman Aug 28, 2018

Member

We need to take the queryset value in memory eventually. So better to use len() to reduce single COUNT sql query

This comment has been minimized.

@humitos

humitos Aug 29, 2018

Member

I wouldn't use len to count queryset elements independently of the context.

@humitos

humitos Aug 29, 2018

Member

I wouldn't use len to count queryset elements independently of the context.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 28, 2018

Member

There are two unrelated tests failing (yaml config file)

Member

humitos commented Aug 28, 2018

There are two unrelated tests failing (yaml config file)

@agjohnson

Nice catch! I still don't fully understand the fix so we might need some more thorough tests to ensure this does not happen again.

Show outdated Hide outdated readthedocs/core/tests/test_signals.py Outdated
@pytest.mark.django_db
class TestProjectOrganizationSignal(object):

This comment has been minimized.

@agjohnson

agjohnson Aug 28, 2018

Contributor

I'm mostly certain we have tests for both of these cases already. Can you provide test output of these tests without your patch applied?

@agjohnson

agjohnson Aug 28, 2018

Contributor

I'm mostly certain we have tests for both of these cases already. Can you provide test output of these tests without your patch applied?

This comment has been minimized.

@safwanrahman

safwanrahman Aug 28, 2018

Member

I missed adding tests while implementing the feature for both of the case.
Here is the output without applying my patch

=============================================================================== FAILURES ===============================================================================
______________________________________ TestProjectOrganizationSignal.test_multiple_users_project_organization_not_delete[Project] ______________________________________

self = <readthedocs.core.tests.test_signals.TestProjectOrganizationSignal object at 0x10eb85d30>, model_class = <class 'readthedocs.projects.models.Project'>

    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
    def test_multiple_users_project_organization_not_delete(self, model_class):
        """
            Check Project or RemoteOrganization which have multiple users do not get deleted
            when any of the user delete his account.
            """
    
        obj = django_dynamic_fixture.get(model_class)
        user1 = django_dynamic_fixture.get(User)
        user2 = django_dynamic_fixture.get(User)
        obj.users.add(user1, user2)
    
        obj.refresh_from_db()
        assert obj.users.all().count() > 1
        # Delete 1 user of the project
        user1.delete()
    
        # The project should still exist and it should have 1 user
>       obj.refresh_from_db()

core/tests/test_signals.py:52: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../.tox/py36/lib/python3.6/site-packages/django/db/models/base.py:592: in refresh_from_db
    db_instance = db_instance_qs.get()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = [], args = (), kwargs = {}, clone = [], num = 0

    def get(self, *args, **kwargs):
        """
            Performs the query and returns a single object matching the given
            keyword arguments.
            """
        clone = self.filter(*args, **kwargs)
        if self.query.can_filter() and not self.query.distinct_fields:
            clone = clone.order_by()
        num = len(clone)
        if num == 1:
            return clone._result_cache[0]
        if not num:
            raise self.model.DoesNotExist(
                "%s matching query does not exist." %
>               self.model._meta.object_name
            )
E           readthedocs.projects.models.DoesNotExist: Project matching query does not exist.

../.tox/py36/lib/python3.6/site-packages/django/db/models/query.py:387: DoesNotExist
________________________________ TestProjectOrganizationSignal.test_multiple_users_project_organization_not_delete[RemoteOrganization] _________________________________

self = <readthedocs.core.tests.test_signals.TestProjectOrganizationSignal object at 0x10eb9f9b0>, model_class = <class 'readthedocs.oauth.models.RemoteOrganization'>

    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
    def test_multiple_users_project_organization_not_delete(self, model_class):
        """
            Check Project or RemoteOrganization which have multiple users do not get deleted
            when any of the user delete his account.
            """
    
        obj = django_dynamic_fixture.get(model_class)
        user1 = django_dynamic_fixture.get(User)
        user2 = django_dynamic_fixture.get(User)
        obj.users.add(user1, user2)
    
        obj.refresh_from_db()
        assert obj.users.all().count() > 1
        # Delete 1 user of the project
        user1.delete()
    
        # The project should still exist and it should have 1 user
>       obj.refresh_from_db()

core/tests/test_signals.py:52: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../.tox/py36/lib/python3.6/site-packages/django/db/models/base.py:592: in refresh_from_db
    db_instance = db_instance_qs.get()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = [], args = (), kwargs = {}, clone = [], num = 0

    def get(self, *args, **kwargs):
        """
            Performs the query and returns a single object matching the given
            keyword arguments.
            """
        clone = self.filter(*args, **kwargs)
        if self.query.can_filter() and not self.query.distinct_fields:
            clone = clone.order_by()
        num = len(clone)
        if num == 1:
            return clone._result_cache[0]
        if not num:
            raise self.model.DoesNotExist(
                "%s matching query does not exist." %
>               self.model._meta.object_name
            )
E           readthedocs.oauth.models.DoesNotExist: RemoteOrganization matching query does not exist.

../.tox/py36/lib/python3.6/site-packages/django/db/models/query.py:387: DoesNotExist
================================================================= 2 failed, 2 passed in 12.99 seconds ==================================================================
ERROR: InvocationError for command '/Users/safwan/readthedocs/.tox/py36/bin/py.test core -s --reuse-db -vvv' (exited with code 1)
@safwanrahman

safwanrahman Aug 28, 2018

Member

I missed adding tests while implementing the feature for both of the case.
Here is the output without applying my patch

=============================================================================== FAILURES ===============================================================================
______________________________________ TestProjectOrganizationSignal.test_multiple_users_project_organization_not_delete[Project] ______________________________________

self = <readthedocs.core.tests.test_signals.TestProjectOrganizationSignal object at 0x10eb85d30>, model_class = <class 'readthedocs.projects.models.Project'>

    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
    def test_multiple_users_project_organization_not_delete(self, model_class):
        """
            Check Project or RemoteOrganization which have multiple users do not get deleted
            when any of the user delete his account.
            """
    
        obj = django_dynamic_fixture.get(model_class)
        user1 = django_dynamic_fixture.get(User)
        user2 = django_dynamic_fixture.get(User)
        obj.users.add(user1, user2)
    
        obj.refresh_from_db()
        assert obj.users.all().count() > 1
        # Delete 1 user of the project
        user1.delete()
    
        # The project should still exist and it should have 1 user
>       obj.refresh_from_db()

core/tests/test_signals.py:52: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../.tox/py36/lib/python3.6/site-packages/django/db/models/base.py:592: in refresh_from_db
    db_instance = db_instance_qs.get()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = [], args = (), kwargs = {}, clone = [], num = 0

    def get(self, *args, **kwargs):
        """
            Performs the query and returns a single object matching the given
            keyword arguments.
            """
        clone = self.filter(*args, **kwargs)
        if self.query.can_filter() and not self.query.distinct_fields:
            clone = clone.order_by()
        num = len(clone)
        if num == 1:
            return clone._result_cache[0]
        if not num:
            raise self.model.DoesNotExist(
                "%s matching query does not exist." %
>               self.model._meta.object_name
            )
E           readthedocs.projects.models.DoesNotExist: Project matching query does not exist.

../.tox/py36/lib/python3.6/site-packages/django/db/models/query.py:387: DoesNotExist
________________________________ TestProjectOrganizationSignal.test_multiple_users_project_organization_not_delete[RemoteOrganization] _________________________________

self = <readthedocs.core.tests.test_signals.TestProjectOrganizationSignal object at 0x10eb9f9b0>, model_class = <class 'readthedocs.oauth.models.RemoteOrganization'>

    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
    def test_multiple_users_project_organization_not_delete(self, model_class):
        """
            Check Project or RemoteOrganization which have multiple users do not get deleted
            when any of the user delete his account.
            """
    
        obj = django_dynamic_fixture.get(model_class)
        user1 = django_dynamic_fixture.get(User)
        user2 = django_dynamic_fixture.get(User)
        obj.users.add(user1, user2)
    
        obj.refresh_from_db()
        assert obj.users.all().count() > 1
        # Delete 1 user of the project
        user1.delete()
    
        # The project should still exist and it should have 1 user
>       obj.refresh_from_db()

core/tests/test_signals.py:52: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../.tox/py36/lib/python3.6/site-packages/django/db/models/base.py:592: in refresh_from_db
    db_instance = db_instance_qs.get()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = [], args = (), kwargs = {}, clone = [], num = 0

    def get(self, *args, **kwargs):
        """
            Performs the query and returns a single object matching the given
            keyword arguments.
            """
        clone = self.filter(*args, **kwargs)
        if self.query.can_filter() and not self.query.distinct_fields:
            clone = clone.order_by()
        num = len(clone)
        if num == 1:
            return clone._result_cache[0]
        if not num:
            raise self.model.DoesNotExist(
                "%s matching query does not exist." %
>               self.model._meta.object_name
            )
E           readthedocs.oauth.models.DoesNotExist: RemoteOrganization matching query does not exist.

../.tox/py36/lib/python3.6/site-packages/django/db/models/query.py:387: DoesNotExist
================================================================= 2 failed, 2 passed in 12.99 seconds ==================================================================
ERROR: InvocationError for command '/Users/safwan/readthedocs/.tox/py36/bin/py.test core -s --reuse-db -vvv' (exited with code 1)

This comment has been minimized.

@stsewd

stsewd Aug 28, 2018

Member

Maybe is worth it to add a test for the delete_account view too

@stsewd

stsewd Aug 28, 2018

Member

Maybe is worth it to add a test for the delete_account view too

This comment has been minimized.

@safwanrahman

safwanrahman Aug 28, 2018

Member

Yeah, I am going to add some test on that! but its out of scope of this PR!
will open another PR for that!

@safwanrahman

safwanrahman Aug 28, 2018

Member

Yeah, I am going to add some test on that! but its out of scope of this PR!
will open another PR for that!

This comment has been minimized.

@agjohnson

agjohnson Aug 28, 2018

Contributor

Great, test output looks good. That's what I'd expect if the delete cascaded to the project

@agjohnson

agjohnson Aug 28, 2018

Contributor

Great, test output looks good. That's what I'd expect if the delete cascaded to the project

Show outdated Hide outdated readthedocs/core/signals.py Outdated
.exclude(num_users__gt=1))
oauth_organizations = (RemoteOrganization.objects.annotate(num_users=Count('users'))
.filter(users=instance.id)
.exclude(num_users__gt=1))

This comment has been minimized.

@agjohnson

agjohnson Aug 28, 2018

Contributor

These changes should conform to our styleguide by running pre-commit on the file

@agjohnson

agjohnson Aug 28, 2018

Contributor

These changes should conform to our styleguide by running pre-commit on the file

This comment has been minimized.

@safwanrahman

safwanrahman Aug 28, 2018

Member

I think it does not break PEP8 styleguide. Can you explain how it would change?

@safwanrahman

safwanrahman Aug 28, 2018

Member

I think it does not break PEP8 styleguide. Can you explain how it would change?

This comment has been minimized.

@davidfischer

davidfischer Aug 28, 2018

Contributor

Perhaps a link to the style guide is warranted. I don't know of any RTD style guide outside of what the tests do. I searched our docs but I came up empty.

@davidfischer

davidfischer Aug 28, 2018

Contributor

Perhaps a link to the style guide is warranted. I don't know of any RTD style guide outside of what the tests do. I searched our docs but I came up empty.

This comment has been minimized.

@stsewd

stsewd Aug 28, 2018

Member

We have the style guide inside pre-commit p:

@stsewd

stsewd Aug 28, 2018

Member

We have the style guide inside pre-commit p:

This comment has been minimized.

@agjohnson

agjohnson Aug 28, 2018

Contributor

Yup, no explicit styleguide. it's all enforced by pre-commit via yapf and friends. In this specific case, we opt for no right side alignment of variables and arguments. Both are pep8 compliant, but right side alignment is too unreadable and annoying to maintain.

@agjohnson

agjohnson Aug 28, 2018

Contributor

Yup, no explicit styleguide. it's all enforced by pre-commit via yapf and friends. In this specific case, we opt for no right side alignment of variables and arguments. Both are pep8 compliant, but right side alignment is too unreadable and annoying to maintain.

This comment has been minimized.

@agjohnson
@agjohnson

agjohnson Aug 28, 2018

Contributor
@safwanrahman

This comment has been minimized.

Show comment
Hide comment
@safwanrahman

safwanrahman Aug 28, 2018

Member

updated!

Member

safwanrahman commented Aug 28, 2018

updated!

@davidfischer

Overall, this is a good fix.

At some level, this bug was a mismatch between the expectations of the user and reality. One thing that would clear this up is to show on the delete screen exactly what is going to be removed by a user deleting their account (at least the projects). The Django Admin does this and I think it's a good pattern. That way there are fewer surprises.

screen shot 2018-08-28 at 12 02 45 pm

.exclude(num_users__gt=1))
oauth_organizations = (RemoteOrganization.objects.annotate(num_users=Count('users'))
.filter(users=instance.id)
.exclude(num_users__gt=1))

This comment has been minimized.

@davidfischer

davidfischer Aug 28, 2018

Contributor

Perhaps a link to the style guide is warranted. I don't know of any RTD style guide outside of what the tests do. I searched our docs but I came up empty.

@davidfischer

davidfischer Aug 28, 2018

Contributor

Perhaps a link to the style guide is warranted. I don't know of any RTD style guide outside of what the tests do. I searched our docs but I came up empty.

# https://github.com/rtfd/readthedocs.org/pull/4577
# https://docs.djangoproject.com/en/2.1/topics/db/aggregation/#order-of-annotate-and-filter-clauses # noqa
projects = (Project.objects.annotate(num_users=Count('users')).filter(users=instance.id)
.exclude(num_users__gt=1))

This comment has been minimized.

@davidfischer

davidfischer Aug 28, 2018

Contributor

I ran this query and it seems correct to me.

@davidfischer

davidfischer Aug 28, 2018

Contributor

I ran this query and it seems correct to me.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 28, 2018

Contributor

Showing changes would be interesting. Though:

  • It might be too noisey unless we prune the list some how
  • It might be inaccurate unless we copy what the admin does -- that is, we don't want to just show our interpretation of what is changing, but what is actually changing at the database

Fixing the logic here i think matches user expectations at least

Contributor

agjohnson commented Aug 28, 2018

Showing changes would be interesting. Though:

  • It might be too noisey unless we prune the list some how
  • It might be inaccurate unless we copy what the admin does -- that is, we don't want to just show our interpretation of what is changing, but what is actually changing at the database

Fixing the logic here i think matches user expectations at least

@agjohnson agjohnson added this to the 2.7 milestone Aug 28, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 29, 2018

Member

I like @davidfischer's idea about showing what's going to delete. Although, I think we can implement that in another PR and merge this fix soon since it's an important bug and may cause more nightmares to us dealing with the backups.

Also, I'd like to think and discuss a little more about what to show. On one side, I suppose that showing the Projects to be deleted is enough, but that seems to be the "common sense" thought and maybe doesn't worth to make it explicit. On another side, I don't want to go too verbose, as @agjohnson said.

Maybe we can take a look at what other services do for a similar flow.

Member

humitos commented Aug 29, 2018

I like @davidfischer's idea about showing what's going to delete. Although, I think we can implement that in another PR and merge this fix soon since it's an important bug and may cause more nightmares to us dealing with the backups.

Also, I'd like to think and discuss a little more about what to show. On one side, I suppose that showing the Projects to be deleted is enough, but that seems to be the "common sense" thought and maybe doesn't worth to make it explicit. On another side, I don't want to go too verbose, as @agjohnson said.

Maybe we can take a look at what other services do for a similar flow.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 29, 2018

Contributor

It might be too noisey unless we prune the list some how

I think only showing the projects is good. If deleting their account means deleting a lot of projects I think it would be nice to know that.

It might be inaccurate unless we copy what the admin does -- that is, we don't want to just show our interpretation of what is changing, but what is actually changing at the database

This is not quite correct. Deleting a user in the admin would not delete any projects. We implemented custom logic to delete projects based on a signal. If we plan to only show which projects are deleted, then we only need to copy our own logic.

I think we can implement that in another PR

That's fair.

Contributor

davidfischer commented Aug 29, 2018

It might be too noisey unless we prune the list some how

I think only showing the projects is good. If deleting their account means deleting a lot of projects I think it would be nice to know that.

It might be inaccurate unless we copy what the admin does -- that is, we don't want to just show our interpretation of what is changing, but what is actually changing at the database

This is not quite correct. Deleting a user in the admin would not delete any projects. We implemented custom logic to delete projects based on a signal. If we plan to only show which projects are deleted, then we only need to copy our own logic.

I think we can implement that in another PR

That's fair.

@agjohnson agjohnson merged commit f46abb5 into rtfd:master Aug 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 29, 2018

Contributor

I want to get this out and style fixes weren't applied. I merged manually.

Contributor

agjohnson commented Aug 29, 2018

I want to get this out and style fixes weren't applied. I merged manually.

@safwanrahman

This comment has been minimized.

Show comment
Hide comment
@safwanrahman

safwanrahman Aug 30, 2018

Member

Thanks @agjohnson. Sorry I missed the styling issue with the pull request discussion!

Member

safwanrahman commented Aug 30, 2018

Thanks @agjohnson. Sorry I missed the styling issue with the pull request discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment