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 a proxy model bug related to ad-free #4390

Merged
merged 4 commits into from Jul 18, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jul 17, 2018

This correctly initializes and tests the APIProject proxy model with respect to ad-free projects.

  • Correctly set ad-free status based on data in the API response
  • Adds a pretty minimal test for initializing the APIProject proxy models

Related to #4387

This has been edited...

@davidfischer davidfischer requested a review from agjohnson Jul 17, 2018
Copy link
Member

@humitos humitos left a comment

I tried this today and it seems that there is still something weird with this code.

(Pdb) project_data
{'id': 220270, 'name': 'rtd-project-b', 'slug': 'rtd-project-b', 'description': '', 'language': 'en', 'programming_language': 'words', 'repo': 'https://github.com/humitos/rtd-project-b.git', 'repo_type': 'git', 'default_version': 'latest', 'default_branch': '', 'documentation_type': 'sphinx', 'users': [2], 'canonical_url': 'http://localhost:8000/docs/rtd-project-b/en/latest/', 'enable_epub_build': True, 'enable_pdf_build': True, 'conf_py_file': '', 'analytics_code': '', 'cdn_enabled': False, 'container_image': '', 'container_mem_limit': '', 'container_time_limit': '', 'install_project': False, 'use_system_packages': False, 'suffix': '.rst', 'skip': False, 'requirements_file': '', 'python_interpreter': 'python', 'features': [], 'has_valid_clone': True, 'has_valid_webhook': True, 'show_advertising': False}
(Pdb) APIProject(**project_data).show_advertising
True
(Pdb) APIProject(**project_data).ad_free
False
(Pdb)

In that example, we are instantiating the APIProject object with show_advertising=False but then it returns True when accessing to it.

I didn't find the reason of this yet, but we need to keep researching about it to make it work properly.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 17, 2018

Thanks. I'll get a fix out for this shortly.

Loading

@humitos
Copy link
Member

@humitos humitos commented Jul 17, 2018

I just wrote a simple test case that shows this error:

diff --git a/readthedocs/rtd_tests/tests/test_project.py b/readthedocs/rtd_tests/tests/test_project.py
index e4d5b371a..41a46fed0 100644
--- a/readthedocs/rtd_tests/tests/test_project.py
+++ b/readthedocs/rtd_tests/tests/test_project.py
@@ -16,7 +16,7 @@ from readthedocs.builds.constants import (
     BUILD_STATE_CLONING, BUILD_STATE_FINISHED, BUILD_STATE_TRIGGERED, LATEST)
 from readthedocs.builds.models import Build
 from readthedocs.projects.exceptions import ProjectConfigurationError
-from readthedocs.projects.models import Project
+from readthedocs.projects.models import APIProject, Project
 from readthedocs.projects.tasks import finish_inactive_builds
 from readthedocs.rtd_tests.mocks.paths import fake_paths_by_regex
 
@@ -125,6 +125,46 @@ class TestProject(ProjectMixin, TestCase):
             self.pip.conf_file()
 
 
+class APIProxyObjectsTests(TestCase):
+
+    def test_api_project(self):
+        data = {
+            'id': 220270,
+            'name': 'project-name',
+            'slug': 'project-name',
+            'description': '',
+            'language': 'en',
+            'programming_language': 'words',
+            'repo': 'https://github.com/fake-repository.git',
+            'repo_type': 'git',
+            'default_version': 'latest',
+            'default_branch': '',
+            'documentation_type': 'sphinx',
+            'users': [2],
+            'canonical_url': 'http://localhost:8000/docs/project-name/en/latest/',
+            'enable_epub_build': True,
+            'enable_pdf_build': True,
+            'conf_py_file': '',
+            'analytics_code': '',
+            'cdn_enabled': False,
+            'container_image': '',
+            'container_mem_limit': '',
+            'container_time_limit': '',
+            'install_project': False,
+            'use_system_packages': False,
+            'suffix': '.rst',
+            'skip': False,
+            'requirements_file': '',
+            'python_interpreter': 'python',
+            'features': [],
+            'has_valid_clone': True,
+            'has_valid_webhook': True,
+            'show_advertising': False,
+        }
+        api_project = APIProject(**data)
+        self.assertFalse(api_project.show_advertising)
+        self.assertTrue(api_project.ad_free)
+
+
 class TestProjectTranslations(ProjectMixin, TestCase):
 
     def test_translations(self):

I didn't find any kind of test around APIProject or APIVersion so it may worth to have some for this.

Loading

Copy link
Member

@humitos humitos left a comment

Changes look good!

Just added some things to consider regarding styling and an extra check.

Loading

api_project = APIProject(**project_data)
self.assertEqual(api_project.slug, 'test-project')
self.assertEquals(api_project.features, [])
self.assertEqual(api_project.ad_free, False)
Copy link
Member

@humitos humitos Jul 17, 2018

Choose a reason for hiding this comment

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

nit: I think it's better to use assertFalse and assertTrue for this cases.

Loading

Copy link
Contributor Author

@davidfischer davidfischer Jul 18, 2018

Choose a reason for hiding this comment

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

Ya, you're right. I don't know why I did that.

Loading

project_data['show_advertising'] = False
api_project = APIProject(**project_data)
self.assertEquals(api_project.features, ['test-feature'])
self.assertEqual(api_project.ad_free, True)
Copy link
Member

@humitos humitos Jul 17, 2018

Choose a reason for hiding this comment

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

Maybe adding an assert for the show_advertising here is good to check that our overriden method does work.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 18, 2018

I updated the PR based on (great) feedback.

Loading

Copy link
Member

@humitos humitos left a comment

👍

Loading


api_project = APIProject(**project_data)
self.assertEqual(api_project.slug, 'test-project')
self.assertEquals(api_project.features, [])
Copy link
Member

@stsewd stsewd Jul 18, 2018

Choose a reason for hiding this comment

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

Any reason to uses assertEquals and not assertEqual?

Loading

Copy link
Contributor Author

@davidfischer davidfischer Jul 18, 2018

Choose a reason for hiding this comment

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

Seems like assertEqual is preferred. I never gave it much thought but I think I use the plural with lists/sets/tuples. I'll switch to singular.

Loading

@davidfischer davidfischer merged commit e506648 into master Jul 18, 2018
1 check passed
Loading
@davidfischer davidfischer deleted the davidfischer/project-proxy-model-bug branch Jul 18, 2018
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

4 participants