Support of including blog in the website search #2136
Conversation
tim-schilling
force-pushed
the
searchable-blog
branch
from
83133b1 to
4f9e4bf
Compare
|
I was given tacit approval (lack of pushback) from the website WG to use this approach. I plan on working on this at the DjangoCon Africa sprints (Aug 14 & 15), but if anyone wants to complete it before that, please do! |
tim-schilling
force-pushed
the
searchable-blog
branch
from
4f9e4bf to
2bf75cd
Compare
|
I'm going to try to clean up the commits shortly here. |
The blog results should have a property of whether it is included in the search results. We should also limit the blogs that are searchable for a version of Django based on the support end. This will allow us to limit the inclusion of blog posts in the search based on the time the entry was created, keeping the search results relevant to that version of Django.
tim-schilling
force-pushed
the
searchable-blog
branch
from
a9a4f8a to
837e808
Compare
|
Cleaned up the commits. Please let me know if any other changes are necessary. |
docs/models.py
Outdated
| path = reverse_path(url_name, kwargs=kwargs) | ||
| request = RequestFactory().get(path, HTTP_HOST=www_host) | ||
| body = resolve(path).func(request).render().text | ||
| # Need to parse the body element. |
There was a problem hiding this comment.
Forgot this TODO yet.
There was a problem hiding this comment.
@bmispelon do you have any good ideas here?
There was a problem hiding this comment.
This is a fantastic PR, thanks for submitting it! It addresses a real issue that's been plaguing the site for a long time, and as far as PR go, it's a really high quality one, kudos 👏🏻
Of course because I'm me I'll always find something to complain about so I've left a few comments ranging from improvement suggestions to open questions.
I'll note that I have not tested the code locally (yet). I'm still working on getting the preview server up and running and I think this PR would be a great candidate for it.
docs/tests/test_models.py
Outdated
| cls.document_index, cls.document_view, cls.document_detail = ( | ||
| cls.release.documents.order_by("path") | ||
| ) |
There was a problem hiding this comment.
This will break if/when more views are added to _sync_views_to_db(), right?
I'd suggest breaking the single test_document_url() method into three separate tests then.
There was a problem hiding this comment.
Judging from the rest of the tests, it seems like there's an effort to keep them efficient. It may make more sense as two separate tests, one for the old document_url and then a new one for the DocumentationCategory.WEBSITE branch. Let me know if you disagree.
docs/models.py
Outdated
| ] | ||
| if not www_hosts or not (www_host := www_hosts[0]): | ||
| return | ||
| synced_views = [ |
There was a problem hiding this comment.
I think this should live in a constant at the top level of a module (I'll let you decide which module makes more sense 😁 )
There was a problem hiding this comment.
If we're moving this from the code away from the usage logic, then we should use a dataclass so it's not relying on positional arguments. That gets a bit more verbose and likely could go in an entirely separate module.
There was a problem hiding this comment.
May be side-stepped due to #2136 (comment)
docs/models.py
Outdated
| path = reverse_path(url_name, kwargs=kwargs) | ||
| request = RequestFactory().get(path, HTTP_HOST=www_host) | ||
| body = resolve(path).func(request).render().text |
There was a problem hiding this comment.
That's a lot of stuff going on in such a few lines: you make it look deceptively simple 😁
I'm curious: did you try other approaches before settling on this? I'm a bit worried that the whole www_host logic could be a bit brittle. If we're calling the view function directly anyway, can we skip setting the host on the request object?
There was a problem hiding this comment.
This is definitely for a reason, but let me try once more and better document the weirdness.
There was a problem hiding this comment.
Okay, so the simplest we can get it is to use get_template("aggregator/ecosystem.html").render(). However, that does sort of open us to the problem of the view (which is defined in the urls.py) of using a different template or implementation than expected. However, using this approach eliminates the need to get the host which eliminates having the search the ALLOWED_HOSTS setting. So I think it's worth it. Let me know what you think.
Co-authored-by: Baptiste Mispelon <[email protected]>
This does mean that when the change is released to production, the Entry table will be locked entirely until it completes.
There was a problem hiding this comment.
I like the move to a dataclass for SearchableView, nice one 👍🏻
I think we can go further and delegate the nitty-gritty details of getting a "document"'s URL and content to that class.
I've got grand ideas of generalizing this and having a collection of searchable/indexable models (sphinx document, blog, template view, flat page, ...) but that'd make the scope of this PR explode and I'd rather see this shipped. We can always iterate.
Other than that I left a question about the new support_end. I think we should be conservative with adding new fields and would like to explore reusing existing ones, to minimize the combinatorial explosion of possible model states. If we have to keep support_end, would it be possible for the existing DocumentRelease to get a sensible default assigned to them with a data migration (we have 100 of them in production, it'd be a shame if we had to go and back-populate them by hand).
Thanks again for your work on this!
docs/models.py
Outdated
| support_end = models.DateField( | ||
| null=True, | ||
| blank=True, | ||
| help_text="The end of support for this release of Django.", | ||
| ) |
There was a problem hiding this comment.
Do we need a dedicated field for this, why couldn't we get the information from release.eol_date instead?
There was a problem hiding this comment.
This is why we do code reviews. Let me get this switched over.
docs/models.py
Outdated
| absolute_url = reverse(searchable_view.url_name, host="www") | ||
| # This must match the template used for the url `community-ecosystem` | ||
| html = get_template("aggregator/ecosystem.html").render() | ||
| # Need to parse the body element. |
There was a problem hiding this comment.
Can we make absolute_url and html attributes (likely a property) of SearchableView? The name of the template should be an attribute of the SearchableView too and not be hardcoded here.
docs/search.py
Outdated
| TOPICS = "topics", _("Using Django") | ||
| HOWTO = "howto", _("How-to guides") | ||
| RELEASE_NOTES = "releases", _("Release notes") | ||
| WEBSITE = "weblog", _("Django Website") |
There was a problem hiding this comment.
Do the breadcrumbs work for you in the first place? I don't think I've ever used that feature and it doesn't seem to work correctly for me locally: even for documents with two or more parents, the links for all the parents are all the same and go to the document's own page.
Not sure I'd want to start adding redirects, that seems like solving the problem at the wrong level to me.
The blog results should have a property of whether it is included in the search results. We should also limit the blogs that are searchable for a version of Django based on the support end. This will allow us to limit the inclusion of blog posts in the search based on the time the entry was created, keeping the search results relevant to that version of Django.
This is a WIP yet. I'm opening the draft PR to solicit early feedback on the approach. I'm thinking we'll re-use theDocumentationCategory.WEBSITEin place of "weblog".