From a1fdc06f12380d686271f5cb52db94f7c3f0ecbf Mon Sep 17 00:00:00 2001 From: Gresille&Siffle <39056254+GresilleSiffle@users.noreply.github.com> Date: Tue, 7 Nov 2023 17:01:33 +0100 Subject: [PATCH 1/6] [back][front] perf: stats API can be filtered by poll This will reduce the amount of SQL queries when only the stats of a single poll are required. --- .../tournesol/tests/test_api_statistics.py | 36 ++++++++++++- backend/tournesol/views/stats.py | 51 +++++++++++++++---- frontend/scripts/openapi.yaml | 6 +++ .../goals/CollectiveGoalWeeklyProgress.tsx | 2 +- .../src/features/statistics/StatsContext.tsx | 39 +++++++------- .../features/statistics/UsageStatsSection.tsx | 2 +- frontend/src/hooks/useStats.ts | 12 +++-- .../src/pages/about/PublicDownloadSection.tsx | 2 +- .../videos/sections/ComparisonSection.tsx | 2 +- .../RecommendationsSection.tsx | 2 +- 10 files changed, 115 insertions(+), 39 deletions(-) diff --git a/backend/tournesol/tests/test_api_statistics.py b/backend/tournesol/tests/test_api_statistics.py index ad53c8a6ec..09f0d7763e 100644 --- a/backend/tournesol/tests/test_api_statistics.py +++ b/backend/tournesol/tests/test_api_statistics.py @@ -4,11 +4,13 @@ from rest_framework.test import APIClient from core.models import User +from core.tests.factories.user import UserFactory from core.utils.time import time_ago from tournesol.tests.factories.comparison import ComparisonFactory from tournesol.tests.factories.entity import EntityFactory, VideoFactory from ..models import Comparison, Entity +from .factories.poll import PollWithCriteriasFactory class StatisticsAPI(TestCase): @@ -42,7 +44,7 @@ def setUp(self): rating_n_ratings=4, ) - non_video = EntityFactory(type="another_type", rating_n_ratings=5) + EntityFactory(type="another_type", rating_n_ratings=5) Entity.objects.filter(pk=video_1.pk).update(add_time=time_ago(days=5)) Entity.objects.filter(pk=video_2.pk).update(add_time=time_ago(days=29)) @@ -131,3 +133,35 @@ def test_user_stats(self): self.assertEqual(user_count, len(self._list_of_users)) self.assertEqual(active_users["joined_last_30_days"], 1) + + def test_url_param_poll(self): + client = APIClient() + + new_poll = "new_poll" + new_user = "new_user" + + response = client.get("/stats/") + data = response.data + + self.assertEqual(len(data["polls"]), 2) + for poll in data["polls"]: + self.assertNotEqual(poll["name"], new_poll) + + poll = PollWithCriteriasFactory.create(name=new_poll) + user = UserFactory(username=new_user) + ComparisonFactory.create_batch(8, poll=poll, user=user) + + response = client.get("/stats/") + data = response.data + self.assertEqual(len(data["polls"]), 3) + + response = client.get("/stats/", {"poll": poll.name}) + data = response.data + + poll = [poll for poll in data["polls"] if poll["name"] == new_poll][0] + self.assertEqual(len(data["polls"]), 1) + self.assertEqual(poll["comparisons"]["total"], 8) + + # Using an unknown poll should return all polls. + response = client.get("/stats/", {"poll": "unknown"}) + self.assertEqual(len(response.data["polls"]), 3) diff --git a/backend/tournesol/views/stats.py b/backend/tournesol/views/stats.py index 5df4f5d54a..a296d55742 100644 --- a/backend/tournesol/views/stats.py +++ b/backend/tournesol/views/stats.py @@ -5,7 +5,7 @@ from datetime import datetime, time, timezone from typing import List -from drf_spectacular.utils import extend_schema, extend_schema_view +from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view from rest_framework import generics from rest_framework.permissions import AllowAny from rest_framework.response import Response @@ -94,15 +94,40 @@ def append_poll( @extend_schema_view( - get=extend_schema(description="Fetch all Tournesol's public statistics.") + get=extend_schema( + description="Fetch all Tournesol's public statistics.", + parameters=[ + OpenApiParameter( + name="poll", + required=False, + description="Only get stats related to this poll.", + ), + ], + ) ) class StatisticsView(generics.GenericAPIView): - """Return popularity statistics about Tournesol""" + """ + This view returns the contribution stats for all polls. + """ permission_classes = [AllowAny] serializer_class = StatisticsSerializer _days_delta = 30 + def _get_poll_stats(self, poll: Poll): + comparisons = self._get_comparisons_statistics(poll) + compared_entities = self._get_compared_entities_statistics(poll) + return {"comparisons": comparisons, "compared_entities": compared_entities} + + def _build_stats_for_all_poll(self, polls: list[Poll], stats: Statistics): + for poll in polls: + poll_stats = self._get_poll_stats(poll) + stats.append_poll( + poll.name, + poll_stats["compared_entities"], + poll_stats["comparisons"], + ) + def get(self, request): statistics = Statistics() @@ -112,15 +137,19 @@ def get(self, request): ).count() statistics.set_active_users(active_users, last_month_active_users) - for poll in Poll.objects.iterator(): - compared_entities_statistics = self._get_compared_entities_statistics(poll) - comparisons_statistics = self._get_comparisons_statistics(poll) - statistics.append_poll( - poll.name, - compared_entities_statistics, - comparisons_statistics, - ) + selected_poll = request.query_params.get("poll") + + if selected_poll: + try: + poll = Poll.objects.get(name=selected_poll) + except Poll.DoesNotExist: + polls = Poll.objects.iterator() + else: + polls = [poll] + else: + polls = Poll.objects.iterator() + self._build_stats_for_all_poll(polls, statistics) return Response(StatisticsSerializer(statistics).data) def _get_compared_entities_statistics(self, poll): diff --git a/frontend/scripts/openapi.yaml b/frontend/scripts/openapi.yaml index b75250da2e..1696ba9bd1 100644 --- a/frontend/scripts/openapi.yaml +++ b/frontend/scripts/openapi.yaml @@ -1006,6 +1006,12 @@ paths: get: operationId: stats_retrieve description: Fetch all Tournesol's public statistics. + parameters: + - in: query + name: poll + schema: + type: string + description: Only get stats related to this poll. tags: - stats security: diff --git a/frontend/src/features/goals/CollectiveGoalWeeklyProgress.tsx b/frontend/src/features/goals/CollectiveGoalWeeklyProgress.tsx index 58fe812445..ca0e040518 100644 --- a/frontend/src/features/goals/CollectiveGoalWeeklyProgress.tsx +++ b/frontend/src/features/goals/CollectiveGoalWeeklyProgress.tsx @@ -15,7 +15,7 @@ const CollectiveGoalWeeklyProgress = () => { const { t } = useTranslation(); const { name: pollName } = useCurrentPoll(); - const stats = useStats(); + const stats = useStats({ poll: pollName }); const pollStats = getPollStats(stats, pollName); const collectiveComparisonsNbr = diff --git a/frontend/src/features/statistics/StatsContext.tsx b/frontend/src/features/statistics/StatsContext.tsx index 2d06cf1411..3fef54123e 100644 --- a/frontend/src/features/statistics/StatsContext.tsx +++ b/frontend/src/features/statistics/StatsContext.tsx @@ -12,8 +12,8 @@ const EXPIRATION_TIME = 4000; interface StatsContextValue { stats: Statistics; - getStats: () => Statistics; - refreshStats: () => void; + getStats: (poll?: string) => Statistics; + refreshStats: (poll?: string) => void; } const initialState: Statistics = { @@ -41,8 +41,8 @@ export const StatsLazyProvider = ({ const [stats, setStats] = useState(initialState); - const refreshStats = useCallback(async () => { - const newStats = await StatsService.statsRetrieve(); + const refreshStats = useCallback(async (poll: string | undefined) => { + const newStats = await StatsService.statsRetrieve({ poll }); loading.current = false; lastRefreshAt.current = Date.now(); setStats(newStats); @@ -52,23 +52,26 @@ export const StatsLazyProvider = ({ * Initialize the stats if they are empty or refresh them if they are * outdated. */ - const getStats = useCallback(() => { - const currentTime = Date.now(); + const getStats = useCallback( + (poll: string | undefined) => { + const currentTime = Date.now(); - if (loading.current) { - return stats; - } + if (loading.current) { + return stats; + } - if ( - stats.polls.length === 0 || - currentTime - lastRefreshAt.current >= EXPIRATION_TIME - ) { - loading.current = true; - refreshStats(); - } + if ( + stats.polls.length === 0 || + currentTime - lastRefreshAt.current >= EXPIRATION_TIME + ) { + loading.current = true; + refreshStats(poll); + } - return stats; - }, [stats, refreshStats]); + return stats; + }, + [stats, refreshStats] + ); const contextValue = useMemo( () => ({ diff --git a/frontend/src/features/statistics/UsageStatsSection.tsx b/frontend/src/features/statistics/UsageStatsSection.tsx index 59bd5975f9..e8da39e17c 100644 --- a/frontend/src/features/statistics/UsageStatsSection.tsx +++ b/frontend/src/features/statistics/UsageStatsSection.tsx @@ -57,7 +57,7 @@ const StatsSection = () => { const { t } = useTranslation(); const { name: pollName } = useCurrentPoll(); - const stats = useStats(); + const stats = useStats({ poll: pollName }); const pollStats = getPollStats(stats, pollName); const comparedEntitiesTitle = useMemo(() => { diff --git a/frontend/src/hooks/useStats.ts b/frontend/src/hooks/useStats.ts index 85d840d903..a8ce690549 100644 --- a/frontend/src/hooks/useStats.ts +++ b/frontend/src/hooks/useStats.ts @@ -2,13 +2,17 @@ import { useState, useEffect, useContext } from 'react'; import StatsContext from 'src/features/statistics/StatsContext'; import { Statistics } from 'src/services/openapi'; -export const useStats = () => { +interface Props { + poll?: string; +} + +export const useStats = ({ poll }: Props) => { const { getStats } = useContext(StatsContext); - const [stats, setStats] = useState(getStats()); + const [stats, setStats] = useState(getStats(poll)); useEffect(() => { - setStats(getStats()); - }, [getStats]); + setStats(getStats(poll)); + }, [poll, getStats]); return stats; }; diff --git a/frontend/src/pages/about/PublicDownloadSection.tsx b/frontend/src/pages/about/PublicDownloadSection.tsx index 7bdd267543..de53864585 100644 --- a/frontend/src/pages/about/PublicDownloadSection.tsx +++ b/frontend/src/pages/about/PublicDownloadSection.tsx @@ -14,7 +14,7 @@ const PublicDownloadSection = () => { const api_url = process.env.REACT_APP_API_URL; - const stats = useStats(); + const stats = useStats({ poll: pollName }); const pollStats = getPollStats(stats, pollName); const userCount = stats.active_users.total ?? 0; diff --git a/frontend/src/pages/home/videos/sections/ComparisonSection.tsx b/frontend/src/pages/home/videos/sections/ComparisonSection.tsx index 2576a8d492..2ea7b4dcc8 100644 --- a/frontend/src/pages/home/videos/sections/ComparisonSection.tsx +++ b/frontend/src/pages/home/videos/sections/ComparisonSection.tsx @@ -14,7 +14,7 @@ const ComparisonSection = () => { const { t } = useTranslation(); const { name: pollName } = useCurrentPoll(); - const stats = useStats(); + const stats = useStats({ poll: pollName }); const pollStats = getPollStats(stats, pollName); const color = '#fff'; diff --git a/frontend/src/pages/home/videos/sections/recommendations/RecommendationsSection.tsx b/frontend/src/pages/home/videos/sections/recommendations/RecommendationsSection.tsx index ba47d61f99..30c78780dc 100644 --- a/frontend/src/pages/home/videos/sections/recommendations/RecommendationsSection.tsx +++ b/frontend/src/pages/home/videos/sections/recommendations/RecommendationsSection.tsx @@ -18,7 +18,7 @@ const RecommendationsSection = () => { const { t } = useTranslation(); const { name: pollName } = useCurrentPoll(); - const stats = useStats(); + const stats = useStats({ poll: pollName }); const pollStats = getPollStats(stats, pollName); const titleColor = '#fff'; From 036c4af2a31780c69419da342afb6ba6e9e3cdc0 Mon Sep 17 00:00:00 2001 From: Gresille&Siffle <39056254+GresilleSiffle@users.noreply.github.com> Date: Fri, 10 Nov 2023 16:07:53 +0100 Subject: [PATCH 2/6] [back] refactor: /stats/ return nothing when the poll param is unknown --- backend/tournesol/tests/test_api_statistics.py | 6 +++++- backend/tournesol/views/stats.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/backend/tournesol/tests/test_api_statistics.py b/backend/tournesol/tests/test_api_statistics.py index 09f0d7763e..a32cf96c32 100644 --- a/backend/tournesol/tests/test_api_statistics.py +++ b/backend/tournesol/tests/test_api_statistics.py @@ -135,6 +135,10 @@ def test_user_stats(self): self.assertEqual(active_users["joined_last_30_days"], 1) def test_url_param_poll(self): + """ + The `poll` URL parameter allows to filter the returned results by + poll. + """ client = APIClient() new_poll = "new_poll" @@ -164,4 +168,4 @@ def test_url_param_poll(self): # Using an unknown poll should return all polls. response = client.get("/stats/", {"poll": "unknown"}) - self.assertEqual(len(response.data["polls"]), 3) + self.assertEqual(len(response.data["polls"]), 0) diff --git a/backend/tournesol/views/stats.py b/backend/tournesol/views/stats.py index a296d55742..7f2418b8e9 100644 --- a/backend/tournesol/views/stats.py +++ b/backend/tournesol/views/stats.py @@ -143,7 +143,7 @@ def get(self, request): try: poll = Poll.objects.get(name=selected_poll) except Poll.DoesNotExist: - polls = Poll.objects.iterator() + polls = [] else: polls = [poll] else: From 9df3994a527b3c68cfcd4fc738d79b3f5108ed54 Mon Sep 17 00:00:00 2001 From: Gresille&Siffle <39056254+GresilleSiffle@users.noreply.github.com> Date: Thu, 16 Nov 2023 11:57:29 +0100 Subject: [PATCH 3/6] [front] fix: getStats refreshes stats when poll changes --- frontend/src/features/statistics/StatsContext.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frontend/src/features/statistics/StatsContext.tsx b/frontend/src/features/statistics/StatsContext.tsx index 3fef54123e..cd3dc7c193 100644 --- a/frontend/src/features/statistics/StatsContext.tsx +++ b/frontend/src/features/statistics/StatsContext.tsx @@ -38,6 +38,7 @@ export const StatsLazyProvider = ({ }) => { const loading = useRef(false); const lastRefreshAt = useRef(0); + const lastPoll = useRef(undefined); const [stats, setStats] = useState(initialState); @@ -62,9 +63,11 @@ export const StatsLazyProvider = ({ if ( stats.polls.length === 0 || - currentTime - lastRefreshAt.current >= EXPIRATION_TIME + currentTime - lastRefreshAt.current >= EXPIRATION_TIME || + poll !== lastPoll.current ) { loading.current = true; + lastPoll.current = poll; refreshStats(poll); } From 094f0306861a316dcc50357ac463bc810dca94af Mon Sep 17 00:00:00 2001 From: Gresille&Siffle <39056254+GresilleSiffle@users.noreply.github.com> Date: Thu, 4 Jan 2024 17:45:39 +0100 Subject: [PATCH 4/6] fix bad stats displayed when two concurrent requests are made --- .../src/features/statistics/StatsContext.tsx | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/frontend/src/features/statistics/StatsContext.tsx b/frontend/src/features/statistics/StatsContext.tsx index cd3dc7c193..70c555b9c5 100644 --- a/frontend/src/features/statistics/StatsContext.tsx +++ b/frontend/src/features/statistics/StatsContext.tsx @@ -52,23 +52,32 @@ export const StatsLazyProvider = ({ /** * Initialize the stats if they are empty or refresh them if they are * outdated. + * + * Note that the getStats implementation assumes that only the stats of a + * single poll are displayed per page. */ const getStats = useCallback( (poll: string | undefined) => { const currentTime = Date.now(); if (loading.current) { - return stats; - } - - if ( - stats.polls.length === 0 || - currentTime - lastRefreshAt.current >= EXPIRATION_TIME || - poll !== lastPoll.current - ) { - loading.current = true; - lastPoll.current = poll; - refreshStats(poll); + if (poll === lastPoll.current) { + return stats; + } else { + loading.current = true; + lastPoll.current = poll; + refreshStats(poll); + } + } else { + if ( + stats.polls.length === 0 || + poll !== lastPoll.current || + currentTime - lastRefreshAt.current >= EXPIRATION_TIME + ) { + loading.current = true; + lastPoll.current = poll; + refreshStats(poll); + } } return stats; From 80654b6439e452bca017c8f60aaf7d4e2a420966 Mon Sep 17 00:00:00 2001 From: Adrien Matissart Date: Thu, 18 Jan 2024 11:19:08 +0100 Subject: [PATCH 5/6] update StatsContext to cancel previous request after poll has changed --- .../src/features/statistics/StatsContext.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/frontend/src/features/statistics/StatsContext.tsx b/frontend/src/features/statistics/StatsContext.tsx index 70c555b9c5..d246bac32d 100644 --- a/frontend/src/features/statistics/StatsContext.tsx +++ b/frontend/src/features/statistics/StatsContext.tsx @@ -5,7 +5,11 @@ import React, { useRef, useState, } from 'react'; -import { Statistics, StatsService } from 'src/services/openapi'; +import { + CancelablePromise, + Statistics, + StatsService, +} from 'src/services/openapi'; // Stats are considered outdated after this amount of miliseconds. const EXPIRATION_TIME = 4000; @@ -36,15 +40,19 @@ export const StatsLazyProvider = ({ }: { children: React.ReactNode; }) => { - const loading = useRef(false); + const loading = useRef | null>(null); const lastRefreshAt = useRef(0); const lastPoll = useRef(undefined); const [stats, setStats] = useState(initialState); const refreshStats = useCallback(async (poll: string | undefined) => { - const newStats = await StatsService.statsRetrieve({ poll }); - loading.current = false; + if (loading.current) { + loading.current.cancel(); + } + loading.current = StatsService.statsRetrieve({ poll }); + const newStats = await loading.current; + loading.current = null; lastRefreshAt.current = Date.now(); setStats(newStats); }, []); @@ -64,7 +72,6 @@ export const StatsLazyProvider = ({ if (poll === lastPoll.current) { return stats; } else { - loading.current = true; lastPoll.current = poll; refreshStats(poll); } @@ -74,7 +81,6 @@ export const StatsLazyProvider = ({ poll !== lastPoll.current || currentTime - lastRefreshAt.current >= EXPIRATION_TIME ) { - loading.current = true; lastPoll.current = poll; refreshStats(poll); } From e90301ffbd33a1ede12fce851506b843663b8fbd Mon Sep 17 00:00:00 2001 From: Adrien Matissart Date: Thu, 18 Jan 2024 12:02:58 +0100 Subject: [PATCH 6/6] catch CancelError in refreshStats() and ensure pollName is always present --- .../comparisons/ComparisonSliders.tsx | 2 +- .../src/features/statistics/StatsContext.tsx | 24 ++++++++++++------- frontend/src/hooks/useStats.ts | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/frontend/src/features/comparisons/ComparisonSliders.tsx b/frontend/src/features/comparisons/ComparisonSliders.tsx index 92320a1029..12a3ae126d 100644 --- a/frontend/src/features/comparisons/ComparisonSliders.tsx +++ b/frontend/src/features/comparisons/ComparisonSliders.tsx @@ -145,7 +145,7 @@ const ComparisonSliders = ({ await submit(comparison); } finally { setDisableSubmit(false); - refreshStats(); + refreshStats(pollName); } // avoid a "memory leak" warning if the component is unmounted on submit. diff --git a/frontend/src/features/statistics/StatsContext.tsx b/frontend/src/features/statistics/StatsContext.tsx index d246bac32d..f822aea195 100644 --- a/frontend/src/features/statistics/StatsContext.tsx +++ b/frontend/src/features/statistics/StatsContext.tsx @@ -6,6 +6,7 @@ import React, { useState, } from 'react'; import { + CancelError, CancelablePromise, Statistics, StatsService, @@ -16,8 +17,8 @@ const EXPIRATION_TIME = 4000; interface StatsContextValue { stats: Statistics; - getStats: (poll?: string) => Statistics; - refreshStats: (poll?: string) => void; + getStats: (poll: string) => Statistics; + refreshStats: (poll: string) => void; } const initialState: Statistics = { @@ -46,15 +47,22 @@ export const StatsLazyProvider = ({ const [stats, setStats] = useState(initialState); - const refreshStats = useCallback(async (poll: string | undefined) => { + const refreshStats = useCallback(async (poll: string) => { if (loading.current) { loading.current.cancel(); } loading.current = StatsService.statsRetrieve({ poll }); - const newStats = await loading.current; - loading.current = null; - lastRefreshAt.current = Date.now(); - setStats(newStats); + try { + const newStats = await loading.current; + lastRefreshAt.current = Date.now(); + loading.current = null; + setStats(newStats); + } catch (err) { + if (err instanceof CancelError) { + return; + } + console.error(err); + } }, []); /** @@ -65,7 +73,7 @@ export const StatsLazyProvider = ({ * single poll are displayed per page. */ const getStats = useCallback( - (poll: string | undefined) => { + (poll: string) => { const currentTime = Date.now(); if (loading.current) { diff --git a/frontend/src/hooks/useStats.ts b/frontend/src/hooks/useStats.ts index a8ce690549..44535e5649 100644 --- a/frontend/src/hooks/useStats.ts +++ b/frontend/src/hooks/useStats.ts @@ -3,7 +3,7 @@ import StatsContext from 'src/features/statistics/StatsContext'; import { Statistics } from 'src/services/openapi'; interface Props { - poll?: string; + poll: string; } export const useStats = ({ poll }: Props) => {