sitemap a11y improvements#5312
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
77a482e to
ea56f0e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
447b61b to
d853bbc
Compare
|
Work Done:
Notes:
|
d853bbc to
043cba5
Compare
|
rebased onto r-v1.9.x |
043cba5 to
b087979
Compare
|
@StephDriver passed this back as tests are failing here. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
...
ooh, that's interesting, because I was running a formatter with a pre-commit hook - which won't let me commit until it passes, so that means my formatter is out of sync with the one testing here. I'll have to dig into it more on Monday as to what and why. |
|
I haven't worked out why my existing hook wasn't firing for that file, when it fired for others - the most likely explanation is that it the difference crept in during a rebase. But the two missing empty lines have now been duly added. |
|
Unpicking the test failures.
Failures that are on the base branch too
I cannot reproduce the test errors from github locally. They seem to come down to |
8522ecf to
2033ddc
Compare
# Conflicts: # src/templates/common/site_map_index.xml # src/themes/clean/templates/elements/press_footer.html
mauromsl
left a comment
mauromsl
left a comment
There was a problem hiding this comment.
Mostly comments regarding cleanup of migrations, unused code and inline import statements.
The more important comment is around the change to the lastmod attribute on article sitemaps. Do you agree that giving a false and potentially outdated lastmod date could be problematic?
Finally a note around performance: I've noted we run a large number of loops during title / date processing of issue items and preprints, including N+1 queries. For publications that run larger items (big repositories or conference proceedings) Would it be worth breaking these sitemaps into smaller ones? My concerns:
- Memory consumption: Since we have to hold all the model instances in memory in order to disambiguate dates/titles, memory requirements for some instances might be very high. Optmise by memoizing only the title and date strings we are worried about
- Compute time: Each of the disambiguation / processing done per sitemap, comes with its own loop. Instead build the dictionary of memoized strings on the caller and pass it to the children to only run a single loop and do checks in constant time.
- (Probably not an issue but worth mentioning) There is a 10MB file size limit and 50K link limit per sitemap on sitemaps.org recommendations.
I can help providing prod-like data to test these if you need them @StephDriver
| if self.is_draft and not self.preview_token: | ||
| self.preview_token = uuid4() | ||
| elif self.pk: | ||
| existing = Page.objects.get(pk=self.pk) | ||
| if self.is_draft and not existing.is_draft: | ||
| self.preview_token = uuid4() | ||
| super().save(*args, **kwargs) |
There was a problem hiding this comment.
Instead of doing these checks (which won't run when an object is created without calling .save()explicitely), is there a reason to not use an uuid fieldwith a default like the one provided in the docs as an example:
preview_token = models.UUIDField(default=uuid.uuid4, editable=False)If the reason is that we want to cycle them, I question why we need to do that instead of having a token for the lifetime of the page
There was a problem hiding this comment.
My take on this was that each time the page becomes a draft it needs a different token because the token grants anonymous access to that draft. A page could exist for years. Staff could change. Allowing old links (i.e old staff) to access that page during new drafts felt like it could be a problem and not what a user would expect. I realise 99.9999% of the time it likely wouldn't matter. But it seemed good practice to me to keep it unique per drafting process for a page, rather than unique per page.
| if not page.is_draft: | ||
| page.preview_token = str(uuid4()) |
There was a problem hiding this comment.
if we provide a token to all pages, we do not need to do this check whenever a page is toggled. Also, toggling the page as draft multiple times would reset the token, which I'm not sure we want. Finally, the same token generation logic lives inside the .save() call below.
| ) | ||
| repos = repo_models.Repository.objects.filter(live=True) | ||
| clash_names = _build_clash_names(press) | ||
| press_label = _suffixed_name(press.name, clash_names, "[press]") |
| is_remote=False, | ||
| ) | ||
| repos = repo_models.Repository.objects.filter(live=True) | ||
| clash_names = _build_clash_names(press) |
| clash_names = _local_clash_names(repo.name, press.name) | ||
| repo_label = _suffixed_name(repo.name, clash_names, "[repository]") | ||
| press_label = _suffixed_name(press.name, clash_names, "[press]") | ||
| child_sitemaps = [ |
There was a problem hiding this comment.
I don't think we are using these here either
5999aae to
91e014c
Compare
|
@mauromsl when testing these changes I found that the repo A11y page was falling back to the press page, but that's because we don't have repo data yet, and haven't built a repo version. So it shouldn't be there at all and the sitemaps shouldn't be listing it. So I've added another commit to remove that link from Repo footers and sitemaps. I also found the material repository had no footer link to the sitemap, so I added that too. |
3 participants
closes #5170