Skip to content

Commit

Permalink
Add changes from the code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@obulat
obulat committed Oct 23, 2023
1 parent 2ff0311 commit e7a091e
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 56 deletions.
2 changes: 1 addition & 1 deletion frontend/src/composables/use-external-sources.ts
View file
Expand Up @@ -31,7 +31,7 @@ export const useExternalSources = () => {
return IMAGE
})
const externalSources = computed(() => {
const query = searchStore.searchQueryParams
const query = searchStore.apiSearchQueryParams
const type = externalSourcesType.value
return getAdditionalSources(type, query)
})
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/pages/search.vue
View file
Expand Up @@ -101,7 +101,7 @@ export default defineComponent({
const {
searchTerm,
searchType,
searchQueryParams: query,
apiSearchQueryParams: query,
searchTypeIsSupported: supported,
} = storeToRefs(searchStore)
Expand Down
19 changes: 7 additions & 12 deletions frontend/src/stores/media/index.ts
View file
Expand Up @@ -443,18 +443,13 @@ export const useMediaStore = defineStore("media", {
mediaType: SupportedMediaType
shouldPersistMedia: boolean
}) {
const queryParams = useSearchStore()
.searchQueryParams as PaginatedSearchQuery
let page = 1
if (shouldPersistMedia) {
/**
* If `shouldPersistMedia` is true, then we increment the page that was set by a previous
* fetch. Normally, if `shouldPersistMedia` is true, `page` should have been set to 1 by the
* previous fetch.
*/
page = this.results[mediaType].page + 1
queryParams.page = `${page}`
let page = this.results[mediaType].page + 1
const queryParams: PaginatedSearchQuery = {
...useSearchStore().apiSearchQueryParams,
// Don't need to set `page` parameter for the first page.
page: shouldPersistMedia ? `${page}` : undefined,
}

this._updateFetchState(mediaType, "start")
try {
const accessToken = this.$nuxt.$openverseApiToken
Expand All @@ -467,7 +462,7 @@ export const useMediaStore = defineStore("media", {
* In such cases, we show the "No results" client error page.
*/
if (!mediaCount) {
page = 0
page = 1
errorData = {
message: `No results found for ${queryParams.q}`,
code: NO_RESULT,
Expand Down
55 changes: 22 additions & 33 deletions frontend/src/stores/search.ts
View file
Expand Up @@ -64,6 +64,7 @@ export interface SearchState {
* Builds the search query parameters for the given search type, filters, and search term.
* `q` parameter is always included as the first query parameter.
* If the search type is not supported, only the `q` parameter is included.
* This is used, for instance, for content switcher links for `video`/`model_3d` search pages.
* Only the filters that are relevant for the search type and have a value are included.
*
* `INCLUDE_SENSITIVE_QUERY_PARAM` is never added to the frontend search query. It is added
Expand All @@ -75,12 +76,13 @@ export function computeQueryParams(
searchTerm: string,
mode: "frontend" | "API"
) {
const q = searchTerm.trim()
if (!isSearchTypeSupported(searchType)) {
return { q: searchTerm.trim() }
return { q }
}
// Ensure that `q` always comes first in the frontend URL.
const search_query: SearchQuery = {
q: searchTerm.trim(),
const searchQuery: SearchQuery = {
q,
// The filters object is converted to a Record<string, string> object.
// e.g., { licenseTypes: [{ code: "commercial", checked: true }] }
// => { license_type: "commercial" }
Expand All @@ -90,10 +92,10 @@ export function computeQueryParams(
// `INCLUDE_SENSITIVE_QUERY_PARAM` is used in the API params, but not shown on the frontend.
const ffStore = useFeatureFlagStore()
if (mode === "API" && ffStore.isOn("fetch_sensitive")) {
search_query[INCLUDE_SENSITIVE_QUERY_PARAM] = "true"
searchQuery[INCLUDE_SENSITIVE_QUERY_PARAM] = "true"
}

return search_query
return searchQuery
}

export const useSearchStore = defineStore("search", {
Expand All @@ -116,8 +118,10 @@ export const useSearchStore = defineStore("search", {

/**
* Returns the search query parameters for API request.
* The main difference between api and frontend query parameters is that
* the API query parameters include the `include_sensitive_results` parameter.
*/
searchQueryParams(state) {
apiSearchQueryParams(state) {
return computeQueryParams(
state.searchType,
state.filters,
Expand All @@ -126,19 +130,6 @@ export const useSearchStore = defineStore("search", {
)
},

/**
* Returns the frontend search query parameters for the current search state.
* If the search type is not supported, returns only the `q` parameter.
*/
frontendSearchUrlParams(state) {
return computeQueryParams(
state.searchType,
state.filters,
state.searchTerm,
"frontend"
)
},

/**
* Returns the number of checked filters.
*/
Expand Down Expand Up @@ -207,30 +198,28 @@ export const useSearchStore = defineStore("search", {
*
* If search type is not provided, returns the path for the current search type.
* If query is not provided, returns current query parameters.
* If only the search type is provided, the query is computed for this search type.
*/
getSearchPath({
type,
query,
}: { type?: SearchType; query?: PaginatedSearchQuery } = {}): string {
const searchType = type || this.searchType
let queryParams = query
if (queryParams === undefined) {
queryParams =
searchType === undefined
? this.frontendSearchUrlParams
: computeQueryParams(
searchType,
this.filters,
this.searchTerm,
"frontend"
)
}
const searchType = type ?? this.searchType
const queryParams =
query ??
computeQueryParams(
searchType,
this.filters,
this.searchTerm,
"frontend"
)

return this.$nuxt.localePath({
path: searchPath(searchType),
query: queryParams as unknown as Dictionary<string>,
})
},

setSearchType(type: SearchType) {
const featureFlagStore = useFeatureFlagStore()
if (
Expand Down Expand Up @@ -483,7 +472,7 @@ export const useSearchStore = defineStore("search", {
isFilterDisabled(
item: FilterItem,
filterCategory: FilterCategory
): boolean | undefined {
): boolean {
if (!["licenseTypes", "licenses"].includes(filterCategory)) {
return false
}
Expand Down
20 changes: 11 additions & 9 deletions frontend/test/unit/specs/stores/search-store.spec.js
View file
Expand Up @@ -150,7 +150,7 @@ describe("Search Store", () => {
${"off"} | ${{ q: "cat", extension: "svg" }} | ${{ q: "cat" }} | ${AUDIO}
${"off"} | ${{ q: ["cat", "dog"], license: ["by", "cc0"] }} | ${{ q: "cat", license: "by" }} | ${IMAGE}
`(
"returns correct searchQueryParams and filter status for $query and searchType $searchType",
"returns correct apiSearchQueryParams and filter status for $query and searchType $searchType",
({ sensitivityFlag, query, expectedQueryParams, searchType }) => {
const featureFlagStore = useFeatureFlagStore()
featureFlagStore.toggleFeature("fetch_sensitive", sensitivityFlag)
Expand All @@ -162,7 +162,7 @@ describe("Search Store", () => {
urlQuery: query,
})

expect(searchStore.searchQueryParams).toEqual(expectedQueryParams)
expect(searchStore.apiSearchQueryParams).toEqual(expectedQueryParams)
}
)
})
Expand Down Expand Up @@ -234,7 +234,7 @@ describe("Search Store", () => {
const featureFlagStore = useFeatureFlagStore()
featureFlagStore.toggleFeature("fetch_sensitive", sensitivityFlag)
const searchStore = useSearchStore()
const expectedQuery = { ...searchStore.searchQueryParams, ...query }
const expectedQuery = { ...searchStore.apiSearchQueryParams, ...query }
// The values that are not applicable for the search type should be discarded
if (searchType === IMAGE) {
delete expectedQuery.length
Expand All @@ -243,7 +243,7 @@ describe("Search Store", () => {
searchStore.setSearchStateFromUrl({ path: path, urlQuery: query })

expect(searchStore.searchType).toEqual(searchType)
expect(searchStore.searchQueryParams).toEqual(expectedQuery)
expect(searchStore.apiSearchQueryParams).toEqual(expectedQuery)
}
)

Expand All @@ -252,7 +252,7 @@ describe("Search Store", () => {
searchStore.updateSearchPath({ type: "audio", searchTerm: "cat" })

expect(searchStore.searchType).toEqual("audio")
expect(searchStore.searchQueryParams).toEqual({ q: "cat" })
expect(searchStore.apiSearchQueryParams).toEqual({ q: "cat" })
expect(searchStore.$nuxt.localePath).toHaveBeenCalledWith({
path: "/search/audio",
query: { q: "cat" },
Expand All @@ -266,7 +266,7 @@ describe("Search Store", () => {
searchStore.updateSearchPath()

expect(searchStore.searchType).toEqual("audio")
expect(searchStore.searchQueryParams).toEqual({ q: "cat" })
expect(searchStore.apiSearchQueryParams).toEqual({ q: "cat" })
expect(searchStore.$nuxt.localePath).toHaveBeenCalledWith({
path: "/search/audio",
query: { q: "cat" },
Expand All @@ -286,7 +286,7 @@ describe("Search Store", () => {
const [filterType, code] = filterItem
searchStore.toggleFilter({ filterType, code })
}
expect(searchStore.searchQueryParams[query[0]]).toEqual(query[1])
expect(searchStore.apiSearchQueryParams[query[0]]).toEqual(query[1])
}
)

Expand All @@ -306,10 +306,12 @@ describe("Search Store", () => {
})
if (supportedSearchTypes.includes(searchType)) {
// eslint-disable-next-line jest/no-conditional-expect
expect(searchStore.query).not.toEqual(expectedQueryParams)
expect(searchStore.apiSearchQueryParams).not.toEqual(
expectedQueryParams
)
}
searchStore.clearFilters()
expect(searchStore.searchQueryParams).toEqual(expectedQueryParams)
expect(searchStore.apiSearchQueryParams).toEqual(expectedQueryParams)
}
)

Expand Down

0 comments on commit e7a091e

Please sign in to comment.