Merge pull request #7607 from artsy/revert-7520-merch-ab-test

Revert "feat: setup AB test for a new default sort on the artist page"

src/desktop/components/split_test/middleware.coffee

@@ -1,22 +1,18 @@
 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
     for k, v of params
       test = new SplitTest req, res, runningTests[k]
       test.set v
       res.locals.sd[k.toUpperCase()] = v
-      sd[k.toUpperCase()] = v
 
   next()

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 = {}

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'

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()
+  })
+})

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<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()
+  }
+}

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.

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<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 =
-    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 (
     <ArtworkFilterContextProvider
-      filters={initialFilters}
-      sortOptions={sortOptions}
+      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.)" },
+      ]}
       // @ts-expect-error STRICT_NULL_CHECK
       aggregations={sidebarAggregations.aggregations as any}
       // @ts-expect-error STRICT_NULL_CHECK

src/v2/Apps/Artist/artistRoutes.tsx

@@ -14,8 +14,6 @@ import { initialArtworkFilterState } from "v2/Components/ArtworkFilter/ArtworkFi
 import { allowedFilters } from "v2/Components/ArtworkFilter/Utils/allowedFilters"
 import { AppRouteConfig } from "v2/Artsy/Router/Route"
 
-import { data as sd } from "sharify" // TODO: Remove after AB test
-
 graphql`
   fragment artistRoutes_Artist on Artist {
     slug
@@ -125,15 +123,9 @@ 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 =
-            sd["DECAYED_MERCH_V2"] === "experiment"
-              ? "-decayed_merch_v2"
-              : "-decayed_merch"
-
           const filterParams = {
             ...initialArtworkFilterState,
             ...paramsToCamelCase(filterStateFromUrl),
-            sort,
           }
 
           // filterParams.hasFilter = Object.entries(filterParams).some(

src/v2/Artsy/Analytics/__tests__/trackingMiddleware.jest.ts

@@ -6,7 +6,6 @@ declare const global: any
 jest.mock("sharify", () => ({
   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: {} }
-      )
-    })
-  })
 })

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)) {