From 3f75c263a2ff3dabf18a3fc59e65e88dea3e8722 Mon Sep 17 00:00:00 2001 From: dab246 Date: Sat, 23 Nov 2024 02:22:27 +0700 Subject: [PATCH 1/2] TF-3298 Fix blink when refresh email list --- .../data/extensions/list_email_extension.dart | 22 +- .../thread/data/network/thread_api.dart | 49 ++--- ...ting_list_email_by_order_id_list_test.dart | 54 ----- .../extensions/list_email_extension_test.dart | 143 +++++++++++++ .../thread/data/network/thread_api_test.dart | 195 ++++++++++++++++++ 5 files changed, 372 insertions(+), 91 deletions(-) delete mode 100644 test/features/email/sorting_list_email_by_order_id_list_test.dart create mode 100644 test/features/thread/data/extensions/list_email_extension_test.dart diff --git a/lib/features/thread/data/extensions/list_email_extension.dart b/lib/features/thread/data/extensions/list_email_extension.dart index 12aebbbb7f..59ec5cbcb4 100644 --- a/lib/features/thread/data/extensions/list_email_extension.dart +++ b/lib/features/thread/data/extensions/list_email_extension.dart @@ -17,23 +17,17 @@ extension ListEmailExtension on List { }; } - List sortingByOrderOfIdList(List ids) { - if (ids.length != length) { - return this; - } + List sortEmailsById(List referenceIds) { + final indexMap = { + for (var i = 0; i < referenceIds.length; i++) + referenceIds[i]: i + }; sort((email1, email2) { - final id1 = email1.id?.id; - final id2 = email2.id?.id; - - if (id1 == null || id2 == null) { - return 0; - } - - final index1 = ids.indexWhere((id) => id == id1); - final index2 = ids.indexWhere((id) => id == id2); + final indexEmail1 = indexMap[email1.id!.id] ?? double.maxFinite; + final indexEmail2 = indexMap[email2.id!.id] ?? double.maxFinite; - return index1.compareTo(index2); + return indexEmail1.compareTo(indexEmail2); }); return this; diff --git a/lib/features/thread/data/network/thread_api.dart b/lib/features/thread/data/network/thread_api.dart index 863309f1cc..bdb0b57d47 100644 --- a/lib/features/thread/data/network/thread_api.dart +++ b/lib/features/thread/data/network/thread_api.dart @@ -20,7 +20,6 @@ import 'package:jmap_dart_client/jmap/mail/email/get/get_email_method.dart'; import 'package:jmap_dart_client/jmap/mail/email/get/get_email_response.dart'; import 'package:jmap_dart_client/jmap/mail/email/query/query_email_method.dart'; import 'package:jmap_dart_client/jmap/mail/email/query/query_email_response.dart'; -import 'package:model/extensions/list_email_extension.dart'; import 'package:tmail_ui_user/features/thread/data/extensions/list_email_extension.dart'; import 'package:jmap_dart_client/jmap/mail/email/search_snippet/search_snippet.dart'; import 'package:jmap_dart_client/jmap/mail/email/search_snippet/search_snippet_get_method.dart'; @@ -91,24 +90,33 @@ class ThreadAPI { QueryEmailResponse.deserialize, ); - List? emailList; + final emailList = sortEmails( + getEmailResponse: responseOfGetEmailMethod, + queryEmailResponse: responseOfQueryEmailMethod, + ); - if (responseOfGetEmailMethod?.list.isNotEmpty == true && - responseOfQueryEmailMethod?.ids.isNotEmpty == true) { - log('ThreadAPI::getAllEmail: QUERY_EMAIL_IDS = ${responseOfQueryEmailMethod?.ids}'); - final listSortedEmail = responseOfGetEmailMethod!.list - .sortingByOrderOfIdList(responseOfQueryEmailMethod!.ids.toList()); - emailList = listSortedEmail; - } else { - emailList = responseOfGetEmailMethod?.list; - } - log('ThreadAPI::getAllEmail: EMAIL_DISPLAYED_IDS = ${emailList?.listEmailIds}'); return EmailsResponse( emailList: emailList, state: responseOfGetEmailMethod?.state, ); } + List? sortEmails({ + GetEmailResponse? getEmailResponse, + QueryEmailResponse? queryEmailResponse, + }) { + final listEmails = getEmailResponse?.list; + final listIds = queryEmailResponse?.ids.toList(); + + if (listEmails?.isNotEmpty != true || listIds?.isNotEmpty != true) { + return listEmails; + } + + final listSortedEmails = listEmails!.sortEmailsById(listIds!); + + return listSortedEmails; + } + Future searchEmails( Session session, AccountId accountId, @@ -166,21 +174,16 @@ class ThreadAPI { .build() .execute(); - final emailResultList = result.parse( + final responseOfGetEmailMethod = result.parse( getEmailInvocation.methodCallId, GetEmailResponse.deserialize); final responseOfQueryEmailMethod = result.parse( queryEmailInvocation.methodCallId, QueryEmailResponse.deserialize); - List? sortedEmailList; - - if (emailResultList?.list.isNotEmpty == true && - responseOfQueryEmailMethod?.ids.isNotEmpty == true) { - sortedEmailList = emailResultList!.list - .sortingByOrderOfIdList(responseOfQueryEmailMethod!.ids.toList()); - } else { - sortedEmailList = emailResultList?.list; - } + final sortedEmailList = sortEmails( + getEmailResponse: responseOfGetEmailMethod, + queryEmailResponse: responseOfQueryEmailMethod, + ); final searchSnippets = _getSearchSnippetsFromResponse( result, @@ -188,7 +191,7 @@ class ThreadAPI { ); return SearchEmailsResponse( emailList: sortedEmailList, - state: emailResultList?.state, + state: responseOfGetEmailMethod?.state, searchSnippets: searchSnippets); } diff --git a/test/features/email/sorting_list_email_by_order_id_list_test.dart b/test/features/email/sorting_list_email_by_order_id_list_test.dart deleted file mode 100644 index ab17742d2e..0000000000 --- a/test/features/email/sorting_list_email_by_order_id_list_test.dart +++ /dev/null @@ -1,54 +0,0 @@ -import 'package:flutter_test/flutter_test.dart'; -import 'package:jmap_dart_client/jmap/core/id.dart'; -import 'package:jmap_dart_client/jmap/mail/email/email.dart'; -import 'package:tmail_ui_user/features/thread/data/extensions/list_email_extension.dart'; - -void main() { - group('sorting_list_email_by_order_id_list test', () { - test('sortingByOrderOfIdList method should return an ordered list of ids when of the same length', () { - List ids = [ - Id('a'), - Id('b'), - Id('c'), - Id('d'), - Id('e') - ]; - List emails = [ - Email(id: EmailId(Id('a'))), - Email(id: EmailId(Id('c'))), - Email(id: EmailId(Id('e'))), - Email(id: EmailId(Id('d'))), - Email(id: EmailId(Id('b'))) - ]; - - List sortedEmails = emails.sortingByOrderOfIdList(ids); - - expect( - sortedEmails.map((e) => e.id?.id.value), - equals(['a', 'b', 'c', 'd', 'e']) - ); - }); - - test('sortingByOrderOfIdList method should return the original list when the length of the two lists is different', () { - List ids = [ - Id('a'), - Id('b'), - Id('c'), - ]; - List emails = [ - Email(id: EmailId(Id('a'))), - Email(id: EmailId(Id('c'))), - Email(id: EmailId(Id('e'))), - Email(id: EmailId(Id('d'))), - Email(id: EmailId(Id('b'))) - ]; - - List sortedEmails = emails.sortingByOrderOfIdList(ids); - - expect( - sortedEmails.map((e) => e.id?.id.value), - equals(['a', 'c', 'e', 'd', 'b']) - ); - }); - }); -} \ No newline at end of file diff --git a/test/features/thread/data/extensions/list_email_extension_test.dart b/test/features/thread/data/extensions/list_email_extension_test.dart new file mode 100644 index 0000000000..845bda0a65 --- /dev/null +++ b/test/features/thread/data/extensions/list_email_extension_test.dart @@ -0,0 +1,143 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:jmap_dart_client/jmap/core/id.dart'; +import 'package:jmap_dart_client/jmap/mail/email/email.dart'; +import 'package:model/extensions/email_id_extensions.dart'; +import 'package:tmail_ui_user/features/thread/data/extensions/list_email_extension.dart'; + +void main() { + group('ListEmailExtension::sortEmailsById::test', () { + test('Sort the full list', () { + final referenceIds = [ + Id('id1'), + Id('id2'), + Id('id3'), + Id('id4'), + ]; + + final emails = [ + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id4'))), + Email(id: EmailId(Id('id2'))), + ]; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + ['id1', 'id2', 'id3', 'id4'], + ); + }); + + test('Emails list has more elements than referenceIds', () { + final referenceIds = [ + Id('id1'), + Id('id2'), + Id('id3'), + ]; + + final emails = [ + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id4'))), + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id5'))), + Email(id: EmailId(Id('id2'))), + ]; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + ['id1', 'id2', 'id3', 'id4', 'id5'], + ); + }); + + test('Emails list has fewer elements than referenceIds', () { + final referenceIds = [ + Id('id1'), + Id('id2'), + Id('id3'), + Id('id4'), + ]; + + final emails = [ + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id1'))), + ]; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + ['id1', 'id3'], + ); + }); + + test('Emails list is empty', () { + final referenceIds = [ + Id('id1'), + Id('id2'), + Id('id3'), + ]; + + final emails = []; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + [], + ); + }); + + test('ReferenceIds list is empty', () { + final referenceIds = []; + + final emails = [ + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id4'))), + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id5'))), + ]; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + ['id3', 'id4', 'id1', 'id5'], + ); + }); + + test('Both lists are empty', () { + final referenceIds = []; + final emails = []; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + [], + ); + }); + + test('Emails have ids that do not match referenceIds', () { + final referenceIds = [ + Id('id1'), + Id('id2'), + ]; + + final emails = [ + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id4'))), + Email(id: EmailId(Id('id5'))), + ]; + + final result = emails.sortEmailsById(referenceIds); + + expect( + result.map((e) => e.id!.asString).toList(), + ['id3', 'id4', 'id5'], + ); + }); + }); +} diff --git a/test/features/thread/data/network/thread_api_test.dart b/test/features/thread/data/network/thread_api_test.dart index 1a236dab08..8bf3a05f4c 100644 --- a/test/features/thread/data/network/thread_api_test.dart +++ b/test/features/thread/data/network/thread_api_test.dart @@ -2,13 +2,18 @@ import 'package:dio/dio.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http_mock_adapter/http_mock_adapter.dart'; import 'package:jmap_dart_client/http/http_client.dart'; +import 'package:jmap_dart_client/jmap/account_id.dart'; import 'package:jmap_dart_client/jmap/core/error/method/error_method_response.dart'; import 'package:jmap_dart_client/jmap/core/filter/filter.dart'; import 'package:jmap_dart_client/jmap/core/id.dart'; import 'package:jmap_dart_client/jmap/core/state.dart'; +import 'package:jmap_dart_client/jmap/core/unsigned_int.dart'; import 'package:jmap_dart_client/jmap/mail/email/email.dart'; import 'package:jmap_dart_client/jmap/mail/email/email_filter_condition.dart'; +import 'package:jmap_dart_client/jmap/mail/email/get/get_email_response.dart'; +import 'package:jmap_dart_client/jmap/mail/email/query/query_email_response.dart'; import 'package:jmap_dart_client/jmap/mail/email/search_snippet/search_snippet.dart'; +import 'package:model/extensions/email_id_extensions.dart'; import 'package:tmail_ui_user/features/thread/data/network/thread_api.dart'; import 'package:tmail_ui_user/features/thread/domain/model/search_email.dart'; import 'package:tmail_ui_user/features/thread/domain/model/search_emails_response.dart'; @@ -16,6 +21,31 @@ import 'package:tmail_ui_user/features/thread/domain/model/search_emails_respons import '../../../../fixtures/account_fixtures.dart'; import '../../../../fixtures/session_fixtures.dart'; +class MockGetEmailResponse extends GetEmailResponse { + final List emailList; + + MockGetEmailResponse(this.emailList) : super( + AccountId(Id('abc')), + State('123'), + emailList, + [], + ); +} + +class MockQueryEmailResponse extends QueryEmailResponse { + final Set idList; + + MockQueryEmailResponse(this.idList) : super( + AccountId(Id('abc')), + State('123'), + false, + UnsignedInt(0), + idList, + UnsignedInt(0), + UnsignedInt(0), + ); +} + void main() { final baseOption = BaseOptions(method: 'POST'); final dio = Dio(baseOption)..options.baseUrl = 'http://domain.com/jmap'; @@ -228,5 +258,170 @@ void main() { ); }); }); + + group('sortEmails::test', () { + test('Should returns emails as is when emailList is empty', () { + final getEmailResponse = MockGetEmailResponse([]); + final queryEmailResponse = MockQueryEmailResponse({ + Id('id1'), + Id('id2'), + }); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect(result, []); + }); + + test('Should returns emails as is when idList is empty', () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + ]); + final queryEmailResponse = MockQueryEmailResponse({}); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect(result, [ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + ]); + }); + + test('Sorts emails according to idList', () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + Email(id: EmailId(Id('id3'))), + ]); + final queryEmailResponse = MockQueryEmailResponse({ + Id('id2'), + Id('id3'), + Id('id1'), + }); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect( + result?.map((e) => e.id!.asString).toList(), + ['id2', 'id3', 'id1'], + ); + }); + + test('Should returns null if getEmailResponse is null', () { + const getEmailResponse = null; + final queryEmailResponse = MockQueryEmailResponse({Id('id1')}); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect(result, isNull); + }); + + test('Should returns emails as is when queryEmailResponse is null', () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + ]); + const queryEmailResponse = null; + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect( + result, + [ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + ], + ); + }); + + test('Should remain in original order when the emailList contains emails whose id does not appear in idList', () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + Email(id: EmailId(Id('id3'))), + ]); + final queryEmailResponse = MockQueryEmailResponse({ + Id('id4'), + Id('id5'), + }); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect( + result?.map((e) => e.id!.asString).toList(), + ['id1', 'id2', 'id3'], + ); + }); + + test( + 'Should still be sorted according to the ids that are present in emailList\n' + 'when idList contains ids that do not match any in emailList', + () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + ]); + final queryEmailResponse = MockQueryEmailResponse({ + Id('id1'), + Id('id2'), + Id('id3'), + }); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect( + result?.map((e) => e.id!.asString).toList(), + ['id1', 'id2'], + ); + }); + + test( + 'When both emailList and idList have ids that do not match\n' + 'only the emails whose ids appear in both lists are sorted according to the order in idList\n' + 'and emails that are not in idList are preserved in their original positions in the final list', + () { + final getEmailResponse = MockGetEmailResponse([ + Email(id: EmailId(Id('id1'))), + Email(id: EmailId(Id('id2'))), + Email(id: EmailId(Id('id3'))), + Email(id: EmailId(Id('id4'))), + ]); + final queryEmailResponse = MockQueryEmailResponse({ + Id('id3'), + Id('id1'), + }); + + final result = threadApi.sortEmails( + getEmailResponse: getEmailResponse, + queryEmailResponse: queryEmailResponse, + ); + + expect( + result?.map((e) => e.id!.asString).toList(), + ['id3', 'id1', 'id2', 'id4'], + ); + }); + }); }); } \ No newline at end of file From e01876a1e79337ad6b2098743a47591f37dc3976 Mon Sep 17 00:00:00 2001 From: dab246 Date: Mon, 25 Nov 2024 12:14:00 +0700 Subject: [PATCH 2/2] fixup! TF-3298 Fix blink when refresh email list --- .../data/extensions/list_email_extension.dart | 11 +++++++-- .../extensions/list_email_extension_test.dart | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/features/thread/data/extensions/list_email_extension.dart b/lib/features/thread/data/extensions/list_email_extension.dart index 59ec5cbcb4..9d4a22b5e4 100644 --- a/lib/features/thread/data/extensions/list_email_extension.dart +++ b/lib/features/thread/data/extensions/list_email_extension.dart @@ -24,8 +24,15 @@ extension ListEmailExtension on List { }; sort((email1, email2) { - final indexEmail1 = indexMap[email1.id!.id] ?? double.maxFinite; - final indexEmail2 = indexMap[email2.id!.id] ?? double.maxFinite; + final emailId1 = email1.id?.id; + final emailId2 = email2.id?.id; + + if (emailId1 == null || emailId2 == null) { + return emailId1 == null ? 1 : -1; + } + + final indexEmail1 = indexMap[emailId1] ?? double.maxFinite; + final indexEmail2 = indexMap[emailId2] ?? double.maxFinite; return indexEmail1.compareTo(indexEmail2); }); diff --git a/test/features/thread/data/extensions/list_email_extension_test.dart b/test/features/thread/data/extensions/list_email_extension_test.dart index 845bda0a65..bb34142cd4 100644 --- a/test/features/thread/data/extensions/list_email_extension_test.dart +++ b/test/features/thread/data/extensions/list_email_extension_test.dart @@ -139,5 +139,28 @@ void main() { ['id3', 'id4', 'id5'], ); }); + + test('should keep emails with null IDs in their original order at the end', () { + // Arrange + final referenceIds = [ + Id('id2'), + Id('id1'), + ]; + + final emails = [ + Email(id: EmailId(Id('id1'))), + Email(id: null), + Email(id: EmailId(Id('id2'))), + ]; + + // Act + final sortedEmails = emails.sortEmailsById(referenceIds); + + // Assert + expect( + sortedEmails.map((e) => e.id?.asString).toList(), + ['id2', 'id1', null], + ); + }); }); }