From 706e245692b95c3c94fd6561247259ef70ecc7c0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 21 Feb 2018 20:52:09 -0500 Subject: [PATCH 1/7] Test for changing subproject privacy level --- .../rtd_tests/tests/test_project_symlinks.py | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_project_symlinks.py b/readthedocs/rtd_tests/tests/test_project_symlinks.py index c55e6ea8f23..c19a6c88361 100644 --- a/readthedocs/rtd_tests/tests/test_project_symlinks.py +++ b/readthedocs/rtd_tests/tests/test_project_symlinks.py @@ -908,3 +908,105 @@ def test_symlink_no_error(self): self.symlink.run() except: self.fail('Symlink run raised an exception on unicode slug') + + +@override_settings() +class TestPublicPrivateSymlink(TempSiterootCase, TestCase): + + def setUp(self): + super(TestPublicPrivateSymlink, self).setUp() + self.project = get(Project, slug='project', privacy_level='public', + main_language_project=None) + self.project.versions.update(privacy_level='public') + self.project.save() + + self.subproject = get(Project, slug='subproject', privacy_level='public', + main_language_project=None) + self.subproject.versions.update(privacy_level='public') + self.subproject.save() + + def test_change_subproject_privacy(self): + filesystem_before = { + 'private_cname_project': {}, + 'private_cname_root': {}, + 'private_web_root': { + 'project': {'en': {},}, + 'subproject': {'en': {},}, + }, + 'public_cname_project': {}, + 'public_cname_root': {}, + 'public_web_root': { + 'project': { + 'en': {'latest': { + 'type': 'link', + 'target': 'user_builds/project/rtd-builds/latest', + },}, + 'projects': { + 'subproject': { + 'type': 'link', + 'target': 'public_web_root/subproject', + }, + }, + }, + 'subproject': { + 'en': {'latest': { + 'type': 'link', + 'target': 'user_builds/subproject/rtd-builds/latest', + },}, + }, + }, + } + + filesystem_after = { + 'private_cname_project': {}, + 'private_cname_root': {}, + 'private_web_root': { + 'project': { + 'en': {}, + }, + 'subproject': { + 'en': { + 'latest': { + 'type': 'link', + 'target': 'user_builds/subproject/rtd-builds/latest', + }, + }, + }, + }, + 'public_cname_project': {}, + 'public_cname_root': {}, + 'public_web_root': { + 'project': { + 'en': { + 'latest': { + 'type': 'link', + 'target': 'user_builds/project/rtd-builds/latest', + }, + }, + 'projects': {}, + }, + 'subproject': { + 'en': {}, + }, + }, + } + + self.project.add_subproject(self.subproject) + + from readthedocs.projects.tasks import symlink_project, symlink_subproject + symlink_project(self.project.pk) + symlink_subproject(self.project.pk) + + self.assertFilesystem(filesystem_before) + + self.subproject.privacy_level = 'private' + # self.subproject.version_privacy_level = 'private' + self.subproject.save() + + # These two lines shouldn't be necessary because this should be done + # automatically on ``self.subproject.save()`` but that is the bug I want + # to fix + symlink_project(self.project.pk) + symlink_subproject(self.project.pk) + + self.assertFilesystem(filesystem_after) From 52546ab2994a3715aed0bfb2133341e630c3f62d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 27 Feb 2018 20:46:15 -0500 Subject: [PATCH 2/7] Proper test for privacy level on subprojects --- .../rtd_tests/tests/test_project_symlinks.py | 44 +++++++++++++------ requirements/testing.txt | 1 + 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_project_symlinks.py b/readthedocs/rtd_tests/tests/test_project_symlinks.py index c19a6c88361..32b4d6f845f 100644 --- a/readthedocs/rtd_tests/tests/test_project_symlinks.py +++ b/readthedocs/rtd_tests/tests/test_project_symlinks.py @@ -930,17 +930,23 @@ def test_change_subproject_privacy(self): 'private_cname_project': {}, 'private_cname_root': {}, 'private_web_root': { - 'project': {'en': {},}, - 'subproject': {'en': {},}, + 'project': { + 'en': {}, + }, + 'subproject': { + 'en': {}, + }, }, 'public_cname_project': {}, 'public_cname_root': {}, 'public_web_root': { 'project': { - 'en': {'latest': { - 'type': 'link', - 'target': 'user_builds/project/rtd-builds/latest', - },}, + 'en': { + 'latest': { + 'type': 'link', + 'target': 'user_builds/project/rtd-builds/latest', + }, + }, 'projects': { 'subproject': { 'type': 'link', @@ -949,10 +955,12 @@ def test_change_subproject_privacy(self): }, }, 'subproject': { - 'en': {'latest': { - 'type': 'link', - 'target': 'user_builds/subproject/rtd-builds/latest', - },}, + 'en': { + 'latest': { + 'type': 'link', + 'target': 'user_builds/subproject/rtd-builds/latest', + }, + }, }, }, } @@ -963,6 +971,12 @@ def test_change_subproject_privacy(self): 'private_web_root': { 'project': { 'en': {}, + 'projects': { + 'subproject': { + 'type': 'link', + 'target': 'private_web_root/subproject', + }, + }, }, 'subproject': { 'en': { @@ -1000,13 +1014,17 @@ def test_change_subproject_privacy(self): self.assertFilesystem(filesystem_before) self.subproject.privacy_level = 'private' - # self.subproject.version_privacy_level = 'private' + self.subproject.versions.update(privacy_level='private') self.subproject.save() # These two lines shouldn't be necessary because this should be done # automatically on ``self.subproject.save()`` but that is the bug I want + # (un-commenting these lines makes the test pass) # to fix - symlink_project(self.project.pk) - symlink_subproject(self.project.pk) + # symlink_project(self.project.pk) + # symlink_subproject(self.project.pk) + + # from datadiff import diff + # print(diff(get_filesystem(self.site_root), filesystem_after)) self.assertFilesystem(filesystem_after) diff --git a/requirements/testing.txt b/requirements/testing.txt index cf52456e67f..d328262d435 100644 --- a/requirements/testing.txt +++ b/requirements/testing.txt @@ -9,3 +9,4 @@ Mercurial==4.4.2 # local debugging tools pdbpp +datadiff From 3fe6cb3f3dfddc87d8c1e658cb7f3ebad4f6f476 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 27 Feb 2018 22:11:36 -0500 Subject: [PATCH 3/7] Trigger re-symlink for superproject when project changes Re-symlink when: * a subproject is deleted * a subproject privacy level is changed * a subproject version privacy level is changed --- readthedocs/projects/views/private.py | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 6db70fb6af5..8fd0d032be6 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -127,6 +127,16 @@ def get_queryset(self): def get_success_url(self): return reverse('projects_detail', args=[self.object.slug]) + def form_valid(self, form): + form.save() + if form.has_changed(): + if 'privacy_level' in form.changed_data: + log.info('Re-symlinking all superprojects due project privacy level has changed') + for superproject in self.object.superprojects.all(): + broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) + + return HttpResponseRedirect(self.get_success_url()) + @login_required def project_versions(request, project_slug): @@ -148,6 +158,20 @@ def project_versions(request, project_slug): if request.method == 'POST' and form.is_valid(): form.save() + + if form.has_changed(): + resymlink_superprojects = False + # Each field is form as ``privacy-{{version.slug}}`` + for changed_data in form.changed_data: + if changed_data.startswith('privacy-'): + resymlink_superprojects = True + break + + if resymlink_superprojects: + log.info('Re-symlinking all superprojects due version privacy level has changed') + for superproject in project.superprojects.all(): + broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) + messages.success(request, _('Project versions updated')) project_dashboard = reverse('projects_detail', args=[project.slug]) return HttpResponseRedirect(project_dashboard) @@ -178,6 +202,13 @@ def project_version_detail(request, project_slug, version_slug): type='app', task=tasks.clear_artifacts, args=[version.pk]) version.built = False version.save() + + if 'privacy_level' in form.changed_data: + log.info('Re-symlinking all superprojects due version privacy level has changed') + for superproject in project.superprojects.all(): + broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) + + url = reverse('project_version_list', args=[project.slug]) return HttpResponseRedirect(url) @@ -200,6 +231,11 @@ def project_delete(request, project_slug): if request.method == 'POST': broadcast(type='app', task=tasks.remove_dir, args=[project.doc_path]) project.delete() + + log.info('Re-symlinking all superprojects due project deletion') + for superproject in project.superprojects.all(): + broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) + messages.success(request, _('Project deleted')) project_dashboard = reverse('projects_dashboard') return HttpResponseRedirect(project_dashboard) From 4854caaa56aa05d946cc1f4edf785baf8063634f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 27 Feb 2018 22:15:00 -0500 Subject: [PATCH 4/7] Update test case for current implementation --- .../rtd_tests/tests/test_project_symlinks.py | 59 ++++++++++++++++--- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_project_symlinks.py b/readthedocs/rtd_tests/tests/test_project_symlinks.py index 32b4d6f845f..5bee5bd7249 100644 --- a/readthedocs/rtd_tests/tests/test_project_symlinks.py +++ b/readthedocs/rtd_tests/tests/test_project_symlinks.py @@ -915,13 +915,18 @@ class TestPublicPrivateSymlink(TempSiterootCase, TestCase): def setUp(self): super(TestPublicPrivateSymlink, self).setUp() - self.project = get(Project, slug='project', privacy_level='public', - main_language_project=None) + from django.contrib.auth.models import User + + self.user = get(User) + self.project = get( + Project, name='project', slug='project', privacy_level='public', + users=[self.user], main_language_project=None) self.project.versions.update(privacy_level='public') self.project.save() - self.subproject = get(Project, slug='subproject', privacy_level='public', - main_language_project=None) + self.subproject = get( + Project, name='subproject', slug='subproject', privacy_level='public', + users=[self.user], main_language_project=None) self.subproject.versions.update(privacy_level='public') self.subproject.save() @@ -1005,17 +1010,21 @@ def test_change_subproject_privacy(self): }, } + self.assertEqual(self.project.subprojects.all().count(), 0) + self.assertEqual(self.subproject.superprojects.all().count(), 0) self.project.add_subproject(self.subproject) + self.assertEqual(self.project.subprojects.all().count(), 1) + self.assertEqual(self.subproject.superprojects.all().count(), 1) from readthedocs.projects.tasks import symlink_project, symlink_subproject symlink_project(self.project.pk) - symlink_subproject(self.project.pk) + # symlink_subproject(self.project.pk) self.assertFilesystem(filesystem_before) - self.subproject.privacy_level = 'private' - self.subproject.versions.update(privacy_level='private') - self.subproject.save() + # self.subproject.privacy_level = 'private' + # self.subproject.versions.update(privacy_level='private') + # self.subproject.save() # These two lines shouldn't be necessary because this should be done # automatically on ``self.subproject.save()`` but that is the bug I want @@ -1027,4 +1036,38 @@ def test_change_subproject_privacy(self): # from datadiff import diff # print(diff(get_filesystem(self.site_root), filesystem_after)) + from django.core.urlresolvers import reverse + + self.client.force_login(self.user) + resp = self.client.post( + reverse('project_version_detail', + kwargs={ + 'project_slug': self.subproject.slug, + 'version_slug': self.subproject.versions.first().slug, + }), + data={'privacy_level': 'private'}, + ) + self.assertEqual(self.subproject.versions.first().privacy_level, 'private') + + resp = self.client.post( + reverse('projects_advanced', + kwargs={ + 'project_slug': self.subproject.slug, + }), + data={ + # Required defaults + 'python_interpreter': 'python', + 'default_version': 'latest', + + 'privacy_level': 'private', + }, + ) + self.subproject.refresh_from_db() + self.assertEqual(self.subproject.privacy_level, 'private') + + from datadiff import diff + print(diff(get_filesystem(self.site_root), filesystem_after)) + + + # import pdb; pdb.set_trace() # yapf: disable self.assertFilesystem(filesystem_after) From 11abe3a45d6d1422b88e4933020de915364d5fe3 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 6 Mar 2018 10:22:52 -0500 Subject: [PATCH 5/7] Revert "Trigger re-symlink for superproject when project changes" This reverts commit 3fe6cb3f3dfddc87d8c1e658cb7f3ebad4f6f476. --- readthedocs/projects/views/private.py | 36 --------------------------- 1 file changed, 36 deletions(-) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 8fd0d032be6..6db70fb6af5 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -127,16 +127,6 @@ def get_queryset(self): def get_success_url(self): return reverse('projects_detail', args=[self.object.slug]) - def form_valid(self, form): - form.save() - if form.has_changed(): - if 'privacy_level' in form.changed_data: - log.info('Re-symlinking all superprojects due project privacy level has changed') - for superproject in self.object.superprojects.all(): - broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) - - return HttpResponseRedirect(self.get_success_url()) - @login_required def project_versions(request, project_slug): @@ -158,20 +148,6 @@ def project_versions(request, project_slug): if request.method == 'POST' and form.is_valid(): form.save() - - if form.has_changed(): - resymlink_superprojects = False - # Each field is form as ``privacy-{{version.slug}}`` - for changed_data in form.changed_data: - if changed_data.startswith('privacy-'): - resymlink_superprojects = True - break - - if resymlink_superprojects: - log.info('Re-symlinking all superprojects due version privacy level has changed') - for superproject in project.superprojects.all(): - broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) - messages.success(request, _('Project versions updated')) project_dashboard = reverse('projects_detail', args=[project.slug]) return HttpResponseRedirect(project_dashboard) @@ -202,13 +178,6 @@ def project_version_detail(request, project_slug, version_slug): type='app', task=tasks.clear_artifacts, args=[version.pk]) version.built = False version.save() - - if 'privacy_level' in form.changed_data: - log.info('Re-symlinking all superprojects due version privacy level has changed') - for superproject in project.superprojects.all(): - broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) - - url = reverse('project_version_list', args=[project.slug]) return HttpResponseRedirect(url) @@ -231,11 +200,6 @@ def project_delete(request, project_slug): if request.method == 'POST': broadcast(type='app', task=tasks.remove_dir, args=[project.doc_path]) project.delete() - - log.info('Re-symlinking all superprojects due project deletion') - for superproject in project.superprojects.all(): - broadcast(type='app', task=tasks.symlink_project, args=[superproject.pk]) - messages.success(request, _('Project deleted')) project_dashboard = reverse('projects_dashboard') return HttpResponseRedirect(project_dashboard) From 5472906a067adc60f42556108993255393408410 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 6 Mar 2018 14:53:08 -0500 Subject: [PATCH 6/7] Move logic from Form to Model Instead of trigger the re-symlink task on each of the Form actions, we trigger it once on ``Project.save()`` or ``Project.delete()`` method. --- readthedocs/projects/models.py | 22 ++++++++- .../rtd_tests/tests/test_project_symlinks.py | 47 +++++++------------ 2 files changed, 38 insertions(+), 31 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 5ff628e899f..c4249351e84 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -326,8 +326,26 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ log.exception('failed to sync supported versions') try: if not first_save: - broadcast(type='app', task=tasks.symlink_project, - args=[self.pk],) + log.info( + 'Re-symlinking project and subprojects: project=%s', + self.slug, + ) + broadcast( + type='app', + task=tasks.symlink_project, + args=[self.pk], + ) + log.info( + 'Re-symlinking superprojects: project=%s', + self.slug, + ) + for superproject in self.superprojects.all(): + broadcast( + type='app', + task=tasks.symlink_project, + args=[superproject.pk], + ) + except Exception: log.exception('failed to symlink project') try: diff --git a/readthedocs/rtd_tests/tests/test_project_symlinks.py b/readthedocs/rtd_tests/tests/test_project_symlinks.py index 5bee5bd7249..ed88795be1a 100644 --- a/readthedocs/rtd_tests/tests/test_project_symlinks.py +++ b/readthedocs/rtd_tests/tests/test_project_symlinks.py @@ -5,16 +5,16 @@ import os import shutil import tempfile -import collections -from functools import wraps import mock from django.conf import settings +from django.core.urlresolvers import reverse from django.test import TestCase, override_settings from django_dynamic_fixture import get from readthedocs.builds.models import Version from readthedocs.projects.models import Project, Domain +from readthedocs.projects.tasks import symlink_project from readthedocs.core.symlink import PublicSymlink, PrivateSymlink @@ -931,6 +931,13 @@ def setUp(self): self.subproject.save() def test_change_subproject_privacy(self): + """ + Change subproject's ``privacy_level`` creates proper symlinks. + + When the ``privacy_level`` changes in the subprojects, we need to + re-symlink the superproject also to keep in sync its symlink under the + private/public roots. + """ filesystem_before = { 'private_cname_project': {}, 'private_cname_root': {}, @@ -1016,40 +1023,26 @@ def test_change_subproject_privacy(self): self.assertEqual(self.project.subprojects.all().count(), 1) self.assertEqual(self.subproject.superprojects.all().count(), 1) - from readthedocs.projects.tasks import symlink_project, symlink_subproject + self.assertTrue(self.project.versions.first().active) + self.assertTrue(self.subproject.versions.first().active) symlink_project(self.project.pk) - # symlink_subproject(self.project.pk) self.assertFilesystem(filesystem_before) - # self.subproject.privacy_level = 'private' - # self.subproject.versions.update(privacy_level='private') - # self.subproject.save() - - # These two lines shouldn't be necessary because this should be done - # automatically on ``self.subproject.save()`` but that is the bug I want - # (un-commenting these lines makes the test pass) - # to fix - # symlink_project(self.project.pk) - # symlink_subproject(self.project.pk) - - # from datadiff import diff - # print(diff(get_filesystem(self.site_root), filesystem_after)) - - from django.core.urlresolvers import reverse - self.client.force_login(self.user) - resp = self.client.post( + self.client.post( reverse('project_version_detail', kwargs={ 'project_slug': self.subproject.slug, 'version_slug': self.subproject.versions.first().slug, }), - data={'privacy_level': 'private'}, + data={'privacy_level': 'private', 'active': True}, ) + self.assertEqual(self.subproject.versions.first().privacy_level, 'private') + self.assertTrue(self.subproject.versions.first().active) - resp = self.client.post( + self.client.post( reverse('projects_advanced', kwargs={ 'project_slug': self.subproject.slug, @@ -1062,12 +1055,8 @@ def test_change_subproject_privacy(self): 'privacy_level': 'private', }, ) + + self.assertTrue(self.subproject.versions.first().active) self.subproject.refresh_from_db() self.assertEqual(self.subproject.privacy_level, 'private') - - from datadiff import diff - print(diff(get_filesystem(self.site_root), filesystem_after)) - - - # import pdb; pdb.set_trace() # yapf: disable self.assertFilesystem(filesystem_after) From ae6768e000847b4de1c1446b7e23aa9500aabb18 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Fri, 9 Mar 2018 11:45:58 -0500 Subject: [PATCH 7/7] Test for calls to broadcast utility on Project.save() --- .../rtd_tests/tests/test_project_symlinks.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_project_symlinks.py b/readthedocs/rtd_tests/tests/test_project_symlinks.py index ed88795be1a..149e10c1242 100644 --- a/readthedocs/rtd_tests/tests/test_project_symlinks.py +++ b/readthedocs/rtd_tests/tests/test_project_symlinks.py @@ -909,6 +909,53 @@ def test_symlink_no_error(self): except: self.fail('Symlink run raised an exception on unicode slug') + def test_symlink_broadcast_calls_on_project_save(self): + """ + Test calls to ``readthedocs.core.utils.broadcast`` on Project.save(). + + When a Project is saved, we need to check that we are calling + ``broadcast`` utility with the proper task and arguments to re-symlink + them. + """ + with mock.patch('readthedocs.projects.models.broadcast') as broadcast: + project = get(Project) + # skipped on first save + broadcast.assert_not_called() + + broadcast.reset_mock() + project.description = 'New description' + project.save() + # called once for this project itself + broadcast.assert_any_calls( + type='app', + task=symlink_project, + args=[project.pk], + ) + + broadcast.reset_mock() + subproject = get(Project) + # skipped on first save + broadcast.assert_not_called() + + project.add_subproject(subproject) + # subproject.save() is not called + broadcast.assert_not_called() + + subproject.description = 'New subproject description' + subproject.save() + # subproject symlinks + broadcast.assert_any_calls( + type='app', + task=symlink_project, + args=[subproject.pk], + ) + # superproject symlinks + broadcast.assert_any_calls( + type='app', + task=symlink_project, + args=[project.pk], + ) + @override_settings() class TestPublicPrivateSymlink(TempSiterootCase, TestCase):