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

Sitemap assumes that all versions are translated Fix. #5535

Merged
merged 3 commits into from Apr 2, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Mar 25, 2019

closes #5365

@humitos humitos requested a review from Mar 25, 2019
Copy link
Member

@humitos humitos left a comment

Thanks! Logic looks good to me.

Although, I got confused with the test. Can you please expand there?

@@ -260,3 +274,11 @@ def test_sitemap_xml(self):
private=True,
),
)
self.assertNotContains(
Copy link
Member

@humitos humitos Mar 28, 2019

This test looks tricky to me.

We are adding a new public translated version of stable, which I expect to be listed in the response, but we are asserting to not contains. Why it does not appear in the response?

Copy link
Member

@humitos humitos Mar 28, 2019

I think the test we want to is a project with 3 public versions and only 1 version translated. So, in the response 2 should not appear and 1 should.

I'm not sure if I'm confused or missing something here.

Copy link
Member Author

@saadmk11 saadmk11 Mar 28, 2019

@humitos Here the public project has 2 public versions latest and stable (ie: Latest version is created automatically). but the translation project has only one version which is latest. so, I'm checking that as the translation project does not have a stable version, stable should not have an URL for that Translation in the sitemap.

Copy link
Member

@humitos humitos Apr 1, 2019

Makes sense.

Can you add this explanation as a comment above the assert line?

When creating this tests, I like to use descriptive names on things when possible. For example, the slug for the stable version in this case could be not-translated-version (stable is confusing since it's also created automatically).

Besides, the slug for the translated project could be something similar to translation-es and similar.

Copy link
Member

@humitos humitos Apr 1, 2019

Then, when checking for the assert everything is clearer:

          self.public.get_docs_url(
                version_slug=not_translated_version.slug,
                lang_slug=translation.language,
                private=False,
            ),

It easier to note that the "not translated version" should not be shown on the sitemap xml because it's not translated :)

Copy link
Member Author

@saadmk11 saadmk11 Apr 1, 2019

@humitos Thanks for Pointing it out. I'll keep that in mind. I have updated the PR have a Look.

readthedocs/core/views/serve.py Outdated Show resolved Hide resolved
humitos
humitos approved these changes Apr 2, 2019
Copy link
Member

@humitos humitos left a comment

Thanks a lot @saadmk11! I appreciate your work here :)

@humitos humitos merged commit 133be79 into readthedocs:master Apr 2, 2019
1 check passed
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Apr 3, 2019

@humitos Thank you for you guidance! 💯

@saadmk11 saadmk11 deleted the sitemap-fix branch Apr 3, 2019
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.

2 participants