Refactor NetworkRequestExecutor; utilise Unit; remove need for du…

…mmy providers targeting `dynamic` types (#503)
AnalogIO · Sep 5, 2023 · ececdfb · ececdfb
1 parent 72a7af1
commit ececdfb
Show file tree

Hide file tree

Showing 49 changed files with 363 additions and 495 deletions.
diff --git a/lib/core/data/datasources/account_remote_data_source.dart b/lib/core/data/datasources/account_remote_data_source.dart
@@ -1,5 +1,4 @@
 import 'package:coffeecard/core/errors/failures.dart';
-import 'package:coffeecard/core/extensions/either_extensions.dart';
 import 'package:coffeecard/core/network/network_request_executor.dart';
 import 'package:coffeecard/features/user/data/models/user_model.dart';
 import 'package:coffeecard/features/user/domain/entities/user.dart';
@@ -24,44 +23,35 @@ class AccountRemoteDataSource {
   Future<Either<NetworkFailure, AuthenticatedUser>> login(
     String email,
     String encodedPasscode,
-  ) async {
-    return executor(
-      () => apiV1.apiV1AccountLoginPost(
-        body: LoginDto(
-          email: email,
-          password: encodedPasscode,
-          version: ApiUriConstants.minAppVersion,
-        ),
-      ),
-    ).bindFuture(
-      (result) => AuthenticatedUser(
-        email: email,
-        token: result.token!,
-      ),
-    );
+  ) {
+    return executor
+        .execute(
+          () => apiV1.apiV1AccountLoginPost(
+            body: LoginDto(
+              email: email,
+              password: encodedPasscode,
+              version: ApiUriConstants.minAppVersion,
+            ),
+          ),
+        )
+        .map((result) => AuthenticatedUser(email: email, token: result.token!));
   }
 
-  Future<Either<NetworkFailure, User>> getUser() async {
-    return executor(
-      apiV2.apiV2AccountGet,
-    ).bindFuture(UserModel.fromResponse);
+  Future<Either<NetworkFailure, User>> getUser() {
+    return executor.execute(apiV2.apiV2AccountGet).map(UserModel.fromResponse);
   }
 
-  Future<Either<NetworkFailure, void>> requestPasscodeReset(
-    String email,
-  ) async {
-    final result = await executor(
-      () => apiV1.apiV1AccountForgotpasswordPost(body: EmailDto(email: email)),
+  Future<Either<NetworkFailure, Unit>> requestPasscodeReset(String email) {
+    final body = EmailDto(email: email);
+    return executor.executeAndDiscard(
+      () => apiV1.apiV1AccountForgotpasswordPost(body: body),
     );
-
-    return result.pure(null);
   }
 
-  Future<Either<NetworkFailure, bool>> emailExists(String email) async {
-    return executor(
-      () => apiV2.apiV2AccountEmailExistsPost(
-        body: EmailExistsRequest(email: email),
-      ),
-    ).bindFuture((result) => result.emailExists);
+  Future<Either<NetworkFailure, bool>> emailExists(String email) {
+    final body = EmailExistsRequest(email: email);
+    return executor
+        .execute(() => apiV2.apiV2AccountEmailExistsPost(body: body))
+        .map((result) => result.emailExists);
   }
 }
diff --git a/lib/core/network/network_request_executor.dart b/lib/core/network/network_request_executor.dart
@@ -4,6 +4,11 @@ import 'package:coffeecard/utils/firebase_analytics_event_logging.dart';
 import 'package:fpdart/fpdart.dart';
 import 'package:logger/logger.dart';
 
+part 'network_request_executor_mapping.dart';
+
+typedef _NetworkRequest<BodyType> = Future<Response<BodyType>> Function();
+typedef _ExecutorResult<R> = Future<Either<NetworkFailure, R>>;
+
 class NetworkRequestExecutor {
   final Logger logger;
   final FirebaseAnalyticsEventLogging firebaseLogger;
@@ -13,35 +18,56 @@ class NetworkRequestExecutor {
     required this.firebaseLogger,
   });
 
-  Future<Either<NetworkFailure, Result>> call<Result>(
-    Future<Response<Result>> Function() request,
-  ) async {
+  /// Executes a network request and returns an [Either].
+  ///
+  /// If the request fails, a [NetworkFailure] is returned in a [Left].
+  /// If the request succeeds, the response body of type
+  /// [Body] is returned in a [Right].
+  ///
+  /// If the response body type is empty or dynamic, use [executeAndDiscard]
+  /// instead, which always returns [Unit] in a [Right] if the request succeeds.
+  _ExecutorResult<Body> execute<Body>(_NetworkRequest<Body> request) async {
     try {
       final response = await request();
 
       // request is successful if response code is >= 200 && <300
       if (!response.isSuccessful) {
-        logResponse(response);
+        _logResponse(response);
         return Left(ServerFailure.fromResponse(response));
       }
-
-      return Right(response.body as Result);
+      return Right(response.body as Body);
     } on Exception catch (e) {
       // could not connect to backend for whatever reason
       logger.e(e.toString());
       return const Left(ConnectionFailure());
     }
   }
 
-  void logResponse(Response response) {
+  /// Executes the network [request] and returns the result as an [Either].
+  ///
+  /// If the request fails, a [NetworkFailure] is returned in a [Left].
+  /// If the request succeeds, [Unit] is returned in a [Right] and
+  /// the orignial response body is discarded.
+  ///
+  /// This method is useful as it allows to discard the response body when its
+  /// type is dynamic, e.g. when it is expected to be empty. This avoids having
+  /// to provide dummy values for dynamic response bodies in Mockito tests.
+  _ExecutorResult<Unit> executeAndDiscard<B>(_NetworkRequest<B> request) async {
+    final result = await execute(request);
+    return result.map((_) => unit);
+  }
+
+  /// Logs the response to the console and to Firebase.
+  ///
+  /// Does not log 401 responses to Firebase since these are expected when
+  /// the user is not logged in.
+  void _logResponse<Body>(Response<Body> response) {
     logger.e(response.toString());
 
-    final ignore = [401];
+    final ignoreCodes = [401];
 
-    if (ignore.contains(response.statusCode)) {
-      return;
+    if (!ignoreCodes.contains(response.statusCode)) {
+      firebaseLogger.errorEvent(response.toString());
     }
-
-    firebaseLogger.errorEvent(response.toString());
   }
 }
diff --git a/lib/core/network/network_request_executor_mapping.dart b/lib/core/network/network_request_executor_mapping.dart
@@ -0,0 +1,19 @@
+part of 'network_request_executor.dart';
+
+extension ExecutorMapX<R> on _ExecutorResult<R> {
+  /// If the result of the [Future] is a [Right], maps the value to a [C].
+  _ExecutorResult<C> map<C>(C Function(R) mapper) async {
+    final result = await this;
+    return result.map(mapper);
+  }
+}
+
+extension ExecutorMapAllX<R> on _ExecutorResult<Iterable<R>> {
+  /// If the result of the [Future] is a [Right],
+  /// maps all values to a [List] of [C].
+  // TODO(marfavi): return Iterable instead of List?
+  _ExecutorResult<List<C>> mapAll<C>(C Function(R) mapper) async {
+    final result = await this;
+    return result.map((items) => items.map(mapper).toList());
+  }
+}
diff --git a/lib/features/environment/data/datasources/environment_remote_data_source.dart b/lib/features/environment/data/datasources/environment_remote_data_source.dart
@@ -1,5 +1,4 @@
 import 'package:coffeecard/core/errors/failures.dart';
-import 'package:coffeecard/core/extensions/either_extensions.dart';
 import 'package:coffeecard/core/network/network_request_executor.dart';
 import 'package:coffeecard/features/environment/domain/entities/environment.dart';
 import 'package:coffeecard/generated/api/coffeecard_api_v2.swagger.dart';
@@ -14,9 +13,9 @@ class EnvironmentRemoteDataSource {
   final CoffeecardApiV2 apiV2;
   final NetworkRequestExecutor executor;
 
-  Future<Either<NetworkFailure, Environment>> getEnvironmentType() async {
-    return executor(
-      apiV2.apiV2AppconfigGet,
-    ).bindFuture(Environment.fromAppConfig);
+  Future<Either<NetworkFailure, Environment>> getEnvironmentType() {
+    return executor
+        .execute(apiV2.apiV2AppconfigGet)
+        .map(Environment.fromAppConfig);
   }
 }
diff --git a/lib/features/leaderboard/data/datasources/leaderboard_remote_data_source.dart b/lib/features/leaderboard/data/datasources/leaderboard_remote_data_source.dart
@@ -1,5 +1,4 @@
 import 'package:coffeecard/core/errors/failures.dart';
-import 'package:coffeecard/core/extensions/either_extensions.dart';
 import 'package:coffeecard/core/network/network_request_executor.dart';
 import 'package:coffeecard/features/leaderboard/data/models/leaderboard_user_model.dart';
 import 'package:coffeecard/features/leaderboard/domain/entities/leaderboard_user.dart';
@@ -27,19 +26,19 @@ class LeaderboardRemoteDataSource {
   Future<Either<NetworkFailure, List<LeaderboardUser>>> getLeaderboard(
     LeaderboardFilter category,
     int top,
-  ) async {
-    return executor(
-      () => apiV2.apiV2LeaderboardTopGet(preset: category.label, top: top),
-    ).bindFuture(
-      (result) => result.map(LeaderboardUserModel.fromDTO).toList(),
-    );
+  ) {
+    return executor
+        .execute(
+          () => apiV2.apiV2LeaderboardTopGet(preset: category.label, top: top),
+        )
+        .mapAll(LeaderboardUserModel.fromDTO);
   }
 
   Future<Either<NetworkFailure, LeaderboardUser>> getLeaderboardUser(
     LeaderboardFilter category,
-  ) async {
-    return executor(
-      () => apiV2.apiV2LeaderboardGet(preset: category.label),
-    ).bindFuture(LeaderboardUserModel.fromDTO);
+  ) {
+    return executor
+        .execute(() => apiV2.apiV2LeaderboardGet(preset: category.label))
+        .map(LeaderboardUserModel.fromDTO);
   }
 }
diff --git a/lib/features/occupation/data/datasources/occupation_remote_data_source.dart b/lib/features/occupation/data/datasources/occupation_remote_data_source.dart
@@ -13,11 +13,9 @@ class OccupationRemoteDataSource {
     required this.executor,
   });
 
-  Future<Either<NetworkFailure, List<OccupationModel>>> getOccupations() async {
-    final result = await executor(
-      () => api.apiV1ProgrammesGet(),
-    );
-    return result
-        .map((result) => result.map(OccupationModel.fromDTOV1).toList());
+  Future<Either<NetworkFailure, List<OccupationModel>>> getOccupations() {
+    return executor
+        .execute(api.apiV1ProgrammesGet)
+        .mapAll(OccupationModel.fromDTOV1);
   }
 }
diff --git a/lib/features/opening_hours/data/datasources/opening_hours_remote_data_source.dart b/lib/features/opening_hours/data/datasources/opening_hours_remote_data_source.dart
@@ -1,5 +1,7 @@
 import 'package:coffeecard/core/errors/failures.dart';
 import 'package:coffeecard/core/network/network_request_executor.dart';
+import 'package:coffeecard/features/opening_hours/data/models/opening_hours_model.dart';
+import 'package:coffeecard/features/opening_hours/domain/entities/opening_hours.dart';
 import 'package:coffeecard/generated/api/shiftplanning_api.swagger.dart';
 import 'package:fpdart/fpdart.dart';
 
@@ -16,18 +18,17 @@ class OpeningHoursRemoteDataSource {
   final shortkey = 'analog';
 
   /// Check if the cafe is open.
-  Future<Either<Failure, bool>> isOpen() async {
-    final result = await executor(
-      () async => api.apiOpenShortKeyGet(shortKey: shortkey),
-    );
-
-    return result.map((result) => result.open);
+  Future<Either<Failure, bool>> isOpen() {
+    return executor
+        .execute(() => api.apiOpenShortKeyGet(shortKey: shortkey))
+        .map((result) => result.open);
   }
 
-  /// Get the opening hours of the cafe.
-  Future<Either<Failure, List<OpeningHoursDTO>>> getOpeningHours() async {
-    return executor(
-      () => api.apiShiftsShortKeyGet(shortKey: shortkey),
-    );
+  /// Get the opening hours of the cafe, including today's opening hours and
+  /// the opening hours for the next 7 days.
+  Future<Either<Failure, OpeningHours>> getOpeningHours() {
+    return executor
+        .execute(() => api.apiShiftsShortKeyGet(shortKey: shortkey))
+        .map(OpeningHoursModel.fromDTO);
   }
 }
diff --git a/...tories/opening_hours_repository_impl.dart → ...ours/data/models/opening_hours_model.dart b/...tories/opening_hours_repository_impl.dart → ...ours/data/models/opening_hours_model.dart
@@ -1,37 +1,31 @@
 import 'package:coffeecard/base/strings.dart';
-import 'package:coffeecard/core/errors/failures.dart';
 import 'package:coffeecard/core/extensions/string_extensions.dart';
 import 'package:coffeecard/features/opening_hours/domain/entities/opening_hours.dart';
-import 'package:coffeecard/features/opening_hours/opening_hours.dart';
-import 'package:coffeecard/generated/api/shiftplanning_api.swagger.dart';
+import 'package:coffeecard/generated/api/shiftplanning_api.models.swagger.dart';
 import 'package:coffeecard/models/opening_hours_day.dart';
-import 'package:fpdart/fpdart.dart';
 
-class OpeningHoursRepositoryImpl implements OpeningHoursRepository {
-  final OpeningHoursRemoteDataSource dataSource;
+class OpeningHoursModel extends OpeningHours {
+  const OpeningHoursModel({
+    required super.allOpeningHours,
+    required super.todaysOpeningHours,
+  });
 
-  OpeningHoursRepositoryImpl({required this.dataSource});
-
-  @override
-  Future<Either<Failure, OpeningHours>> getOpeningHours(int weekday) async {
-    final openingHours = await dataSource.getOpeningHours();
-
-    return openingHours.map((openingHours) {
-      final openingHoursMap = transformOpeningHours(openingHours);
+  factory OpeningHoursModel.fromDTO(List<OpeningHoursDTO> allShifts) {
+    final openingHours = _transformOpeningHours(allShifts);
+    final todaysOpeningHours = _calculateTodaysOpeningHours(
+      DateTime.now().weekday,
+      openingHours,
+    );
 
-      return OpeningHours(
-        allOpeningHours: openingHoursMap,
-        todaysOpeningHours: calculateTodaysOpeningHours(
-          weekday,
-          openingHoursMap,
-        ),
-      );
-    });
+    return OpeningHoursModel(
+      allOpeningHours: openingHours,
+      todaysOpeningHours: todaysOpeningHours,
+    );
   }
 
-  // An [OpeningHoursDTO] actually represents a barista shift, so "dto.start"
-  // means the start of the shift and "dto.end" means the end of the shift.
-  Map<int, String> transformOpeningHours(List<OpeningHoursDTO> allShifts) {
+  static Map<int, String> _transformOpeningHours(
+    List<OpeningHoursDTO> allShifts,
+  ) {
     final shiftsByWeekday = <int, List<OpeningHoursDTO>>{
       DateTime.monday: [],
       DateTime.tuesday: [],
@@ -47,6 +41,11 @@ class OpeningHoursRepositoryImpl implements OpeningHoursRepository {
       shiftsByWeekday[weekday]!.add(shift);
     }
 
+    // For each weekday, map the shifts to a string representation of the
+    // opening hours e.g '8 - 16' or 'Closed'
+    //
+    // This assumes that the shifts are sorted by start time and that there are
+    // no overlapping shifts.
     return shiftsByWeekday.map(
       (day, shifts) => MapEntry(
         day,
@@ -58,8 +57,8 @@ class OpeningHoursRepositoryImpl implements OpeningHoursRepository {
   }
 
   /// Return the current weekday and the corresponding opening hours e.g
-  /// 'Monday: 8 - 16'
-  String calculateTodaysOpeningHours(
+  /// 'Mondays: 8 - 16'
+  static String _calculateTodaysOpeningHours(
     int weekday,
     Map<int, String> openingHours,
   ) {

diff --git a/lib/features/opening_hours/domain/repositories/opening_hours_repository.dart b/lib/features/opening_hours/domain/repositories/opening_hours_repository.dart
diff --git a/lib/features/opening_hours/domain/usecases/check_open_status.dart b/lib/features/opening_hours/domain/usecases/check_open_status.dart
@@ -1,6 +1,6 @@
 import 'package:coffeecard/core/errors/failures.dart';
 import 'package:coffeecard/core/usecases/usecase.dart';
-import 'package:coffeecard/features/opening_hours/opening_hours.dart';
+import 'package:coffeecard/features/opening_hours/data/datasources/opening_hours_remote_data_source.dart';
 import 'package:fpdart/fpdart.dart';
 
 class CheckOpenStatus implements UseCase<bool, NoParams> {