From bcccf65b6e8be8e3af2002c52997d5a7a9635541 Mon Sep 17 00:00:00 2001 From: Matt Zikherman Date: Thu, 20 May 2021 18:09:40 -0400 Subject: [PATCH] Revert "feat: setup AB test for a new default sort on the artist page (#7520)" This reverts commit 133eecd8413282372d6f445481d0671ac648e834. --- .../components/split_test/middleware.coffee | 6 +- .../split_test/running_tests.coffee | 10 +- src/desktop/config.coffee | 1 + .../middleware/__tests__/pageCache.jest.ts | 64 ++++++++++ src/lib/middleware/pageCache.ts | 115 ++++++++++++++++++ src/middleware.ts | 2 + .../Components/ArtistArtworkFilter.tsx | 34 ++---- src/v2/Apps/Artist/artistRoutes.tsx | 8 -- .../__tests__/trackingMiddleware.jest.ts | 29 +---- .../Artsy/Analytics/trackExperimentViewed.tsx | 2 +- src/v2/Artsy/Analytics/trackingMiddleware.ts | 23 +--- src/v2/Artsy/Router/buildClientApp.tsx | 6 - .../ArtworkFilter/Utils/isDefaultFilter.tsx | 8 +- 13 files changed, 199 insertions(+), 109 deletions(-) create mode 100644 src/lib/middleware/__tests__/pageCache.jest.ts create mode 100644 src/lib/middleware/pageCache.ts diff --git a/src/desktop/components/split_test/middleware.coffee b/src/desktop/components/split_test/middleware.coffee index 9d142fa0c1e..3b93d088796 100644 --- a/src/desktop/components/split_test/middleware.coffee +++ b/src/desktop/components/split_test/middleware.coffee @@ -1,15 +1,12 @@ SplitTest = require './server_split_test.coffee' runningTests = require './running_tests' qs = require 'qs' -sd = require('sharify').data module.exports = (req, res, next) -> for key, configuration of runningTests unless res.locals.sd[key?.toUpperCase()]? test = new SplitTest req, res, configuration - outcome = test.outcome() - res.locals.sd[key.toUpperCase()] = outcome - sd[key.toUpperCase()] = outcome + res.locals.sd[key.toUpperCase()] = test.outcome() if req.query?.split_test params = qs.parse req.query?.split_test @@ -17,6 +14,5 @@ module.exports = (req, res, next) -> test = new SplitTest req, res, runningTests[k] test.set v res.locals.sd[k.toUpperCase()] = v - sd[k.toUpperCase()] = v next() diff --git a/src/desktop/components/split_test/running_tests.coffee b/src/desktop/components/split_test/running_tests.coffee index 7cebf0f0938..2e789a7b747 100644 --- a/src/desktop/components/split_test/running_tests.coffee +++ b/src/desktop/components/split_test/running_tests.coffee @@ -26,12 +26,4 @@ # this should export empty Object # module.exports = {} -module.exports = { - decayed_merch_v2: - key: "decayed_merch_v2" - outcomes: [ - 'control' - 'experiment' - ] - weighting: 'equal' -} +module.exports = {} diff --git a/src/desktop/config.coffee b/src/desktop/config.coffee index 47920ea6072..f642809e1b9 100644 --- a/src/desktop/config.coffee +++ b/src/desktop/config.coffee @@ -75,6 +75,7 @@ module.exports = MOBILE_MEDIA_QUERY: "only screen and (max-width: 640px)" NODE_ENV: 'development' OPENREDIS_URL: null + PAGE_CACHE_ENABLED: false PAGE_CACHE_TYPES: 'artist' PAGE_CACHE_NAMESPACE: 'page-cache' PAGE_CACHE_VERSION: '1' diff --git a/src/lib/middleware/__tests__/pageCache.jest.ts b/src/lib/middleware/__tests__/pageCache.jest.ts new file mode 100644 index 00000000000..6e0461cbd77 --- /dev/null +++ b/src/lib/middleware/__tests__/pageCache.jest.ts @@ -0,0 +1,64 @@ +import { pageCacheMiddleware } from "../pageCache" + +const { cache } = require("lib/cache") + +jest.mock("lib/cache", () => ({ + cache: { + get: jest.fn().mockResolvedValue(true), + set: jest.fn(), + }, +})) + +jest.mock("config", () => ({ + PAGE_CACHE_ENABLED: true, + PAGE_CACHE_TYPES: "artist", + PAGE_CACHE_NAMESPACE: "page-cache", + PAGE_CACHE_VERSION: "1", + PAGE_CACHE_EXPIRY_SECONDS: 600, + PAGE_CACHE_RETRIEVAL_TIMEOUT_MS: 400, +})) + +describe("pageCacheMiddleware", () => { + let req + let res + let next + beforeEach(() => { + req = { + path: "/artist/test-artist", + url: "https://artsy.net/artist/test-artist", + } + res = { + once: jest.fn((_e, cb) => cb()), + locals: { + PAGE_CACHE: { + status: 200, + key: "key", + html: "html", + }, + }, + } + next = jest.fn() + cache.get.mockClear() + cache.set.mockClear() + }) + + it("sets up cache for valid pageTypes", async () => { + await pageCacheMiddleware(req, res, next) + expect(cache.set).toBeCalledWith("page-cache|1|key", "html", 600) + expect(cache.get.mock.calls[0][0]).toBe( + "page-cache|1|https://artsy.net/artist/test-artist" + ) + expect(next).toBeCalled() + }) + + it("skips cache for invalid pageTypes", async () => { + req = { + path: "/artist-series/test-artist", + url: "https://artsy.net/artist-series/test-artist", + } + await pageCacheMiddleware(req, res, next) + expect(cache.set).not.toBeCalled() + expect(cache.get).not.toBeCalled() + expect(next).toBeCalled() + }) +}) diff --git a/src/lib/middleware/pageCache.ts b/src/lib/middleware/pageCache.ts new file mode 100644 index 00000000000..a94bbd1ffdb --- /dev/null +++ b/src/lib/middleware/pageCache.ts @@ -0,0 +1,115 @@ +import type { NextFunction } from "express" +import type { ArtsyRequest, ArtsyResponse } from "./artsyExpress" + +import { getContextPageFromReq } from "lib/getContextPage" +import { cache } from "lib/cache" +import config from "../../config" + +const { + PAGE_CACHE_ENABLED, + PAGE_CACHE_EXPIRY_SECONDS, + PAGE_CACHE_NAMESPACE, + PAGE_CACHE_RETRIEVAL_TIMEOUT_MS, + PAGE_CACHE_TYPES, + PAGE_CACHE_VERSION, +} = config + +const runningTests = Object.keys( + require("desktop/components/split_test/running_tests.coffee") +).sort() +const cacheablePageTypes: string[] = PAGE_CACHE_TYPES.split("|") + +// Middleware will `next` and do nothing if any of the following is true: +// +// * page cache feature is disabled. +// * a user is signed in. +// * this isnt a supported cacheable path (there is an allow-list set in ENV). +// * the page content is uncached. +// * the cache errors. +export async function pageCacheMiddleware( + req: ArtsyRequest, + res: ArtsyResponse, + next: NextFunction +) { + if (!PAGE_CACHE_ENABLED) return next() + + // Returns true if the page type corresponding to `url` is configured cacheable. + const isCacheablePageType = (req: ArtsyRequest) => { + const { pageType } = getContextPageFromReq(req) + + return cacheablePageTypes.includes(pageType) + } + if (!isCacheablePageType(req)) return next() + + // @ts-ignore + const hasUser = !!req.user + if (hasUser) return next() + + // Generate cache key that includes all currently running AB tests and outcomes. + const runningTestsCacheKey = runningTests + .map(test => { + const outcome = res.locals.sd[test.toUpperCase()] + return `${test}:${outcome}` + }) + .join("|") + + // `key` should be a full URL w/ query params, and not a path. + // This is to separate URL's like `/collect` and `/collect?acquireable=true`. + const cacheKey = (key: string) => { + return [PAGE_CACHE_NAMESPACE, runningTestsCacheKey, PAGE_CACHE_VERSION, key] + .filter(Boolean) + .join("|") + } + + // `key` is the full URL. + const cacheHtmlForPage = ({ status, key, html }) => { + if (status !== 200) return + + cache.set(cacheKey(key), html, PAGE_CACHE_EXPIRY_SECONDS) + } + + const cacheKeyForRequest = cacheKey(req.url) + + // Register callback to write rendered page data to cache. + res.once("finish", () => { + if (res.locals.PAGE_CACHE) { + // eslint-disable-next-line no-console + console.log(`[Page Cache]: Writing ${cacheKeyForRequest} to cache`) + cacheHtmlForPage(res.locals.PAGE_CACHE) + } + }) + + try { + await new Promise((resolve, reject) => { + // Cache timeout handler, will reject if hit. + let timeoutId = setTimeout(() => { + // @ts-expect-error STRICT_NULL_CHECK + timeoutId = null + const error = new Error( + `Timeout of ${PAGE_CACHE_RETRIEVAL_TIMEOUT_MS}ms, skipping...` + ) + reject(error) + }, PAGE_CACHE_RETRIEVAL_TIMEOUT_MS) + + const handleCacheGet = (_err, html) => { + if (!timeoutId) return // Already timed out. + + clearTimeout(timeoutId) + + if (html) { + // eslint-disable-next-line no-console + console.log(`[Page Cache]: Reading ${cacheKeyForRequest} from cache`) + return res.send(html) + } + + resolve() + } + + cache.get(cacheKey(req.url), handleCacheGet) + }) + } catch (e) { + console.error(`[Page Cache Middleware]: ${e.message}`) + } finally { + next() + } +} diff --git a/src/middleware.ts b/src/middleware.ts index e9f62b72e9f..a2d5f0177db 100644 --- a/src/middleware.ts +++ b/src/middleware.ts @@ -37,6 +37,7 @@ import { escapedFragmentMiddleware } from "./lib/middleware/escapedFragment" import { hardcodedRedirectsMiddleware } from "./lib/middleware/hardcodedRedirects" import { localsMiddleware } from "./lib/middleware/locals" import { marketingModalsMiddleware } from "./lib/middleware/marketingModals" +import { pageCacheMiddleware } from "./lib/middleware/pageCache" import { proxyReflectionMiddleware } from "./lib/middleware/proxyReflection" import { sameOriginMiddleware } from "./lib/middleware/sameOrigin" import { unsupportedBrowserMiddleware } from "./lib/middleware/unsupportedBrowser" @@ -118,6 +119,7 @@ export function initializeMiddleware(app) { app.use(sameOriginMiddleware) app.use(escapedFragmentMiddleware) app.use(unsupportedBrowserMiddleware) + app.use(pageCacheMiddleware) /** * Blank page used by Eigen for caching web views. diff --git a/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx b/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx index bed6e51cf2e..b3aeb7a8507 100644 --- a/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx +++ b/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx @@ -7,7 +7,6 @@ import { Match, RouterState, withRouter } from "found" import React from "react" import { RelayRefetchProp, createRefetchContainer, graphql } from "react-relay" import { ZeroState } from "./ZeroState" -import { data as sd } from "sharify" // TODO: Remove after AB test interface ArtistArtworkFilterProps { artist: ArtistArtworkFilter_artist @@ -25,31 +24,18 @@ const ArtistArtworkFilter: React.FC = props => { // we still want to render the rest of the page. if (!hasFilter) return null - const sortOptions = [ - { value: "-has_price,-prices", text: "Price (desc.)" }, - { value: "-has_price,prices", text: "Price (asc.)" }, - { value: "-partner_updated_at", text: "Recently updated" }, - { value: "-published_at", text: "Recently added" }, - { value: "-year", text: "Artwork year (desc.)" }, - { value: "year", text: "Artwork year (asc.)" }, - ] - - const defaultSortValue = - sd["DECAYED_MERCH_V2"] === "experiment" - ? "-decayed_merch_v2" - : "-decayed_merch" - - sortOptions.unshift({ value: defaultSortValue, text: "Default" }) - - const initialFilters = { - ...(match && match.location.query), - sort: defaultSortValue, - } - return ( ({ data: { APP_URL: "http://testing.com", - ALL_ARTWORKS_AS_CATS: "experiment", }, })) @@ -20,7 +19,7 @@ describe("trackingMiddleware", () => { beforeEach(() => { // FIXME: reaction migration // @ts-ignore - window.analytics = { track: jest.fn(), page: jest.fn() } + window.analytics = { page: jest.fn() } }) afterEach(() => { @@ -146,30 +145,4 @@ describe("trackingMiddleware", () => { }) }) }) - - describe("triggering AB test experiment viewed events", () => { - it("triggers for a given route", () => { - trackingMiddleware({ - abTestRouteMap: [ - { abTest: "all_artworks_as_cats", routes: ["/artwork(.*)"] }, - ], - })(store)(noop)({ - type: ActionTypes.UPDATE_LOCATION, - payload: { - pathname: "/artwork/some-id", - }, - }) - expect(global.analytics.track).toBeCalledWith( - "Experiment Viewed", - { - experiment_id: "all_artworks_as_cats", - experiment_name: "all_artworks_as_cats", - variation_id: "experiment", - variation_name: "experiment", - nonInteraction: 1, - }, - { page: {} } - ) - }) - }) }) diff --git a/src/v2/Artsy/Analytics/trackExperimentViewed.tsx b/src/v2/Artsy/Analytics/trackExperimentViewed.tsx index df0089b214e..e44d25725b9 100644 --- a/src/v2/Artsy/Analytics/trackExperimentViewed.tsx +++ b/src/v2/Artsy/Analytics/trackExperimentViewed.tsx @@ -1,7 +1,7 @@ import * as Schema from "v2/Artsy/Analytics" import { data as sd } from "sharify" -export const trackExperimentViewed = (name: string, trackingData = {}) => { +export const trackExperimentViewed = (name: string, trackingData) => { if (typeof window.analytics !== "undefined") { const variation = sd[name.toUpperCase()] if (!Boolean(variation)) { diff --git a/src/v2/Artsy/Analytics/trackingMiddleware.ts b/src/v2/Artsy/Analytics/trackingMiddleware.ts index b482e912aa1..2b10151a06a 100644 --- a/src/v2/Artsy/Analytics/trackingMiddleware.ts +++ b/src/v2/Artsy/Analytics/trackingMiddleware.ts @@ -1,4 +1,4 @@ -import { trackExperimentViewed } from "v2/Artsy/Analytics/trackExperimentViewed" +// import { trackExperimentViewed } from "v2/Artsy/Analytics/trackExperimentViewed" import { ActionTypes } from "farce" import { data as sd } from "sharify" import { get } from "v2/Utils/get" @@ -12,19 +12,13 @@ import { match } from "path-to-regexp" * @see https://github.com/4Catalyzer/farce/blob/master/src/ActionTypes.js */ -interface ABTestRouteMap { - abTest: string - routes: string[] -} - interface TrackingMiddlewareOptions { excludePaths?: string[] - abTestRouteMap?: ABTestRouteMap[] } export function trackingMiddleware(options: TrackingMiddlewareOptions = {}) { return store => next => action => { - const { excludePaths = [], abTestRouteMap = [] } = options + const { excludePaths = [] } = options const { type, payload } = action switch (type) { @@ -115,19 +109,6 @@ export function trackingMiddleware(options: TrackingMiddlewareOptions = {}) { } }) } - - // AB Test - abTestRouteMap.forEach(({ abTest, routes }) => { - routes.some(route => { - const matcher = match(route, { decode: decodeURIComponent }) - const foundMatch = !!matcher(pathname) - - if (foundMatch) { - trackExperimentViewed(abTest) - return true - } - }) - }) } return next(action) diff --git a/src/v2/Artsy/Router/buildClientApp.tsx b/src/v2/Artsy/Router/buildClientApp.tsx index 85e49686950..8ec1f2eabcf 100644 --- a/src/v2/Artsy/Router/buildClientApp.tsx +++ b/src/v2/Artsy/Router/buildClientApp.tsx @@ -76,12 +76,6 @@ export function buildClientApp(config: RouterConfig): Promise { // @see https://github.com/artsy/force/blob/2c0db041fa6cb50e9f747ea95860ad5c38290653/src/v2/Apps/Artwork/ArtworkApp.tsx#L117-L121 "/artwork(.*)", ], - abTestRouteMap: [ - { - abTest: "decayed_merch_v2", - routes: ["/artist/:artistID/works-for-sale"], - }, - ], }), ] const resolver = new Resolver(relayEnvironment) diff --git a/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx b/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx index 445c115964e..9d1297777af 100644 --- a/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx +++ b/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx @@ -1,5 +1,4 @@ import { ArtworkFilters } from "../ArtworkFilterContext" -import { data as sd } from "sharify" // TODO: Remove after AB test export const isDefaultFilter: ( name: keyof ArtworkFilters, @@ -9,11 +8,6 @@ export const isDefaultFilter: ( return false } - const defaultSort = - sd["DECAYED_MERCH_V2"] === "experiment" - ? "-decayed_merch_v2" - : "-decayed_merch" - switch (true) { case name === "sizes" || name === "artistIDs" || @@ -27,7 +21,7 @@ export const isDefaultFilter: ( name === "materialsTerms": return value.length === 0 case name === "sort": - return value === defaultSort + return value === "-decayed_merch" case name === "medium": return value === "*" || !value case name === "priceRange" || name === "height" || name === "width":