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

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

Merged
merged 4 commits into from Aug 29, 2018

Conversation

@safwanrahman
Copy link
Member

@safwanrahman 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?

Copy link
Member

@humitos humitos left a comment

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

Loading

@@ -79,12 +80,16 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
def delete_projects_and_organizations(sender, instance, *args, **kwargs):
# Here we count the owner list from the projects that the user own
# Then exclude the projects where there are more than one owner
projects = instance.projects.all().annotate(num_users=Count('users')).exclude(num_users__gt=1)
# Add annotate before filter
# https://bit.ly/2Nne6ZJ
Copy link
Member

@humitos humitos Aug 28, 2018

Choose a reason for hiding this comment

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

nit: I'd use the original link from the docs instead of this one that hides the topic/section

Loading

Copy link
Member Author

@safwanrahman safwanrahman Aug 28, 2018

Choose a reason for hiding this comment

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

the link is >100 charecters. linter raise error on that!

Loading

Copy link
Member

@humitos humitos Aug 28, 2018

Choose a reason for hiding this comment

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

I personally prefer to put a # noqa or similar on this line and have the original link to the docs.

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

+1 on dropping bitly link from code. pointing to a github issue or self-reference the pr, or noqa on the line all work for this case.

Loading

obj.refresh_from_db()
assert obj.id
obj_users = obj.users.all()
assert len(obj_users) == 1
Copy link
Member

@humitos humitos Aug 28, 2018

Choose a reason for hiding this comment

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

nit: use .count() here.

Loading

Copy link
Member Author

@safwanrahman safwanrahman Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@humitos humitos Aug 29, 2018

Choose a reason for hiding this comment

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

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

Loading

@humitos
Copy link
Member

@humitos humitos commented Aug 28, 2018

There are two unrelated tests failing (yaml config file)

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

Loading

import pytest

from django.contrib.auth.models import User
from django_dynamic_fixture import G
Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

nitpick: we don't use django_dynamic_fixture.G at all, it's a poorly named variable -- linting would pick up on this if we were running pre-commit on our tests. django_dynamic_fixture.get is also poorly named, but much better than G. My personal preference is import django_dynamic_fixture as fixture; fixture.get.

Loading



@pytest.mark.django_db
class TestProjectOrganizationSignal(object):
Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Member Author

@safwanrahman safwanrahman Aug 28, 2018

Choose a reason for hiding this comment

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

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)

Loading

Copy link
Member

@stsewd stsewd Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@safwanrahman safwanrahman Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

@@ -79,12 +80,16 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
def delete_projects_and_organizations(sender, instance, *args, **kwargs):
# Here we count the owner list from the projects that the user own
# Then exclude the projects where there are more than one owner
projects = instance.projects.all().annotate(num_users=Count('users')).exclude(num_users__gt=1)
# Add annotate before filter
# https://bit.ly/2Nne6ZJ
Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

+1 on dropping bitly link from code. pointing to a github issue or self-reference the pr, or noqa on the line all work for this case.

Loading

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

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@safwanrahman safwanrahman Aug 28, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@davidfischer davidfischer Aug 28, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@stsewd stsewd Aug 28, 2018

Choose a reason for hiding this comment

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

We have the style guide inside pre-commit p:

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor

@agjohnson agjohnson Aug 28, 2018

Choose a reason for hiding this comment

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

Loading

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Aug 28, 2018

updated!

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

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

Loading

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

@davidfischer davidfischer Aug 28, 2018

Choose a reason for hiding this comment

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

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.

Loading

# 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))
Copy link
Contributor

@davidfischer davidfischer Aug 28, 2018

Choose a reason for hiding this comment

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

I ran this query and it seems correct to me.

Loading

@agjohnson
Copy link
Contributor

@agjohnson 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

Loading

@agjohnson agjohnson added this to the 2.7 milestone Aug 28, 2018
@humitos
Copy link
Member

@humitos 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.

Loading

@davidfischer
Copy link
Contributor

@davidfischer 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.

Loading

@agjohnson agjohnson merged commit f46abb5 into readthedocs:master Aug 29, 2018
1 check passed
Loading
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Aug 29, 2018

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

Loading

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Aug 30, 2018

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants