Tidy up TODOs

wellcomecollection · Feb 13, 2025 · 5e823f1 · 5e823f1
1 parent 8205686
commit 5e823f1
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 69 deletions.
diff --git a/api/src/controllers/events.ts b/api/src/controllers/events.ts
@@ -71,14 +71,7 @@ const isAvailableOnlineValidator = queryValidator({
   singleValue: true,
 });
 
-const timespans = [
-  'all',
-  'today',
-  'this-week',
-  'this-weekend',
-  'this-month',
-  'future',
-  'past',
+export const MONTHS = [
   'january',
   'february',
   'march',
@@ -92,6 +85,16 @@ const timespans = [
   'november',
   'december',
 ] as const;
+const timespans = [
+  'all',
+  'today',
+  'this-week',
+  'this-weekend',
+  'this-month',
+  'future',
+  'past',
+  ...MONTHS,
+] as const;
 export type Timespan = (typeof timespans)[number];
 export function isValidTimespan(type?: string | string[]): type is Timespan {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any

diff --git a/api/src/helpers/timespan.ts b/api/src/helpers/timespan.ts
@@ -1,83 +1,69 @@
-import { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types';
+import {
+  Field,
+  QueryDslRangeQuery,
+} from '@elastic/elasticsearch/lib/api/types';
 import { DateTime } from 'luxon';
 
-import { Timespan } from '@weco/content-api/src/controllers/events';
+import { MONTHS, Timespan } from '@weco/content-api/src/controllers/events';
 
 // TODO
+// change query.times.startDateTime to filter.times.startDateTim and filter.times.endDateTime
 // review lt and gt vs lte and gte
 // review where should use end date vs start date
 // review the sorting order, it's opposite from what's on?
-// TODO what do we do with TODAY when it's a Monday? Events with ranges will display results
-export const getTimespanQuery = (
+// review relation param
+export const getTimespanRange = (
   timespan: Timespan
-): QueryDslQueryContainer | undefined => {
+): Partial<Record<Field, QueryDslRangeQuery>> | undefined => {
   const now = DateTime.local({ zone: 'Europe/London' });
 
   switch (timespan) {
-    // TODO https://github.com/wellcomecollection/content-api/issues/130#issuecomment-2656772912
     case 'today':
+      // TODO https://github.com/wellcomecollection/content-api/issues/130#issuecomment-2656772912
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: 'now',
-            lte: now.endOf('day').toISO(),
-          },
+        'query.times.startDateTime': {
+          gte: 'now',
+          lte: now.endOf('day').toISO(),
         },
       };
 
+    // Review what this weekend is as currently on the site it's FRIDAY - SUNDAY
     // Maybe we want to pass in a different relation here? within or contain?
     case 'this-weekend':
-      console.log(now.startOf('week').plus({ days: 6 }).endOf('day'));
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: now.startOf('week').plus({ days: 5 }),
-            lte: now.startOf('week').plus({ days: 6 }).endOf('day'),
-          },
+        'query.times.startDateTime': {
+          gte: now.startOf('week').plus({ days: 5 }),
+          lte: now.startOf('week').plus({ days: 6 }).endOf('day'),
         },
       };
 
     case 'this-week':
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: 'now',
-            lte: now.endOf('week').toISO(),
-          },
+        'query.times.startDateTime': {
+          gte: 'now',
+          lte: now.endOf('week').toISO(),
         },
       };
 
     case 'this-month':
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: 'now',
-            lte: now.endOf('month').toISO(),
-          },
+        'query.times.startDateTime': {
+          gte: 'now',
+          lte: now.endOf('month').toISO(),
         },
       };
 
     case 'future':
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: 'now',
-          },
+        'query.times.startDateTime': {
+          gte: 'now',
         },
       };
 
     case 'past':
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            lt: 'now',
-          },
+        'query.times.startDateTime': {
+          lt: 'now',
         },
       };
 
@@ -93,21 +79,14 @@ export const getTimespanQuery = (
     case 'october':
     case 'november':
     case 'december': {
-      // TODO this is a bit workaround-y, might be best to just go with an array
-      const monthNumber = DateTime.fromFormat(
-        `${timespan} 01, 2001`,
-        'MMMM dd, yyyy'
-      ).month;
+      const monthNumber = MONTHS.indexOf(timespan) + 1;
       const startOfMonth = DateTime.local(now.year, monthNumber);
       const endOfMonth = startOfMonth.endOf('month');
 
       return {
-        range: {
-          // TODO change as it should be filter values and maybe end date
-          'query.times.startDateTime': {
-            gte: startOfMonth,
-            lte: endOfMonth,
-          },
+        'query.times.startDateTime': {
+          gte: startOfMonth,
+          lte: endOfMonth,
         },
       };
     }

diff --git a/api/src/queries/events.ts b/api/src/queries/events.ts
@@ -1,7 +1,7 @@
 import { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types';
 
 import { isValidTimespan } from '@weco/content-api/src/controllers/events';
-import { getTimespanQuery } from '@weco/content-api/src/helpers/timespan';
+import { getTimespanRange } from '@weco/content-api/src/helpers/timespan';
 
 import { TermsFilter } from './common';
 
@@ -64,14 +64,14 @@ export const eventsFilter = {
     },
   }),
   timespan: (timespan: string[]): QueryDslQueryContainer => {
-    let query;
+    let queryRange;
 
-    // validation on single value only gets done in timespanValidator, we can therefore
-    // asume that this array only has one value.
-    if (isValidTimespan(timespan[0])) query = getTimespanQuery(timespan[0]);
+    // Validation for single value gets done in timespanValidator
+    // we can therefore assume that this array only has one value to care about.
+    if (isValidTimespan(timespan[0]))
+      queryRange = getTimespanRange(timespan[0]);
 
-    // TODO keep range?
-    return query || { range: {} };
+    return { range: queryRange || {} };
   },
 };
 
@@ -106,10 +106,11 @@ export const eventsAggregations = {
       field: 'aggregatableValues.isAvailableOnline',
     },
   },
+  // https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-daterange-aggregation.html
   timespan: {
     terms: {
-      size: 2, // TODO figure out what this is
-      field: 'filter.timespan', // use filter values?
+      size: 20, // TODO figure out what this is https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html#sig-terms-shard-size
+      field: 'filter.timespan', // use filter values and not create aggregations for it
     },
   },
 } as const;