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.

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

fix: do not fetch assets if they are in the store #1734

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions webapp/src/components/AssetBrowse/AssetBrowse.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { ReactNode, useEffect, useState } from 'react'
import { matchPath } from 'react-router-dom'
import { matchPath, useHistory, useLocation } from 'react-router-dom'
import classNames from 'classnames'
import { Container, Mobile, NotMobile, Page, Tabs } from 'decentraland-ui'
import { t } from 'decentraland-dapps/dist/modules/translation/utils'
Expand Down Expand Up @@ -53,8 +53,15 @@ const AssetBrowse = (props: Props) => {
isMapViewFiltersEnabled
} = props

const location = useLocation()
const history = useHistory()
// Prevent fetching more than once while browsing
const [hasFetched, setHasFetched] = useState(false)
const lastLocation = visitedLocations[visitedLocations.length - 2]
const [hasFetched, setHasFetched] = useState(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasFetched is a mechanism that we have nowadays to fetch the data just once. If we're going back to the /browse and the data is in the store, we don't need to re-fetch (it should be true)

history.action === 'POP' &&
lastLocation?.pathname === location.pathname &&
lastLocation?.search === location.search
)
const isCurrentAccount = view === View.CURRENT_ACCOUNT
const isAccountOrCurrentAccount = view === View.ACCOUNT || isCurrentAccount
const [showOwnedLandOnMap, setShowOwnedLandOnMap] = useState(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
isFetchingCollection,
getError as getCollectionError
} from '../../modules/collection/selectors'
import { fetchItemsRequest } from '../../modules/item/actions'
import { fetchCollectionItemsRequest } from '../../modules/item/actions'
import {
getItemsByContractAddress,
isFetchingItemsOfCollection
Expand Down Expand Up @@ -46,11 +46,9 @@ const mapDispatch = (
dispatch(fetchSingleCollectionRequest(contractAddress)),
onFetchCollectionItems: (collection: Collection) =>
dispatch(
fetchItemsRequest({
filters: {
first: collection.size,
contractAddresses: [collection.contractAddress]
}
fetchCollectionItemsRequest({
first: collection.size,
contractAddresses: [collection.contractAddress]
})
)
})
Expand Down
14 changes: 8 additions & 6 deletions webapp/src/modules/collection/sagas.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { select } from '@redux-saga/core/effects'
import { expectSaga } from 'redux-saga-test-plan'
import { call } from 'redux-saga-test-plan/matchers'
import { fetchItemsRequest } from '../item/actions'
import { fetchCollectionItemsRequest } from '../item/actions'
import { getItemsByContractAddress } from '../item/selectors'
import { collectionAPI } from '../vendor/decentraland'
import {
Expand Down Expand Up @@ -52,7 +52,7 @@ describe('when handling a fetch collections request', () => {
})
})
describe('when should fetch items argument is true', () => {
it('should put a fetch items request action for each collection', () => {
it('should put a fetch collection items request action for each collection', () => {
const filters = {}
const contractAddress1 = 'contract address 1'
const contractAddress2 = 'contract address 2'
Expand Down Expand Up @@ -94,14 +94,16 @@ describe('when handling a fetch collections request', () => {
.put(fetchCollectionsSuccess(collections as any, 100))
// Fetches items for contract address 2 because sizes are different
.put(
fetchItemsRequest({
filters: { contractAddresses: [contractAddress2], first: size }
fetchCollectionItemsRequest({
contractAddresses: [contractAddress2],
first: size
})
)
// Fetches items for contract address 3 because there are no items for that collection stored
.put(
fetchItemsRequest({
filters: { contractAddresses: [contractAddress3], first: size }
fetchCollectionItemsRequest({
contractAddresses: [contractAddress3],
first: size
})
)
.dispatch(fetchCollectionsRequest(filters, true))
Expand Down
18 changes: 7 additions & 11 deletions webapp/src/modules/collection/sagas.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { call, takeEvery, put, select } from '@redux-saga/core/effects'
import { t } from 'decentraland-dapps/dist/modules/translation/utils'
import { isErrorWithMessage } from '../../lib/error'
import { fetchItemsRequest } from '../item/actions'
import { fetchCollectionItemsRequest } from '../item/actions'
import { getItemsByContractAddress } from '../item/selectors'
import { collectionAPI } from '../vendor/decentraland'
import { CollectionResponse } from '../vendor/decentraland/collection/types'
Expand Down Expand Up @@ -47,11 +47,9 @@ export function* handleFetchCollectionsRequest(

if (!items || items.length !== collection.size) {
yield put(
fetchItemsRequest({
filters: {
first: collection.size,
contractAddresses: [collection.contractAddress]
}
fetchCollectionItemsRequest({
first: collection.size,
contractAddresses: [collection.contractAddress]
})
)
}
Expand Down Expand Up @@ -97,11 +95,9 @@ export function* handleFetchSingleCollectionRequest(

if (!items || items.length !== collection.size) {
yield put(
fetchItemsRequest({
filters: {
first: collection.size,
contractAddresses: [collection.contractAddress]
}
fetchCollectionItemsRequest({
first: collection.size,
contractAddresses: [collection.contractAddress]
})
)
}
Expand Down
41 changes: 40 additions & 1 deletion webapp/src/modules/item/actions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@ import {
FETCH_ITEMS_FAILURE,
FETCH_ITEMS_REQUEST,
FETCH_ITEMS_SUCCESS,
FETCH_TRENDING_ITEMS_REQUEST
FETCH_TRENDING_ITEMS_REQUEST,
fetchCollectionItemsRequest,
FETCH_COLLECTION_ITEMS_REQUEST,
fetchCollectionItemsSuccess,
FETCH_COLLECTION_ITEMS_SUCCESS,
fetchCollectionItemsFailure,
FETCH_COLLECTION_ITEMS_FAILURE
} from './actions'

const itemBrowseOptions = {
Expand Down Expand Up @@ -200,3 +206,36 @@ describe('when creating the action to fetch the trending items request', () => {
})
})
})

describe('when creating the action to signal the start of the collection items request', () => {
let first: number
let contractAddresses: string[]
it('should return an object representing the action', () => {
expect(fetchCollectionItemsRequest({ first, contractAddresses })).toEqual({
type: FETCH_COLLECTION_ITEMS_REQUEST,
meta: undefined,
payload: { first, contractAddresses }
})
})
})

describe('when creating the action to signal a success in the collection items request', () => {
const items = [{} as Item]
it('should return an object representing the action', () => {
expect(fetchCollectionItemsSuccess(items)).toEqual({
type: FETCH_COLLECTION_ITEMS_SUCCESS,
meta: undefined,
payload: { items }
})
})
})

describe('when creating the action to signal a failure in the collection items request', () => {
it('should return an object representing the action', () => {
expect(fetchCollectionItemsFailure(anErrorMessage)).toEqual({
type: FETCH_COLLECTION_ITEMS_FAILURE,
meta: undefined,
payload: { error: anErrorMessage }
})
})
})
28 changes: 27 additions & 1 deletion webapp/src/modules/item/actions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ChainId, Item } from '@dcl/schemas'
import { ChainId, Item, ItemFilters } from '@dcl/schemas'
import { NFTPurchase } from 'decentraland-dapps/dist/modules/gateway/types'
import {
buildTransactionPayload,
Expand Down Expand Up @@ -57,6 +57,32 @@ export type FetchTrendingItemsFailureAction = ReturnType<
typeof fetchTrendingItemsFailure
>

// Fetch Collection Items

export const FETCH_COLLECTION_ITEMS_REQUEST = '[Request] Fetch Collection Items'
export const FETCH_COLLECTION_ITEMS_SUCCESS = '[Success] Fetch Collection Items'
export const FETCH_COLLECTION_ITEMS_FAILURE = '[Failure] Fetch Collection Items'

export const fetchCollectionItemsRequest = (
options: Pick<ItemFilters, 'first' | 'contractAddresses'>
) => action(FETCH_COLLECTION_ITEMS_REQUEST, options)

export const fetchCollectionItemsSuccess = (items: Item[]) =>
action(FETCH_COLLECTION_ITEMS_SUCCESS, { items })

export const fetchCollectionItemsFailure = (error: string) =>
action(FETCH_COLLECTION_ITEMS_FAILURE, { error })

export type FetchCollectionItemsRequestAction = ReturnType<
typeof fetchCollectionItemsRequest
>
export type FetchCollectionItemsSuccessAction = ReturnType<
typeof fetchCollectionItemsSuccess
>
export type FetchCollectionItemsFailureAction = ReturnType<
typeof fetchCollectionItemsFailure
>

// Buy Item
export const BUY_ITEM_REQUEST = '[Request] Buy item'
export const BUY_ITEM_SUCCESS = '[Success] Buy item'
Expand Down
14 changes: 14 additions & 0 deletions webapp/src/modules/item/reducer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,12 @@ import {
buyItemWithCardFailure,
buyItemWithCardRequest,
buyItemWithCardSuccess,
FETCH_COLLECTION_ITEMS_SUCCESS,
FETCH_ITEM_SUCCESS,
FETCH_TRENDING_ITEMS_SUCCESS,
fetchCollectionItemsFailure,
fetchCollectionItemsRequest,
fetchCollectionItemsSuccess,
fetchItemFailure,
fetchItemRequest,
fetchItemsFailure,
Expand Down Expand Up @@ -79,6 +83,7 @@ const trendingItemsBatchSize = 20
const requestActions = [
fetchTrendingItemsRequest(trendingItemsBatchSize),
fetchItemsRequest(itemBrowseOptions),
fetchCollectionItemsRequest({ contractAddresses: [], first: 10 }),
fetchItemRequest(item.contractAddress, item.itemId),
buyItemRequest(item),
buyItemWithCardRequest(item),
Expand Down Expand Up @@ -115,6 +120,10 @@ const failureActions = [
request: fetchItemsRequest(itemBrowseOptions),
failure: fetchItemsFailure(anErrorMessage, itemBrowseOptions)
},
{
request: fetchCollectionItemsRequest({ contractAddresses: [], first: 10 }),
failure: fetchCollectionItemsFailure(anErrorMessage)
},
{
request: fetchItemRequest(item.contractAddress, item.itemId),
failure: fetchItemFailure(item.contractAddress, item.itemId, anErrorMessage)
Expand Down Expand Up @@ -192,6 +201,11 @@ describe('when reducing the successful action of fetching items', () => {
})

describe.each([
[
FETCH_COLLECTION_ITEMS_SUCCESS,
fetchCollectionItemsRequest({ contractAddresses: [], first: 10 }),
fetchCollectionItemsSuccess([item])
],
[
FETCH_ITEM_SUCCESS,
fetchItemRequest(item.contractAddress, item.itemId),
Expand Down
14 changes: 13 additions & 1 deletion webapp/src/modules/item/reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ import {
BuyItemWithCardFailureAction,
BuyItemWithCardRequestAction,
BuyItemWithCardSuccessAction,
BUY_ITEM_WITH_CARD_FAILURE
BUY_ITEM_WITH_CARD_FAILURE,
FETCH_COLLECTION_ITEMS_SUCCESS,
FetchCollectionItemsRequestAction,
FetchCollectionItemsSuccessAction,
FetchCollectionItemsFailureAction,
FETCH_COLLECTION_ITEMS_REQUEST,
FETCH_COLLECTION_ITEMS_FAILURE
} from './actions'

export type ItemState = {
Expand Down Expand Up @@ -70,6 +76,9 @@ type ItemReducerAction =
| BuyItemWithCardRequestAction
| BuyItemWithCardSuccessAction
| BuyItemWithCardFailureAction
| FetchCollectionItemsRequestAction
| FetchCollectionItemsSuccessAction
| FetchCollectionItemsFailureAction

export function itemReducer(
state = INITIAL_STATE,
Expand All @@ -82,6 +91,7 @@ export function itemReducer(
case BUY_ITEM_WITH_CARD_SUCCESS:
case FETCH_ITEMS_REQUEST:
case FETCH_TRENDING_ITEMS_REQUEST:
case FETCH_COLLECTION_ITEMS_REQUEST:
case FETCH_ITEM_REQUEST: {
return {
...state,
Expand All @@ -90,6 +100,7 @@ export function itemReducer(
}
case FETCH_TRENDING_ITEMS_SUCCESS:
case FETCH_FAVORITED_ITEMS_SUCCESS:
case FETCH_COLLECTION_ITEMS_SUCCESS:
case FETCH_ITEMS_SUCCESS: {
const { items } = action.payload
return {
Expand Down Expand Up @@ -122,6 +133,7 @@ export function itemReducer(

case BUY_ITEM_FAILURE:
case BUY_ITEM_WITH_CARD_FAILURE:
case FETCH_COLLECTION_ITEMS_FAILURE:
case FETCH_ITEMS_FAILURE:
case FETCH_TRENDING_ITEMS_FAILURE:
case FETCH_ITEM_FAILURE: {
Expand Down
41 changes: 39 additions & 2 deletions webapp/src/modules/item/sagas.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ import {
buyItemWithCardRequest,
buyItemWithCardFailure,
buyItemWithCardSuccess,
FETCH_ITEM_FAILURE
FETCH_ITEM_FAILURE,
fetchCollectionItemsRequest,
fetchCollectionItemsSuccess,
fetchCollectionItemsFailure
} from './actions'
import { itemSaga } from './sagas'
import { getData as getItems } from './selectors'
Expand Down Expand Up @@ -352,6 +355,40 @@ describe('when handling the set purchase action', () => {
})
})

describe('when handling the fetch collections items request action', () => {
describe('when the request is successful', () => {
const fetchResult = { data: [item] }

it('should dispatch a successful action with the fetched items', () => {
return expectSaga(itemSaga, getIdentity)
.provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding the call.like assertion to be sure that the API gets called with the correct parameters?

Suggested change
.provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])
.provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])
.call.like({
fn: ItemAPI.prototype.get,
args: [{ first: 10, contractAddresses: [] }]
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

.call.like({
fn: ItemAPI.prototype.get,
args: [{ first: 10, contractAddresses: [] }]
})
.put(fetchCollectionItemsSuccess(fetchResult.data))
.dispatch(
fetchCollectionItemsRequest({ contractAddresses: [], first: 10 })
)
.run({ silenceTimeout: true })
})
})

describe('when the request fails', () => {
it('should dispatch a failing action with the error and the options', () => {
return expectSaga(itemSaga, getIdentity)
.provide([
[matchers.call.fn(ItemAPI.prototype.get), Promise.reject(anError)]
])
.put(fetchCollectionItemsFailure(anError.message))
.dispatch(
fetchCollectionItemsRequest({ contractAddresses: [], first: 10 })
)
.run({ silenceTimeout: true })
})
})
})

describe('when handling the fetch items request action', () => {
describe('when the request is successful', () => {
let dateNowSpy: jest.SpyInstance
Expand Down Expand Up @@ -388,7 +425,7 @@ describe('when handling the fetch items request action', () => {
})

describe('when the request fails', () => {
it('should dispatching a failing action with the error and the options', () => {
it('should dispatch a failing action with the error and the options', () => {
return expectSaga(itemSaga, getIdentity)
.provide([
[matchers.call.fn(CatalogAPI.prototype.get), Promise.reject(anError)],
Expand Down
Loading