fix: allow Explore searches in places again (#2634)

* remove assumed and undocumented meaning of page being zero from our use of
  useInfiniteScroll in Explore; uses explicit type checks instead
* document byzantine pagination logic in useInfiniteExploreScroll
* fix broken infinite scroll when sorting by faves (was only showing first
  page)

The underlying problem from 96c316a was setting initialPageParam to 1 in
useAuthenticatedInfiniteQuery, which seems rational, except
useInfiniteExploreScroll was using a false-ish value of page to detect when
it was requesting its first page. This does the same by being more explicit
about page's type without injecting undocumented meaning into the params we
hand to useInfiniteScroll.

* refactor: move Explore page param munging into unit tested helper

Closes MOB-400

README.md

@@ -324,6 +324,7 @@ fastlane prod
             1. Delete the observation you just made
             1. Go to Explore and view a taxon
             1. Change to observations view and view an observation
+            1. Search for observations in a place
             1. Go to the observers profile
             1. Go to one project this user joined (if not available try another user)
             1. Sign in

src/api/types.d.ts

@@ -119,9 +119,50 @@ export interface ApiNotification {
   viewed?: boolean;
 }
 
-export interface ApiObservation {
+export interface ApiRecord {
+  created_at?: string;
+  id?: number;
+  updated_at?: string;
+}
+
+export interface ApiObservation extends ApiRecord {
   observation_photos?: ApiObservationPhoto[];
   observation_sounds?: ApiObservationSound[];
+  time_observed_at?: string;
   user?: ApiUser;
   uuid: string;
 }
+
+export interface ApiObservationsSearchResponse extends ApiResponse {
+  results: ApiObservation[]
+}
+
+export const ORDER_BY_CREATED_AT = "created_at";
+export const ORDER_BY_GEO_SCORE = "geo_score";
+export const ORDER_BY_ID = "id";
+export const ORDER_BY_OBSERVED_ON = "observed_on";
+export const ORDER_BY_RANDOM = "random";
+export const ORDER_BY_SPECIES_GUESS = "species_guess";
+export const ORDER_BY_UPDATED_AT = "updated_at";
+export const ORDER_BY_VOTES = "votes";
+
+export const ORDER_ASC = "asc";
+export const ORDER_DESC = "desc";
+
+export interface ApiObservationsSearchParams extends ApiParams {
+  created_d1?: string;
+  created_d2?: string;
+  d1?: string;
+  d2?: string;
+  id_below?: number;
+  order?: typeof ORDER_ASC | typeof ORDER_DESC;
+  order_by?: typeof ORDER_BY_CREATED_AT |
+    typeof ORDER_BY_GEO_SCORE |
+    typeof ORDER_BY_ID |
+    typeof ORDER_BY_OBSERVED_ON |
+    typeof ORDER_BY_RANDOM |
+    typeof ORDER_BY_SPECIES_GUESS |
+    typeof ORDER_BY_UPDATED_AT |
+    typeof ORDER_BY_VOTES;
+  return_bounds?: boolean;
+}

src/components/Explore/helpers/exploreParams.ts

@@ -0,0 +1,119 @@
+// Collection of helpers that help prepare API request parameters
+
+import type {
+  ApiObservationsSearchParams,
+  ApiObservationsSearchResponse
+} from "api/types";
+import { addSeconds, formatISO, parseISO } from "date-fns";
+import { last } from "lodash";
+
+interface ApiObservationsSearchParamsForInfiniteQuery extends ApiObservationsSearchParams {
+  pageParam: number | string | undefined | null;
+}
+
+// Gets the next page param for Explore given the last page
+//
+// Pagination using our API is complicated. The page param will only work
+// for the first 10k results. Frequently-updated result sets (like the
+// default), can potentially show duplicate observations on subsequent
+// pages using the page param as well. To avoid both of those issues, we're
+// paginating results using the id_below / id_above or d1 / d2 params when
+// possible, i.e. when sorting by date created or date observed. Other
+// sorts must use the page param and suffer the limitations described
+// above.
+function getNextPageParamForExplore(
+  lastPage: ApiObservationsSearchResponse,
+  params: ApiObservationsSearchParams
+) {
+  const lastObs = last( lastPage.results );
+  const orderBy = params.order_by;
+
+  // Returning null tells useInfiniteQuery there is no next page, so the
+  // query method should not even run. If there are no observations in the
+  // results, we're done and can stop requesting new pages.
+  if ( !lastObs ) return null;
+
+  // Datetime sorts need to use d1 / d2 or created_d1 / created_d2 to
+  // paginate results, so were storing a datetime from the last observation
+  // as the "page" and we'll use that to adjust the query when we perform
+  // it
+  if ( ["observed_on", "created_at"].includes( String( orderBy ) ) ) {
+    const lastObsDate = orderBy === "observed_on"
+      ? lastObs?.time_observed_at
+      : lastObs?.created_at;
+
+    // If there are results but the last one doesn't have a datetime, we're
+    // also done... but this is probably impossible.
+    if ( !lastObsDate ) {
+      return null;
+    }
+
+    const lastObsDateParsed = parseISO( lastObsDate );
+    // Adding / subtracting a second helps us not include this last
+    // observation on the next page of results
+    const newObsDate = addSeconds( lastObsDateParsed, params.order === "asc"
+      ? 1
+      : -1 );
+    return formatISO( newObsDate );
+  }
+
+  // Any sort that isn't observed_on or created_at and isn't the default
+  // (blank, meaning created_at) should use basic pagination.
+  if ( typeof ( orderBy ) === "string" ) {
+    return lastPage.page + 1;
+  }
+
+  // If we got here that means orderBy is undefined or null, i.e. the
+  // default sort order by id
+  return lastObs?.id;
+}
+
+// Actually modifieds the Explore API params before the request to include the
+// page params
+function addPageParamsForExplore( params: ApiObservationsSearchParamsForInfiniteQuery ) {
+  const { pageParam } = params;
+  const newParams = { ...params };
+
+  // For the purposes of Explore, an undefined page or a page of 0 is the
+  // default state before any data has loaded, so those values mean we're
+  // requesting the first page
+  const requestingFirstPage = typeof ( pageParam ) === "undefined" || pageParam === 0;
+  if ( requestingFirstPage ) {
+    // For the first page and only for the first page, we need to retrieve
+    // the georaphic bounds of the results so we can pan and zoom the map
+    // to contain them
+    newParams.return_bounds = true;
+  } else if ( newParams.order_by === "observed_on" && typeof ( pageParam ) === "string" ) {
+    // If we're ordering by date observed, we are "paginating" by date and
+    // getNextPageParam will have set the pageParam to a date string from
+    // the last obs in the previous page
+    if ( params.order === "asc" ) {
+      newParams.d1 = pageParam;
+    } else {
+      newParams.d2 = pageParam;
+    }
+  } else if ( newParams.order_by === "created_at" && typeof ( pageParam ) === "string" ) {
+    // If we're ordering by date created, we are "paginating" by date and
+    // getNextPageParam will have set the pageParam to a date string from
+    // the last obs in the previous page
+    if ( params.order === "asc" ) {
+      newParams.created_d1 = pageParam;
+    } else {
+      newParams.created_d2 = pageParam;
+    }
+  } else if ( typeof ( newParams.order_by ) === "string" && typeof ( pageParam ) === "number" ) {
+    // If this is any kind of sort other than the date sorts and isn't the
+    // default (blank), assume basic pagination using the page param
+    newParams.page = pageParam;
+  } else {
+    // If we're using default sort order by id, getNextPageParam will have
+    // set pageParam to a serial id
+    newParams.id_below = Number( pageParam );
+  }
+  return newParams;
+}
+
+export {
+  addPageParamsForExplore,
+  getNextPageParamForExplore
+};

src/components/Explore/hooks/useInfiniteExploreScroll.js

@@ -2,57 +2,40 @@
 
 import { useQueryClient } from "@tanstack/react-query";
 import { searchObservations } from "api/observations";
-import { addSeconds, formatISO, parseISO } from "date-fns";
-import { flatten, last } from "lodash";
-import { useCallback } from "react";
+import { flatten } from "lodash";
+import { useCallback, useMemo } from "react";
 import Observation from "realmModels/Observation";
 import { useAuthenticatedInfiniteQuery } from "sharedHooks";
 
+import {
+  addPageParamsForExplore,
+  getNextPageParamForExplore
+} from "../helpers/exploreParams";
+
 const useInfiniteExploreScroll = ( { params: newInputParams, enabled }: Object ): Object => {
   const queryClient = useQueryClient( );
 
-  const fields = {
-    ...Observation.EXPLORE_LIST_FIELDS,
-    user: { // included here for "exclude by current user" in explore filters
-      id: true,
-      uuid: true,
-      login: true
-    }
-  };
-  const baseParams = {
+  const baseParams = useMemo( () => ( {
     ...newInputParams,
-    fields,
+    fields: {
+      ...Observation.EXPLORE_LIST_FIELDS,
+      user: { // included here for "exclude by current user" in explore filters
+        id: true,
+        uuid: true,
+        login: true
+      }
+    },
     ttl: -1
-  };
+  } ), [newInputParams] );
 
   const excludedUser = newInputParams.excludeUser;
 
   const queryKey = ["useInfiniteExploreScroll", newInputParams];
 
-  const getNextPageParam = useCallback( lastPage => {
-    const lastObs = last( lastPage.results );
-    const orderBy = baseParams.order_by;
-
-    if ( !lastObs ) return null;
-
-    if ( ["observed_on", "created_at"].includes( orderBy ) ) {
-      const lastObsDate = orderBy === "observed_on"
-        ? lastObs?.time_observed_at
-        : lastObs?.created_at;
-
-      if ( !lastObsDate ) {
-        return null;
-      }
-
-      const lastObsDateParsed = parseISO( lastObsDate );
-      const newObsDate = addSeconds( lastObsDateParsed, baseParams.order === "asc"
-        ? 1
-        : -1 );
-      return formatISO( newObsDate );
-    }
-
-    return lastObs?.id;
-  }, [baseParams.order_by, baseParams.order] );
+  const getNextPageParam = useCallback(
+    lastPage => getNextPageParamForExplore( lastPage, baseParams ),
+    [baseParams]
+  );
 
   const {
     data,
@@ -64,38 +47,10 @@ const useInfiniteExploreScroll = ( { params: newInputParams, enabled }: Object )
     status
   } = useAuthenticatedInfiniteQuery(
     queryKey,
-    async ( { pageParam }, optsWithAuth ) => {
-      const params = {
-        ...baseParams
-      };
-
-      if ( pageParam ) {
-        if ( params.order_by === "observed_on" ) {
-          if ( baseParams.order === "asc" ) {
-            params.d1 = pageParam;
-          } else {
-            params.d2 = pageParam;
-          }
-        } else if ( params.order_by === "created_at" ) {
-          if ( baseParams.order === "asc" ) {
-            params.created_d1 = pageParam;
-          } else {
-            params.created_d2 = pageParam;
-          }
-        } else {
-          // $FlowIgnore
-          params.id_below = pageParam;
-        }
-      } else {
-        // $FlowIgnore
-        params.page = 1;
-        // For the first page and only for the first page, get the bounds as
-        // well
-        params.return_bounds = true;
-      }
-      const response = await searchObservations( params, optsWithAuth );
-      return response;
-    },
+    async ( params, optsWithAuth ) => searchObservations(
+      addPageParamsForExplore( { ...baseParams, ...params } ),
+      optsWithAuth
+    ),
     {
       getNextPageParam,
       enabled

src/sharedHooks/useAuthenticatedInfiniteQuery.ts

@@ -24,7 +24,6 @@ const useAuthenticatedInfiniteQuery = (
   retry: ( failureCount, error ) => reactQueryRetry( failureCount, error, {
     queryKey
   } ),
-  initialPageParam: 1,
   ...queryOptions
 } );
 

tests/factories/RemoteObservation.js

@@ -2,16 +2,28 @@ import { define } from "factoria";
 
 import userFactory from "./RemoteUser";
 
-export default define( "RemoteObservation", faker => ( {
-  id: faker.number.int( ),
-  uuid: faker.string.uuid( ),
-  geojson: {
-    coordinates: [
-      Number( faker.location.longitude( ) ),
-      Number( faker.location.latitude( ) )
-    ]
-  },
-  positional_accuracy: 10,
-  observed_on_string: "2020-04-03",
-  user: userFactory( "RemoteUser" )
-} ) );
+export default define( "RemoteObservation", faker => {
+  const dateObserved = faker.date.past( );
+  const dateCreated = faker.date.between( { from: dateObserved, to: new Date() } );
+  const dateUpdated = faker.date.between( { from: dateCreated, to: new Date() } );
+  return {
+    created_at: dateCreated.toISOString(),
+    id: faker.number.int( ),
+    uuid: faker.string.uuid( ),
+    geojson: {
+      coordinates: [
+        Number( faker.location.longitude( ) ),
+        Number( faker.location.latitude( ) )
+      ]
+    },
+    positional_accuracy: 10,
+    observed_on_string: [
+      dateObserved.getFullYear(),
+      `0${dateObserved.getMonth() + 1}`.slice( -2 ),
+      `0${dateObserved.getDate()}`.slice( -2 )
+    ].join( "-" ),
+    time_observed_at: dateObserved.toISOString(),
+    update_at: dateUpdated.toISOString(),
+    user: userFactory( "RemoteUser" )
+  };
+} );

tests/unit/components/Explore/helpers/exploreParams.test.js

@@ -0,0 +1,69 @@
+import {
+  addPageParamsForExplore,
+  getNextPageParamForExplore
+} from "components/Explore/helpers/exploreParams.ts";
+import factory, { makeResponse } from "tests/factory";
+import faker from "tests/helpers/faker";
+
+describe( "getNextPageParamForExplore", ( ) => {
+  const page = makeResponse( [factory( "RemoteObservation" )] );
+  it( "should return a date string if order_by is observed_on", ( ) => {
+    const params = { order_by: "observed_on" };
+    expect( getNextPageParamForExplore( page, params ) ).toMatch( /\d{4}-\d{2}-\d{2}/ );
+  } );
+  it( "should return a date string if order_by is created_at", ( ) => {
+    const params = { order_by: "created_at" };
+    expect( getNextPageParamForExplore( page, params ) ).toMatch( /\d{4}-\d{2}-\d{2}/ );
+  } );
+  it( "should return an obs id if order_by is blank", ( ) => {
+    const params = { };
+    expect( getNextPageParamForExplore( page, params ) ).toEqual( page.results[0].id );
+  } );
+  it( "should return the next page number if order_by is votes", ( ) => {
+    const params = { order_by: "votes" };
+    expect( getNextPageParamForExplore( page, params ) ).toEqual( page.page + 1 );
+  } );
+} );
+
+describe( "addPageParamsForExplore", ( ) => {
+  it( "should include return_bounds when pageParam is undefined", ( ) => {
+    const params = {};
+    expect( addPageParamsForExplore( params ).return_bounds ).toEqual( true );
+  } );
+  it( "should include return_bounds when pageParam is 0", ( ) => {
+    const params = { pageParam: 0 };
+    expect( addPageParamsForExplore( params ).return_bounds ).toEqual( true );
+  } );
+  it( "should not include return_bounds when pageParam is 1 ", ( ) => {
+    const params = { pageParam: 1 };
+    expect( addPageParamsForExplore( params ).return_bounds ).toBeUndefined( );
+  } );
+  it( "should set d2 if order_by is observed_on and order is default desc", ( ) => {
+    const dateString = faker.date.recent( ).toISOString( );
+    const params = { pageParam: dateString, order_by: "observed_on" };
+    expect( addPageParamsForExplore( params ).d2 ).toEqual( dateString );
+  } );
+  it( "should set d1 if order_by is observed_on and order is asc", ( ) => {
+    const dateString = faker.date.recent( ).toISOString( );
+    const params = { pageParam: dateString, order_by: "observed_on", order: "asc" };
+    expect( addPageParamsForExplore( params ).d1 ).toEqual( dateString );
+  } );
+  it( "should set created_d2 if order_by is created_at and order is default desc", ( ) => {
+    const dateString = faker.date.recent( ).toISOString( );
+    const params = { pageParam: dateString, order_by: "created_at" };
+    expect( addPageParamsForExplore( params ).created_d2 ).toEqual( dateString );
+  } );
+  it( "should set created_d1 if order_by is created_at and order is asc", ( ) => {
+    const dateString = faker.date.recent( ).toISOString( );
+    const params = { pageParam: dateString, order_by: "created_at", order: "asc" };
+    expect( addPageParamsForExplore( params ).created_d1 ).toEqual( dateString );
+  } );
+  it( "should set page if order_by is votes", ( ) => {
+    const params = { order_by: "votes", pageParam: 1 };
+    expect( addPageParamsForExplore( params ).page ).toEqual( params.pageParam );
+  } );
+  it( "should set id_below if order_by is not specified", ( ) => {
+    const params = { pageParam: 123 };
+    expect( addPageParamsForExplore( params ).id_below ).toEqual( params.pageParam );
+  } );
+} );