Topic cards revised to show three articles #6471
Conversation
…a#6408) * Improved topic card now show three top articles * Alter the help_topics macro to support showing three articles * Add show more section with sectional line above * Add logic to the product_landing view to return docs and counts * A utility function builds the topics_context for the cards * Fix card scroll for topic cards on mobile * Topics can only wrap once before being cut off with ellipses * Use query annoation in `utils.py` to gather topics * Use `chain` and `islice` to ensure proper doc order and count
|
Nice work so far! |
* Improved db impact of `get_featured_articles` by handling filters in memory. * Corrected various other requested items
Erroneous passing of topics from aaq step 2 removed
kitsune/wiki/utils.py
Outdated
| visits = visits.filter(document__products__in=[product.id]) | ||
| visits = visits.filter(document__products=product) | ||
| if topics: | ||
| visits = visits.filter(document__topics__in=topics).distinct() |
There was a problem hiding this comment.
Why is distinct needed here? I assume that we do want the same article to appear in different topics if it's the most visited
There was a problem hiding this comment.
My thought was that the document we need is singular, but the results we'd get here would have duplicates because a document can have multiple topics. The document itself, once retrieved, will still have it's connection to the topics - so nothing is lost, but we have a smaller result to deal with.
I guess technically it could be dropped if you know a good reason - is distinct() expensive for the lack of value? Or am I wrong that document visits don't change regardless of topics/products?
There was a problem hiding this comment.
We are querying WikiDocumentVisits and not Document. This means that each visit record related to one document b/c of the FK. I believe we can drop this without having any duplicates or affecting the query in any way
kitsune/wiki/utils.py
Outdated
| visits = visits.order_by("-visits") | ||
|
|
||
| # Convert to list to avoid additional queries | ||
| visits = list(visits) |
There was a problem hiding this comment.
Why is this needed? How do we avoid additional queries by forcing the evaluation here?
There was a problem hiding this comment.
That's an artifact - you are right. Since no further evaluations are happening there is no need for this.
kitsune/wiki/utils.py
Outdated
| if len(documents) <= 4: | ||
| return documents | ||
| return random.sample(documents, 4) | ||
| # Return all documents - the filtering per topic will happen in build_topics_data |
kitsune/wiki/utils.py
Outdated
| .annotate(topic_ids=StringAgg(Cast("topics__id", TextField()), delimiter=",", default="")) | ||
| ) | ||
|
|
||
| # Get featured articles for all topics in one query |
kitsune/wiki/utils.py
Outdated
| return documents | ||
| return random.sample(documents, 4) | ||
| # Return all documents - the filtering per topic will happen in build_topics_data | ||
| return documents |
There was a problem hiding this comment.
this no longer returns 4 articles like it used to. It will probably work for the topic cards but it will break functionality elsewhere
There was a problem hiding this comment.
You are right - this will break the homepage. I've reverted.
kitsune/wiki/utils.py
Outdated
| visits = visits.filter(document__products__in=[product.id]) | ||
| visits = visits.filter(document__products=product) | ||
| if topics: | ||
| visits = visits.filter(document__topics__in=topics).distinct() |
There was a problem hiding this comment.
We are querying WikiDocumentVisits and not Document. This means that each visit record related to one document b/c of the FK. I believe we can drop this without having any duplicates or affecting the query in any way
Remove list conversion of visits Ensure get_featured_articles only returns 4 or less articles for backwards compatibility.
This reverts commit 01b2e16.
* Revert "Remove duplicate docs per topic" This reverts commit e625a79. * Revert "Do not show duplicate docs" This reverts commit f2678dc. * Revert "pass template context to help_topics (#6492)" This reverts commit f62ada8. * Revert "use request locale in topic card links (#6491)" This reverts commit f2d651c. * Revert "Expose fallback documents in topic cards." This reverts commit 14dcc4a. * Revert "Do not clip article counter (#6488)" This reverts commit 2ddb33c. * Revert "Fix queries for non en-US locales (#6487)" This reverts commit 0d52f40. * Revert "Update padding in topic cards" This reverts commit fae102f. * Revert "Update wording in macro." This reverts commit 676e36e. * Revert "Topic cards revised to show three articles (#6471)" This reverts commit 01b2e16.
utils.pyto gather topicschainandisliceto ensure proper doc order and countThis creates a new card for topics that contains three article titles. The articles are the three most frequently viewed for each topic and of that product, falling back three regular documents if we don't get enough documents from frequently viewed data.
The article titles are allowed to wrap once if they are lengthy, but then are cut off with an ellipse if they exceed two lines.
The cards are in three columns, using a flex layout.
The cards scroll left to right on mobile, where they have fixed heights and widths to keep the experience optimal.