-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thanks. I'll get a fix out for this shortly. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good!
Just added some things to consider regarding styling and an extra check.
api_project = APIProject(**project_data) | ||
self.assertEqual(api_project.slug, 'test-project') | ||
self.assertEquals(api_project.features, []) | ||
self.assertEqual(api_project.ad_free, False) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
project_data['show_advertising'] = False | ||
api_project = APIProject(**project_data) | ||
self.assertEquals(api_project.features, ['test-feature']) | ||
self.assertEqual(api_project.ad_free, True) |
There was a problem hiding this comment.
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.
I updated the PR based on (great) feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
api_project = APIProject(**project_data) | ||
self.assertEqual(api_project.slug, 'test-project') | ||
self.assertEquals(api_project.features, []) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
This correctly initializes and tests the APIProject proxy model with respect to ad-free projects.
Related to #4387
This has been edited...