-
Notifications
You must be signed in to change notification settings - Fork 297
feat: unified view logging – track article views on post open #6380
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,7 @@ export function PostContentRaw({ | |
| hideSubscribeAction, | ||
| }; | ||
|
|
||
| useTrackPostView({ post, shouldTrack: isVideoType }); | ||
| useTrackPostView({ post }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one-liner is the actual behavior change of the PR: Now opening the page or modal counts, same as freeform/welcome/share/collection/poll/brief/digest already do. The click-through path still exists unchanged; the backend dedups the two within a one-week window per (user, post) — see dailydotdev/daily-api#4026. |
||
|
|
||
| const postMainColumn = ( | ||
| <PostContainer | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,13 +69,6 @@ const PostCodeSnippets = dynamic(() => | |
|
|
||
| export type FocusCardLeftVariant = 'lean' | 'rich'; | ||
|
|
||
| const viewTrackedPostTypes = [ | ||
| PostType.Share, | ||
| PostType.Collection, | ||
| PostType.Freeform, | ||
| PostType.Welcome, | ||
| ]; | ||
|
|
||
| interface PostFocusCardProps { | ||
| post: Post; | ||
| origin: PostOrigin; | ||
|
|
@@ -289,10 +282,7 @@ export const PostFocusCard = ({ | |
| const [isVideoExpanded, setIsVideoExpanded] = useState(false); | ||
| const readHref = getReadArticleHref(post); | ||
|
|
||
| useTrackPostView({ | ||
| post, | ||
| shouldTrack: isVideoPost(post) || viewTrackedPostTypes.includes(post.type), | ||
| }); | ||
| useTrackPostView({ post }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same change for the layout-v2 focus card, which duplicated the article exclusion via its own |
||
|
|
||
| useEffect(() => { | ||
| if (!isVideoType || isVideoExpanded) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,6 +278,17 @@ function renderPost( | |
| }, | ||
| result: () => ({ data: { _: true } }), | ||
| }, | ||
| // Opening any post (including articles) now logs a view on mount; tests | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default catch-all mock so the ~40 existing article-page tests don't each need to care about the new on-mount mutation; the new |
||
| // that don't assert on this explicitly still need it mocked so the | ||
| // mutation doesn't fire an unmatched request against nock. Registered | ||
| // last so a test-supplied `mocks` entry for the same request wins. | ||
| { | ||
| request: { | ||
| query: VIEW_POST_MUTATION, | ||
| variables: { id: defaultProps.id }, | ||
| }, | ||
| result: () => ({ data: { viewPost: { _: true } } }), | ||
| }, | ||
| ]; | ||
|
|
||
| defaultMocks.forEach(mockGraphQL); | ||
|
|
@@ -1035,6 +1046,39 @@ describe('article', () => { | |
| }), | ||
| ); | ||
| }); | ||
|
|
||
| // Unified view logging: opening an article now logs a view on mount too, | ||
| // matching every other post type, rather than only on "Read article" click. | ||
| it('should log post view on mount', async () => { | ||
| let viewPostMutationCalled = false; | ||
| renderPost({}, [ | ||
| createPostMock(), | ||
| createCommentsMock(), | ||
| { | ||
| request: { | ||
| query: VIEW_POST_MUTATION, | ||
| variables: { | ||
| id: '0e4005b2d3cf191f8c44c2718a457a1e', | ||
| }, | ||
| }, | ||
| result: () => { | ||
| viewPostMutationCalled = true; | ||
|
|
||
| return { | ||
| data: { | ||
| viewPost: { | ||
| _: true, | ||
| }, | ||
| }, | ||
| }; | ||
| }, | ||
| }, | ||
| ]); | ||
|
|
||
| await waitFor(() => { | ||
| expect(viewPostMutationCalled).toBe(true); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('post redesign', () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer note: this router-mock reset (and the one in the next test) fixes a pre-existing test-isolation gap rather than anything introduced here. The two preceding tests set a
pmidrouter query viamockImplementation, whichjest.clearAllMocks()doesn't reset, so these tests were silently rendering with a leftoverpmidand auto-opening the post modal all along. That was harmless while articles didn't track views; once they do, the modal fires an unmockedviewPostand nock fails the test — which is how the latent bleed-over surfaced.