Skip to content

expand pinned articles #6947

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

Jump to bottom
Merged
merged 4 commits into from
Oct 20, 2025
Merged

expand pinned articles #6947

merged 4 commits into from
Oct 20, 2025

Conversation

@escattone
Copy link
Contributor

@escattone escattone commented Oct 7, 2025 •
edited
Loading

mozilla/sumo#2562

Notes

  • This PR adds a new PinnedArticleConfig model that defines pinned articles for use by the home page, the product-landing pages, and the AAQ product-landing pages.
  • The pinned_articles field within the AAQConfig model has been removed, and replaced by a foreign key to a PinnedArticleConfig instance. I've added a data migration for any existing pinned articles associated with AAQConfig instances, and of course, it's run prior to the removal of the pinned_articles field.
  • A foreign-key to a PinnedArticleConfig instance has been added to the Product model to specify pinned articles for that product's landing page.
  • Pinned articles are defined via the Pinned Article Configurations under the WIKI section in the admin console.
  • Only parent articles can be pinned, but their translations will be provided if they exist for non-English locales.
  • This PR also adds a major improvement to the get_featured_articles function. That function now takes into account the viewing user, and can deliver restricted articles if the user has the permission to see them.

Screenshots

image image image image image

@escattone escattone force-pushed the pinned-articles-2562 branch 4 times, most recently from 8ce3f18 to e68ed07 Compare October 8, 2025 22:03
@escattone escattone marked this pull request as ready for review October 8, 2025 22:13
@escattone escattone force-pushed the pinned-articles-2562 branch from e68ed07 to 6fddf8e Compare October 8, 2025 22:24
@escattone escattone requested a review from akatsoulas October 8, 2025 23:26
akatsoulas
akatsoulas reviewed Oct 9, 2025
View reviewed changes
Copy link
Collaborator

@akatsoulas akatsoulas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apart from the comment about the user, this looks good!

My main concern here is that we are going to have two distinct systems to handle featured articles which will make maintainability and readability a complicated task and it's prone to future bugs.

How about unifying the two systems? We could replace the pinned_articles in the AAQConfig with a FK to the PinnedArticlesConfig. That would also require to relax the DB constraints ofc on the config and maybe a small DB migration but the lift feels low compared to the benefits. Thoughts?

akatsoulas
akatsoulas reviewed Oct 9, 2025
View reviewed changes
@escattone
Copy link
Contributor Author

escattone commented Oct 9, 2025 •
edited
Loading

How about unifying the two systems?

Great! I would prefer that as well! I'll do that.

@escattone escattone force-pushed the pinned-articles-2562 branch 5 times, most recently from 38e4bc0 to 47f0bd5 Compare October 11, 2025 20:18
@escattone escattone requested a review from akatsoulas October 11, 2025 20:26
akatsoulas
akatsoulas reviewed Oct 13, 2025
View reviewed changes
Copy link
Collaborator

@akatsoulas akatsoulas left a comment

Choose a reason for hiding this comment

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

Overall this looks good. My concern with this approach is that we are forcing the same pinned articles for the product landing page and the AAQ. Adding the pinned article config as a FK to the AAQConfig will allow for greater flexibility by having a different configuration file per page. Thoughts?

@escattone
Copy link
Contributor Author

escattone commented Oct 13, 2025

My concern with this approach is that we are forcing the same pinned articles for the product landing page and the AAQ. Adding the pinned article config as a FK to the AAQConfig will allow for greater flexibility by having a different configuration file per page. Thoughts?

I don't see when we'd ever want different pinned articles on a product landing page versus its AAQ product page, so it feels like we're adding something that we might not ever use? I can add it though.

@akatsoulas
Copy link
Collaborator

akatsoulas commented Oct 14, 2025

My concern with this approach is that we are forcing the same pinned articles for the product landing page and the AAQ. Adding the pinned article config as a FK to the AAQConfig will allow for greater flexibility by having a different configuration file per page. Thoughts?

I don't see when we'd ever want different pinned articles on a product landing page versus its AAQ product page, so it feels like we're adding something that we might not ever use? I can add it though.

The AAQ has a subset of topics compared to the product so I would assume that in most cases the AAQ will need to differentiate when we have the need to divert from the default behavior and have pinned articles

@escattone escattone force-pushed the pinned-articles-2562 branch 2 times, most recently from 27c43a6 to 9e11c8b Compare October 15, 2025 21:06
@escattone escattone requested a review from akatsoulas October 15, 2025 21:09
@escattone escattone force-pushed the pinned-articles-2562 branch from 9e11c8b to 4323eb6 Compare October 15, 2025 23:50
akatsoulas
akatsoulas reviewed Oct 16, 2025
View reviewed changes
Copy link
Collaborator

@akatsoulas akatsoulas left a comment

Choose a reason for hiding this comment

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

Overall this looks good. My main comment is about migrating existing data and moving the FK to the AAQConfig instead.

@escattone escattone force-pushed the pinned-articles-2562 branch 6 times, most recently from 84908c5 to eca91a6 Compare October 17, 2025 22:44
@escattone escattone force-pushed the pinned-articles-2562 branch from eca91a6 to 91f5122 Compare October 17, 2025 23:10
@escattone escattone force-pushed the pinned-articles-2562 branch from 91f5122 to 797bfc7 Compare October 17, 2025 23:12
@escattone escattone requested a review from akatsoulas October 17, 2025 23:45
akatsoulas
akatsoulas approved these changes Oct 20, 2025
View reviewed changes
Copy link
Collaborator

@akatsoulas akatsoulas left a comment

Choose a reason for hiding this comment

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

looks good 🚀 r+wc

@escattone escattone merged commit 3d20c30 into mozilla:main Oct 20, 2025
1 check passed
@escattone escattone deleted the pinned-articles-2562 branch October 20, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akatsoulas akatsoulas akatsoulas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@escattone @akatsoulas