From cfb4cafcee8bbb60e2afcabb75e9806dcd46285e Mon Sep 17 00:00:00 2001 From: Omar Abou Selo Date: Tue, 20 Aug 2024 12:51:01 +0300 Subject: [PATCH] Add artefacts sorting (#206) * Make tapping a header change url sort query parameters * Slightly increase artefact list view width * Show sort arrow on headers * Fix bug in dashboard view * Actually sort artefacts by column * Use constant for search query parameter * Use constants for duplicate hardcoded strings * Refactor sort by query parameters values into * Remove unused import * Fix sort query parameters vanishing when changing filters * Add trailing comma * Some code style improvements --- .../providers/filtered_family_artefacts.dart | 26 +++- .../providers/filtered_test_executions.dart | 3 +- frontend/lib/routing.dart | 11 ++ .../artefacts_list_view.dart | 6 +- .../artefacts_list_view/column_metadata.dart | 120 +++++++++++++++--- .../artefacts_list_view/headers.dart | 52 +++++++- .../lib/ui/page_filters/page_filters.dart | 12 +- .../lib/ui/page_filters/page_search_bar.dart | 5 +- frontend/lib/utils/artefact_sorting.dart | 83 ++++++++++++ .../filtered_family_artefacts_test.dart | 4 +- 10 files changed, 289 insertions(+), 33 deletions(-) create mode 100644 frontend/lib/utils/artefact_sorting.dart diff --git a/frontend/lib/providers/filtered_family_artefacts.dart b/frontend/lib/providers/filtered_family_artefacts.dart index a67e2ebe..7634df2d 100644 --- a/frontend/lib/providers/filtered_family_artefacts.dart +++ b/frontend/lib/providers/filtered_family_artefacts.dart @@ -1,15 +1,18 @@ +import 'dart:collection'; + import 'package:dartx/dartx.dart'; import 'package:riverpod_annotation/riverpod_annotation.dart'; import '../models/artefact.dart'; import '../models/filters.dart'; import '../routing.dart'; +import '../utils/artefact_sorting.dart'; import 'family_artefacts.dart'; part 'filtered_family_artefacts.g.dart'; @riverpod -Map filteredFamilyArtefacts( +LinkedHashMap filteredFamilyArtefacts( FilteredFamilyArtefactsRef ref, Uri pageUri, ) { @@ -17,12 +20,23 @@ Map filteredFamilyArtefacts( final artefacts = ref.watch(familyArtefactsProvider(family)).requireValue; final filters = emptyArtefactFilters.copyWithQueryParams(pageUri.queryParametersAll); - final searchValue = pageUri.queryParameters['q'] ?? ''; + final searchValue = + pageUri.queryParameters[CommonQueryParameters.searchQuery] ?? ''; + + final filteredArtefacts = artefacts.values + .filter( + (artefact) => + _artefactPassesSearch(artefact, searchValue) && + filters.doesObjectPassFilters(artefact), + ) + .toList(); + + sortArtefacts(pageUri.queryParameters, filteredArtefacts); - return artefacts.filterValues( - (artefact) => - _artefactPassesSearch(artefact, searchValue) && - filters.doesObjectPassFilters(artefact), + return LinkedHashMap.fromIterable( + filteredArtefacts, + key: (a) => a.id, + value: (a) => a, ); } diff --git a/frontend/lib/providers/filtered_test_executions.dart b/frontend/lib/providers/filtered_test_executions.dart index 9413e613..2b348758 100644 --- a/frontend/lib/providers/filtered_test_executions.dart +++ b/frontend/lib/providers/filtered_test_executions.dart @@ -17,7 +17,8 @@ Future> filteredTestExecutions( final artefactId = AppRoutes.artefactIdFromUri(pageUri); final filters = emptyTestExecutionFilters.copyWithQueryParams(pageUri.queryParametersAll); - final searchValue = pageUri.queryParameters['q'] ?? ''; + final searchValue = + pageUri.queryParameters[CommonQueryParameters.searchQuery] ?? ''; final builds = await ref.watch(artefactBuildsProvider(artefactId).future); final testExecutions = [ diff --git a/frontend/lib/routing.dart b/frontend/lib/routing.dart index 46d26446..72b2cbe1 100644 --- a/frontend/lib/routing.dart +++ b/frontend/lib/routing.dart @@ -51,6 +51,17 @@ final appRouter = GoRouter( ], ); +enum SortDirection { + asc, + desc, +} + +class CommonQueryParameters { + static const searchQuery = 'q'; + static const sortBy = 'sortBy'; + static const sortDirection = 'direction'; +} + class AppRoutes { static const snaps = '/snaps'; static const debs = '/debs'; diff --git a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/artefacts_list_view.dart b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/artefacts_list_view.dart index 3211f2d2..7d7bac5a 100644 --- a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/artefacts_list_view.dart +++ b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/artefacts_list_view.dart @@ -5,6 +5,7 @@ import 'package:go_router/go_router.dart'; import '../../../../models/artefact.dart'; import '../../../../providers/filtered_family_artefacts.dart'; import '../../../../routing.dart'; +import '../../../../utils/artefact_sorting.dart'; part 'row.dart'; part 'headers.dart'; @@ -42,9 +43,10 @@ class ArtefactsListView extends ConsumerWidget { final artefacts = ref.watch(filteredFamilyArtefactsProvider(pageUri)).values.toList(); - return Center( + return Align( + alignment: Alignment.topLeft, child: SizedBox( - width: 1200, + width: 1300, child: ListView.separated( itemCount: artefacts.length + 1, itemBuilder: (_, i) => diff --git a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/column_metadata.dart b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/column_metadata.dart index 80901a24..65e56b3b 100644 --- a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/column_metadata.dart +++ b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/column_metadata.dart @@ -2,31 +2,117 @@ part of 'artefacts_list_view.dart'; typedef ColumnMetadata = ({ String name, + ArtefactSortingQuery queryParam, int flex, Widget Function(BuildContext, Artefact) cellBuilder }); const _snapColumnsMetadata = [ - (name: 'Name', flex: 2, cellBuilder: _buildNameCell), - (name: 'Version', flex: 2, cellBuilder: _buildVersionCell), - (name: 'Track', flex: 1, cellBuilder: _buildTrackCell), - (name: 'Risk', flex: 1, cellBuilder: _buildStageCell), - (name: 'Due date', flex: 1, cellBuilder: _buildDueDateCell), - (name: 'Reviews remaining', flex: 1, cellBuilder: _buildReviewsRemainingCell), - (name: 'Status', flex: 1, cellBuilder: _buildStatusCell), - (name: 'Assignee', flex: 1, cellBuilder: _buildAssigneeCell), + ( + name: 'Name', + queryParam: ArtefactSortingQuery.name, + flex: 2, + cellBuilder: _buildNameCell, + ), + ( + name: 'Version', + queryParam: ArtefactSortingQuery.version, + flex: 2, + cellBuilder: _buildVersionCell, + ), + ( + name: 'Track', + queryParam: ArtefactSortingQuery.track, + flex: 1, + cellBuilder: _buildTrackCell, + ), + ( + name: 'Risk', + queryParam: ArtefactSortingQuery.risk, + flex: 1, + cellBuilder: _buildStageCell, + ), + ( + name: 'Due date', + queryParam: ArtefactSortingQuery.dueDate, + flex: 1, + cellBuilder: _buildDueDateCell, + ), + ( + name: 'Reviews remaining', + queryParam: ArtefactSortingQuery.reviewsRemaining, + flex: 1, + cellBuilder: _buildReviewsRemainingCell, + ), + ( + name: 'Status', + queryParam: ArtefactSortingQuery.status, + flex: 1, + cellBuilder: _buildStatusCell, + ), + ( + name: 'Assignee', + queryParam: ArtefactSortingQuery.assignee, + flex: 1, + cellBuilder: _buildAssigneeCell, + ), ]; const _debColumnsMetadata = [ - (name: 'Name', flex: 2, cellBuilder: _buildNameCell), - (name: 'Version', flex: 2, cellBuilder: _buildVersionCell), - (name: 'Series', flex: 1, cellBuilder: _buildSeriesCell), - (name: 'Repo', flex: 1, cellBuilder: _buildRepoCell), - (name: 'Pocket', flex: 1, cellBuilder: _buildStageCell), - (name: 'Due date', flex: 1, cellBuilder: _buildDueDateCell), - (name: 'Reviews remaining', flex: 1, cellBuilder: _buildReviewsRemainingCell), - (name: 'Status', flex: 1, cellBuilder: _buildStatusCell), - (name: 'Assignee', flex: 1, cellBuilder: _buildAssigneeCell), + ( + name: 'Name', + queryParam: ArtefactSortingQuery.name, + flex: 2, + cellBuilder: _buildNameCell, + ), + ( + name: 'Version', + queryParam: ArtefactSortingQuery.version, + flex: 2, + cellBuilder: _buildVersionCell, + ), + ( + name: 'Series', + queryParam: ArtefactSortingQuery.series, + flex: 1, + cellBuilder: _buildSeriesCell, + ), + ( + name: 'Repo', + queryParam: ArtefactSortingQuery.repo, + flex: 1, + cellBuilder: _buildRepoCell, + ), + ( + name: 'Pocket', + queryParam: ArtefactSortingQuery.pocket, + flex: 1, + cellBuilder: _buildStageCell, + ), + ( + name: 'Due date', + queryParam: ArtefactSortingQuery.dueDate, + flex: 1, + cellBuilder: _buildDueDateCell, + ), + ( + name: 'Reviews remaining', + queryParam: ArtefactSortingQuery.reviewsRemaining, + flex: 1, + cellBuilder: _buildReviewsRemainingCell, + ), + ( + name: 'Status', + queryParam: ArtefactSortingQuery.status, + flex: 1, + cellBuilder: _buildStatusCell, + ), + ( + name: 'Assignee', + queryParam: ArtefactSortingQuery.assignee, + flex: 1, + cellBuilder: _buildAssigneeCell, + ), ]; Widget _buildNameCell(BuildContext context, Artefact artefact) => diff --git a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/headers.dart b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/headers.dart index 7c910330..dfc1f4a2 100644 --- a/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/headers.dart +++ b/frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/headers.dart @@ -11,16 +11,36 @@ class _Headers extends StatelessWidget { @override Widget build(BuildContext context) { + final uri = AppRoutes.uriFromContext(context); + final sortBy = uri.queryParameters[CommonQueryParameters.sortBy]; + final sortDirection = + uri.queryParameters[CommonQueryParameters.sortDirection]; + return SizedBox( height: 56, child: Row( + crossAxisAlignment: CrossAxisAlignment.stretch, children: columnsMetaData .map( (data) => Expanded( flex: data.flex, - child: Text( - data.name, - style: Theme.of(context).textTheme.titleLarge, + child: InkWell( + onTap: () => _handleFilterTap(context, uri, data), + child: Row( + children: [ + Flexible( + child: Text( + data.name, + style: Theme.of(context).textTheme.titleLarge, + ), + ), + if (sortBy == data.queryParam.name) + if (sortDirection == SortDirection.asc.name) + const Icon(Icons.arrow_upward) + else + const Icon(Icons.arrow_downward), + ], + ), ), ), ) @@ -28,4 +48,30 @@ class _Headers extends StatelessWidget { ), ); } + + void _handleFilterTap( + BuildContext context, + Uri pageUri, + ColumnMetadata columnData, + ) { + final queryParameters = pageUri.queryParameters; + final existingSortBy = queryParameters[CommonQueryParameters.sortBy]; + final existingDirection = + queryParameters[CommonQueryParameters.sortDirection]; + + final newSortBy = columnData.queryParam.name; + final isSameSortBy = existingSortBy == newSortBy; + final wasAscending = existingDirection == SortDirection.asc.name; + final newDirection = isSameSortBy && wasAscending + ? SortDirection.desc.name + : SortDirection.asc.name; + + final newQueryParameters = { + ...queryParameters, + CommonQueryParameters.sortBy: newSortBy, + CommonQueryParameters.sortDirection: newDirection, + }; + + context.go(pageUri.replace(queryParameters: newQueryParameters).toString()); + } } diff --git a/frontend/lib/ui/page_filters/page_filters.dart b/frontend/lib/ui/page_filters/page_filters.dart index 5750d68c..52ee3b0e 100644 --- a/frontend/lib/ui/page_filters/page_filters.dart +++ b/frontend/lib/ui/page_filters/page_filters.dart @@ -22,7 +22,8 @@ class PageFiltersView extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { final pageUri = AppRoutes.uriFromContext(context); - final searchQuery = pageUri.queryParameters['q']; + final searchQuery = + pageUri.queryParameters[CommonQueryParameters.searchQuery]; final filters = ref.watch(pageFiltersProvider(pageUri)); return SizedBox( @@ -69,10 +70,17 @@ class PageFiltersView extends ConsumerWidget { Uri pageUri, BuildContext context, ) { + final sortBy = pageUri.queryParameters[CommonQueryParameters.sortBy]; + final sortDirection = + pageUri.queryParameters[CommonQueryParameters.sortDirection]; final searchValue = ref.read(searchValueProvider(searchQuery)).trim(); final queryParams = { - if (searchValue.isNotEmpty) 'q': searchValue, + if (searchValue.isNotEmpty) + CommonQueryParameters.searchQuery: searchValue, ...ref.read(pageFiltersProvider(pageUri)).toQueryParams(), + if (sortBy != null) CommonQueryParameters.sortBy: sortBy, + if (sortDirection != null) + CommonQueryParameters.sortDirection: sortDirection, }; context.go( pageUri.replace(queryParameters: queryParams).toString(), diff --git a/frontend/lib/ui/page_filters/page_search_bar.dart b/frontend/lib/ui/page_filters/page_search_bar.dart index e79454a7..1ff759f6 100644 --- a/frontend/lib/ui/page_filters/page_search_bar.dart +++ b/frontend/lib/ui/page_filters/page_search_bar.dart @@ -3,6 +3,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; import '../../providers/search_value.dart'; +import '../../routing.dart'; import '../vanilla/vanilla_search_bar.dart'; final pageSearchBarKey = GlobalKey<_PageSearchBarState>(); @@ -35,7 +36,9 @@ class _PageSearchBarState extends ConsumerState { @override Widget build(BuildContext context) { - final searchQuery = GoRouterState.of(context).uri.queryParameters['q']; + final searchQuery = GoRouterState.of(context) + .uri + .queryParameters[CommonQueryParameters.searchQuery]; return _SearchNotifierListener( searchQuery: searchQuery, diff --git a/frontend/lib/utils/artefact_sorting.dart b/frontend/lib/utils/artefact_sorting.dart new file mode 100644 index 00000000..02d03069 --- /dev/null +++ b/frontend/lib/utils/artefact_sorting.dart @@ -0,0 +1,83 @@ +import '../models/artefact.dart'; +import '../routing.dart'; + +enum ArtefactSortingQuery { + name, + version, + track, + risk, + dueDate, + reviewsRemaining, + status, + assignee, + series, + repo, + pocket +} + +void sortArtefacts( + Map queryParameters, + List artefacts, +) { + final sortBy = queryParameters[CommonQueryParameters.sortBy] ?? ''; + final sortDirection = queryParameters[CommonQueryParameters.sortDirection]; + + ArtefactSortingQuery sortByParsed = ArtefactSortingQuery.name; + try { + sortByParsed = ArtefactSortingQuery.values.byName(sortBy); + } on ArgumentError { + // use default + } + + final compare = _getArtefactCompareFunction(sortByParsed); + + if (sortDirection == SortDirection.desc.name) { + artefacts.sort((a1, a2) => compare(a2, a1)); + } else { + artefacts.sort(compare); + } +} + +int Function(Artefact, Artefact) _getArtefactCompareFunction( + ArtefactSortingQuery sortByParsed, +) { + switch (sortByParsed) { + case ArtefactSortingQuery.name: + return (a1, a2) => a1.name.compareTo(a2.name); + case ArtefactSortingQuery.version: + return (a1, a2) => a1.version.compareTo(a2.version); + case ArtefactSortingQuery.track: + return (a1, a2) => a1.track.compareTo(a2.track); + case ArtefactSortingQuery.risk: + return (a1, a2) => a1.stage.name.compareTo(a2.stage.name); + case ArtefactSortingQuery.dueDate: + return (a1, a2) { + final a1DueDate = a1.dueDate; + final a2DueDate = a2.dueDate; + // no due date is always larger + if (a1DueDate == null) return 1; + if (a2DueDate == null) return -1; + return a1DueDate.compareTo(a2DueDate); + }; + case ArtefactSortingQuery.reviewsRemaining: + return (a1, a2) => a1.remainingTestExecutionCount + .compareTo(a2.remainingTestExecutionCount); + case ArtefactSortingQuery.status: + return (a1, a2) => a1.status.name.compareTo(a2.status.name); + case ArtefactSortingQuery.assignee: + return (a1, a2) { + final a1Assignee = a1.assignee?.name; + final a2Assignee = a2.assignee?.name; + // no assignee is always larger + if (a1Assignee == null) return 1; + if (a2Assignee == null) return -1; + return a1Assignee.compareTo(a2Assignee); + }; + case ArtefactSortingQuery.series: + return (a1, a2) => a1.series.compareTo(a2.series); + case ArtefactSortingQuery.repo: + return (a1, a2) => a1.repo.compareTo(a2.repo); + case ArtefactSortingQuery.pocket: + return (a1, a2) => a1.stage.name.compareTo(a2.stage.name); + } +} diff --git a/frontend/test/providers/filtered_family_artefacts_test.dart b/frontend/test/providers/filtered_family_artefacts_test.dart index c732ff02..4204e2ed 100644 --- a/frontend/test/providers/filtered_family_artefacts_test.dart +++ b/frontend/test/providers/filtered_family_artefacts_test.dart @@ -96,7 +96,9 @@ void main() { filteredFamilyArtefactsProvider( Uri( path: AppRoutes.snaps, - queryParameters: {'q': firstArtefact.name}, + queryParameters: { + CommonQueryParameters.searchQuery: firstArtefact.name, + }, ), ), );