Skip to content

Sitemap sort order priorities updated #5724

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jun 17, 2019
Merged

Sitemap sort order priorities updated #5724

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8a23646
Sitemap sort order priorities updated
saadmk11 May 23, 2019
f2f392b
ordering fix
saadmk11 May 25, 2019
65bfd9b
doc string fix
saadmk11 May 25, 2019
6e7a87e
sort_by_priority function added to sitemap
saadmk11 May 29, 2019
6fecc6b
lint fix
saadmk11 May 29, 2019
55f6f3e
Sorting by swapping positions
saadmk11 Jun 11, 2019
6175b66
index out of range issue fix
saadmk11 Jun 12, 2019
d959933
Merge branch 'master' into sitemap-sort-order
saadmk11 Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions readthedocs/core/views/serve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ def priorities_generator():
It generates values from 1 to 0.1 by decreasing in 0.1 on each
iteration. After 0.1 is reached, it will keep returning 0.1.
"""
priorities = [1, 0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2]
priorities = [0.9, 1, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2]
Copy link
Member

Choose a reason for hiding this comment

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

We can leave this list sorted as it was, to avoid confusions.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we change this

latest with priority=0.9, changefreq=daily
stable with priority=1, changefreq=weekly

won't work. then we'll have to take another approach. So, I'll try another approach but this seem to have less complexity :)

Copy link
Member

Choose a reason for hiding this comment

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

Why not changing the order of changefreqs_generators to return weekly first?

Copy link
Member Author

Choose a reason for hiding this comment

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

changing the order of changefreqs_generators won't set the priority of stable to 1 and latest to 0.9.
priorities = [0.9, 1, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2]
this is the easiest way to set the priority correctly and also get the changefreq in order.
i.e:

[
  {
    'loc': 'http://public.readthedocs.io/en/stable/',
    'priority': 1,
    'changefreq': 'weekly',
    'languages': [
       ..........
    ]
  },
  {
    'loc': 'http://public.readthedocs.io/en/latest/',
    'priority': 0.9,
    'changefreq': 'daily',
    'languages': [
       ...........
    ]
  }
]

otherwise we have to look for another solution for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

or we have to define a function like this inside the sitemap view which will sort the versions keeping stable as the first position
https://github.com/rtfd/readthedocs.org/blob/900c57532a3199ffdb8975b732eade1be87a6e5c/readthedocs/projects/version_handling.py#L42-L65

even though we are calling it (comparable_version) from here, we have to remove this with the function we create
https://github.com/rtfd/readthedocs.org/blob/900c57532a3199ffdb8975b732eade1be87a6e5c/readthedocs/core/views/serve.py#L385

let me know if there is a better approach then this one or the previous one?

Copy link
Member

Choose a reason for hiding this comment

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

I was saying this because returning [0.9, 1...] contains some logic hidden in the woods. What we want in the end is assign 1 to stable an 0.9 to latest.

To keep things simple, why not just swapping these two versions from the result returned by sort_version_aware. Like,

sorted_versions = sort_versions_aware(...)
# TODO: add comment explaining why we do this here
sorted_versions[0], sorted_version[1] = sorted_version[1], sorted_version[0]

It's not good either, but at least it does not hide logic and show the problem explicitly.

I'm not sold in any of the "solutions" that I'm proposing. Follow the one that you feel it's better, more legible and maintainable. Whatever you choose, please add a comment in the correct place explaining what's happening and how is the sorting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I thought about swapping but it felt like a hacky way and wasn't able find anything simpler to do thats why I was asking you if you know of a better way.
I think we Shouldn’t go for a too complex way to do this small thing. Swapping is simple enough. I think I'll go with your idea with a good comment :)

yield from itertools.chain(priorities, itertools.repeat(0.1))

def hreflang_formatter(lang):
Expand Down Expand Up @@ -379,6 +379,14 @@ def changefreqs_generator():
changefreqs = ['daily', 'weekly']
yield from itertools.chain(changefreqs, itertools.repeat('monthly'))

def sort_by_priority(version_list):
Copy link
Member

Choose a reason for hiding this comment

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

nit: this argument could be called just versions.

"""Sorts the versions by priority. i.e: 1, 0.9, 0.8..."""
return sorted(
version_list,
key=lambda version: version['priority'],
reverse=True
)

if project.privacy_level == constants.PRIVATE:
raise Http404

Expand Down Expand Up @@ -431,7 +439,7 @@ def changefreqs_generator():
versions.append(element)

context = {
'versions': versions,
'versions': sort_by_priority(versions),
}
return render(
request,
Expand Down
33 changes: 32 additions & 1 deletion readthedocs/rtd_tests/tests/test_doc_serving.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,15 @@ def test_sitemap_xml(self):
project=self.public,
active=True
)
stable_version = fixture.get(
Version,
identifier='stable',
verbose_name='stable',
slug='stable',
privacy_level=constants.PUBLIC,
project=self.public,
active=True
)
# This also creates a Version `latest` Automatically for this project
translation = fixture.get(
Project,
Expand Down Expand Up @@ -269,7 +278,7 @@ def test_sitemap_xml(self):
),
)

# stable is marked as PRIVATE and should not appear here
# PRIVATE version should not appear here
self.assertNotContains(
response,
self.public.get_docs_url(
Expand All @@ -293,3 +302,25 @@ def test_sitemap_xml(self):
# hreflang should use hyphen instead of underscore
# in language and country value. (zh_CN should be zh-CN)
self.assertContains(response, 'zh-CN')

# Check if STABLE version has 'priority of 1 and changefreq of weekly.
self.assertEqual(
response.context['versions'][0]['loc'],
self.public.get_docs_url(
version_slug=stable_version.slug,
lang_slug=self.public.language,
private=False,
),)
self.assertEqual(response.context['versions'][0]['priority'], 1)
self.assertEqual(response.context['versions'][0]['changefreq'], 'weekly')

# Check if LATEST version has priority of 0.9 and changefreq of daily.
self.assertEqual(
response.context['versions'][1]['loc'],
self.public.get_docs_url(
version_slug='latest',
lang_slug=self.public.language,
private=False,
),)
self.assertEqual(response.context['versions'][1]['priority'], 0.9)
self.assertEqual(response.context['versions'][1]['changefreq'], 'daily')