Drop stale 7-day Rails.cache + 5-min HTTP cache from analytics dashboard#23260
Draft
mikeydorje wants to merge 6 commits intoforem:mainfrom
Draft
Drop stale 7-day Rails.cache + 5-min HTTP cache from analytics dashboard#23260mikeydorje wants to merge 6 commits intoforem:mainfrom
mikeydorje wants to merge 6 commits intoforem:mainfrom
Conversation
The bundled /api/analytics/dashboard endpoint introduced in forem#23183 wrapped its payload in a 7-day Rails.cache.fetch and a 5-minute HTTP Cache-Control header. With the ArticleActivity fast-path now serving totals/historical from a single indexed row lookup, both layers do nothing useful and just hide fresh activity (page views, reactions, comments) from creators for days at a time. - Remove Rails.cache.fetch wrappers from AnalyticsController#dashboard and AnalyticsService#grouped_by_day. The ArticleActivity row IS the cache; worker writes need to be visible on the next request. - Replace 'expires_in 5.minutes, public: false' with 'Cache-Control: no-store' so the browser stops holding a 5-minute stale copy and a plain reload reflects new numbers immediately. - Update specs to match the new no-store header and to assert that the dashboard does NOT server-side memoize the payload. Rate-limiting is still handled by the existing Rack::Attack throttle on the analytics endpoints; the bundled request shape (one HTTP call instead of five parallel calls) means the cache was never load-bearing for that. Verified end-to-end against dev: created a comment via the real CommentCreator -> after_commit -> Comments::CalculateScore -> Articles::UpdateArticleActivityWorker chain, confirmed ArticleActivity.total_comments incremented, confirmed /api/analytics/dashboard returned the new total on the very next request with Cache-Control: no-store.
Contributor
|
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
The org tabs in dashboards/analytics.erb render as
`<a data-organization-id="...">` with no `organization` class, but
analyticsDashboard.js was scanning `getElementsByClassName('organization')`.
That selector always returned an empty NodeList, the org-detection
`else` branch never ran, and every analytics page (personal AND org)
booted with `organizationId: null` — so the org dashboards quietly
rendered the user's personal stats instead of the org's.
- Replace the broken class-based scan with
`document.querySelector('.analytics-nav a[aria-current="page"]')`,
which matches the actual nav markup. Read `dataset.organizationId`
off the active tab; the personal tab has no such attribute, so the
helper returns `null` for it.
- Export `getActiveOrganizationId` so it can be unit tested in
isolation without booting the chart pack.
- Add a regression spec covering the personal tab, the active org tab,
multi-org navs, and the empty-DOM defensive case.
Verified manually against dev: visiting /dashboard/analytics issues
GET /api/analytics/dashboard?start=... (no organization_id), and
visiting /dashboard/analytics/org/2 issues
GET /api/analytics/dashboard?start=...&organization_id=2.
When the analytics dashboard is scoped to a single article (article_id is present), the start-date floor should be the article's published_at, not the owner's registered_at / created_at. Previously, an article cross-posted into an organization that was created after the article was published had its pre-org activity silently chopped off the totals. Now the controller and AnalyticsService both use the article's published_at as the floor whenever article_id is set; non-article views are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bundled /api/analytics/dashboard endpoint introduced in #23183 wrapped its payload in a 7-day Rails.cache.fetch and a 5-minute HTTP Cache-Control header. With the ArticleActivity fast-path now serving totals/historical from a single indexed row lookup, both layers do nothing useful and just hide fresh activity (page views, reactions, comments) from creators for days at a time.
Rate-limiting is still handled by the existing Rack::Attack throttle on the analytics endpoints; the bundled request shape (one HTTP call instead of five parallel calls) means the cache was never load-bearing for that.
Verified end-to-end against dev: created a comment via the real CommentCreator -> after_commit -> Comments::CalculateScore -> Articles::UpdateArticleActivityWorker chain, confirmed ArticleActivity.total_comments incremented, confirmed /api/analytics/dashboard returned the new total on the very next request with Cache-Control: no-store.
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
UI accessibility checklist
If your PR includes UI changes, please utilize this checklist:
CriticalandSeriousissues?For more info, check out the
Forem Accessibility Docs.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?