From 3f7d1fbc1a42395c9095ba7bb36c8076ccee19a3 Mon Sep 17 00:00:00 2001 From: PJ Date: Mon, 11 Sep 2023 13:32:36 +0100 Subject: [PATCH] Create feed impression and click events (#20043) * getting started: model setup * basic controller action & spec * somewhat working click events * pick up on impressions * there was an attempt at batching without duplicates * (better) bulk upsert service * fix specs? * touch up feed events JS * end-to-end specs?? * workers wip * fix failing user delete spec --- .../javascripts/initializers/initScrolling.js | 3 + .../javascripts/utilities/buildArticleHTML.js | 6 + app/controllers/feed_events_controller.rb | 26 +++ app/javascript/articles/Article.jsx | 4 +- .../__snapshots__/Article.test.jsx.snap | 2 + app/javascript/packs/feedEvents.js | 181 ++++++++++++++++++ app/javascript/packs/homePage.jsx | 3 + app/javascript/packs/homePageFeed.jsx | 6 +- app/models/article.rb | 1 + app/models/feed_event.rb | 31 +++ app/models/user.rb | 1 + app/services/feed_events/bulk_upsert.rb | 119 ++++++++++++ app/views/articles/_single_story.html.erb | 2 +- app/views/articles/index.html.erb | 6 +- config/routes.rb | 1 + .../seededFlows/homeFeedFlows/events.spec.js | 126 ++++++++++++ cypress/support/commands.js | 20 ++ .../20230814175212_create_feed_events.rb | 15 ++ ...at_and_composite_indices_to_feed_events.rb | 12 ++ ...0831105357_add_feed_counters_to_article.rb | 8 + db/schema.rb | 19 ++ spec/factories/feed_events.rb | 9 + spec/models/article_spec.rb | 2 + spec/models/feed_event_spec.rb | 14 ++ spec/models/user_spec.rb | 1 + spec/requests/feed_events_spec.rb | 81 ++++++++ spec/services/feed_events/bulk_upsert_spec.rb | 162 ++++++++++++++++ spec/services/users/delete_spec.rb | 3 +- 28 files changed, 857 insertions(+), 7 deletions(-) create mode 100644 app/controllers/feed_events_controller.rb create mode 100644 app/javascript/packs/feedEvents.js create mode 100644 app/models/feed_event.rb create mode 100644 app/services/feed_events/bulk_upsert.rb create mode 100644 cypress/e2e/seededFlows/homeFeedFlows/events.spec.js create mode 100644 db/migrate/20230814175212_create_feed_events.rb create mode 100644 db/migrate/20230818083219_add_created_at_and_composite_indices_to_feed_events.rb create mode 100644 db/migrate/20230831105357_add_feed_counters_to_article.rb create mode 100644 spec/factories/feed_events.rb create mode 100644 spec/models/feed_event_spec.rb create mode 100644 spec/requests/feed_events_spec.rb create mode 100644 spec/services/feed_events/bulk_upsert_spec.rb diff --git a/app/assets/javascripts/initializers/initScrolling.js b/app/assets/javascripts/initializers/initScrolling.js index a186514de..e053a4ca7 100644 --- a/app/assets/javascripts/initializers/initScrolling.js +++ b/app/assets/javascripts/initializers/initScrolling.js @@ -369,6 +369,9 @@ function insertArticles(articles) { ); var lastElement = singleArticles[singleArticles.length - 1]; insertAfter(newNode, lastElement); + if (window.observeFeedElements && articles.length > 0) { + window.observeFeedElements(); + } if (nextPage > 0) { fetching = false; } diff --git a/app/assets/javascripts/utilities/buildArticleHTML.js b/app/assets/javascripts/utilities/buildArticleHTML.js index e96cecfcb..51365d92d 100644 --- a/app/assets/javascripts/utilities/buildArticleHTML.js +++ b/app/assets/javascripts/utilities/buildArticleHTML.js @@ -379,9 +379,15 @@ function buildArticleHTML(article, currentUserId = null) { `; + let feedContentAttribute = ''; + if (article.class_name === 'Article') { + feedContentAttribute = `data-feed-content-id="${article.id}"`; + } + return `
\ ${navigationLink}\
\ diff --git a/app/controllers/feed_events_controller.rb b/app/controllers/feed_events_controller.rb new file mode 100644 index 000000000..192b98497 --- /dev/null +++ b/app/controllers/feed_events_controller.rb @@ -0,0 +1,26 @@ +class FeedEventsController < ApplicationMetalController + include ActionController::Head + + FEED_EVENT_ALLOWED_PARAMS = %i[ + article_id + article_position + category + context_type + ].freeze + + def create + if session_current_user_id + FeedEvents::BulkUpsert.call(feed_events_params) + end + + head :ok + end + + private + + def feed_events_params + @feed_events_params ||= params[:feed_events].map do |event| + event.slice(*FEED_EVENT_ALLOWED_PARAMS).merge(user_id: session_current_user_id) + end + end +end diff --git a/app/javascript/articles/Article.jsx b/app/javascript/articles/Article.jsx index 471cd2d72..e2b0e9f25 100644 --- a/app/javascript/articles/Article.jsx +++ b/app/javascript/articles/Article.jsx @@ -29,6 +29,7 @@ export const Article = ({ return ; } + const isArticle = article.class_name === 'Article'; const clickableClassList = [ 'crayons-story', 'crayons-story__top', @@ -53,6 +54,7 @@ export const Article = ({ isFeatured ? ' crayons-story--featured' : '' }`} id={isFeatured ? 'featured-story-marker' : `article-${article.id}`} + data-feed-content-id={isArticle ? article.id : null} data-content-user-id={article.user_id} > - {article.class_name === 'Article' && ( + {isArticle && ( // eslint-disable-next-line no-underscore-dangle )} diff --git a/app/javascript/articles/__tests__/__snapshots__/Article.test.jsx.snap b/app/javascript/articles/__tests__/__snapshots__/Article.test.jsx.snap index 52bbe3c76..2d5b4b2b0 100644 --- a/app/javascript/articles/__tests__/__snapshots__/Article.test.jsx.snap +++ b/app/javascript/articles/__tests__/__snapshots__/Article.test.jsx.snap @@ -54,6 +54,7 @@ Object {
{ + if (element.classList.contains('paged-stories')) { + // This was inserted by `initScrolling, and will contain feed items within. + findAndTrackFeedItems(element); + } else if (element.dataset?.feedContentId) { + element.dataset.feedPosition = tracker.nextFeedPosition; + element.addEventListener('click', trackFeedClickListener, true); + tracker.observer.observe(element); + + tracker.nextFeedPosition += 1; + } + }); +} + +/** + * Attempts to send any pending queued events before state is lost - e.g. when + * navigating to a different page, or (on mobile) switching to a different app + * (which can eventually cause the browser tab to be discarded in the background + * at the operating system's discretion). + * Both the unload event and the page visibility API are used as each one covers + * some gaps that the other does not. + */ +function ensureQueueIsClearedBeforeExit() { + document.addEventListener('visibilitychange', () => { + if (document.visibilityState == 'hidden') submitEventsBatch(); + }); + window.addEventListener('beforeunload', submitEventsBatch); +} + +/** + * Collects feed impressions, counted as at least a quarter of the article card + * coming into view. This is typically enough to at least see the title and/or + * a significant portion of the cover image. + * @param {IntersectionObserverEntry[]} entries + */ +function trackFeedImpressions(entries) { + entries.forEach((entry) => { + // At least a quarter of the card is in view; not quite enough to read the + // title for many articles, but it'll do + if (entry.isIntersecting && entry.intersectionRatio >= VISIBLE_THRESHOLD) { + queueMicrotask(() => { + const post = entry.target; + if (!post.dataset.impressionRecorded) { + queueEvent(post, tracker.categoryImpression); + post.dataset.impressionRecorded = true; + } + }); + } + }); +} + +/** + * Sends click events to the server immediately along with any currently-batched + * events. + * These may not necessarily be clicks that open the article (e.g. the user may + * have clicked on the author's profile image). + * TODO: Only track link-opening clicks instead? + * @param {MouseEvent} event + */ +function trackFeedClickListener(event) { + const post = event.currentTarget; + + if (!post.dataset.clickRecorded) { + queueEvent(post, tracker.categoryClick); + post.dataset.clickRecorded = true; + submitEventsBatch(); + } +} + +function queueEvent(post, category) { + const { feedContentId, feedPosition } = post.dataset; + + tracker.queue.push({ + article_id: feedContentId, + article_position: feedPosition, + category, + context_type: tracker.contextType, + }); + + if (tracker.queue.length >= MAX_BATCH_SIZE) { + submitEventsBatch(); + } +} + +/** + * Sends a batch of feed events to the server. + * Note: requests made with `navigator.sendBeacon` have greater guarantees to + * actually complete than regular fetch requests with the `keepalive` property + * set. However, the former is a bit tedious to implement and is often + * inadvertently blocked by users (e.g. via extensions like uBlock). So a fallback + * to `fetch` is included to cover that. + */ +function submitEventsBatch() { + if (tracker.queue.length === 0) return; + + const tokenMeta = document.querySelector("meta[name='csrf-token']"); + const authenticity_token = tokenMeta?.getAttribute('content'); + + if (tracker.beaconEnabled) { + // The Beacon API doesn't actually let you set headers, so we set the content + // type and CSRF token within the body itself (the browser will recognise the + // former, and Rails will recognise the latter) + const data = new Blob( + [JSON.stringify({ authenticity_token, feed_events: tracker.queue })], + { type: 'application/json' }, + ); + // `sendBeacon` returns true if sending a beacon worked, and false otherwise. + tracker.beaconEnabled = navigator.sendBeacon('/feed_events', data); + } else { + fallbackRequest(authenticity_token); + } + + tracker.queue = []; +} + +function fallbackRequest(authenticity_token) { + window + .fetch('/feed_events', { + method: 'POST', + headers: { + 'X-CSRF-Token': authenticity_token, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ feed_events: tracker.queue }), + credentials: 'same-origin', + keepalive: true, + }) + .catch((error) => console.error(error)); +} diff --git a/app/javascript/packs/homePage.jsx b/app/javascript/packs/homePage.jsx index dfc01d774..7ea4ce50d 100644 --- a/app/javascript/packs/homePage.jsx +++ b/app/javascript/packs/homePage.jsx @@ -5,6 +5,7 @@ import { observeBillboards, initializeBillboardVisibility, } from '../packs/billboardAfterRenderActions'; +import { observeFeedElements } from '../packs/feedEvents'; import { setupBillboardDropdown } from '@utilities/billboardDropdown'; import { trackCreateAccountClicks } from '@utilities/ahoy/trackEvents'; @@ -101,6 +102,7 @@ if (!document.getElementById('featured-story-marker')) { initializeBillboardVisibility(); observeBillboards(); setupBillboardDropdown(); + observeFeedElements(); }; renderFeed(feedTimeFrame, callback); @@ -123,6 +125,7 @@ if (!document.getElementById('featured-story-marker')) { initializeBillboardVisibility(); observeBillboards(); setupBillboardDropdown(); + observeFeedElements(); }; renderFeed(changedFeedTimeFrame, callback); diff --git a/app/javascript/packs/homePageFeed.jsx b/app/javascript/packs/homePageFeed.jsx index d15a8dda9..3b225a08b 100644 --- a/app/javascript/packs/homePageFeed.jsx +++ b/app/javascript/packs/homePageFeed.jsx @@ -1,4 +1,4 @@ -import { h, render } from 'preact'; +import { h, Fragment, render } from 'preact'; import PropTypes from 'prop-types'; import { Article, LoadingArticle } from '../articles'; import { Feed } from '../articles/Feed'; @@ -150,7 +150,7 @@ export const renderFeed = async (timeFrame, afterRender) => { } return ( -
+ {feedConstruct( pinnedItem, imageItem, @@ -159,7 +159,7 @@ export const renderFeed = async (timeFrame, afterRender) => { bookmarkClick, currentUserId, )} -
+ ); }; diff --git a/app/models/article.rb b/app/models/article.rb index f3c0e8a30..3bbaa9086 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -125,6 +125,7 @@ class Article < ApplicationRecord has_many :context_notifications, as: :context, inverse_of: :context, dependent: :delete_all has_many :context_notifications_published, -> { where(context_notifications_published: { action: "Published" }) }, as: :context, inverse_of: :context, class_name: "ContextNotification" + has_many :feed_events, dependent: :delete_all has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :page_views, dependent: :delete_all diff --git a/app/models/feed_event.rb b/app/models/feed_event.rb new file mode 100644 index 000000000..d95e647c9 --- /dev/null +++ b/app/models/feed_event.rb @@ -0,0 +1,31 @@ +class FeedEvent < ApplicationRecord + # These are "optional" mostly so that we can perform validated bulk inserts + # without triggering article/user validation. + # Since there are database-level constraints, it's fine to skip the automatic + # Rails-side association validation (which causes an N+1 query). + belongs_to :article, optional: true + belongs_to :user, optional: true + + enum category: { + impression: 0, + click: 1, + reaction: 2, + comment: 3 + } + + CONTEXT_TYPE_HOME = "home".freeze + CONTEXT_TYPE_SEARCH = "search".freeze + CONTEXT_TYPE_TAG = "tag".freeze + VALID_CONTEXT_TYPES = [ + CONTEXT_TYPE_HOME, + CONTEXT_TYPE_SEARCH, + CONTEXT_TYPE_TAG, + ].freeze + DEFAULT_TIMEBOX = 5.minutes.freeze + + validates :article_position, numericality: { only_integer: true, greater_than: 0 } + validates :context_type, inclusion: { in: VALID_CONTEXT_TYPES }, presence: true + # Since we have disabled association validation, this is handy to filter basic bad data + validates :article_id, presence: true, numericality: { only_integer: true } + validates :user_id, numericality: { only_integer: true }, allow_nil: true +end diff --git a/app/models/user.rb b/app/models/user.rb index 4d77fd5cb..8e368cbda 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -73,6 +73,7 @@ class User < ApplicationRecord has_many :billboard_events, dependent: :nullify has_many :email_authorizations, dependent: :delete_all has_many :email_messages, class_name: "Ahoy::Message", dependent: :destroy + has_many :feed_events, dependent: :nullify has_many :field_test_memberships, class_name: "FieldTest::Membership", as: :participant, dependent: :destroy # Consider that we might be able to use dependent: :delete_all as the GithubRepo busts the user cache has_many :github_repos, dependent: :destroy diff --git a/app/services/feed_events/bulk_upsert.rb b/app/services/feed_events/bulk_upsert.rb new file mode 100644 index 000000000..dd9eaf07b --- /dev/null +++ b/app/services/feed_events/bulk_upsert.rb @@ -0,0 +1,119 @@ +module FeedEvents + # Inserts a collection of feed events into the database. + # + # If there are duplicate events in the collection (i.e. having the same user, + # article, and category) only one will be inserted. + # + # If a timebox is provided and there is a duplicate event (using the criteria + # for uniqueness above) that was created within it, any matching new event will + # not be created. + # + # This avoids inflating metrics (whether by accident or deliberately). + class BulkUpsert + ATTRIBUTES_FOR_INSERT = %i[article_id user_id article_position category context_type].freeze + + def self.call(...) + new(...).call + end + + # @param feed_events_data [Array] A list of feed events attributes to upsert + # @param timebox [ActiveSupport::Duration] A time window (in minutes) within which feed events must be unique + def initialize(feed_events_data, timebox: FeedEvent::DEFAULT_TIMEBOX) + @feed_events_data = feed_events_data + @timebox = timebox + end + + def call + return if valid_events.blank? + + if valid_events.size == 1 + create_single_event! + return + end + + # It's *possible* to construct a single SQL query that does what we want + # here, (i.e. find existing records, filter out duplicates, and insert the + # resulting set) but it is more complicated than it sounds. + # For instance, using Postgres' upserting feature (`ON CONFLICT DO...`) is + # not an option because we *don't* actually want the records to be unique + # in general, just within the provided timebox, so there is no constraint + # to trigger a conflict. + # Instead we take a Rails-ish `find_or_create_by` approach: first try to + # find matching record(s), then figure out which new record(s) to insert + # application-side. Making two queries does potentially allow duplicates + # (through race conditions), but that's an acceptable margin of error and + # would be the case anyway with a single query (without locking the table) + if timebox.present? + find_existing_events_within_timebox do |event| + track_recent_event(event) + end + end + + records_to_insert = valid_events.filter_map do |event| + unless recent_event?(event) + track_recent_event(event) + ATTRIBUTES_FOR_INSERT.index_with { |attr| event[attr] } + end + end + + FeedEvent.insert_all(records_to_insert) if records_to_insert.present? + end + + private + + attr_reader :feed_events_data, :timebox + + def valid_events + @valid_events ||= feed_events_data.filter_map do |event_data| + event = FeedEvent.new(event_data) + event if event.valid? + rescue ArgumentError + # Enums raise ArgumentError if assigned with invalid value + end + end + + def create_single_event! + event = valid_events.first + FeedEvent + .where.not("created_at > ?", timebox.ago.utc) # Only proceed if + .create_with(event.slice(:article_position, :context_type)) + .find_or_create_by(event.slice(:article_id, :user_id, :category)) + end + + def recent_events + @recent_events ||= Hash.new do |users, user_id| + users[user_id] = Hash.new do |categories, category| + categories[category] = {} # articles + end + end + end + + def recent_event?(event) + recent_events[event.user_id][event.category][event.article_id] + end + + def track_recent_event(event) + recent_events[event.user_id][event.category][event.article_id] = true + end + + def find_existing_events_within_timebox(&block) + values = valid_events.reduce([]) do |acc, event| + acc << event.article_id + # Postgres is...iffy about comparing NULL (NULL !== NULL), and chokes if it is present in a tuple comparison. + # Coalescing to 0 is fine because primary keys auto-increment from 1. + acc << (event.user_id || 0) + acc << FeedEvent.categories[event[:category]] + end + + values_clause = Array.new(valid_events.length) { "(?, ?, ?)" }.join(", ") + + FeedEvent + .where("created_at > ?", timebox.ago.utc) + .where( + "(article_id, COALESCE(user_id, 0), category) IN (#{values_clause})", + *values, + ) + .each(&block) + end + end +end diff --git a/app/views/articles/_single_story.html.erb b/app/views/articles/_single_story.html.erb index 3d7c2b9f4..ec14d17d7 100644 --- a/app/views/articles/_single_story.html.erb +++ b/app/views/articles/_single_story.html.erb @@ -1,4 +1,4 @@ -
+
<%= story.title %> <% if featured == true %>
diff --git a/app/views/articles/index.html.erb b/app/views/articles/index.html.erb index a033fa553..2a42c60f6 100644 --- a/app/views/articles/index.html.erb +++ b/app/views/articles/index.html.erb @@ -33,6 +33,10 @@ data-params="<%= params.merge(sort_by: "hotness_score", sort_direction: "desc").to_json(only: %i[tag username q sort_by sort_direction]) %>" data-which="<%= @list_of %>" data-tag="" data-feed="<%= params[:timeframe] || "base-feed" %>" + data-feed-context-type="<%= FeedEvent::CONTEXT_TYPE_HOME %>" + <% FeedEvent.categories.keys.each do |category| %> + data-feed-category-<%= category %>="<%= category %>" + <% end %> data-articles-since="<%= Timeframe.datetime_iso8601(params[:timeframe]) %>"> <%= render "articles/sidebar" %> @@ -104,5 +108,5 @@ <%= render "articles/sidebar_additional" %>
- <%= javascript_packs_with_chunks_tag "storiesList", "followButtons", "feedPreviewCards", "heroBannerClose", "localizeArticleDates", defer: true %> + <%= javascript_packs_with_chunks_tag "storiesList", "followButtons", "feedPreviewCards", "heroBannerClose", "localizeArticleDates", "feedEvents", defer: true %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 7c1a671b6..39f4aeda2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -142,6 +142,7 @@ Rails.application.routes.draw do resources :tag_adjustments, only: %i[create destroy] resources :rating_votes, only: [:create] resources :page_views, only: %i[create update] + resources :feed_events, only: %i[create] resources :credits, only: %i[index new create] do get "purchase", on: :collection, to: "credits#new" end diff --git a/cypress/e2e/seededFlows/homeFeedFlows/events.spec.js b/cypress/e2e/seededFlows/homeFeedFlows/events.spec.js new file mode 100644 index 000000000..1aff243e7 --- /dev/null +++ b/cypress/e2e/seededFlows/homeFeedFlows/events.spec.js @@ -0,0 +1,126 @@ +describe('Home page feed events', () => { + beforeEach(() => { + cy.testSetup(); + }); + + const scrollBackAndForth = () => { + // The delay is necessary for Cypress to give `IntersectionObserver` time to actually work. + // Also, we scroll twice so we can confirm we don't send extraneous events. + cy.scrollTo('bottom', { duration: 700 }); + cy.scrollTo('top', { duration: 700 }); + }; + + context('when a user is not logged in', () => { + beforeEach(() => { + const spy = cy.spy().as('feedEventsSpy'); + cy.intercept('/feed_events', spy); + cy.clock(new Date(), ['setInterval']); + + cy.visit('/'); + cy.tick(500); // Allow regular page load timers to run + }); + + it('does not send feed events', () => { + scrollBackAndForth(); + // Again for good measure + scrollBackAndForth(); + + // More than autosubmit threshold + cy.tick(7000); + cy.get('@feedEventsSpy').should('not.have.been.called'); + }); + }); + + context('when a user is logged in', () => { + beforeEach(() => { + cy.fixture('users/articleEditorV1User.json').as('user'); + cy.intercept('/feed_events').as('feedEventsSubmission'); + cy.get('@user').then((user) => cy.loginUser(user)); + }); + + const visitAndWaitForLoad = (url, { expectedPostCount }) => { + cy.visit(url); + cy.get('[data-feed-position]').should('have.length', expectedPostCount); + }; + + const findAndDetermineImpressions = (callback) => { + cy.get('[data-feed-content-id]').then((posts) => { + const ids = posts + .map(function () { + return this.dataset.feedContentId; + }) + .get(); + + const expectedEvents = ids.map((id, index) => ({ + article_id: `${id}`, + article_position: `${index + 1}`, + category: 'impression', + context_type: 'home', + })); + callback(expectedEvents); + }); + }; + + it('submits click events immediately', () => { + visitAndWaitForLoad('/', { expectedPostCount: 11 }); + + // The first/featured article is `test-article-slug` + cy.get('#featured-story-marker').within((post) => { + const id = post.data('feedContentId'); + + cy.findByRole('heading').click(); + // Less than auto-submit threshold of 5 seconds. + cy.wait('@feedEventsSubmission', { timeout: 1000 }) + .its('request.body.feed_events') + .should('deep.contain', { + // For some reason Cypress leaves the integers in the request body as strings. + article_id: `${id}`, + article_position: '1', + category: 'click', + context_type: 'home', + }); + }); + }); + + it('auto-submits incomplete batches of events after a few seconds', () => { + visitAndWaitForLoad('/', { expectedPostCount: 11 }); + + // There are 11 articles currently in the seeder! If this line is failing + // and a seeded article has recently been added or removed, just bump + // this number. + findAndDetermineImpressions((events) => { + scrollBackAndForth(); + + cy.wait('@feedEventsSubmission') + .its('request.body.feed_events') + .should('have.deep.members', events); + }); + }); + + it('submits impression events in batches of 20', () => { + const createArticles = [...Array(15)].map((_, index) => + cy.createArticle({ + title: `Feed post ${index}`, + content: `Content for feed post ${index}`, + }), + ); + cy.all(createArticles).then(() => { + visitAndWaitForLoad('/', { expectedPostCount: 26 }); + + findAndDetermineImpressions((events) => { + const firstBatch = events.slice(0, 20); + const secondBatch = events.slice(20); + scrollBackAndForth(); + + cy.wait('@feedEventsSubmission') + .its('request.body.feed_events') + .should('have.deep.members', firstBatch); + + cy.wait('@feedEventsSubmission') + .its('request.body.feed_events') + .should('have.deep.members', secondBatch); + }); + }); + }); + }); +}); diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 1ba99d259..4fbd28487 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -26,6 +26,26 @@ import { getInterceptsForLingeringUserRequests } from '../util/networkUtils'; // -- This will overwrite an existing command -- // Cypress.Commands.overwrite("visit", (originalFn, url, options) => { ... }) +/** + * Use this function to wait for multiple async/then-able commands to complete. + * Useful for making multiple requests. + */ +Cypress.Commands.add( + 'all', + /** + * @param {Cypress.Chainable[]} commands Functions that return + * a thenable + */ + (commands) => + commands.reduce( + (allResults, command) => + allResults.then((results) => + command.then((result) => [...results, result]), + ), + cy.wrap([]), + ), +); + /** * Use this function to sign a user out without lingering network calls causing unintended side-effects. */ diff --git a/db/migrate/20230814175212_create_feed_events.rb b/db/migrate/20230814175212_create_feed_events.rb new file mode 100644 index 000000000..c9fbe1776 --- /dev/null +++ b/db/migrate/20230814175212_create_feed_events.rb @@ -0,0 +1,15 @@ +class CreateFeedEvents < ActiveRecord::Migration[7.0] + def change + create_table :feed_events do |t| + t.integer :article_position + t.integer :category, null: false + t.string :context_type, null: false + t.integer :counts_for, null: false, default: 1 + + t.references :article, null: false, foreign_key: { on_delete: :cascade } + t.references :user, foreign_key: { on_delete: :nullify } + + t.timestamps + end + end +end diff --git a/db/migrate/20230818083219_add_created_at_and_composite_indices_to_feed_events.rb b/db/migrate/20230818083219_add_created_at_and_composite_indices_to_feed_events.rb new file mode 100644 index 000000000..bcc7c79fa --- /dev/null +++ b/db/migrate/20230818083219_add_created_at_and_composite_indices_to_feed_events.rb @@ -0,0 +1,12 @@ +class AddCreatedAtAndCompositeIndicesToFeedEvents < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + add_index :feed_events, :created_at, algorithm: :concurrently + + add_index :feed_events, + %i[article_id user_id category], + name: "index_feed_events_on_article_user_and_category", + algorithm: :concurrently + end +end diff --git a/db/migrate/20230831105357_add_feed_counters_to_article.rb b/db/migrate/20230831105357_add_feed_counters_to_article.rb new file mode 100644 index 000000000..9cc93917c --- /dev/null +++ b/db/migrate/20230831105357_add_feed_counters_to_article.rb @@ -0,0 +1,8 @@ +class AddFeedCountersToArticle < ActiveRecord::Migration[7.0] + def change + safety_assured do + add_column :articles, :feed_impressions_count, :integer, default: 0 + add_column :articles, :feed_clicks_count, :integer, default: 0 + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 2974ec5ee..0fb8712ca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -104,6 +104,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_31_160030) do t.float "experience_level_rating", default: 5.0 t.float "experience_level_rating_distribution", default: 5.0 t.boolean "featured", default: false + t.integer "feed_clicks_count", default: 0 + t.integer "feed_impressions_count", default: 0 t.string "feed_source_url" t.integer "hotness_score", default: 0 t.string "language" @@ -501,6 +503,21 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_31_160030) do t.index ["user_id"], name: "index_email_authorizations_on_user_id" end + create_table "feed_events", force: :cascade do |t| + t.bigint "article_id", null: false + t.integer "article_position" + t.integer "category", null: false + t.string "context_type", null: false + t.integer "counts_for", default: 1, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.bigint "user_id" + t.index ["article_id", "user_id", "category"], name: "index_feed_events_on_article_user_and_category" + t.index ["article_id"], name: "index_feed_events_on_article_id" + t.index ["created_at"], name: "index_feed_events_on_created_at" + t.index ["user_id"], name: "index_feed_events_on_user_id" + end + create_table "feedback_messages", force: :cascade do |t| t.bigint "affected_id" t.string "category" @@ -1391,6 +1408,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_31_160030) do add_foreign_key "display_ad_events", "users", on_delete: :cascade add_foreign_key "display_ads", "organizations", on_delete: :cascade add_foreign_key "email_authorizations", "users", on_delete: :cascade + add_foreign_key "feed_events", "articles", on_delete: :cascade + add_foreign_key "feed_events", "users", on_delete: :nullify add_foreign_key "feedback_messages", "users", column: "affected_id", on_delete: :nullify add_foreign_key "feedback_messages", "users", column: "offender_id", on_delete: :nullify add_foreign_key "feedback_messages", "users", column: "reporter_id", on_delete: :nullify diff --git a/spec/factories/feed_events.rb b/spec/factories/feed_events.rb new file mode 100644 index 000000000..7e614ed49 --- /dev/null +++ b/spec/factories/feed_events.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + factory :feed_event do + sequence(:article_position) + category { :impression } + context_type { FeedEvent::CONTEXT_TYPE_HOME } + article + user + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index d817571c8..29c628acc 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -25,6 +25,7 @@ RSpec.describe Article do it { is_expected.to have_many(:comments).dependent(:nullify) } it { is_expected.to have_many(:context_notifications).dependent(:delete_all) } + it { is_expected.to have_many(:feed_events).dependent(:delete_all) } it { is_expected.to have_many(:mentions).dependent(:delete_all) } it { is_expected.to have_many(:notification_subscriptions).dependent(:delete_all) } it { is_expected.to have_many(:notifications).dependent(:delete_all) } @@ -1420,6 +1421,7 @@ RSpec.describe Article do end end end + describe "#detect_language" do let(:detected_language) { :kl } # kl for Klingon diff --git a/spec/models/feed_event_spec.rb b/spec/models/feed_event_spec.rb new file mode 100644 index 000000000..713e97437 --- /dev/null +++ b/spec/models/feed_event_spec.rb @@ -0,0 +1,14 @@ +require "rails_helper" + +RSpec.describe FeedEvent do + describe "validations" do + it { is_expected.to belong_to(:article).optional } + it { is_expected.to validate_numericality_of(:article_id).only_integer } + it { is_expected.to belong_to(:user).optional } + it { is_expected.to validate_numericality_of(:user_id).only_integer.allow_nil } + + it { is_expected.to define_enum_for(:category).with_values(%i[impression click reaction comment]) } + it { is_expected.to validate_numericality_of(:article_position).is_greater_than(0).only_integer } + it { is_expected.to validate_inclusion_of(:context_type).in_array(%w[home search tag]) } + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9fd33fc4b..6172ff99b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -82,6 +82,7 @@ RSpec.describe User do it { is_expected.to have_many(:billboard_events).dependent(:nullify) } it { is_expected.to have_many(:email_authorizations).dependent(:delete_all) } it { is_expected.to have_many(:email_messages).class_name("Ahoy::Message").dependent(:destroy) } + it { is_expected.to have_many(:feed_events).dependent(:nullify) } it { is_expected.to have_many(:field_test_memberships).class_name("FieldTest::Membership").dependent(:destroy) } it { is_expected.to have_many(:github_repos).dependent(:destroy) } it { is_expected.to have_many(:html_variants).dependent(:destroy) } diff --git a/spec/requests/feed_events_spec.rb b/spec/requests/feed_events_spec.rb new file mode 100644 index 000000000..0febfdb3b --- /dev/null +++ b/spec/requests/feed_events_spec.rb @@ -0,0 +1,81 @@ +require "rails_helper" + +RSpec.describe "FeedEvents" do + let(:user) { create(:user) } + let(:article) { create(:article) } + let(:event_params) do + { + article_id: article.id, + article_position: 4, + category: :click, + context_type: FeedEvent::CONTEXT_TYPE_HOME + } + end + + describe "POST /feed_events" do + context "when user is signed in" do + before do + sign_in user + end + + it "creates a feed click event" do + expect { post "/feed_events", params: { feed_events: [event_params] } }.to change(FeedEvent, :count).by(1) + + expect(response).to be_successful + expect(article.feed_events.first).to have_attributes( + user_id: user.id, + article_position: 4, + category: "click", + context_type: "home", + ) + end + + it "creates a feed impression event" do + params = event_params.merge(category: :impression) + expect { post "/feed_events", params: { feed_events: [params] } }.to change(FeedEvent, :count).by(1) + + expect(response).to be_successful + expect(article.feed_events.first.category).to eq("impression") + end + + it "creates multiple events in a batch" do + second_article = create(:article) + events = [ + { + article_id: article.id, + article_position: 20, + category: :impression, + context_type: FeedEvent::CONTEXT_TYPE_HOME + }, + { + article_id: article.id, + article_position: 20, + category: :click, + context_type: FeedEvent::CONTEXT_TYPE_HOME + }, + { + article_id: second_article.id, + article_position: 7, + category: :impression, + context_type: FeedEvent::CONTEXT_TYPE_TAG + }, + { + article_id: second_article.id, + article_position: 7, + category: :click, + context_type: FeedEvent::CONTEXT_TYPE_TAG + }, + ] + expect { post "/feed_events", params: { feed_events: events } }.to change(FeedEvent, :count).by(4) + expect(response).to be_successful + end + end + + context "when user is not signed in" do + it "silently does not create an event" do + expect { post "/feed_events", params: { feed_events: [event_params] } }.not_to change(FeedEvent, :count) + expect(response).to be_successful + end + end + end +end diff --git a/spec/services/feed_events/bulk_upsert_spec.rb b/spec/services/feed_events/bulk_upsert_spec.rb new file mode 100644 index 000000000..2cfabccc3 --- /dev/null +++ b/spec/services/feed_events/bulk_upsert_spec.rb @@ -0,0 +1,162 @@ +require "rails_helper" + +RSpec.describe FeedEvents::BulkUpsert, type: :service do + let(:user) { create(:user) } + let(:second_user) { create(:user) } + + def feed_event(**attributes) + build(:feed_event, **attributes).attributes + end + + context "when there are no duplicates in the list or database" do + let(:articles) { create_list(:article, 5) } + let(:feed_events_data) do + articles + .map + .with_index do |article, index| + { + article: article, + user: user, + article_position: index + 1, + category: :impression, + context_type: FeedEvent::CONTEXT_TYPE_HOME + } + end + end + + it "inserts all the feed events" do + expect { described_class.call(feed_events_data) }.to change(FeedEvent, :count).by(5) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to match_array( + articles.map { |article| [article.id, user.id, "impression"] }, + ) + end + end + + context "when there are duplicate events within the list" do + let(:article) { create(:article) } + let(:feed_events_data) do + Array.new(5) do + feed_event(article: article, user: user, category: :click) + end + end + + it "does not insert extra duplicate events" do + expect { described_class.call(feed_events_data) }.to change(FeedEvent, :count).by(1) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, user.id, "click"], + ) + end + end + + context "when there are invalid events in the list" do + let(:article) { create(:article) } + let(:feed_events_data) do + [ + feed_event(article: nil, user: user, category: :click), + feed_event(article: article, user: user).merge(category: :not_a_real_category), + feed_event(article: article, user: user, category: :impression), + feed_event(article: article, user: nil, category: :impression), + feed_event(article: article, user: second_user, category: :impression, context_type: "blahblahblah"), + feed_event(article: article, user: second_user, category: :click, article_position: -2), + ] + end + + it "filters them out" do + expect { described_class.call(feed_events_data, timebox: nil) }.to change(FeedEvent, :count).by(2) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, user.id, "impression"], + [article.id, nil, "impression"], + ) + end + end + + context "when there is only one valid item in the list" do + let(:article) { create(:article) } + let(:feed_event_data) do + [ + feed_event(article: article, user: user, category: :click, context_type: "foobar"), + feed_event(article: article, user: second_user, category: :impression), + ] + end + + it "handles it appropriately" do + expect { described_class.call(feed_event_data) }.to change(FeedEvent, :count).by(1) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, second_user.id, "impression"], + ) + end + end + + context "when there are no valid items in the list" do + let(:feed_event_data) do + [ + feed_event(article: nil, user: user, category: :click), + feed_event(article: nil, user: second_user, category: :click), + ] + end + + it "does nothing and returns" do + allow(FeedEvent).to receive(:where).and_call_original + + expect { described_class.call(feed_event_data) }.not_to change(FeedEvent, :count) + + expect(FeedEvent).not_to have_received(:where) + end + end + + context "when there are already existing events with the same article, user and category" do + let(:article) { create(:article) } + let(:second_article) { create(:article) } + let(:feed_events_data) do + [ + feed_event(article: article, user: user, category: :impression), + feed_event(article: article, user: user, category: :click), + feed_event(article: article, user: second_user, category: :impression), + feed_event(article: second_article, user: user, category: :impression), + ] + end + + before do + create(:feed_event, article: article, user: user, category: :impression) + create(:feed_event, article: article, user: second_user, category: :impression) + end + + it "ignores them if no timebox is provided" do + expect { described_class.call(feed_events_data, timebox: nil) }.to change(FeedEvent, :count).by(4) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, user.id, "impression"], + [article.id, second_user.id, "impression"], + [article.id, user.id, "impression"], + [article.id, user.id, "click"], + [article.id, second_user.id, "impression"], + [second_article.id, user.id, "impression"], + ) + end + + it "does not create new events if the existing matching events were created within the provided timebox" do + Timecop.travel(7.minutes.from_now) do + expect { described_class.call(feed_events_data, timebox: 10.minutes) }.to change(FeedEvent, :count).by(2) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, user.id, "impression"], + [article.id, second_user.id, "impression"], + [article.id, user.id, "click"], + [second_article.id, user.id, "impression"], + ) + end + end + + it "creates new events if the existing matching events were created outside the provided timebox" do + Timecop.travel(12.minutes.from_now) do + expect { described_class.call(feed_events_data, timebox: 10.minutes) }.to change(FeedEvent, :count).by(4) + expect(FeedEvent.pluck(:article_id, :user_id, :category)).to contain_exactly( + [article.id, user.id, "impression"], + [article.id, second_user.id, "impression"], + [article.id, user.id, "impression"], + [article.id, user.id, "click"], + [article.id, second_user.id, "impression"], + [second_article.id, user.id, "impression"], + ) + end + end + end +end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 88deba140..778f1df74 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -84,8 +84,9 @@ RSpec.describe Users::Delete, type: :service do affected_feedback_messages audit_logs banished_users - created_podcasts billboard_events + created_podcasts + feed_events offender_feedback_messages page_views rating_votes