From c2904652479d3bc424ed9e078d79b6e5a6417281 Mon Sep 17 00:00:00 2001 From: Jorge Coca Date: Fri, 29 Apr 2022 20:28:21 -0500 Subject: [PATCH] refactor(leaderboard_repository): only keep the top-10 score in the leaderboard collection (#265) --- .../lib/src/leaderboard_repository.dart | 218 +++++++----------- .../lib/src/models/exceptions.dart | 69 ++++++ .../lib/src/models/leaderboard_ranking.dart | 20 -- .../lib/src/models/models.dart | 2 +- .../test/src/leaderboard_repository_test.dart | 192 +++++++++++++-- .../src/models/leaderboard_ranking_test.dart | 19 -- 6 files changed, 315 insertions(+), 205 deletions(-) create mode 100644 packages/leaderboard_repository/lib/src/models/exceptions.dart delete mode 100644 packages/leaderboard_repository/lib/src/models/leaderboard_ranking.dart delete mode 100644 packages/leaderboard_repository/test/src/models/leaderboard_ranking_test.dart diff --git a/packages/leaderboard_repository/lib/src/leaderboard_repository.dart b/packages/leaderboard_repository/lib/src/leaderboard_repository.dart index 9d8b2434..c522584c 100644 --- a/packages/leaderboard_repository/lib/src/leaderboard_repository.dart +++ b/packages/leaderboard_repository/lib/src/leaderboard_repository.dart @@ -1,91 +1,6 @@ import 'package:cloud_firestore/cloud_firestore.dart'; import 'package:leaderboard_repository/leaderboard_repository.dart'; -/// {@template leaderboard_exception} -/// Base exception for leaderboard repository failures. -/// {@endtemplate} -abstract class LeaderboardException implements Exception { - /// {@macro leaderboard_exception} - const LeaderboardException(this.error, this.stackTrace); - - /// The error that was caught. - final Object error; - - /// The Stacktrace associated with the [error]. - final StackTrace stackTrace; -} - -/// {@template leaderboard_deserialization_exception} -/// Exception thrown when leaderboard data cannot be deserialized in the -/// expected way. -/// {@endtemplate} -class LeaderboardDeserializationException extends LeaderboardException { - /// {@macro leaderboard_deserialization_exception} - const LeaderboardDeserializationException( - Object error, - StackTrace stackTrace, - ) : super( - error, - stackTrace, - ); -} - -/// {@template fetch_top_10_leaderboard_exception} -/// Exception thrown when failure occurs while fetching top 10 leaderboard. -/// {@endtemplate} -class FetchTop10LeaderboardException extends LeaderboardException { - /// {@macro fetch_top_10_leaderboard_exception} - const FetchTop10LeaderboardException( - Object error, - StackTrace stackTrace, - ) : super( - error, - stackTrace, - ); -} - -/// {@template add_leaderboard_entry_exception} -/// Exception thrown when failure occurs while adding entry to leaderboard. -/// {@endtemplate} -class AddLeaderboardEntryException extends LeaderboardException { - /// {@macro add_leaderboard_entry_exception} - const AddLeaderboardEntryException( - Object error, - StackTrace stackTrace, - ) : super( - error, - stackTrace, - ); -} - -/// {@template fetch_player_ranking_exception} -/// Exception thrown when failure occurs while fetching player ranking. -/// {@endtemplate} -class FetchPlayerRankingException extends LeaderboardException { - /// {@macro fetch_player_ranking_exception} - const FetchPlayerRankingException( - Object error, - StackTrace stackTrace, - ) : super( - error, - stackTrace, - ); -} - -/// {@template fetch_prohibited_initials_exception} -/// Exception thrown when failure occurs while fetching prohibited initials. -/// {@endtemplate} -class FetchProhibitedInitialsException extends LeaderboardException { - /// {@macro fetch_prohibited_initials_exception} - const FetchProhibitedInitialsException( - Object error, - StackTrace stackTrace, - ) : super( - error, - stackTrace, - ); -} - /// {@template leaderboard_repository} /// Repository to access leaderboard data in Firebase Cloud Firestore. /// {@endtemplate} @@ -97,73 +12,40 @@ class LeaderboardRepository { final FirebaseFirestore _firebaseFirestore; + static const _leaderboardLimit = 10; + static const _leaderboardCollectionName = 'leaderboard'; + static const _scoreFieldName = 'score'; + /// Acquires top 10 [LeaderboardEntryData]s. Future> fetchTop10Leaderboard() async { - final leaderboardEntries = []; - late List documents; - try { final querySnapshot = await _firebaseFirestore - .collection('leaderboard') - .orderBy('score', descending: true) - .limit(10) + .collection(_leaderboardCollectionName) + .orderBy(_scoreFieldName, descending: true) + .limit(_leaderboardLimit) .get(); - documents = querySnapshot.docs; + final documents = querySnapshot.docs; + return documents.toLeaderboard(); + } on LeaderboardDeserializationException { + rethrow; } on Exception catch (error, stackTrace) { throw FetchTop10LeaderboardException(error, stackTrace); } - - for (final document in documents) { - final data = document.data() as Map?; - if (data != null) { - try { - leaderboardEntries.add(LeaderboardEntryData.fromJson(data)); - } catch (error, stackTrace) { - throw LeaderboardDeserializationException(error, stackTrace); - } - } - } - - return leaderboardEntries; } - /// Adds player's score entry to the leaderboard and gets their - /// [LeaderboardRanking]. - Future addLeaderboardEntry( + /// Adds player's score entry to the leaderboard if it is within the top-10 + Future addLeaderboardEntry( LeaderboardEntryData entry, ) async { - late DocumentReference entryReference; - try { - entryReference = await _firebaseFirestore - .collection('leaderboard') - .add(entry.toJson()); - } on Exception catch (error, stackTrace) { - throw AddLeaderboardEntryException(error, stackTrace); - } - - try { - final querySnapshot = await _firebaseFirestore - .collection('leaderboard') - .orderBy('score', descending: true) - .get(); - - // TODO(allisonryan0002): see if we can find a more performant solution. - final documents = querySnapshot.docs; - final ranking = documents.indexWhere( - (document) => document.id == entryReference.id, - ) + - 1; - - if (ranking > 0) { - return LeaderboardRanking(ranking: ranking, outOf: documents.length); - } else { - throw FetchPlayerRankingException( - 'Player score could not be found and ranking cannot be provided.', - StackTrace.current, - ); + final leaderboard = await _fetchLeaderboardSortedByScore(); + if (leaderboard.length < 10) { + await _saveScore(entry); + } else { + final tenthPositionScore = leaderboard[9].score; + if (entry.score > tenthPositionScore) { + await _saveScore(entry); + await _deleteScoresUnder(tenthPositionScore); } - } on Exception catch (error, stackTrace) { - throw FetchPlayerRankingException(error, stackTrace); } } @@ -174,7 +56,6 @@ class LeaderboardRepository { if (!initialsRegex.hasMatch(initials)) { return false; } - try { final document = await _firebaseFirestore .collection('prohibitedInitials') @@ -187,4 +68,61 @@ class LeaderboardRepository { throw FetchProhibitedInitialsException(error, stackTrace); } } + + Future> _fetchLeaderboardSortedByScore() async { + try { + final querySnapshot = await _firebaseFirestore + .collection(_leaderboardCollectionName) + .orderBy(_scoreFieldName, descending: true) + .get(); + final documents = querySnapshot.docs; + return documents.toLeaderboard(); + } on Exception catch (error, stackTrace) { + throw FetchLeaderboardException(error, stackTrace); + } + } + + Future _saveScore(LeaderboardEntryData entry) { + try { + return _firebaseFirestore + .collection(_leaderboardCollectionName) + .add(entry.toJson()); + } on Exception catch (error, stackTrace) { + throw AddLeaderboardEntryException(error, stackTrace); + } + } + + Future _deleteScoresUnder(int score) async { + try { + final querySnapshot = await _firebaseFirestore + .collection(_leaderboardCollectionName) + .where(_scoreFieldName, isLessThanOrEqualTo: score) + .get(); + final documents = querySnapshot.docs; + for (final document in documents) { + await document.reference.delete(); + } + } on LeaderboardDeserializationException { + rethrow; + } on Exception catch (error, stackTrace) { + throw DeleteLeaderboardException(error, stackTrace); + } + } +} + +extension on List { + List toLeaderboard() { + final leaderboardEntries = []; + for (final document in this) { + final data = document.data() as Map?; + if (data != null) { + try { + leaderboardEntries.add(LeaderboardEntryData.fromJson(data)); + } catch (error, stackTrace) { + throw LeaderboardDeserializationException(error, stackTrace); + } + } + } + return leaderboardEntries; + } } diff --git a/packages/leaderboard_repository/lib/src/models/exceptions.dart b/packages/leaderboard_repository/lib/src/models/exceptions.dart new file mode 100644 index 00000000..f709a27e --- /dev/null +++ b/packages/leaderboard_repository/lib/src/models/exceptions.dart @@ -0,0 +1,69 @@ +/// {@template leaderboard_exception} +/// Base exception for leaderboard repository failures. +/// {@endtemplate} +abstract class LeaderboardException implements Exception { + /// {@macro leaderboard_exception} + const LeaderboardException(this.error, this.stackTrace); + + /// The error that was caught. + final Object error; + + /// The Stacktrace associated with the [error]. + final StackTrace stackTrace; +} + +/// {@template leaderboard_deserialization_exception} +/// Exception thrown when leaderboard data cannot be deserialized in the +/// expected way. +/// {@endtemplate} +class LeaderboardDeserializationException extends LeaderboardException { + /// {@macro leaderboard_deserialization_exception} + const LeaderboardDeserializationException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} + +/// {@template fetch_top_10_leaderboard_exception} +/// Exception thrown when failure occurs while fetching top 10 leaderboard. +/// {@endtemplate} +class FetchTop10LeaderboardException extends LeaderboardException { + /// {@macro fetch_top_10_leaderboard_exception} + const FetchTop10LeaderboardException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} + +/// {@template fetch_leaderboard_exception} +/// Exception thrown when failure occurs while fetching the leaderboard. +/// {@endtemplate} +class FetchLeaderboardException extends LeaderboardException { + /// {@macro fetch_top_10_leaderboard_exception} + const FetchLeaderboardException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} + +/// {@template delete_leaderboard_exception} +/// Exception thrown when failure occurs while deleting the leaderboard under +/// the tenth position. +/// {@endtemplate} +class DeleteLeaderboardException extends LeaderboardException { + /// {@macro fetch_top_10_leaderboard_exception} + const DeleteLeaderboardException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} + +/// {@template add_leaderboard_entry_exception} +/// Exception thrown when failure occurs while adding entry to leaderboard. +/// {@endtemplate} +class AddLeaderboardEntryException extends LeaderboardException { + /// {@macro add_leaderboard_entry_exception} + const AddLeaderboardEntryException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} + +/// {@template fetch_prohibited_initials_exception} +/// Exception thrown when failure occurs while fetching prohibited initials. +/// {@endtemplate} +class FetchProhibitedInitialsException extends LeaderboardException { + /// {@macro fetch_prohibited_initials_exception} + const FetchProhibitedInitialsException(Object error, StackTrace stackTrace) + : super(error, stackTrace); +} diff --git a/packages/leaderboard_repository/lib/src/models/leaderboard_ranking.dart b/packages/leaderboard_repository/lib/src/models/leaderboard_ranking.dart deleted file mode 100644 index 4a322e00..00000000 --- a/packages/leaderboard_repository/lib/src/models/leaderboard_ranking.dart +++ /dev/null @@ -1,20 +0,0 @@ -import 'package:equatable/equatable.dart'; -import 'package:leaderboard_repository/leaderboard_repository.dart'; - -/// {@template leaderboard_ranking} -/// Contains [ranking] for a single [LeaderboardEntryData] and the number of -/// players the [ranking] is [outOf]. -/// {@endtemplate} -class LeaderboardRanking extends Equatable { - /// {@macro leaderboard_ranking} - const LeaderboardRanking({required this.ranking, required this.outOf}); - - /// Place ranking by score for a [LeaderboardEntryData]. - final int ranking; - - /// Number of [LeaderboardEntryData]s at the time of score entry. - final int outOf; - - @override - List get props => [ranking, outOf]; -} diff --git a/packages/leaderboard_repository/lib/src/models/models.dart b/packages/leaderboard_repository/lib/src/models/models.dart index e10a743b..a612f3ac 100644 --- a/packages/leaderboard_repository/lib/src/models/models.dart +++ b/packages/leaderboard_repository/lib/src/models/models.dart @@ -1,2 +1,2 @@ +export 'exceptions.dart'; export 'leaderboard_entry_data.dart'; -export 'leaderboard_ranking.dart'; diff --git a/packages/leaderboard_repository/test/src/leaderboard_repository_test.dart b/packages/leaderboard_repository/test/src/leaderboard_repository_test.dart index 9d31983f..af3c5fa3 100644 --- a/packages/leaderboard_repository/test/src/leaderboard_repository_test.dart +++ b/packages/leaderboard_repository/test/src/leaderboard_repository_test.dart @@ -153,7 +153,6 @@ void main() { character: CharacterType.dash, ); const entryDocumentId = 'id$entryScore'; - final ranking = LeaderboardRanking(ranking: 3, outOf: 4); setUp(() { leaderboardRepository = LeaderboardRepository(firestore); @@ -165,13 +164,12 @@ void main() { final queryDocumentSnapshot = MockQueryDocumentSnapshot(); when(queryDocumentSnapshot.data).thenReturn({ 'character': 'dash', - 'username': 'user$score', + 'playerInitials': 'AAA', 'score': score }); when(() => queryDocumentSnapshot.id).thenReturn('id$score'); return queryDocumentSnapshot; }).toList(); - when(() => firestore.collection('leaderboard')) .thenAnswer((_) => collectionReference); when(() => collectionReference.add(any())) @@ -184,19 +182,29 @@ void main() { }); test( - 'adds leaderboard entry and returns player ranking when ' - 'firestore operations succeed', () async { - final rankingResult = - await leaderboardRepository.addLeaderboardEntry(leaderboardEntry); - - expect(rankingResult, equals(ranking)); + 'throws FetchLeaderboardException ' + 'when querying the leaderboard fails', () { + when(() => firestore.collection('leaderboard')).thenThrow(Exception()); + expect( + () => leaderboardRepository.addLeaderboardEntry(leaderboardEntry), + throwsA(isA()), + ); }); test( - 'throws AddLeaderboardEntryException when Exception occurs ' - 'when trying to add entry to firestore', () async { - when(() => firestore.collection('leaderboard')).thenThrow(Exception()); + 'saves the new score if the existing leaderboard ' + 'has less than 10 scores', () async { + await leaderboardRepository.addLeaderboardEntry(leaderboardEntry); + verify( + () => collectionReference.add(leaderboardEntry.toJson()), + ).called(1); + }); + test( + 'throws AddLeaderboardEntryException ' + 'when adding a new entry fails', () async { + when(() => collectionReference.add(leaderboardEntry.toJson())) + .thenThrow(Exception('oops')); expect( () => leaderboardRepository.addLeaderboardEntry(leaderboardEntry), throwsA(isA()), @@ -204,26 +212,160 @@ void main() { }); test( - 'throws FetchPlayerRankingException when Exception occurs ' - 'when trying to retrieve information from firestore', () async { - when(() => collectionReference.orderBy('score', descending: true)) - .thenThrow(Exception()); + 'does nothing if there are more than 10 scores in the leaderboard ' + 'and the new score is smaller than the top 10', () async { + final leaderboardScores = [ + 10000, + 9500, + 9000, + 8500, + 8000, + 7500, + 7000, + 6500, + 6000, + 5500, + 5000 + ]; + final queryDocumentSnapshots = leaderboardScores.map((score) { + final queryDocumentSnapshot = MockQueryDocumentSnapshot(); + when(queryDocumentSnapshot.data).thenReturn({ + 'character': 'dash', + 'playerInitials': 'AAA', + 'score': score + }); + when(() => queryDocumentSnapshot.id).thenReturn('id$score'); + return queryDocumentSnapshot; + }).toList(); + when(() => querySnapshot.docs).thenReturn(queryDocumentSnapshots); - expect( - () => leaderboardRepository.addLeaderboardEntry(leaderboardEntry), - throwsA(isA()), + await leaderboardRepository.addLeaderboardEntry(leaderboardEntry); + verifyNever( + () => collectionReference.add(leaderboardEntry.toJson()), ); }); test( - 'throws FetchPlayerRankingException when score cannot be found ' - 'in firestore leaderboard data', () async { - when(() => documentReference.id).thenReturn('nonexistentDocumentId'); - + 'throws DeleteLeaderboardException ' + 'when deleting scores outside the top 10 fails', () async { + final deleteQuery = MockQuery(); + final deleteQuerySnapshot = MockQuerySnapshot(); + final newScore = LeaderboardEntryData( + playerInitials: 'ABC', + score: 15000, + character: CharacterType.android, + ); + final leaderboardScores = [ + 10000, + 9500, + 9000, + 8500, + 8000, + 7500, + 7000, + 6500, + 6000, + 5500, + 5000, + ]; + final deleteDocumentSnapshots = [5500, 5000].map((score) { + final queryDocumentSnapshot = MockQueryDocumentSnapshot(); + when(queryDocumentSnapshot.data).thenReturn({ + 'character': 'dash', + 'playerInitials': 'AAA', + 'score': score + }); + when(() => queryDocumentSnapshot.id).thenReturn('id$score'); + when(() => queryDocumentSnapshot.reference) + .thenReturn(documentReference); + return queryDocumentSnapshot; + }).toList(); + when(deleteQuery.get).thenAnswer((_) async => deleteQuerySnapshot); + when(() => deleteQuerySnapshot.docs) + .thenReturn(deleteDocumentSnapshots); + final queryDocumentSnapshots = leaderboardScores.map((score) { + final queryDocumentSnapshot = MockQueryDocumentSnapshot(); + when(queryDocumentSnapshot.data).thenReturn({ + 'character': 'dash', + 'playerInitials': 'AAA', + 'score': score + }); + when(() => queryDocumentSnapshot.id).thenReturn('id$score'); + when(() => queryDocumentSnapshot.reference) + .thenReturn(documentReference); + return queryDocumentSnapshot; + }).toList(); + when( + () => collectionReference.where('score', isLessThanOrEqualTo: 5500), + ).thenAnswer((_) => deleteQuery); + when(() => documentReference.delete()).thenThrow(Exception('oops')); + when(() => querySnapshot.docs).thenReturn(queryDocumentSnapshots); expect( - () => leaderboardRepository.addLeaderboardEntry(leaderboardEntry), - throwsA(isA()), + () => leaderboardRepository.addLeaderboardEntry(newScore), + throwsA(isA()), + ); + }); + + test( + 'saves the new score when there are more than 10 scores in the ' + 'leaderboard and the new score is higher than the lowest top 10, and ' + 'deletes the scores that are not in the top 10 anymore', () async { + final deleteQuery = MockQuery(); + final deleteQuerySnapshot = MockQuerySnapshot(); + final newScore = LeaderboardEntryData( + playerInitials: 'ABC', + score: 15000, + character: CharacterType.android, ); + final leaderboardScores = [ + 10000, + 9500, + 9000, + 8500, + 8000, + 7500, + 7000, + 6500, + 6000, + 5500, + 5000, + ]; + final deleteDocumentSnapshots = [5500, 5000].map((score) { + final queryDocumentSnapshot = MockQueryDocumentSnapshot(); + when(queryDocumentSnapshot.data).thenReturn({ + 'character': 'dash', + 'playerInitials': 'AAA', + 'score': score + }); + when(() => queryDocumentSnapshot.id).thenReturn('id$score'); + when(() => queryDocumentSnapshot.reference) + .thenReturn(documentReference); + return queryDocumentSnapshot; + }).toList(); + when(deleteQuery.get).thenAnswer((_) async => deleteQuerySnapshot); + when(() => deleteQuerySnapshot.docs) + .thenReturn(deleteDocumentSnapshots); + final queryDocumentSnapshots = leaderboardScores.map((score) { + final queryDocumentSnapshot = MockQueryDocumentSnapshot(); + when(queryDocumentSnapshot.data).thenReturn({ + 'character': 'dash', + 'playerInitials': 'AAA', + 'score': score + }); + when(() => queryDocumentSnapshot.id).thenReturn('id$score'); + when(() => queryDocumentSnapshot.reference) + .thenReturn(documentReference); + return queryDocumentSnapshot; + }).toList(); + when( + () => collectionReference.where('score', isLessThanOrEqualTo: 5500), + ).thenAnswer((_) => deleteQuery); + when(() => documentReference.delete()) + .thenAnswer((_) async => Future.value()); + when(() => querySnapshot.docs).thenReturn(queryDocumentSnapshots); + await leaderboardRepository.addLeaderboardEntry(newScore); + verify(() => collectionReference.add(newScore.toJson())).called(1); + verify(() => documentReference.delete()).called(2); }); }); diff --git a/packages/leaderboard_repository/test/src/models/leaderboard_ranking_test.dart b/packages/leaderboard_repository/test/src/models/leaderboard_ranking_test.dart deleted file mode 100644 index 577251e4..00000000 --- a/packages/leaderboard_repository/test/src/models/leaderboard_ranking_test.dart +++ /dev/null @@ -1,19 +0,0 @@ -import 'package:leaderboard_repository/leaderboard_repository.dart'; -import 'package:test/test.dart'; - -void main() { - group('LeaderboardRanking', () { - test('can be instantiated', () { - const leaderboardRanking = LeaderboardRanking(ranking: 1, outOf: 1); - - expect(leaderboardRanking, isNotNull); - }); - - test('supports value equality.', () { - const leaderboardRanking = LeaderboardRanking(ranking: 1, outOf: 1); - const leaderboardRanking2 = LeaderboardRanking(ranking: 1, outOf: 1); - - expect(leaderboardRanking, equals(leaderboardRanking2)); - }); - }); -}