diff --git a/CHANGELOG.md b/CHANGELOG.md index d3089e92..2b1b1ebc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#103](https://github.com/shlinkio/shlink-web-client/issues/103) Fixed visits page getting freezed when loading large amounts of visits. + + ## 2.0.1 - 2019-03-03 #### Added diff --git a/package.json b/package.json index 2720942b..6b7a4245 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "promise": "^8.0.1", "prop-types": "^15.6.2", "qs": "^6.5.2", - "ramda": "^0.25.0", + "ramda": "^0.26.1", "react": "^16.7.0", "react-autosuggest": "^9.4.0", "react-chartjs-2": "^2.7.4", diff --git a/src/visits/ShortUrlVisits.js b/src/visits/ShortUrlVisits.js index d3644404..cd14bc22 100644 --- a/src/visits/ShortUrlVisits.js +++ b/src/visits/ShortUrlVisits.js @@ -8,20 +8,13 @@ import DateInput from '../utils/DateInput'; import MutedMessage from '../utils/MuttedMessage'; import SortableBarGraph from './SortableBarGraph'; import { shortUrlVisitsType } from './reducers/shortUrlVisits'; -import { VisitsHeader } from './VisitsHeader'; +import VisitsHeader from './VisitsHeader'; import GraphCard from './GraphCard'; import { shortUrlDetailType } from './reducers/shortUrlDetail'; import './ShortUrlVisits.scss'; import OpenMapModalBtn from './helpers/OpenMapModalBtn'; -const ShortUrlVisits = ({ - processOsStats, - processBrowserStats, - processCountriesStats, - processCitiesStats, - processReferrersStats, - processCitiesStatsForMap, -}) => class ShortUrlVisits extends React.Component { +const ShortUrlVisits = ({ processStatsFromVisits }) => class ShortUrlVisits extends React.PureComponent { static propTypes = { match: PropTypes.shape({ params: PropTypes.object, @@ -35,28 +28,37 @@ const ShortUrlVisits = ({ state = { startDate: undefined, endDate: undefined }; loadVisits = () => { const { match: { params }, getShortUrlVisits } = this.props; - - getShortUrlVisits(params.shortCode, mapObjIndexed( + const { shortCode } = params; + const dates = mapObjIndexed( (value) => value && value.format ? value.format('YYYY-MM-DD') : value, this.state - )); + ); + const { startDate, endDate } = dates; + + // While the "page" is loaded, use the timestamp + filtering dates as memoization IDs for stats calcs + this.memoizationId = `${this.timeWhenMounted}_${shortCode}_${startDate}_${endDate}`; + getShortUrlVisits(shortCode, dates); }; componentDidMount() { const { match: { params }, getShortUrlDetail } = this.props; + const { shortCode } = params; + this.timeWhenMounted = new Date().getTime(); this.loadVisits(); - getShortUrlDetail(params.shortCode); + getShortUrlDetail(shortCode); } render() { const { shortUrlVisits, shortUrlDetail } = this.props; const renderVisitsContent = () => { - const { visits, loading, error } = shortUrlVisits; + const { visits, loading, loadingLarge, error } = shortUrlVisits; if (loading) { - return Loading...; + const message = loadingLarge ? 'This is going to take a while... :S' : 'Loading...'; + + return {message}; } if (error) { @@ -71,17 +73,21 @@ const ShortUrlVisits = ({ return There are no visits matching current filter :(; } + const { os, browsers, referrers, countries, cities, citiesForMap } = processStatsFromVisits( + { id: this.memoizationId, visits } + ); + return (
- +
- +
( ), ]} @@ -133,7 +139,7 @@ const ShortUrlVisits = ({ placeholderText="Since" isClearable maxDate={this.state.endDate} - onChange={(date) => this.setState({ startDate: date }, () => this.loadVisits())} + onChange={(date) => this.setState({ startDate: date }, this.loadVisits)} />
@@ -143,7 +149,7 @@ const ShortUrlVisits = ({ placeholderText="Until" isClearable minDate={this.state.startDate} - onChange={(date) => this.setState({ endDate: date }, () => this.loadVisits())} + onChange={(date) => this.setState({ endDate: date }, this.loadVisits)} />
diff --git a/src/visits/VisitsHeader.js b/src/visits/VisitsHeader.js index 3103c2a4..34bb0288 100644 --- a/src/visits/VisitsHeader.js +++ b/src/visits/VisitsHeader.js @@ -11,7 +11,7 @@ const propTypes = { shortUrlVisits: shortUrlVisitsType.isRequired, }; -export function VisitsHeader({ shortUrlDetail, shortUrlVisits }) { +export default function VisitsHeader({ shortUrlDetail, shortUrlVisits }) { const { shortUrl, loading } = shortUrlDetail; const { visits } = shortUrlVisits; const shortLink = shortUrl && shortUrl.shortUrl ? shortUrl.shortUrl : ''; diff --git a/src/visits/reducers/shortUrlVisits.js b/src/visits/reducers/shortUrlVisits.js index 32a86f47..3d97d4df 100644 --- a/src/visits/reducers/shortUrlVisits.js +++ b/src/visits/reducers/shortUrlVisits.js @@ -1,9 +1,11 @@ import PropTypes from 'prop-types'; +import { flatten, range, splitEvery } from 'ramda'; /* eslint-disable padding-line-between-statements, newline-after-var */ export const GET_SHORT_URL_VISITS_START = 'shlink/shortUrlVisits/GET_SHORT_URL_VISITS_START'; export const GET_SHORT_URL_VISITS_ERROR = 'shlink/shortUrlVisits/GET_SHORT_URL_VISITS_ERROR'; export const GET_SHORT_URL_VISITS = 'shlink/shortUrlVisits/GET_SHORT_URL_VISITS'; +export const GET_SHORT_URL_VISITS_LARGE = 'shlink/shortUrlVisits/GET_SHORT_URL_VISITS_LARGE'; /* eslint-enable padding-line-between-statements, newline-after-var */ export const shortUrlVisitsType = PropTypes.shape({ @@ -15,6 +17,7 @@ export const shortUrlVisitsType = PropTypes.shape({ const initialState = { visits: [], loading: false, + loadingLarge: false, error: false, }; @@ -24,19 +27,27 @@ export default function reducer(state = initialState, action) { return { ...state, loading: true, + loadingLarge: false, }; case GET_SHORT_URL_VISITS_ERROR: return { ...state, loading: false, + loadingLarge: false, error: true, }; case GET_SHORT_URL_VISITS: return { visits: action.visits, loading: false, + loadingLarge: false, error: false, }; + case GET_SHORT_URL_VISITS_LARGE: + return { + ...state, + loadingLarge: true, + }; default: return state; } @@ -58,9 +69,36 @@ export const getShortUrlVisits = (buildShlinkApiClient) => (shortCode, dates) => return data; } - return data.concat(await loadVisits(page + 1)); + // If there are more pages, make requests in blocks of 4 + const parallelRequestsCount = 4; + const parallelStartingPage = 2; + const pagesRange = range(parallelStartingPage, pagination.pagesCount + 1); + const pagesBlocks = splitEvery(parallelRequestsCount, pagesRange); + + if (pagination.pagesCount - 1 > parallelRequestsCount) { + dispatch({ type: GET_SHORT_URL_VISITS_LARGE }); + } + + return data.concat(await loadPagesBlocks(pagesBlocks)); }; + const loadPagesBlocks = async (pagesBlocks, index = 0) => { + const data = await loadVisitsInParallel(pagesBlocks[index]); + + if (index < pagesBlocks.length - 1) { + return data.concat(await loadPagesBlocks(pagesBlocks, index + 1)); + } + + return data; + }; + + const loadVisitsInParallel = (pages) => + Promise.all(pages.map(async (page) => { + const { data } = await getShortUrlVisits(shortCode, { ...dates, page, itemsPerPage }); + + return data; + })).then(flatten); + try { const visits = await loadVisits(); diff --git a/src/visits/services/VisitsParser.js b/src/visits/services/VisitsParser.js index 174291d1..8aef797f 100644 --- a/src/visits/services/VisitsParser.js +++ b/src/visits/services/VisitsParser.js @@ -1,4 +1,4 @@ -import { assoc, isNil, isEmpty, reduce } from 'ramda'; +import { isNil, isEmpty, memoizeWith, prop } from 'ramda'; const osFromUserAgent = (userAgent) => { const lowerUserAgent = userAgent.toLowerCase(); @@ -42,79 +42,69 @@ const extractDomain = (url) => { return domain.split(':')[0]; }; -export const processOsStats = (visits) => - reduce( - (stats, { userAgent }) => { - const os = isNil(userAgent) ? 'Others' : osFromUserAgent(userAgent); - - return assoc(os, (stats[os] || 0) + 1, stats); - }, - {}, - visits, - ); - -export const processBrowserStats = (visits) => - reduce( - (stats, { userAgent }) => { - const browser = isNil(userAgent) ? 'Others' : browserFromUserAgent(userAgent); - - return assoc(browser, (stats[browser] || 0) + 1, stats); - }, - {}, - visits, - ); - -export const processReferrersStats = (visits) => - reduce( - (stats, visit) => { - const notHasDomain = isNil(visit.referer) || isEmpty(visit.referer); - const domain = notHasDomain ? 'Unknown' : extractDomain(visit.referer); - - return assoc(domain, (stats[domain] || 0) + 1, stats); - }, - {}, - visits, - ); - const visitLocationHasProperty = (visitLocation, propertyName) => !isNil(visitLocation) && !isNil(visitLocation[propertyName]) && !isEmpty(visitLocation[propertyName]); -const buildLocationStatsProcessorByProperty = (propertyName) => (visits) => - reduce( - (stats, { visitLocation }) => { - const hasLocationProperty = visitLocationHasProperty(visitLocation, propertyName); - const value = hasLocationProperty ? visitLocation[propertyName] : 'Unknown'; +const updateOsStatsForVisit = (osStats, { userAgent }) => { + const os = isNil(userAgent) ? 'Others' : osFromUserAgent(userAgent); - return assoc(value, (stats[value] || 0) + 1, stats); + osStats[os] = (osStats[os] || 0) + 1; +}; + +const updateBrowsersStatsForVisit = (browsersStats, { userAgent }) => { + const browser = isNil(userAgent) ? 'Others' : browserFromUserAgent(userAgent); + + browsersStats[browser] = (browsersStats[browser] || 0) + 1; +}; + +const updateReferrersStatsForVisit = (referrersStats, { referer }) => { + const notHasDomain = isNil(referer) || isEmpty(referer); + const domain = notHasDomain ? 'Unknown' : extractDomain(referer); + + referrersStats[domain] = (referrersStats[domain] || 0) + 1; +}; + +const updateLocationsStatsForVisit = (propertyName) => (stats, { visitLocation }) => { + const hasLocationProperty = visitLocationHasProperty(visitLocation, propertyName); + const value = hasLocationProperty ? visitLocation[propertyName] : 'Unknown'; + + stats[value] = (stats[value] || 0) + 1; +}; + +const updateCountriesStatsForVisit = updateLocationsStatsForVisit('countryName'); +const updateCitiesStatsForVisit = updateLocationsStatsForVisit('cityName'); + +const updateCitiesForMapForVisit = (citiesForMapStats, { visitLocation }) => { + if (!visitLocationHasProperty(visitLocation, 'cityName')) { + return; + } + + const { cityName, latitude, longitude } = visitLocation; + const currentCity = citiesForMapStats[cityName] || { + cityName, + count: 0, + latLong: [ parseFloat(latitude), parseFloat(longitude) ], + }; + + currentCity.count++; + + citiesForMapStats[cityName] = currentCity; +}; + +export const processStatsFromVisits = memoizeWith(prop('id'), ({ visits }) => + visits.reduce( + (stats, visit) => { + // We mutate the original object because it has a big side effect when large data sets are processed + updateOsStatsForVisit(stats.os, visit); + updateBrowsersStatsForVisit(stats.browsers, visit); + updateReferrersStatsForVisit(stats.referrers, visit); + updateCountriesStatsForVisit(stats.countries, visit); + updateCitiesStatsForVisit(stats.cities, visit); + updateCitiesForMapForVisit(stats.citiesForMap, visit); + + return stats; }, - {}, - visits, - ); - -export const processCountriesStats = buildLocationStatsProcessorByProperty('countryName'); - -export const processCitiesStats = buildLocationStatsProcessorByProperty('cityName'); - -export const processCitiesStatsForMap = (visits) => - reduce( - (stats, { visitLocation }) => { - if (!visitLocationHasProperty(visitLocation, 'cityName')) { - return stats; - } - - const { cityName, latitude, longitude } = visitLocation; - const currentCity = stats[cityName] || { - cityName, - count: 0, - latLong: [ parseFloat(latitude), parseFloat(longitude) ], - }; - - currentCity.count++; - - return assoc(cityName, currentCity, stats); - }, - {}, - visits, - ); + { os: {}, browsers: {}, referrers: {}, countries: {}, cities: {}, citiesForMap: {} } + )); diff --git a/test/visits/ShortUrlVisits.test.js b/test/visits/ShortUrlVisits.test.js index 843bc68a..8f792852 100644 --- a/test/visits/ShortUrlVisits.test.js +++ b/test/visits/ShortUrlVisits.test.js @@ -11,21 +11,16 @@ import SortableBarGraph from '../../src/visits/SortableBarGraph'; describe('', () => { let wrapper; - const statsProcessor = () => ({}); + const processStatsFromVisits = () => ( + { os: {}, browsers: {}, referrers: {}, countries: {}, cities: {}, citiesForMap: {} } + ); const getShortUrlVisitsMock = sinon.spy(); const match = { params: { shortCode: 'abc123' }, }; const createComponent = (shortUrlVisits) => { - const ShortUrlVisits = createShortUrlVisits({ - processBrowserStats: statsProcessor, - processCountriesStats: statsProcessor, - processOsStats: statsProcessor, - processReferrersStats: statsProcessor, - processCitiesStats: statsProcessor, - processCitiesStatsForMap: statsProcessor, - }); + const ShortUrlVisits = createShortUrlVisits({ processStatsFromVisits }); wrapper = shallow( ', () => { } }); - it('Renders a preloader when visits are loading', () => { + it('renders a preloader when visits are loading', () => { const wrapper = createComponent({ loading: true }); const loadingMessage = wrapper.find(MutedMessage); @@ -56,6 +51,14 @@ describe('', () => { expect(loadingMessage.html()).toContain('Loading...'); }); + it('renders a warning when loading large amounts of visits', () => { + const wrapper = createComponent({ loading: true, loadingLarge: true }); + const loadingMessage = wrapper.find(MutedMessage); + + expect(loadingMessage).toHaveLength(1); + expect(loadingMessage.html()).toContain('This is going to take a while... :S'); + }); + it('renders an error message when visits could not be loaded', () => { const wrapper = createComponent({ loading: false, error: true }); const errorMessage = wrapper.find(Card); diff --git a/test/visits/VisitsHeader.test.js b/test/visits/VisitsHeader.test.js index 797f6a29..a0fce13f 100644 --- a/test/visits/VisitsHeader.test.js +++ b/test/visits/VisitsHeader.test.js @@ -1,7 +1,7 @@ import React from 'react'; import { shallow } from 'enzyme'; import Moment from 'react-moment'; -import { VisitsHeader } from '../../src/visits/VisitsHeader'; +import VisitsHeader from '../../src/visits/VisitsHeader'; import ExternalLink from '../../src/utils/ExternalLink'; describe('', () => { diff --git a/test/visits/reducers/shortUrlVisits.test.js b/test/visits/reducers/shortUrlVisits.test.js index 676490e7..93e8fbe4 100644 --- a/test/visits/reducers/shortUrlVisits.test.js +++ b/test/visits/reducers/shortUrlVisits.test.js @@ -4,6 +4,7 @@ import reducer, { GET_SHORT_URL_VISITS_START, GET_SHORT_URL_VISITS_ERROR, GET_SHORT_URL_VISITS, + GET_SHORT_URL_VISITS_LARGE, } from '../../../src/visits/reducers/shortUrlVisits'; describe('shortUrlVisitsReducer', () => { @@ -15,6 +16,13 @@ describe('shortUrlVisitsReducer', () => { expect(loading).toEqual(true); }); + it('returns loadingLarge on GET_SHORT_URL_VISITS_LARGE', () => { + const state = reducer({ loadingLarge: false }, { type: GET_SHORT_URL_VISITS_LARGE }); + const { loadingLarge } = state; + + expect(loadingLarge).toEqual(true); + }); + it('stops loading and returns error on GET_SHORT_URL_VISITS_ERROR', () => { const state = reducer({ loading: true, error: false }, { type: GET_SHORT_URL_VISITS_ERROR }); const { loading, error } = state; diff --git a/test/visits/services/VisitsParser.test.js b/test/visits/services/VisitsParser.test.js index f5002e7c..c6dfd717 100644 --- a/test/visits/services/VisitsParser.test.js +++ b/test/visits/services/VisitsParser.test.js @@ -1,11 +1,4 @@ -import { - processOsStats, - processBrowserStats, - processReferrersStats, - processCountriesStats, - processCitiesStats, - processCitiesStatsForMap, -} from '../../../src/visits/services/VisitsParser'; +import { processStatsFromVisits } from '../../../src/visits/services/VisitsParser'; describe('VisitsParser', () => { const visits = [ @@ -50,64 +43,71 @@ describe('VisitsParser', () => { }, ]; - describe('processOsStats', () => { + describe('processStatsFromVisits', () => { + let stats; + + beforeAll(() => { + stats = processStatsFromVisits({ id: 'id', visits }); + }); + it('properly parses OS stats', () => { - expect(processOsStats(visits)).toEqual({ + const { os } = stats; + + expect(os).toEqual({ Linux: 3, Windows: 1, MacOS: 1, }); }); - }); - describe('processBrowserStats', () => { it('properly parses browser stats', () => { - expect(processBrowserStats(visits)).toEqual({ + const { browsers } = stats; + + expect(browsers).toEqual({ Firefox: 2, Chrome: 2, Opera: 1, }); }); - }); - describe('processReferrersStats', () => { it('properly parses referrer stats', () => { - expect(processReferrersStats(visits)).toEqual({ + const { referrers } = stats; + + expect(referrers).toEqual({ 'Unknown': 2, 'google.com': 2, 'm.facebook.com': 1, }); }); - }); - describe('processCountriesStats', () => { it('properly parses countries stats', () => { - expect(processCountriesStats(visits)).toEqual({ + const { countries } = stats; + + expect(countries).toEqual({ 'Spain': 3, 'United States': 1, 'Unknown': 1, }); }); - }); - describe('processCitiesStats', () => { it('properly parses cities stats', () => { - expect(processCitiesStats(visits)).toEqual({ + const { cities } = stats; + + expect(cities).toEqual({ 'Zaragoza': 2, 'New York': 1, 'Unknown': 2, }); }); - }); - describe('processCitiesStatsForMap', () => { it('properly parses cities stats with lat and long', () => { + const { citiesForMap } = stats; const zaragozaLat = 123.45; const zaragozaLong = -543.21; const newYorkLat = 1029; const newYorkLong = 6758; - expect(processCitiesStatsForMap(visits)).toEqual({ + expect(citiesForMap).toEqual({ 'Zaragoza': { cityName: 'Zaragoza', count: 2, diff --git a/yarn.lock b/yarn.lock index ef2345f2..cfb4bbea 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7910,9 +7910,10 @@ railroad-diagrams@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/railroad-diagrams/-/railroad-diagrams-1.0.0.tgz#eb7e6267548ddedfb899c1b90e57374559cddb7e" -ramda@^0.25.0: - version "0.25.0" - resolved "https://registry.yarnpkg.com/ramda/-/ramda-0.25.0.tgz#8fdf68231cffa90bc2f9460390a0cb74a29b29a9" +ramda@^0.26.1: + version "0.26.1" + resolved "https://registry.yarnpkg.com/ramda/-/ramda-0.26.1.tgz#8d41351eb8111c55353617fc3bbffad8e4d35d06" + integrity sha512-hLWjpy7EnsDBb0p+Z3B7rPi3GDeRG5ZtiI33kJhTt+ORCd38AbAIjB/9zRIUoeTbE/AVX5ZkU7m6bznsvrf8eQ== randexp@0.4.6: version "0.4.6"