Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2926: Fallback language in search function #3024

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

2926: Fallback language in search function #3024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
04f8710
2926: Add a fallback language in web
LeandraH Nov 27, 2024
f4cd441
2926: Refactoring
LeandraH Nov 27, 2024
78170ac
2926: More refactoring
LeandraH Nov 27, 2024
d111ff7
2926: Even more refactoring
LeandraH Nov 27, 2024
eb6cf41
2926: Add a fallback language in native
LeandraH Nov 27, 2024
0245b3f
2926: Fix tests
LeandraH Dec 2, 2024
0696cb9
2926: Cleaning up
LeandraH Dec 11, 2024
5b6bf61
Merge branch 'main' into 2926-fallback-language-in-search
LeandraH Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion native/src/routes/SearchModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const SearchCounter = styled.Text`

export type SearchModalProps = {
allPossibleResults: SearchResult[]
allPossibleFallbackResults: SearchResult[]
languageCode: string
cityCode: string
closeModal: (query: string) => void
Expand All @@ -37,6 +38,7 @@ export type SearchModalProps = {

const SearchModal = ({
allPossibleResults,
allPossibleFallbackResults,
languageCode,
cityCode,
closeModal,
Expand All @@ -46,7 +48,10 @@ const SearchModal = ({
const resourceCache = useResourceCache({ cityCode, languageCode })
const { t } = useTranslation('search')

const searchResults = useSearch(allPossibleResults, query)
const mainResults = useSearch(allPossibleResults, query)
const fallbackResults = useSearch(allPossibleFallbackResults, query)

const searchResults = mainResults?.length === 0 ? fallbackResults : mainResults
Comment on lines +51 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Hmmm.... I think minisearch often returns search results even if there are no direct matches in the content. What is the reason to not always returning both (main results first, fallback concatenated afterwards)?


if (!searchResults) {
return null
Expand Down
24 changes: 17 additions & 7 deletions native/src/routes/SearchModalContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import React, { ReactElement, useMemo } from 'react'

Check notice on line 1 in native/src/routes/SearchModalContainer.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ No longer an issue: Complex Method 

SearchModalContainer is no longer above the threshold for cyclomatic complexity

import { SearchRouteType } from 'shared'
import { CategoriesMapModel, EventModel, PoiModel } from 'shared/api'
import { formatPossibleSearchResults } from 'shared/hooks/useSearch'
import { config } from 'translations'

import { NavigationProps, RouteProps } from '../constants/NavigationTypes'
import useCityAppContext from '../hooks/useCityAppContext'
Expand All @@ -13,18 +16,24 @@
route: RouteProps<SearchRouteType>
}

const useMemoizeResults = (
categories?: CategoriesMapModel | null,
events?: EventModel[] | null,
pois?: PoiModel[] | null,
) => useMemo(() => formatPossibleSearchResults(categories, events, pois), [categories, events, pois])
Comment on lines +19 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Do we really have to memoize this? Doesn't really look calculation intense at all 🙈


const SearchModalContainer = ({ navigation, route }: SearchModalContainerProps): ReactElement | null => {
const { cityCode, languageCode } = useCityAppContext()
const initialSearchText = route.params.searchText ?? ''
const { data, ...response } = useLoadCityContent({ cityCode, languageCode })
const { data: fallbackData } = useLoadCityContent({ cityCode, languageCode: config.sourceLanguage })

const allPossibleResults = useMemoizeResults(data?.categories, data?.events, data?.pois)

const allPossibleResults = useMemo(
() => [
...(data?.categories.toArray().filter(category => !category.isRoot()) || []),
...(data?.events || []),
...(data?.pois || []),
],
[data?.categories, data?.events, data?.pois],
const allPossibleFallbackResults = useMemoizeResults(
fallbackData?.categories,
fallbackData?.events,
fallbackData?.pois,
)

return (
Expand All @@ -34,6 +43,7 @@
cityCode={cityCode}
closeModal={navigation.goBack}
allPossibleResults={allPossibleResults}
allPossibleFallbackResults={allPossibleFallbackResults}
languageCode={languageCode}
initialSearchText={initialSearchText}
/>
Expand Down
1 change: 1 addition & 0 deletions native/src/routes/__tests__/SearchModal.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ describe('SearchModal', () => {

const props: SearchModalProps = {
allPossibleResults,
allPossibleFallbackResults: [],
languageCode,
cityCode,
closeModal: dummy,
Expand Down
13 changes: 13 additions & 0 deletions shared/hooks/useSearch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,23 @@ import MiniSearch from 'minisearch'
import { useCallback } from 'react'

import useLoadAsync from '../api/endpoints/hooks/useLoadAsync'
import CategoriesMapModel from '../api/models/CategoriesMapModel'
import EventModel from '../api/models/EventModel'
import ExtendedPageModel from '../api/models/ExtendedPageModel'
import PoiModel from '../api/models/PoiModel'

export type SearchResult = ExtendedPageModel

export const formatPossibleSearchResults = (
categories?: CategoriesMapModel | null,
events?: EventModel[] | null,
pois?: PoiModel[] | null,
): SearchResult[] => [
...(categories?.toArray().filter(category => !category.isRoot()) || []),
...(events || []),
...(pois || []),
]

const useSearch = (allPossibleResults: SearchResult[], query: string): SearchResult[] | null => {
const initializeMiniSearch = useCallback(async () => {
const search = new MiniSearch({
Expand Down
3 changes: 0 additions & 3 deletions web/src/hooks/__tests__/useAllPossibleSearchResults.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,8 @@ describe('useAllPossibleSearchResults', () => {

const city = new CityModelBuilder(1).build()[0]!.code
const language = new LanguageModelBuilder(1).build()[0]!.code

const categories = new CategoriesMapModelBuilder(city, language).build()

const events = new EventModelBuilder('seed', 2, city, language).build()

const locations = new PoiModelBuilder(3).build()

it('should return the correct results', () => {
Expand Down
15 changes: 8 additions & 7 deletions web/src/hooks/useAllPossibleSearchResults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
ExtendedPageModel,
useLoadFromEndpoint,
} from 'shared/api'
import { formatPossibleSearchResults } from 'shared/hooks/useSearch'

type UseAllPossibleSearchResultsProps = {
city: string
Expand All @@ -27,19 +28,19 @@
}: UseAllPossibleSearchResultsProps): UseAllPossibleSearchResultsReturn => {
const params = { city, language }

const { data: categories, ...categoriesReturn } = useLoadFromEndpoint(createCategoriesEndpoint, cmsApiBaseUrl, params)
const { data: events, ...eventsReturn } = useLoadFromEndpoint(createEventsEndpoint, cmsApiBaseUrl, params)
const { data: pois, ...poisReturn } = useLoadFromEndpoint(createPOIsEndpoint, cmsApiBaseUrl, params)
const categories = useLoadFromEndpoint(createCategoriesEndpoint, cmsApiBaseUrl, params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 In theory we could perhaps add a parameter include root set to true as default or something to the categories endpoint so we don't have to filter it out after again?

Probably only really works well for web though, so not sure if its a good idea.

const events = useLoadFromEndpoint(createEventsEndpoint, cmsApiBaseUrl, params)
const pois = useLoadFromEndpoint(createPOIsEndpoint, cmsApiBaseUrl, params)

const allPossibleResults = useMemo(
() => [...(categories?.toArray().filter(category => !category.isRoot()) || []), ...(events || []), ...(pois || [])],
[categories, events, pois],
() => formatPossibleSearchResults(categories.data, events.data, pois.data),
[categories.data, events.data, pois.data],
)
Comment on lines 35 to 38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 Same as for native


return {
data: allPossibleResults,
loading: categoriesReturn.loading || eventsReturn.loading || poisReturn.loading,
error: categoriesReturn.error || eventsReturn.error || poisReturn.error,
loading: categories.loading || events.loading || pois.loading,
error: categories.error || events.error || pois.error,

Check notice on line 43 in web/src/hooks/useAllPossibleSearchResults.ts

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ No longer an issue: Complex Method 

useAllPossibleSearchResults is no longer above the threshold for cyclomatic complexity
}
}

Expand Down
11 changes: 10 additions & 1 deletion web/src/routes/SearchPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useLocation, useNavigate } from 'react-router-dom'
import styled from 'styled-components'

import { parseHTML, pathnameFromRouteInformation, SEARCH_ROUTE, useSearch } from 'shared'
import { config } from 'translations'

import { CityRouteProps } from '../CityContentSwitcher'
import CityContentLayout, { CityContentLayoutProps } from '../components/CityContentLayout'
Expand Down Expand Up @@ -45,8 +46,16 @@ const SearchPage = ({ city, cityCode, languageCode, pathname }: CityRouteProps):
language: languageCode,
cmsApiBaseUrl,
})
const mainResults = useSearch(allPossibleResults, query)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 I think something like contentResults or contentLanguageResults would be a little more speaking and explaining what is happening. Accordingly, fallbackLanguageResults might be better.


const results = useSearch(allPossibleResults, query)
const { data: allPossibleFallbackResults } = useAllPossibleSearchResults({
city: cityCode,
language: config.sourceLanguage,
cmsApiBaseUrl,
})
const fallbackResults = useSearch(allPossibleFallbackResults, query)

const results = mainResults?.length === 0 ? fallbackResults : mainResults
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 See native above


if (!city) {
return null
Expand Down
Loading