From 9f07b4c79738e53299812d55fc75bb932ea681ba Mon Sep 17 00:00:00 2001 From: Matthew Zikherman <mzikherman@gmail.com> Date: Wed, 26 May 2021 13:50:22 -0400 Subject: [PATCH] feat: launch downrank merch. AB test (decayed_merch_v3) --- .../split_test/running_tests.coffee | 10 +- .../split_test/splitTestMiddleware.ts | 8 +- src/desktop/config.coffee | 1 - .../middleware/__tests__/pageCache.jest.ts | 64 ---------- ...tstrapSharifyAndContextLocalsMiddleware.ts | 2 +- src/lib/middleware/pageCache.ts | 115 ------------------ src/middleware.ts | 2 - src/typings/sharify.d.ts | 1 + .../Components/ArtistArtworkFilter.tsx | 34 ++++-- src/v2/Apps/Artist/artistRoutes.tsx | 6 + .../__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 +- 15 files changed, 110 insertions(+), 201 deletions(-) delete mode 100644 src/lib/middleware/__tests__/pageCache.jest.ts delete mode 100644 src/lib/middleware/pageCache.ts diff --git a/src/desktop/components/split_test/running_tests.coffee b/src/desktop/components/split_test/running_tests.coffee index 2e789a7b747..e51f3b57ef6 100644 --- a/src/desktop/components/split_test/running_tests.coffee +++ b/src/desktop/components/split_test/running_tests.coffee @@ -26,4 +26,12 @@ # this should export empty Object # module.exports = {} -module.exports = {} +module.exports = { + decayed_merch_v3: + key: "decayed_merch_v3" + outcomes: [ + 'control' + 'experiment' + ] + weighting: 'equal' +} diff --git a/src/desktop/components/split_test/splitTestMiddleware.ts b/src/desktop/components/split_test/splitTestMiddleware.ts index a6ec4b27ba8..5188618f50d 100644 --- a/src/desktop/components/split_test/splitTestMiddleware.ts +++ b/src/desktop/components/split_test/splitTestMiddleware.ts @@ -1,5 +1,6 @@ import { NextFunction } from "express" import { ArtsyRequest, ArtsyResponse } from "lib/middleware/artsyExpress" +import { updateSharifyAndContext } from "lib/middleware/bootstrapSharifyAndContextLocalsMiddleware" import qs from "qs" const runningTests = require("./running_tests.coffee") const SplitTest = require("./server_split_test.coffee") @@ -14,7 +15,9 @@ export function splitTestMiddleware( const name = key.toUpperCase() if (!res.locals.sd[name]) { const test = new SplitTest(req, res, configuration) - res.locals.sd[name] = test.outcome() + const outcome = test.outcome() + + updateSharifyAndContext(res, name, outcome) } } @@ -26,7 +29,8 @@ export function splitTestMiddleware( const test = new SplitTest(req, res, runningTests[key]) const value = params[key] test.set(value) - res.locals.sd[key.toUpperCase()] = value + + updateSharifyAndContext(res, key.toUpperCase(), value) } } diff --git a/src/desktop/config.coffee b/src/desktop/config.coffee index f642809e1b9..47920ea6072 100644 --- a/src/desktop/config.coffee +++ b/src/desktop/config.coffee @@ -75,7 +75,6 @@ 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 deleted file mode 100644 index 6e0461cbd77..00000000000 --- a/src/lib/middleware/__tests__/pageCache.jest.ts +++ /dev/null @@ -1,64 +0,0 @@ -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/bootstrapSharifyAndContextLocalsMiddleware.ts b/src/lib/middleware/bootstrapSharifyAndContextLocalsMiddleware.ts index 542004d5587..8900d5a38ba 100644 --- a/src/lib/middleware/bootstrapSharifyAndContextLocalsMiddleware.ts +++ b/src/lib/middleware/bootstrapSharifyAndContextLocalsMiddleware.ts @@ -79,7 +79,7 @@ export function bootstrapSharifyAndContextLocalsMiddleware( * Updates both the sharify locals for template injection along with the context * globals for the request. */ -function updateSharifyAndContext(res, key, value) { +export function updateSharifyAndContext(res, key, value) { res.locals.sd[key] = value const asyncLocalStorage = getAsyncLocalStorage() asyncLocalStorage.getStore()?.set(key, value) diff --git a/src/lib/middleware/pageCache.ts b/src/lib/middleware/pageCache.ts deleted file mode 100644 index a94bbd1ffdb..00000000000 --- a/src/lib/middleware/pageCache.ts +++ /dev/null @@ -1,115 +0,0 @@ -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<void>((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 138d26c0013..df831aafcae 100644 --- a/src/middleware.ts +++ b/src/middleware.ts @@ -38,7 +38,6 @@ 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" @@ -110,7 +109,6 @@ 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/typings/sharify.d.ts b/src/typings/sharify.d.ts index 5a5b519c212..380d587697e 100644 --- a/src/typings/sharify.d.ts +++ b/src/typings/sharify.d.ts @@ -24,6 +24,7 @@ declare module "sharify" { readonly CMS_URL: string CURRENT_PATH: string CURRENT_USER: User + readonly DECAYED_MERCH_V3: string // TODO: Remove after A/B test readonly DEPLOY_ENV: string readonly EIGEN: boolean readonly ENABLE_NEW_ARTWORK_FILTERS: boolean diff --git a/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx b/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx index b3aeb7a8507..896152784ae 100644 --- a/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx +++ b/src/v2/Apps/Artist/Routes/Overview/Components/ArtistArtworkFilter.tsx @@ -7,6 +7,7 @@ import { Match, RouterState, withRouter } from "found" import React from "react" import { RelayRefetchProp, createRefetchContainer, graphql } from "react-relay" import { ZeroState } from "./ZeroState" +import { getENV } from "v2/Utils/getENV" interface ArtistArtworkFilterProps { artist: ArtistArtworkFilter_artist @@ -24,18 +25,31 @@ const ArtistArtworkFilter: React.FC<ArtistArtworkFilterProps> = 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 = + getENV("DECAYED_MERCH_V3") === "experiment" + ? "-decayed_merch_v2" + : "-decayed_merch" + + sortOptions.unshift({ value: defaultSortValue, text: "Default" }) + + const initialFilters = { + ...(match && match.location.query), + sort: defaultSortValue, + } + return ( <ArtworkFilterContextProvider - filters={match && match.location.query} - sortOptions={[ - { value: "-decayed_merch", text: "Default" }, - { 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.)" }, - ]} + filters={initialFilters} + sortOptions={sortOptions} // @ts-expect-error STRICT_NULL_CHECK aggregations={sidebarAggregations.aggregations as any} // @ts-expect-error STRICT_NULL_CHECK diff --git a/src/v2/Apps/Artist/artistRoutes.tsx b/src/v2/Apps/Artist/artistRoutes.tsx index 010a42ab40b..6af7b895c8e 100644 --- a/src/v2/Apps/Artist/artistRoutes.tsx +++ b/src/v2/Apps/Artist/artistRoutes.tsx @@ -123,9 +123,15 @@ export const artistRoutes: AppRouteConfig[] = [ // renders (such as tabbing back to this route in your browser) will not. const filterStateFromUrl = props.location ? props.location.query : {} + const sort = + getENV("DECAYED_MERCH_V3") === "experiment" + ? "-decayed_merch_v2" + : "-decayed_merch" + const filterParams = { ...initialArtworkFilterState, ...paramsToCamelCase(filterStateFromUrl), + sort, } // filterParams.hasFilter = Object.entries(filterParams).some( diff --git a/src/v2/Artsy/Analytics/__tests__/trackingMiddleware.jest.ts b/src/v2/Artsy/Analytics/__tests__/trackingMiddleware.jest.ts index ed1d990fcb0..542a166652a 100644 --- a/src/v2/Artsy/Analytics/__tests__/trackingMiddleware.jest.ts +++ b/src/v2/Artsy/Analytics/__tests__/trackingMiddleware.jest.ts @@ -6,6 +6,7 @@ declare const global: any jest.mock("sharify", () => ({ data: { APP_URL: "http://testing.com", + ALL_ARTWORKS_AS_CATS: "experiment", }, })) @@ -19,7 +20,7 @@ describe("trackingMiddleware", () => { beforeEach(() => { // FIXME: reaction migration // @ts-ignore - window.analytics = { page: jest.fn() } + window.analytics = { track: jest.fn(), page: jest.fn() } }) afterEach(() => { @@ -145,4 +146,30 @@ 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 e44d25725b9..df0089b214e 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 2b10151a06a..b482e912aa1 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,13 +12,19 @@ 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 = [] } = options + const { excludePaths = [], abTestRouteMap = [] } = options const { type, payload } = action switch (type) { @@ -109,6 +115,19 @@ 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 8ec1f2eabcf..404b0267e24 100644 --- a/src/v2/Artsy/Router/buildClientApp.tsx +++ b/src/v2/Artsy/Router/buildClientApp.tsx @@ -76,6 +76,12 @@ export function buildClientApp(config: RouterConfig): Promise<Resolve> { // @see https://github.com/artsy/force/blob/2c0db041fa6cb50e9f747ea95860ad5c38290653/src/v2/Apps/Artwork/ArtworkApp.tsx#L117-L121 "/artwork(.*)", ], + abTestRouteMap: [ + { + abTest: "decayed_merch_v3", + 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 9d1297777af..605a6938334 100644 --- a/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx +++ b/src/v2/Components/ArtworkFilter/Utils/isDefaultFilter.tsx @@ -1,4 +1,5 @@ import { ArtworkFilters } from "../ArtworkFilterContext" +import { getENV } from "v2/Utils/getENV" export const isDefaultFilter: ( name: keyof ArtworkFilters, @@ -8,6 +9,11 @@ export const isDefaultFilter: ( return false } + const defaultSort = + getENV("DECAYED_MERCH_V3") === "experiment" + ? "-decayed_merch_v2" + : "-decayed_merch" + switch (true) { case name === "sizes" || name === "artistIDs" || @@ -21,7 +27,7 @@ export const isDefaultFilter: ( name === "materialsTerms": return value.length === 0 case name === "sort": - return value === "-decayed_merch" + return value === defaultSort case name === "medium": return value === "*" || !value case name === "priceRange" || name === "height" || name === "width":