Issue #77

- fixed bug in person getter - reduced number of http-calls when fetching affiliations
DISCOOS · Sep 6, 2020 · 0b6e70c · 0b6e70c
1 parent 3e9f8ec
commit 0b6e70c
Show file tree

Hide file tree

Showing 11 changed files with 140 additions and 69 deletions.
diff --git a/lib/core/domain/repository.dart b/lib/core/domain/repository.dart
@@ -158,14 +158,14 @@ abstract class ConnectionAwareRepository<K, T extends Aggregate, U extends Servi
   /// Get all states as unmodifiable map
   Map<K, StorageState<T>> get states => Map.unmodifiable(
         isReady
-            ? _states.toMap().map((key, value) => MapEntry(
-                key,
-                _StorageState<T>(
-                  (value) => _onGet == null ? value : _onGet(value),
-                  value,
-                )))
+            ? _states.toMap().map((key, value) => MapEntry(key, _StorageState<T>(_toValue, value)))
             : <K, StorageState<T>>{},
       );
+
+  T _toValue(T value) {
+    return _onGet == null ? value : _onGet(value);
+  }
+
   Box<StorageState<T>> _states;
 
   /// Get all (key,value)-pairs as unmodifiable map
@@ -596,6 +596,14 @@ abstract class ConnectionAwareRepository<K, T extends Aggregate, U extends Servi
     );
   }
 
+  /// Should handle missing dependency.
+  ///
+  /// Default handling is to return current state.
+  ///
+  /// Any [Exception] will be forwarded to [onError]
+  @visibleForOverriding
+  Future<StorageState<T>> onNotFound(StorageState<T> state, ServiceResponse response) => Future.value(state);
+
   /// Should handle errors
   @mustCallSuper
   @visibleForOverriding
@@ -858,6 +866,8 @@ abstract class ConnectionAwareRepository<K, T extends Aggregate, U extends Servi
       } on ServiceException catch (e) {
         if (e.is409) {
           return onResolve(state, e.response);
+        } else if (e.is404) {
+          return onNotFound(state, e.response);
         }
         rethrow;
       }

diff --git a/lib/core/extensions.dart b/lib/core/extensions.dart
@@ -35,4 +35,17 @@ extension MapX on Map {
 
 extension IterableX<T> on Iterable<T> {
   T get firstOrNull => this.isNotEmpty ? this.first : null;
+  Iterable<T> whereNotNull([dynamic map(T element)]) =>
+      where((element) => map == null ? element != null : map(element) != null);
+
+  Iterable<T> toPage({int offset = 0, int limit = 20}) {
+    if (offset < 0 || limit < 0) {
+      throw ArgumentError('Offset and limit can not be negative');
+    } else if (offset > length) {
+      throw ArgumentError('Index out of bounds: offset $offset > length $length');
+    } else if (offset == 0 && limit == 0) {
+      return toList();
+    }
+    return skip(offset).take(limit);
+  }
 }
diff --git a/lib/features/affiliation/data/repositories/affiliation_repository_impl.dart b/lib/features/affiliation/data/repositories/affiliation_repository_impl.dart
@@ -1,6 +1,8 @@
 import 'dart:async';
 import 'dart:io';
 
+import 'package:flutter/foundation.dart';
+
 import 'package:SarSys/features/affiliation/data/models/affiliation_model.dart';
 import 'package:SarSys/features/affiliation/data/services/affiliation_service.dart';
 import 'package:SarSys/features/affiliation/domain/entities/Affiliation.dart';
@@ -11,8 +13,8 @@ import 'package:SarSys/features/affiliation/domain/repositories/division_reposit
 import 'package:SarSys/features/affiliation/domain/repositories/organisation_repository.dart';
 import 'package:SarSys/features/affiliation/domain/repositories/person_repository.dart';
 import 'package:SarSys/core/data/services/service.dart';
-import 'package:flutter/foundation.dart';
 
+import 'package:SarSys/core/extensions.dart';
 import 'package:SarSys/core/data/storage.dart';
 import 'package:SarSys/core/data/services/connectivity_service.dart';
 import 'package:SarSys/core/domain/repository.dart';
@@ -67,6 +69,7 @@ class AffiliationRepositoryImpl extends ConnectionAwareRepository<String, Affili
     );
     return _fetch(
       keys,
+      replace: true,
       onRemote: onRemote,
     );
   }
@@ -106,38 +109,25 @@ class AffiliationRepositoryImpl extends ConnectionAwareRepository<String, Affili
   }) async {
     scheduleLoad(
       () async {
-        final values = <Affiliation>[];
-        final errors = <ServiceResponse>[];
-        for (var uuid in uuids) {
-          // Do not attempt to load local values
-          final state = getState(uuid);
-          if (state == null || state?.shouldLoad == true) {
-            final response = await service.get(uuid);
-            if (response.is200) {
-              values.add(response.body);
-            } else {
-              errors.add(response);
-            }
-          } else {
-            values.add(state.value);
-          }
-        }
-        if (errors.isNotEmpty) {
-          return ServiceResponse<List<Affiliation>>(
-            body: values,
-            error: errors,
-            statusCode: values.isNotEmpty ? HttpStatus.partialContent : errors.first.statusCode,
-            reasonPhrase: values.isNotEmpty ? 'Partial fetch failure' : 'Fetch failed',
+        // Keep local values! Will be
+        // overwritten by remote values
+        // if exists. If replace = true,
+        // this will remove local values
+        // with remote state.
+        final next = states.values.where((state) => state.isLocal).map((state) => state.value).toList();
+        final response = await service.getAll(uuids);
+        if (response.is200) {
+          next.addAll(response.body);
+          return ServiceResponse.ok<List<Affiliation>>(
+            body: next,
           );
         }
-        return ServiceResponse.ok<List<Affiliation>>(
-          body: values,
-        );
+        return response;
       },
       onResult: onRemote,
       shouldEvict: replace,
     );
-    return uuids.map((uuid) => get(uuid)).toList();
+    return uuids.map((uuid) => get(uuid)).whereNotNull().toList();
   }
 
   Future<List<Affiliation>> _search(
@@ -173,7 +163,7 @@ class AffiliationRepositoryImpl extends ConnectionAwareRepository<String, Affili
 
   @override
   Future<Iterable<Affiliation>> onReset({Iterable<Affiliation> previous}) => _fetch(
-        previous.map((a) => a.uuid).toList(),
+        (previous ?? values).map((a) => a.uuid).toList(),
         replace: true,
       );
 

diff --git a/lib/features/affiliation/data/services/affiliation_service.chopper.dart b/lib/features/affiliation/data/services/affiliation_service.chopper.dart
diff --git a/lib/features/affiliation/data/services/affiliation_service.dart b/lib/features/affiliation/data/services/affiliation_service.dart
@@ -1,8 +1,11 @@
 import 'dart:async';
+
+import 'package:chopper/chopper.dart';
+
 import 'package:SarSys/core/data/api.dart';
 import 'package:SarSys/features/affiliation/domain/entities/Affiliation.dart';
 import 'package:SarSys/core/data/services/service.dart';
-import 'package:chopper/chopper.dart';
+import 'package:SarSys/core/extensions.dart';
 
 part 'affiliation_service.chopper.dart';
 
@@ -20,7 +23,7 @@ class AffiliationService with ServiceGet<Affiliation> implements ServiceDelegate
         filter,
         offset: offset,
         limit: limit,
-        expand: const ['person'],
+        expand: 'person',
       ),
     );
   }
@@ -29,11 +32,41 @@ class AffiliationService with ServiceGet<Affiliation> implements ServiceDelegate
     return Api.from<Affiliation, Affiliation>(
       await delegate.get(
         uuid: uuid,
-        expand: const ['person'],
+        expand: 'person',
+      ),
+    );
+  }
+
+  Future<ServiceResponse<List<Affiliation>>> _fetch(List<String> uuids, int offset, int limit) async {
+    return Api.from<PagedList<Affiliation>, List<Affiliation>>(
+      await delegate.getAll(
+        // Limit query string length
+        uuids?.toPage(offset: offset, limit: limit)?.join(','),
+        expand: 'person',
+        offset: 0,
+        limit: limit,
       ),
     );
   }
 
+  Future<ServiceResponse<List<Affiliation>>> getAll(List<String> uuids) async {
+    var offset = 0;
+    final limit = 20;
+    final body = <Affiliation>[];
+    // This method limits the length of each query
+    var response = await _fetch(uuids, offset, limit);
+    while (response.is200) {
+      body.addAll(response.body);
+      offset += limit;
+      if (offset < uuids.length) {
+        response = await _fetch(uuids, offset, limit);
+      } else {
+        return ServiceResponse.ok(body: body);
+      }
+    }
+    return response;
+  }
+
   Future<ServiceResponse<Affiliation>> create(Affiliation affiliation) async {
     return Api.from<String, Affiliation>(
       await delegate.create(
@@ -69,15 +102,23 @@ abstract class AffiliationServiceImpl extends ChopperService {
   @Get(path: '{uuid}')
   Future<Response<Affiliation>> get({
     @Path('uuid') String uuid,
-    @Query('expand') List<String> expand = const [],
+    @Query('expand') String expand,
+  });
+
+  @Get()
+  Future<Response<PagedList<Affiliation>>> getAll(
+    @Query('uuids') String uuids, {
+    @Query('expand') String expand,
+    @Query('limit') int limit = 20,
+    @Query('offset') int offset = 0,
   });
 
   @Get()
   Future<Response<PagedList<Affiliation>>> search(
     @Query('filter') String filter, {
-    @Query('offset') int offset = 0,
+    @Query('expand') String expand,
     @Query('limit') int limit = 20,
-    @Query('expand') List<String> expand = const [],
+    @Query('offset') int offset = 0,
   });
 
   @Post()

diff --git a/lib/features/affiliation/presentation/blocs/affiliation_bloc.dart b/lib/features/affiliation/presentation/blocs/affiliation_bloc.dart
@@ -856,7 +856,7 @@ class AffiliationBloc extends BaseBloc<AffiliationCommand, AffiliationState, Aff
     List<Affiliation> values, {
     @required bool isRemote,
   }) {
-    final updated = values.where((a) => a.person != null).map((a) => _toPerson(a, isRemote: isRemote)).toList();
+    final updated = values.whereNotNull((a) => a.person).map((a) => _toPerson(a, isRemote: isRemote)).toList();
     return updated;
   }
 

diff --git a/lib/features/mapping/data/services/background_geolocation_service.dart b/lib/features/mapping/data/services/background_geolocation_service.dart
@@ -188,7 +188,7 @@ class BackgroundGeolocationService implements LocationService {
     _duuid = duuid ?? _duuid;
     _token = token ?? _token;
     _share = share ?? _share;
-    debugPrint(_toUrl());
+    debugPrint('bg.url: ${_toUrl()}');
     return bg.Config(
       batchSync: true,
       maxBatchSize: 10,

diff --git a/lib/features/personnel/presentation/blocs/personnel_bloc.dart b/lib/features/personnel/presentation/blocs/personnel_bloc.dart
@@ -59,7 +59,10 @@ class PersonnelBloc extends BaseBloc<PersonnelCommand, PersonnelState, Personnel
 
     // Keep personnel in sync with person
     repo.onValue(
-      onGet: (value) => value.withPerson(affiliationBloc.persons[value.person?.uuid]),
+      onGet: (value) {
+        final affiliation = affiliationBloc.repo[value.affiliation?.uuid];
+        return value.withPerson(affiliation?.person);
+      },
     );
   }
 
@@ -109,7 +112,7 @@ class PersonnelBloc extends BaseBloc<PersonnelCommand, PersonnelState, Personnel
         final duplicate = event.from.value.uuid;
         final existing = PersonModel.fromJson(event.conflict.base);
         find(
-          where: (personnel) => personnel.person.uuid == duplicate,
+          where: (personnel) => personnel.person?.uuid == duplicate,
         ).map((personnel) => personnel.withPerson(existing)).forEach(update);
       }
     } catch (error, stackTrace) {

diff --git a/lib/features/personnel/presentation/pages/personnels_page.dart b/lib/features/personnel/presentation/pages/personnels_page.dart
@@ -148,7 +148,7 @@ class PersonnelsPageState extends State<PersonnelsPage> {
             },
             groupBy: (personnel) {
               final affiliation = _toAffiliation(personnel);
-              final org = affiliationBloc.orgs[affiliation.org?.uuid];
+              final org = affiliationBloc.orgs[affiliation?.org?.uuid];
               return AffiliationGroupEntry(
                 prefix: org?.prefix ?? '0',
                 name: affiliationBloc

diff --git a/test/blocs/affiliation_tests.dart b/test/blocs/affiliation_tests.dart
@@ -152,7 +152,7 @@ void main() async {
         );
 
         // Act
-        await _authenticate(harness);
+        await _authenticate(harness, exists: true);
 
         // Assert
         await expectThroughLater(harness.affiliationBloc, emits(isA<UserOnboarded>()));
@@ -329,6 +329,7 @@ Future _seed(
 Future _authenticate(
   BlocTestHarness harness, {
   bool reset = true,
+  bool exists = false,
 }) async {
   await harness.userBloc.login(
     username: UNTRUSTED,
@@ -348,19 +349,20 @@ Future _authenticate(
       isTrue,
     )),
   );
-  await expectStorageStatusLater(
-    harness.affiliationBloc.persons.values.first.uuid,
-    harness.affiliationBloc.persons,
-    StorageStatus.created,
-    remote: true,
-  );
-  await expectStorageStatusLater(
-    harness.affiliationBloc.repo.values.first.uuid,
-    harness.affiliationBloc.repo,
-    StorageStatus.created,
-    remote: true,
-  );
-
+  if (!exists) {
+    await expectStorageStatusLater(
+      harness.affiliationBloc.persons.values.first.uuid,
+      harness.affiliationBloc.persons,
+      StorageStatus.created,
+      remote: true,
+    );
+    await expectStorageStatusLater(
+      harness.affiliationBloc.repo.values.first.uuid,
+      harness.affiliationBloc.repo,
+      StorageStatus.created,
+      remote: true,
+    );
+  }
   if (reset) {
     clearInteractions(harness.organisationService);
   }

diff --git a/test/mock/affiliation_service_mock.dart b/test/mock/affiliation_service_mock.dart
@@ -145,6 +145,17 @@ class AffiliationServiceMock extends Mock implements AffiliationService {
         message: "Affiliation not found: $uuid",
       );
     });
+    when(mock.getAll(any)).thenAnswer((_) async {
+      await _doThrottle();
+      final uuids = List<String>.from(_.positionalArguments[0]);
+      final affiliations = uuids
+          .where((uuid) => affiliationRepo.containsKey(uuid))
+          .map((uuid) => _withPerson(affiliationRepo, uuid, persons))
+          .toList();
+      return ServiceResponse.ok(
+        body: affiliations,
+      );
+    });
     when(mock.create(any)).thenAnswer((_) async {
       await _doThrottle();
       final affiliation = _toAffiliation(_);