From 9b42fe6c952f5d2b4688a6597372437fa3f89f50 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Tue, 2 Jul 2024 12:02:15 +0200 Subject: [PATCH 01/15] Initial implementation of UI for showing percentage completed --- .../controllers/artefacts/artefacts.py | 58 +++++++++++++++++++ .../controllers/artefacts/models.py | 1 + .../test_observer/data_access/repository.py | 4 +- frontend/lib/models/artefact.dart | 1 + .../ui/artefact_page/artefact_page_body.dart | 40 ++++++++++++- .../artefact_page/artefact_page_header.dart | 36 ++++++++++-- frontend/lib/ui/dashboard/artefact_card.dart | 6 +- frontend/lib/ui/user_avatar.dart | 48 +++++++++++---- 8 files changed, 175 insertions(+), 19 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index eb8f0348..ce4d1ba3 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -18,6 +18,7 @@ # Omar Selo # Nadzeya Hutsko from fastapi import APIRouter, Depends, HTTPException +from sqlalchemy import case, func from sqlalchemy.orm import Session, joinedload from test_observer.data_access import queries @@ -73,6 +74,63 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db order_by_columns=order_by, ) + # Define subquery to count all TestExecutions for each Artefact + all_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("total"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .group_by(ArtefactBuild.artefact_id) + .subquery() + ) + + # Define subquery to count completed TestExecutions for each Artefact + completed_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("completed"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .filter(func.array_length(TestExecution.review_decision, 1) > 0) + .group_by(ArtefactBuild.artefact_id) + ) + # print(completed_tests.all()) + + completed_tests = completed_tests.subquery() + + # Define subquery to calculate the ratio of completed TestExecutions to all TestExecutions + ratio_completed = ( + db.query( + all_tests.c.artefact_id, + func.coalesce((completed_tests.c.completed / all_tests.c.total), 0).label( + "ratio_completed" + ), + ) + .outerjoin( + completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id + ) + .subquery() + ) + + # Execute the query and fetch all results + results = ( + db.query(Artefact.id, ratio_completed.c.ratio_completed) + .outerjoin(ratio_completed, Artefact.id == ratio_completed.c.artefact_id) + .all() + ) + + # Convert the results to a dictionary + ratio_completed_dict = { + artefact_id: ratio_completed for artefact_id, ratio_completed in results + } + + print(ratio_completed_dict) + + # Add the ratio_completed to the artefacts + for artefact in artefacts: + artefact.ratio_completed = ratio_completed_dict.get(artefact.id, 0) + return artefacts diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 8f22450e..54c8ecaa 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -51,6 +51,7 @@ class ArtefactDTO(BaseModel): assignee: UserDTO | None due_date: date | None bug_link: str + ratio_completed: float = 0 class EnvironmentDTO(BaseModel): diff --git a/backend/test_observer/data_access/repository.py b/backend/test_observer/data_access/repository.py index bb96e636..84b1baa0 100644 --- a/backend/test_observer/data_access/repository.py +++ b/backend/test_observer/data_access/repository.py @@ -23,11 +23,11 @@ from collections.abc import Iterable from typing import Any -from sqlalchemy import and_, func +from sqlalchemy import and_, func, case from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, joinedload -from .models import Artefact, DataModel, Family, Stage +from .models import Artefact, ArtefactBuild, DataModel, Family, Stage, TestExecution from .models_enums import FamilyName diff --git a/frontend/lib/models/artefact.dart b/frontend/lib/models/artefact.dart index 6e029212..61ae575e 100644 --- a/frontend/lib/models/artefact.dart +++ b/frontend/lib/models/artefact.dart @@ -22,6 +22,7 @@ class Artefact with _$Artefact { required String repo, required ArtefactStatus status, required StageName stage, + @JsonKey(name: 'ratio_completed') required double ratioCompleted, User? assignee, @JsonKey(name: 'bug_link') required String bugLink, @JsonKey(name: 'due_date') DateTime? dueDate, diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index f7541588..227264e2 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -3,6 +3,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/artefact.dart'; +import '../../models/artefact_build.dart'; import '../../providers/artefact_builds.dart'; import '../page_filters/page_filters.dart'; import '../spacing.dart'; @@ -14,6 +15,30 @@ class ArtefactPageBody extends ConsumerWidget { final Artefact artefact; + int _getCompletedTestExecutionCount(List artefactBuilds) { + int completedCount = 0; + for (final artefactBuild in artefactBuilds) { + for (final testExecution in artefactBuild.testExecutions) { + if (testExecution.reviewDecision.isNotEmpty) completedCount++; + } + } + return completedCount; + } + + int _getTotalTestExecutionCount(List artefactBuilds) { + int totalCount = 0; + for (final artefactBuild in artefactBuilds) { + totalCount += artefactBuild.testExecutions.length; + } + return totalCount; + } + + double _getPercentage(List artefactBuilds) { + final completedCount = _getCompletedTestExecutionCount(artefactBuilds); + final totalCount = _getTotalTestExecutionCount(artefactBuilds); + return (completedCount / totalCount); + } + @override Widget build(BuildContext context, WidgetRef ref) { final artefactBuilds = ref.watch(ArtefactBuildsProvider(artefact.id)); @@ -30,17 +55,30 @@ class ArtefactPageBody extends ConsumerWidget { children: [ const SizedBox(height: Spacing.level3), Row( - crossAxisAlignment: CrossAxisAlignment.baseline, + crossAxisAlignment: CrossAxisAlignment.center, textBaseline: TextBaseline.alphabetic, children: [ Text( 'Environments', style: Theme.of(context).textTheme.headlineSmall, ), + const SizedBox(width: Spacing.level4), + SizedBox( + width: 25.0, // specify the width + height: 25.0, // specify the height + child: CircularProgressIndicator( + value: _getPercentage(artefactBuilds), + semanticsLabel: 'Circular progress indicator', + ), + ), const Spacer(), const RerunFilteredEnvironmentsButton(), ], ), + // LinearProgressIndicator( + // value: _getPercentage(artefactBuilds), + // semanticsLabel: 'Linear progress indicator', + // ), Expanded( child: ListView.builder( itemCount: artefactBuilds.length, diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index a36bf08b..ffdfdcdb 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -1,27 +1,55 @@ import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru/yaru.dart'; import '../../models/artefact.dart'; +import '../../models/artefact_build.dart'; +import '../../providers/artefact_builds.dart'; import '../spacing.dart'; import '../user_avatar.dart'; import 'artefact_signoff_button.dart'; -class ArtefactPageHeader extends StatelessWidget { - const ArtefactPageHeader({super.key, required this.artefact}); +class ArtefactPageHeader extends ConsumerWidget { + const ArtefactPageHeader({Key? key, required this.artefact}) + : super(key: key); final Artefact artefact; + double _computeRatioCompleted(List artefactBuilds) { + int totalTestExecutions = 0, completedTestExecution = 0; + for (final artefactBuild in artefactBuilds) { + totalTestExecutions += artefactBuild.testExecutions.length; + for (final testExecution in artefactBuild.testExecutions) { + if (testExecution.reviewDecision.isNotEmpty) { + completedTestExecution++; + } + } + } + + return totalTestExecutions == 0 + ? 0 + : completedTestExecution / totalTestExecutions; + } + @override - Widget build(BuildContext context) { + Widget build(BuildContext context, WidgetRef ref) { final assignee = artefact.assignee; final dueDate = artefact.dueDateString; + + final artefactBuilds = + ref.watch(ArtefactBuildsProvider(artefact.id)).requireValue; + return Row( children: [ Text(artefact.name, style: Theme.of(context).textTheme.headlineLarge), const SizedBox(width: Spacing.level4), ArtefactSignoffButton(artefact: artefact), const SizedBox(width: Spacing.level4), - if (assignee != null) UserAvatar(user: assignee), + if (assignee != null) + UserAvatar( + user: assignee, + ratioCompleted: _computeRatioCompleted(artefactBuilds), + ), const SizedBox(width: Spacing.level4), if (dueDate != null) Text( diff --git a/frontend/lib/ui/dashboard/artefact_card.dart b/frontend/lib/ui/dashboard/artefact_card.dart index 7827c5e5..1d69cb21 100644 --- a/frontend/lib/ui/dashboard/artefact_card.dart +++ b/frontend/lib/ui/dashboard/artefact_card.dart @@ -63,7 +63,11 @@ class ArtefactCard extends ConsumerWidget { fontColor: YaruColors.red, ), const Spacer(), - if (assignee != null) UserAvatar(user: assignee), + if (assignee != null) + UserAvatar( + user: assignee, + ratioCompleted: artefact.ratioCompleted, + ), ], ), ], diff --git a/frontend/lib/ui/user_avatar.dart b/frontend/lib/ui/user_avatar.dart index 1c4b4fac..0f90124f 100644 --- a/frontend/lib/ui/user_avatar.dart +++ b/frontend/lib/ui/user_avatar.dart @@ -15,23 +15,49 @@ Color userIdToColor(int userId) { return possibleColors[userId % possibleColors.length]; } +Color getDarkerColor(Color color, [double amount = 0.2]) { + final hsl = HSLColor.fromColor(color); + final double lightness = (hsl.lightness - amount).clamp(0.0, 1.0); + + return hsl.withLightness(lightness).toColor(); +} + class UserAvatar extends StatelessWidget { - const UserAvatar({super.key, required this.user}); + const UserAvatar( + {super.key, required this.user, required this.ratioCompleted}); final User user; + final double ratioCompleted; @override Widget build(BuildContext context) { - return CircleAvatar( - backgroundColor: userIdToColor(user.id), - child: Tooltip( - message: - '${user.name}\n${user.launchpadHandle}\n${user.launchpadEmail}', - child: Text( - user.initials, - style: - Theme.of(context).textTheme.labelLarge?.apply(fontWeightDelta: 4), - ), + return Tooltip( + message: + '${user.name}\n${user.launchpadHandle}\n${user.launchpadEmail}\nCompleted ${(ratioCompleted * 100).round()}%', + child: Stack( + alignment: Alignment.center, + children: [ + CircleAvatar( + backgroundColor: userIdToColor(user.id), + child: Text( + user.initials, + style: Theme.of(context) + .textTheme + .labelLarge + ?.apply(fontWeightDelta: 4), + ), + ), + SizedBox( + width: 43.0, // specify the width + height: 43.0, // specify the height + child: CircularProgressIndicator( + color: getDarkerColor(userIdToColor(user.id)), + backgroundColor: userIdToColor(user.id), + value: ratioCompleted, + semanticsLabel: 'Circular progress indicator', + ), + ), + ], ), ); } From c4976441f4752b2b286c6fba6785b3658b41dd0f Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Tue, 9 Jul 2024 10:52:45 +0200 Subject: [PATCH 02/15] Fix error when loading artefact builds --- .../lib/ui/artefact_page/artefact_page.dart | 32 ++++-- .../ui/artefact_page/artefact_page_body.dart | 104 +++++++++--------- .../artefact_page/artefact_page_header.dart | 8 +- frontend/test/dummy_data.dart | 1 + 4 files changed, 75 insertions(+), 70 deletions(-) diff --git a/frontend/lib/ui/artefact_page/artefact_page.dart b/frontend/lib/ui/artefact_page/artefact_page.dart index 6f82b4dc..7a223d5c 100644 --- a/frontend/lib/ui/artefact_page/artefact_page.dart +++ b/frontend/lib/ui/artefact_page/artefact_page.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru_widgets/yaru_widgets.dart'; +import '../../providers/artefact_builds.dart'; import '../../providers/family_artefacts.dart'; import '../../routing.dart'; import '../dialog_header.dart'; @@ -43,17 +44,26 @@ class ArtefactPage extends ConsumerWidget { data: (artefacts) { final artefact = artefacts[artefactId]; if (artefact == null) return _invalidArtefactErrorMessage; - return Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - ArtefactPageHeader(artefact: artefact), - const SizedBox(height: Spacing.level4), - ArtefactPageInfoSection(artefact: artefact), - const SizedBox(height: Spacing.level4), - Expanded( - child: ArtefactPageBody(artefact: artefact), - ), - ], + + final artefactBuilds = ref.watch(ArtefactBuildsProvider(artefact.id)); + return artefactBuilds.when( + data: (artefactBuilds) => Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + ArtefactPageHeader( + artefact: artefact, artefactBuilds: artefactBuilds), + const SizedBox(height: Spacing.level4), + ArtefactPageInfoSection(artefact: artefact), + const SizedBox(height: Spacing.level4), + Expanded( + child: ArtefactPageBody( + artefact: artefact, artefactBuilds: artefactBuilds), + ), + ], + ), + error: (e, stack) => + Center(child: Text('Error:\n$e\nStackTrace:\n$stack')), + loading: () => const Center(child: YaruCircularProgressIndicator()), ); }, error: (e, stack) => diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index 227264e2..4acb376c 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -11,9 +11,11 @@ import 'artefact_build_expandable.dart'; import 'rerun_filtered_environments_button.dart'; class ArtefactPageBody extends ConsumerWidget { - const ArtefactPageBody({super.key, required this.artefact}); + const ArtefactPageBody( + {super.key, required this.artefact, required this.artefactBuilds}); final Artefact artefact; + final List artefactBuilds; int _getCompletedTestExecutionCount(List artefactBuilds) { int completedCount = 0; @@ -41,65 +43,57 @@ class ArtefactPageBody extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { - final artefactBuilds = ref.watch(ArtefactBuildsProvider(artefact.id)); - - return artefactBuilds.when( - data: (artefactBuilds) => Row( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const PageFiltersView(searchHint: 'Search by environment name'), - const SizedBox(width: Spacing.level5), - Expanded( - child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const SizedBox(height: Spacing.level3), - Row( - crossAxisAlignment: CrossAxisAlignment.center, - textBaseline: TextBaseline.alphabetic, - children: [ - Text( - 'Environments', - style: Theme.of(context).textTheme.headlineSmall, - ), - const SizedBox(width: Spacing.level4), - SizedBox( - width: 25.0, // specify the width - height: 25.0, // specify the height - child: CircularProgressIndicator( - value: _getPercentage(artefactBuilds), - semanticsLabel: 'Circular progress indicator', - ), + return Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const PageFiltersView(searchHint: 'Search by environment name'), + const SizedBox(width: Spacing.level5), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const SizedBox(height: Spacing.level3), + Row( + crossAxisAlignment: CrossAxisAlignment.center, + textBaseline: TextBaseline.alphabetic, + children: [ + Text( + 'Environments', + style: Theme.of(context).textTheme.headlineSmall, + ), + const SizedBox(width: Spacing.level4), + SizedBox( + width: 25.0, // specify the width + height: 25.0, // specify the height + child: CircularProgressIndicator( + value: _getPercentage(artefactBuilds), + semanticsLabel: 'Circular progress indicator', ), - const Spacer(), - const RerunFilteredEnvironmentsButton(), - ], - ), - // LinearProgressIndicator( - // value: _getPercentage(artefactBuilds), - // semanticsLabel: 'Linear progress indicator', - // ), - Expanded( - child: ListView.builder( - itemCount: artefactBuilds.length, - itemBuilder: (_, i) => Padding( - // Padding is to avoid scroll bar covering trailing buttons - padding: const EdgeInsets.only(right: Spacing.level3), - child: ArtefactBuildExpandable( - artefactBuild: artefactBuilds[i], - ), + ), + const Spacer(), + const RerunFilteredEnvironmentsButton(), + ], + ), + LinearProgressIndicator( + value: _getPercentage(artefactBuilds), + semanticsLabel: 'Linear progress indicator', + ), + Expanded( + child: ListView.builder( + itemCount: artefactBuilds.length, + itemBuilder: (_, i) => Padding( + // Padding is to avoid scroll bar covering trailing buttons + padding: const EdgeInsets.only(right: Spacing.level3), + child: ArtefactBuildExpandable( + artefactBuild: artefactBuilds[i], ), ), ), - ], - ), + ), + ], ), - ], - ), - loading: () => const Center(child: YaruCircularProgressIndicator()), - error: (error, stackTrace) { - return Center(child: Text('Error: $error')); - }, + ), + ], ); } } diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index ffdfdcdb..8483fc36 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru/yaru.dart'; +import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/artefact.dart'; import '../../models/artefact_build.dart'; @@ -10,10 +11,12 @@ import '../user_avatar.dart'; import 'artefact_signoff_button.dart'; class ArtefactPageHeader extends ConsumerWidget { - const ArtefactPageHeader({Key? key, required this.artefact}) + const ArtefactPageHeader( + {Key? key, required this.artefact, required this.artefactBuilds}) : super(key: key); final Artefact artefact; + final List artefactBuilds; double _computeRatioCompleted(List artefactBuilds) { int totalTestExecutions = 0, completedTestExecution = 0; @@ -36,9 +39,6 @@ class ArtefactPageHeader extends ConsumerWidget { final assignee = artefact.assignee; final dueDate = artefact.dueDateString; - final artefactBuilds = - ref.watch(ArtefactBuildsProvider(artefact.id)).requireValue; - return Row( children: [ Text(artefact.name, style: Theme.of(context).textTheme.headlineLarge), diff --git a/frontend/test/dummy_data.dart b/frontend/test/dummy_data.dart index 18737701..c51fe219 100644 --- a/frontend/test/dummy_data.dart +++ b/frontend/test/dummy_data.dart @@ -24,6 +24,7 @@ const dummyArtefact = Artefact( stage: StageName.beta, assignee: dummyUser, bugLink: '', + ratioCompleted: 0.0, ); const dummyEnvironment = Environment( From bae3dfe703b1eac23568ab96c717da05521038c0 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Tue, 9 Jul 2024 14:15:46 +0200 Subject: [PATCH 03/15] Update code to show exact counts --- .../controllers/artefacts/artefacts.py | 104 ++++++++++-------- .../controllers/artefacts/models.py | 3 +- frontend/lib/models/artefact.dart | 5 +- .../ui/artefact_page/artefact_page_body.dart | 37 ------- .../artefact_page/artefact_page_header.dart | 29 +++-- frontend/lib/ui/dashboard/artefact_card.dart | 4 +- frontend/lib/ui/user_avatar.dart | 17 ++- frontend/test/dummy_data.dart | 3 +- 8 files changed, 98 insertions(+), 104 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index ce4d1ba3..8ddb7750 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -18,8 +18,10 @@ # Omar Selo # Nadzeya Hutsko from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import case, func +from sqlalchemy import func from sqlalchemy.orm import Session, joinedload +from sqlalchemy.orm.query import RowReturningQuery + from test_observer.data_access import queries from test_observer.data_access.models import ( @@ -44,6 +46,44 @@ router = APIRouter(tags=["artefacts"]) +def _get_test_execution_counts_subquery(db: Session) -> RowReturningQuery: + # Define subquery to count all TestExecutions for each Artefact + all_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("total"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .group_by(ArtefactBuild.artefact_id) + .subquery() + ) + + # Define subquery to count completed TestExecutions for each Artefact + completed_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("completed"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .filter(func.array_length(TestExecution.review_decision, 1) > 0) + .group_by(ArtefactBuild.artefact_id) + .subquery() + ) + + # Define subquery to calculate the ratio of completed TestExecutions to all TestExecutions + return ( + db.query( + all_tests.c.artefact_id, + func.coalesce(all_tests.c.total, 0).label("total"), + func.coalesce(completed_tests.c.completed, 0).label("completed"), + ) + .outerjoin( + completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id + ) + .subquery() + ) + + def _get_artefact_from_db(artefact_id: int, db: Session = Depends(get_db)) -> Artefact: a = db.get(Artefact, artefact_id) if a is None: @@ -74,62 +114,38 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db order_by_columns=order_by, ) - # Define subquery to count all TestExecutions for each Artefact - all_tests = ( - db.query( - ArtefactBuild.artefact_id, - func.count(TestExecution.id).label("total"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .group_by(ArtefactBuild.artefact_id) - .subquery() - ) - - # Define subquery to count completed TestExecutions for each Artefact - completed_tests = ( - db.query( - ArtefactBuild.artefact_id, - func.count(TestExecution.id).label("completed"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .filter(func.array_length(TestExecution.review_decision, 1) > 0) - .group_by(ArtefactBuild.artefact_id) - ) - # print(completed_tests.all()) - - completed_tests = completed_tests.subquery() + test_execution_counts = _get_test_execution_counts_subquery(db) - # Define subquery to calculate the ratio of completed TestExecutions to all TestExecutions - ratio_completed = ( + # Execute the query and fetch all results + results = ( db.query( - all_tests.c.artefact_id, - func.coalesce((completed_tests.c.completed / all_tests.c.total), 0).label( - "ratio_completed" - ), + Artefact.id, + test_execution_counts.c.total, + test_execution_counts.c.completed, ) .outerjoin( - completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id + test_execution_counts, Artefact.id == test_execution_counts.c.artefact_id ) - .subquery() - ) - - # Execute the query and fetch all results - results = ( - db.query(Artefact.id, ratio_completed.c.ratio_completed) - .outerjoin(ratio_completed, Artefact.id == ratio_completed.c.artefact_id) + .filter(Artefact.id.in_([artefact.id for artefact in artefacts])) .all() ) # Convert the results to a dictionary - ratio_completed_dict = { - artefact_id: ratio_completed for artefact_id, ratio_completed in results + counts_dict = { + artefact_id: { + "total": total, + "completed": completed, + } + for artefact_id, total, completed in results } - print(ratio_completed_dict) - # Add the ratio_completed to the artefacts for artefact in artefacts: - artefact.ratio_completed = ratio_completed_dict.get(artefact.id, 0) + if counts_dict.get(artefact.id): + artefact.all_test_executions_count = counts_dict[artefact.id]["total"] + artefact.completed_test_executions_count = counts_dict[artefact.id][ + "completed" + ] return artefacts diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 54c8ecaa..1c702fd4 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -51,7 +51,8 @@ class ArtefactDTO(BaseModel): assignee: UserDTO | None due_date: date | None bug_link: str - ratio_completed: float = 0 + all_test_executions_count: int = 0 + completed_test_executions_count: int = 0 class EnvironmentDTO(BaseModel): diff --git a/frontend/lib/models/artefact.dart b/frontend/lib/models/artefact.dart index 61ae575e..91e1bdc4 100644 --- a/frontend/lib/models/artefact.dart +++ b/frontend/lib/models/artefact.dart @@ -22,7 +22,10 @@ class Artefact with _$Artefact { required String repo, required ArtefactStatus status, required StageName stage, - @JsonKey(name: 'ratio_completed') required double ratioCompleted, + @JsonKey(name: 'all_test_executions_count') + required int allTestExecutionsCount, + @JsonKey(name: 'completed_test_executions_count') + required int completedTestExecutionsCount, User? assignee, @JsonKey(name: 'bug_link') required String bugLink, @JsonKey(name: 'due_date') DateTime? dueDate, diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index 4acb376c..a8e80bda 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -17,30 +17,6 @@ class ArtefactPageBody extends ConsumerWidget { final Artefact artefact; final List artefactBuilds; - int _getCompletedTestExecutionCount(List artefactBuilds) { - int completedCount = 0; - for (final artefactBuild in artefactBuilds) { - for (final testExecution in artefactBuild.testExecutions) { - if (testExecution.reviewDecision.isNotEmpty) completedCount++; - } - } - return completedCount; - } - - int _getTotalTestExecutionCount(List artefactBuilds) { - int totalCount = 0; - for (final artefactBuild in artefactBuilds) { - totalCount += artefactBuild.testExecutions.length; - } - return totalCount; - } - - double _getPercentage(List artefactBuilds) { - final completedCount = _getCompletedTestExecutionCount(artefactBuilds); - final totalCount = _getTotalTestExecutionCount(artefactBuilds); - return (completedCount / totalCount); - } - @override Widget build(BuildContext context, WidgetRef ref) { return Row( @@ -61,23 +37,10 @@ class ArtefactPageBody extends ConsumerWidget { 'Environments', style: Theme.of(context).textTheme.headlineSmall, ), - const SizedBox(width: Spacing.level4), - SizedBox( - width: 25.0, // specify the width - height: 25.0, // specify the height - child: CircularProgressIndicator( - value: _getPercentage(artefactBuilds), - semanticsLabel: 'Circular progress indicator', - ), - ), const Spacer(), const RerunFilteredEnvironmentsButton(), ], ), - LinearProgressIndicator( - value: _getPercentage(artefactBuilds), - semanticsLabel: 'Linear progress indicator', - ), Expanded( child: ListView.builder( itemCount: artefactBuilds.length, diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index 8483fc36..52ab1547 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -1,11 +1,9 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru/yaru.dart'; -import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/artefact.dart'; import '../../models/artefact_build.dart'; -import '../../providers/artefact_builds.dart'; import '../spacing.dart'; import '../user_avatar.dart'; import 'artefact_signoff_button.dart'; @@ -18,20 +16,18 @@ class ArtefactPageHeader extends ConsumerWidget { final Artefact artefact; final List artefactBuilds; - double _computeRatioCompleted(List artefactBuilds) { - int totalTestExecutions = 0, completedTestExecution = 0; - for (final artefactBuild in artefactBuilds) { - totalTestExecutions += artefactBuild.testExecutions.length; - for (final testExecution in artefactBuild.testExecutions) { - if (testExecution.reviewDecision.isNotEmpty) { - completedTestExecution++; - } - } - } + int get allTestExecutionsCount { + return artefactBuilds + .map((build) => build.testExecutions.length) + .fold(0, (a, b) => a + b); + } - return totalTestExecutions == 0 - ? 0 - : completedTestExecution / totalTestExecutions; + int get completedTestExecutionsCount { + return artefactBuilds + .map((build) => build.testExecutions + .where((testExecution) => testExecution.reviewDecision.isNotEmpty) + .length) + .fold(0, (a, b) => a + b); } @override @@ -48,7 +44,8 @@ class ArtefactPageHeader extends ConsumerWidget { if (assignee != null) UserAvatar( user: assignee, - ratioCompleted: _computeRatioCompleted(artefactBuilds), + allTestExecutionsCount: allTestExecutionsCount, + completedTestExecutionsCount: completedTestExecutionsCount, ), const SizedBox(width: Spacing.level4), if (dueDate != null) diff --git a/frontend/lib/ui/dashboard/artefact_card.dart b/frontend/lib/ui/dashboard/artefact_card.dart index 1d69cb21..e722d491 100644 --- a/frontend/lib/ui/dashboard/artefact_card.dart +++ b/frontend/lib/ui/dashboard/artefact_card.dart @@ -66,7 +66,9 @@ class ArtefactCard extends ConsumerWidget { if (assignee != null) UserAvatar( user: assignee, - ratioCompleted: artefact.ratioCompleted, + allTestExecutionsCount: artefact.allTestExecutionsCount, + completedTestExecutionsCount: + artefact.completedTestExecutionsCount, ), ], ), diff --git a/frontend/lib/ui/user_avatar.dart b/frontend/lib/ui/user_avatar.dart index 0f90124f..e13611ce 100644 --- a/frontend/lib/ui/user_avatar.dart +++ b/frontend/lib/ui/user_avatar.dart @@ -24,16 +24,27 @@ Color getDarkerColor(Color color, [double amount = 0.2]) { class UserAvatar extends StatelessWidget { const UserAvatar( - {super.key, required this.user, required this.ratioCompleted}); + {super.key, + required this.user, + required this.allTestExecutionsCount, + required this.completedTestExecutionsCount}); final User user; - final double ratioCompleted; + final int allTestExecutionsCount; + final int completedTestExecutionsCount; + + double get ratioCompleted { + if (allTestExecutionsCount == 0) { + return 0.0; + } + return completedTestExecutionsCount / allTestExecutionsCount; + } @override Widget build(BuildContext context) { return Tooltip( message: - '${user.name}\n${user.launchpadHandle}\n${user.launchpadEmail}\nCompleted ${(ratioCompleted * 100).round()}%', + '${user.name}\n${user.launchpadHandle}\n${user.launchpadEmail}\nCompleted: $completedTestExecutionsCount / $allTestExecutionsCount (${(ratioCompleted * 100).round()}%)', child: Stack( alignment: Alignment.center, children: [ diff --git a/frontend/test/dummy_data.dart b/frontend/test/dummy_data.dart index c51fe219..d05a2e56 100644 --- a/frontend/test/dummy_data.dart +++ b/frontend/test/dummy_data.dart @@ -24,7 +24,8 @@ const dummyArtefact = Artefact( stage: StageName.beta, assignee: dummyUser, bugLink: '', - ratioCompleted: 0.0, + allTestExecutionsCount: 1, + completedTestExecutionsCount: 0, ); const dummyEnvironment = Environment( From a5d9336e5aa311c9d979785363ddf3cf94dbb801 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Tue, 9 Jul 2024 14:22:09 +0200 Subject: [PATCH 04/15] Fix import issues --- backend/test_observer/controllers/artefacts/artefacts.py | 2 +- backend/test_observer/data_access/repository.py | 4 ++-- frontend/lib/ui/artefact_page/artefact_page_body.dart | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 8ddb7750..de6187a4 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -70,7 +70,7 @@ def _get_test_execution_counts_subquery(db: Session) -> RowReturningQuery: .subquery() ) - # Define subquery to calculate the ratio of completed TestExecutions to all TestExecutions + # Define the query to merge subquery for all and completed test executions return ( db.query( all_tests.c.artefact_id, diff --git a/backend/test_observer/data_access/repository.py b/backend/test_observer/data_access/repository.py index 84b1baa0..bb96e636 100644 --- a/backend/test_observer/data_access/repository.py +++ b/backend/test_observer/data_access/repository.py @@ -23,11 +23,11 @@ from collections.abc import Iterable from typing import Any -from sqlalchemy import and_, func, case +from sqlalchemy import and_, func from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, joinedload -from .models import Artefact, ArtefactBuild, DataModel, Family, Stage, TestExecution +from .models import Artefact, DataModel, Family, Stage from .models_enums import FamilyName diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index a8e80bda..30e6f0f6 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -1,10 +1,8 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; -import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/artefact.dart'; import '../../models/artefact_build.dart'; -import '../../providers/artefact_builds.dart'; import '../page_filters/page_filters.dart'; import '../spacing.dart'; import 'artefact_build_expandable.dart'; From 579e5c9e6b71fc2c999f79da39f2c24a608c1d26 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Tue, 9 Jul 2024 14:27:47 +0200 Subject: [PATCH 05/15] Fix mypy errors --- .../controllers/artefacts/artefacts.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index de6187a4..03d0efac 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -18,9 +18,8 @@ # Omar Selo # Nadzeya Hutsko from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import func +from sqlalchemy import Subquery, func from sqlalchemy.orm import Session, joinedload -from sqlalchemy.orm.query import RowReturningQuery from test_observer.data_access import queries @@ -46,7 +45,7 @@ router = APIRouter(tags=["artefacts"]) -def _get_test_execution_counts_subquery(db: Session) -> RowReturningQuery: +def _get_test_execution_counts_subquery(db: Session) -> Subquery: # Define subquery to count all TestExecutions for each Artefact all_tests = ( db.query( @@ -140,14 +139,19 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db } # Add the ratio_completed to the artefacts + parsed_artefacts: list[ArtefactDTO] = [] for artefact in artefacts: + parsed_artefact = ArtefactDTO.model_validate(artefact) if counts_dict.get(artefact.id): - artefact.all_test_executions_count = counts_dict[artefact.id]["total"] - artefact.completed_test_executions_count = counts_dict[artefact.id][ + parsed_artefact.all_test_executions_count = counts_dict[artefact.id][ + "total" + ] + parsed_artefact.completed_test_executions_count = counts_dict[artefact.id][ "completed" ] + parsed_artefacts.append(parsed_artefact) - return artefacts + return parsed_artefacts @router.get("/{artefact_id}", response_model=ArtefactDTO) From 7a884e2a74a1838b0726f0e59ade1ada3380a4d3 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Wed, 10 Jul 2024 13:57:56 +0200 Subject: [PATCH 06/15] Fix issue with tests --- .../controllers/artefacts/artefacts.py | 94 ++++--------------- .../controllers/artefacts/helpers.py | 90 ++++++++++++++++++ .../controllers/artefacts/models.py | 2 + .../controllers/artefacts/test_artefacts.py | 42 +++++++++ 4 files changed, 150 insertions(+), 78 deletions(-) create mode 100644 backend/test_observer/controllers/artefacts/helpers.py diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 03d0efac..c5a4cd50 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -18,10 +18,13 @@ # Omar Selo # Nadzeya Hutsko from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import Subquery, func from sqlalchemy.orm import Session, joinedload +from test_observer.controllers.artefacts.helpers import ( + _get_test_executions_count_dict, + parse_artefact_orm_object, +) from test_observer.data_access import queries from test_observer.data_access.models import ( Artefact, @@ -45,44 +48,6 @@ router = APIRouter(tags=["artefacts"]) -def _get_test_execution_counts_subquery(db: Session) -> Subquery: - # Define subquery to count all TestExecutions for each Artefact - all_tests = ( - db.query( - ArtefactBuild.artefact_id, - func.count(TestExecution.id).label("total"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .group_by(ArtefactBuild.artefact_id) - .subquery() - ) - - # Define subquery to count completed TestExecutions for each Artefact - completed_tests = ( - db.query( - ArtefactBuild.artefact_id, - func.count(TestExecution.id).label("completed"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .filter(func.array_length(TestExecution.review_decision, 1) > 0) - .group_by(ArtefactBuild.artefact_id) - .subquery() - ) - - # Define the query to merge subquery for all and completed test executions - return ( - db.query( - all_tests.c.artefact_id, - func.coalesce(all_tests.c.total, 0).label("total"), - func.coalesce(completed_tests.c.completed, 0).label("completed"), - ) - .outerjoin( - completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id - ) - .subquery() - ) - - def _get_artefact_from_db(artefact_id: int, db: Session = Depends(get_db)) -> Artefact: a = db.get(Artefact, artefact_id) if a is None: @@ -113,50 +78,23 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db order_by_columns=order_by, ) - test_execution_counts = _get_test_execution_counts_subquery(db) - - # Execute the query and fetch all results - results = ( - db.query( - Artefact.id, - test_execution_counts.c.total, - test_execution_counts.c.completed, - ) - .outerjoin( - test_execution_counts, Artefact.id == test_execution_counts.c.artefact_id - ) - .filter(Artefact.id.in_([artefact.id for artefact in artefacts])) - .all() + test_executions_count_dict = _get_test_executions_count_dict( + db, [artefact.id for artefact in artefacts] ) - # Convert the results to a dictionary - counts_dict = { - artefact_id: { - "total": total, - "completed": completed, - } - for artefact_id, total, completed in results - } - - # Add the ratio_completed to the artefacts - parsed_artefacts: list[ArtefactDTO] = [] - for artefact in artefacts: - parsed_artefact = ArtefactDTO.model_validate(artefact) - if counts_dict.get(artefact.id): - parsed_artefact.all_test_executions_count = counts_dict[artefact.id][ - "total" - ] - parsed_artefact.completed_test_executions_count = counts_dict[artefact.id][ - "completed" - ] - parsed_artefacts.append(parsed_artefact) - - return parsed_artefacts + # Parse artefacts and add test execution counts + return [ + parse_artefact_orm_object(artefact, test_executions_count_dict) + for artefact in artefacts + ] @router.get("/{artefact_id}", response_model=ArtefactDTO) -def get_artefact(artefact: Artefact = Depends(_get_artefact_from_db)): - return artefact +def get_artefact( + artefact: Artefact = Depends(_get_artefact_from_db), db: Session = Depends(get_db) +): + test_executions_count_dict = _get_test_executions_count_dict(db, [artefact.id]) + return parse_artefact_orm_object(artefact, test_executions_count_dict) @router.patch("/{artefact_id}", response_model=ArtefactDTO) diff --git a/backend/test_observer/controllers/artefacts/helpers.py b/backend/test_observer/controllers/artefacts/helpers.py new file mode 100644 index 00000000..d849d8a1 --- /dev/null +++ b/backend/test_observer/controllers/artefacts/helpers.py @@ -0,0 +1,90 @@ +from sqlalchemy import Subquery, func +from sqlalchemy.orm import Session + +from test_observer.controllers.artefacts.models import ArtefactDTO +from test_observer.data_access.models import ( + Artefact, + ArtefactBuild, + TestExecution, +) + + +def _get_test_execution_counts_subquery(db: Session) -> Subquery: + # Define subquery to count all TestExecutions for each Artefact + all_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("total"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .group_by(ArtefactBuild.artefact_id) + .subquery() + ) + + # Define subquery to count completed TestExecutions for each Artefact + completed_tests = ( + db.query( + ArtefactBuild.artefact_id, + func.count(TestExecution.id).label("completed"), + ) + .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) + .filter(func.array_length(TestExecution.review_decision, 1) > 0) + .group_by(ArtefactBuild.artefact_id) + .subquery() + ) + + # Define the query to merge subquery for all and completed test executions + return ( + db.query( + all_tests.c.artefact_id, + func.coalesce(all_tests.c.total, 0).label("total"), + func.coalesce(completed_tests.c.completed, 0).label("completed"), + ) + .outerjoin( + completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id + ) + .subquery() + ) + + +def _get_test_executions_count_dict( + db: Session, artefact_ids: list[int] +) -> dict[int, dict[str, int]]: + test_execution_counts = _get_test_execution_counts_subquery(db) + + # Execute the query and fetch all results + results = ( + db.query( + Artefact.id, + func.coalesce(test_execution_counts.c.total, 0).label("total"), + func.coalesce(test_execution_counts.c.completed, 0).label("completed"), + ) + .outerjoin( + test_execution_counts, Artefact.id == test_execution_counts.c.artefact_id + ) + .filter(Artefact.id.in_(artefact_ids)) + .all() + ) + + # Convert the results to a dictionary + return { + artefact_id: { + "total": total, + "completed": completed, + } + for artefact_id, total, completed in results + } + + +def parse_artefact_orm_object( + artefact: Artefact, counts_dict: dict[int, dict[str, int]] +) -> ArtefactDTO: + parsed_artefact = ArtefactDTO.model_validate(artefact) + if counts_dict.get(artefact.id): + parsed_artefact.all_test_executions_count = counts_dict[artefact.id].get( + "total", 0 + ) + parsed_artefact.completed_test_executions_count = counts_dict[artefact.id].get( + "completed", 0 + ) + return parsed_artefact diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 1c702fd4..ff7de385 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -30,6 +30,8 @@ class UserDTO(BaseModel): + model_config = ConfigDict(from_attributes=True) + id: int launchpad_handle: str launchpad_email: str diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 308f9ea9..fd636fac 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -19,6 +19,8 @@ # Nadzeya Hutsko from datetime import date, timedelta +from sqlalchemy.orm import Session + from fastapi.testclient import TestClient from test_observer.data_access.models import TestExecution @@ -64,6 +66,8 @@ def test_get_latest_artefacts_by_family( else None ), "bug_link": "", + "all_test_executions_count": 0, + "completed_test_executions_count": 0, } ] @@ -100,9 +104,45 @@ def test_get_artefact(test_client: TestClient, generator: DataGenerator): }, "due_date": "2024-12-24", "bug_link": a.bug_link, + "all_test_executions_count": 0, + "completed_test_executions_count": 0, } +def test_get_artefact_test_execution_counts( + test_client: TestClient, + generator: DataGenerator, + db_session: Session, +): + u = generator.gen_user() + a = generator.gen_artefact( + "edge", + status=ArtefactStatus.APPROVED, + bug_link="localhost/bug", + due_date=date(2024, 12, 24), + assignee_id=u.id, + ) + ab = generator.gen_artefact_build(a) + e = generator.gen_environment() + te = generator.gen_test_execution(ab, e) + + # Verify completed test execution count is zero, it is not reviewed yet + response = test_client.get(f"/v1/artefacts/{a.id}") + assert response.status_code == 200 + assert response.json()["all_test_executions_count"] == 1 + assert response.json()["completed_test_executions_count"] == 0 + + te.review_decision = [TestExecutionReviewDecision.APPROVED_ALL_TESTS_PASS] + db_session.commit() + db_session.refresh(te) + + # Verify completed test execution count is one, it is reviewed + response = test_client.get(f"/v1/artefacts/{a.id}") + assert response.status_code == 200 + assert response.json()["all_test_executions_count"] == 1 + assert response.json()["completed_test_executions_count"] == 1 + + def test_get_artefact_builds(test_client: TestClient, generator: DataGenerator): a = generator.gen_artefact("beta") ab = generator.gen_artefact_build(a) @@ -268,6 +308,8 @@ def test_artefact_signoff_approve(test_client: TestClient, generator: DataGenera artefact.due_date.strftime("%Y-%m-%d") if artefact.due_date else None ), "bug_link": "", + "all_test_executions_count": 0, + "completed_test_executions_count": 0, } From a8be3310b42c6fdc864d88c78bd729ff9a86e27e Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Wed, 10 Jul 2024 15:29:00 +0200 Subject: [PATCH 07/15] Add doc-string to new helper methods --- .../controllers/artefacts/helpers.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/backend/test_observer/controllers/artefacts/helpers.py b/backend/test_observer/controllers/artefacts/helpers.py index d849d8a1..d15f51a3 100644 --- a/backend/test_observer/controllers/artefacts/helpers.py +++ b/backend/test_observer/controllers/artefacts/helpers.py @@ -10,6 +10,22 @@ def _get_test_execution_counts_subquery(db: Session) -> Subquery: + """ + Helper method to get a subquery that fetches the counts of both: + * total Test Executions related to the Artefact + * completed Test Executions related to the Artefact + + Completed Test Execution is defined as one having at least one + review decision + + Parameters: + db (Session): Database session object used to generate the query for + + Returns: + Subquery SQLAlchemy object to be used in the main query, where we can + merge Artefacts queries with other filters with the counts fetched + from this helper method + """ # Define subquery to count all TestExecutions for each Artefact all_tests = ( db.query( @@ -50,6 +66,18 @@ def _get_test_execution_counts_subquery(db: Session) -> Subquery: def _get_test_executions_count_dict( db: Session, artefact_ids: list[int] ) -> dict[int, dict[str, int]]: + """ + Helper method to fetch the counts for all/completed test executions + related to an artefact and return them as a dictionary where the key is + the artefact id + + Parameters: + db (Session): Database session object used to generate the query for + artefact_ids (list[int]): List of Artefact IDs to fetch the counts for + Returns: + Dictionary where the key is the Artefact ID and the value is a dictionary + with the counts of all and completed test executions + """ test_execution_counts = _get_test_execution_counts_subquery(db) # Execute the query and fetch all results @@ -79,6 +107,18 @@ def _get_test_executions_count_dict( def parse_artefact_orm_object( artefact: Artefact, counts_dict: dict[int, dict[str, int]] ) -> ArtefactDTO: + """ + Parses the Artefact ORM object to a DTO object and adds the counts of + all and completed test executions to the DTO object + + Parameters: + artefact (Artefact): Artefact ORM object to parse + counts_dict (dict[int, dict[str, int]]): Dictionary with the counts of + all and completed test executions for each Artefact ID + Returns: + ArtefactDTO object with the counts of all and completed test executions + added to the object + """ parsed_artefact = ArtefactDTO.model_validate(artefact) if counts_dict.get(artefact.id): parsed_artefact.all_test_executions_count = counts_dict[artefact.id].get( From d581cdfe3451ae81553d978da4e26218114a6c2a Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Mon, 22 Jul 2024 13:21:39 +0200 Subject: [PATCH 08/15] Implement new logic for fetching --- .../controllers/artefacts/helpers.py | 46 +++++++++++-------- .../controllers/artefacts/test_artefacts.py | 31 +++++++++++++ frontend/lib/providers/artefact_builds.dart | 20 ++++++++ frontend/lib/providers/family_artefacts.dart | 14 ++++++ .../ui/artefact_page/artefact_page_body.dart | 2 +- .../artefact_page/artefact_page_header.dart | 18 +------- .../test_execution_pop_over.dart | 6 +++ .../artefact_page/test_execution_review.dart | 2 + 8 files changed, 104 insertions(+), 35 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/helpers.py b/backend/test_observer/controllers/artefacts/helpers.py index d15f51a3..93c73d31 100644 --- a/backend/test_observer/controllers/artefacts/helpers.py +++ b/backend/test_observer/controllers/artefacts/helpers.py @@ -1,6 +1,7 @@ from sqlalchemy import Subquery, func from sqlalchemy.orm import Session +from test_observer.data_access import queries from test_observer.controllers.artefacts.models import ArtefactDTO from test_observer.data_access.models import ( Artefact, @@ -26,39 +27,37 @@ def _get_test_execution_counts_subquery(db: Session) -> Subquery: merge Artefacts queries with other filters with the counts fetched from this helper method """ - # Define subquery to count all TestExecutions for each Artefact + # Define subquery to count all TestExecutions for each ArtefactBuild all_tests = ( db.query( - ArtefactBuild.artefact_id, + ArtefactBuild.id, func.count(TestExecution.id).label("total"), ) .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .group_by(ArtefactBuild.artefact_id) + .group_by(ArtefactBuild.id) .subquery() ) - # Define subquery to count completed TestExecutions for each Artefact + # Define subquery to count completed TestExecutions for each ArtefactBuild completed_tests = ( db.query( - ArtefactBuild.artefact_id, + ArtefactBuild.id, func.count(TestExecution.id).label("completed"), ) .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) .filter(func.array_length(TestExecution.review_decision, 1) > 0) - .group_by(ArtefactBuild.artefact_id) + .group_by(ArtefactBuild.id) .subquery() ) - # Define the query to merge subquery for all and completed test executions + # Define the query to merge the subqueries for all and completed test executions return ( db.query( - all_tests.c.artefact_id, + all_tests.c.id, func.coalesce(all_tests.c.total, 0).label("total"), func.coalesce(completed_tests.c.completed, 0).label("completed"), ) - .outerjoin( - completed_tests, all_tests.c.artefact_id == completed_tests.c.artefact_id - ) + .outerjoin(completed_tests, all_tests.c.id == completed_tests.c.id) .subquery() ) @@ -80,25 +79,36 @@ def _get_test_executions_count_dict( """ test_execution_counts = _get_test_execution_counts_subquery(db) + artefact_build_ids = list( + artefact_build.id + for artefact_build in db.scalars( + queries.latest_artefact_builds.where( + ArtefactBuild.artefact_id.in_(artefact_ids) + ) + ).unique() + ) + # Execute the query and fetch all results results = ( db.query( - Artefact.id, - func.coalesce(test_execution_counts.c.total, 0).label("total"), - func.coalesce(test_execution_counts.c.completed, 0).label("completed"), + ArtefactBuild.artefact_id, + func.sum(test_execution_counts.c.total).label("total"), + func.sum(test_execution_counts.c.completed).label("completed"), ) .outerjoin( - test_execution_counts, Artefact.id == test_execution_counts.c.artefact_id + test_execution_counts, + ArtefactBuild.id == test_execution_counts.c.id, ) - .filter(Artefact.id.in_(artefact_ids)) + .filter(test_execution_counts.c.id.in_(artefact_build_ids)) + .group_by(ArtefactBuild.artefact_id) .all() ) # Convert the results to a dictionary return { artefact_id: { - "total": total, - "completed": completed, + "total": int(total), + "completed": int(completed), } for artefact_id, total, completed in results } diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index fd636fac..b2610f3b 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -109,6 +109,37 @@ def test_get_artefact(test_client: TestClient, generator: DataGenerator): } +def test_get_artefact_test_execution_counts_only_latest_build( + test_client: TestClient, generator: DataGenerator +): + u = generator.gen_user() + a = generator.gen_artefact( + "edge", + status=ArtefactStatus.APPROVED, + bug_link="localhost/bug", + due_date=date(2024, 12, 24), + assignee_id=u.id, + ) + ab = generator.gen_artefact_build(artefact=a, revision=1) + e = generator.gen_environment() + # Test Execution for the first artefact build + generator.gen_test_execution(ab, e) + + ab_second = generator.gen_artefact_build(artefact=a, revision=2) + # Test Execution for the second artefact build + generator.gen_test_execution( + artefact_build=ab_second, + environment=e, + review_decision=[TestExecutionReviewDecision.APPROVED_ALL_TESTS_PASS], + ) + + response = test_client.get(f"/v1/artefacts/{a.id}") + assert response.status_code == 200 + # Verify only the counts of the latest build is returned + assert response.json()["all_test_executions_count"] == 1 + assert response.json()["completed_test_executions_count"] == 1 + + def test_get_artefact_test_execution_counts( test_client: TestClient, generator: DataGenerator, diff --git a/frontend/lib/providers/artefact_builds.dart b/frontend/lib/providers/artefact_builds.dart index 780cf165..109fb41f 100644 --- a/frontend/lib/providers/artefact_builds.dart +++ b/frontend/lib/providers/artefact_builds.dart @@ -1,8 +1,12 @@ import 'package:riverpod_annotation/riverpod_annotation.dart'; +import '../models/artefact.dart'; import '../models/artefact_build.dart'; +import '../models/family_name.dart'; import '../models/test_execution.dart'; +import '../routing.dart'; import 'api.dart'; +import 'family_artefacts.dart'; part 'artefact_builds.g.dart'; @@ -18,6 +22,8 @@ class ArtefactBuilds extends _$ArtefactBuilds { int testExecutionId, String reviewComment, List reviewDecision, + FamilyName familyName, + int artefactId, ) async { final api = ref.read(apiProvider); final testExecution = await api.changeTestExecutionReview( @@ -27,6 +33,20 @@ class ArtefactBuilds extends _$ArtefactBuilds { ); await _updateTestExecutions({testExecutionId}, (_) => testExecution); + + final artefactBuilds = await future; + final newCompletedTestExecutionsCount = artefactBuilds + .map((build) => build.testExecutions + .where((testExecution) => testExecution.reviewDecision.isNotEmpty) + .length) + .fold(0, (a, b) => a + b); + + ref + .read(familyArtefactsProvider(familyName).notifier) + .updateCompletedTestExecutionsCount( + artefactId, + newCompletedTestExecutionsCount, + ); } Future rerunTestExecutions(Set testExecutionIds) async { diff --git a/frontend/lib/providers/family_artefacts.dart b/frontend/lib/providers/family_artefacts.dart index 0ddc794d..d407c16e 100644 --- a/frontend/lib/providers/family_artefacts.dart +++ b/frontend/lib/providers/family_artefacts.dart @@ -24,4 +24,18 @@ class FamilyArtefacts extends _$FamilyArtefacts { final previousState = await future; state = AsyncData({...previousState, artefact.id: artefact}); } + + Future updateCompletedTestExecutionsCount( + int artefactId, + int count, + ) async { + final previousState = await future; + final artefact = previousState[artefactId]; + if (artefact != null) { + state = AsyncData({ + ...previousState, + artefactId: artefact.copyWith(completedTestExecutionsCount: count), + }); + } + } } diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index 30e6f0f6..fc502ee8 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -32,7 +32,7 @@ class ArtefactPageBody extends ConsumerWidget { textBaseline: TextBaseline.alphabetic, children: [ Text( - 'Environments', + 'Environments (${artefact.completedTestExecutionsCount}/${artefact.allTestExecutionsCount})', style: Theme.of(context).textTheme.headlineSmall, ), const Spacer(), diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index 52ab1547..fa7ecc8d 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -16,20 +16,6 @@ class ArtefactPageHeader extends ConsumerWidget { final Artefact artefact; final List artefactBuilds; - int get allTestExecutionsCount { - return artefactBuilds - .map((build) => build.testExecutions.length) - .fold(0, (a, b) => a + b); - } - - int get completedTestExecutionsCount { - return artefactBuilds - .map((build) => build.testExecutions - .where((testExecution) => testExecution.reviewDecision.isNotEmpty) - .length) - .fold(0, (a, b) => a + b); - } - @override Widget build(BuildContext context, WidgetRef ref) { final assignee = artefact.assignee; @@ -44,8 +30,8 @@ class ArtefactPageHeader extends ConsumerWidget { if (assignee != null) UserAvatar( user: assignee, - allTestExecutionsCount: allTestExecutionsCount, - completedTestExecutionsCount: completedTestExecutionsCount, + allTestExecutionsCount: artefact.allTestExecutionsCount, + completedTestExecutionsCount: artefact.completedTestExecutionsCount, ), const SizedBox(width: Spacing.level4), if (dueDate != null) diff --git a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart index 61895887..acbd4280 100644 --- a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart +++ b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart @@ -2,8 +2,10 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru_widgets/yaru_widgets.dart'; +import '../../models/family_name.dart'; import '../../models/test_execution.dart'; import '../../providers/artefact_builds.dart'; +import '../../routing.dart'; import '../spacing.dart'; class TestExecutionPopOver extends ConsumerStatefulWidget { @@ -11,10 +13,12 @@ class TestExecutionPopOver extends ConsumerStatefulWidget { super.key, required this.testExecution, required this.artefactId, + required this.family, }); final TestExecution testExecution; final int artefactId; + final FamilyName family; @override TestExecutionPopOverState createState() => TestExecutionPopOverState(); @@ -122,6 +126,8 @@ class TestExecutionPopOverState extends ConsumerState { widget.testExecution.id, reviewCommentController.text, reviewDecisions, + widget.family, + widget.artefactId, ); Navigator.pop(context); }, diff --git a/frontend/lib/ui/artefact_page/test_execution_review.dart b/frontend/lib/ui/artefact_page/test_execution_review.dart index 43017fbb..f136f5a6 100644 --- a/frontend/lib/ui/artefact_page/test_execution_review.dart +++ b/frontend/lib/ui/artefact_page/test_execution_review.dart @@ -37,6 +37,7 @@ class TestExecutionReviewButton extends StatelessWidget { @override Widget build(BuildContext context) { + final family = AppRoutes.familyFromUri(AppRoutes.uriFromContext(context)); final artefactId = AppRoutes.artefactIdFromUri(AppRoutes.uriFromContext(context)); return GestureDetector( @@ -46,6 +47,7 @@ class TestExecutionReviewButton extends StatelessWidget { bodyBuilder: (context) => TestExecutionPopOver( testExecution: testExecution, artefactId: artefactId, + family: family, ), direction: PopoverDirection.bottom, width: 500, From aea54080dd3bd827ef0aff52c2b28df1d4a3d3e5 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Mon, 22 Jul 2024 13:26:18 +0200 Subject: [PATCH 09/15] Fix ruff issue --- backend/test_observer/controllers/artefacts/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/helpers.py b/backend/test_observer/controllers/artefacts/helpers.py index 93c73d31..8bf69aa7 100644 --- a/backend/test_observer/controllers/artefacts/helpers.py +++ b/backend/test_observer/controllers/artefacts/helpers.py @@ -79,14 +79,14 @@ def _get_test_executions_count_dict( """ test_execution_counts = _get_test_execution_counts_subquery(db) - artefact_build_ids = list( + artefact_build_ids = [ artefact_build.id for artefact_build in db.scalars( queries.latest_artefact_builds.where( ArtefactBuild.artefact_id.in_(artefact_ids) ) ).unique() - ) + ] # Execute the query and fetch all results results = ( From bdf8c4811d3e7d4d1cc8af13ef1a115bf4dc293c Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Mon, 22 Jul 2024 13:46:07 +0200 Subject: [PATCH 10/15] Remove unnecessary changes on frontend --- .../lib/ui/artefact_page/artefact_page.dart | 30 +++---- .../ui/artefact_page/artefact_page_body.dart | 83 ++++++++++--------- .../artefact_page/artefact_page_header.dart | 6 +- 3 files changed, 57 insertions(+), 62 deletions(-) diff --git a/frontend/lib/ui/artefact_page/artefact_page.dart b/frontend/lib/ui/artefact_page/artefact_page.dart index 7a223d5c..fd8e5122 100644 --- a/frontend/lib/ui/artefact_page/artefact_page.dart +++ b/frontend/lib/ui/artefact_page/artefact_page.dart @@ -45,25 +45,17 @@ class ArtefactPage extends ConsumerWidget { final artefact = artefacts[artefactId]; if (artefact == null) return _invalidArtefactErrorMessage; - final artefactBuilds = ref.watch(ArtefactBuildsProvider(artefact.id)); - return artefactBuilds.when( - data: (artefactBuilds) => Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - ArtefactPageHeader( - artefact: artefact, artefactBuilds: artefactBuilds), - const SizedBox(height: Spacing.level4), - ArtefactPageInfoSection(artefact: artefact), - const SizedBox(height: Spacing.level4), - Expanded( - child: ArtefactPageBody( - artefact: artefact, artefactBuilds: artefactBuilds), - ), - ], - ), - error: (e, stack) => - Center(child: Text('Error:\n$e\nStackTrace:\n$stack')), - loading: () => const Center(child: YaruCircularProgressIndicator()), + return Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + ArtefactPageHeader(artefact: artefact), + const SizedBox(height: Spacing.level4), + ArtefactPageInfoSection(artefact: artefact), + const SizedBox(height: Spacing.level4), + Expanded( + child: ArtefactPageBody(artefact: artefact), + ), + ], ); }, error: (e, stack) => diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index fc502ee8..26a8c7f4 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -1,60 +1,67 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/artefact.dart'; -import '../../models/artefact_build.dart'; +import '../../providers/artefact_builds.dart'; import '../page_filters/page_filters.dart'; import '../spacing.dart'; import 'artefact_build_expandable.dart'; import 'rerun_filtered_environments_button.dart'; class ArtefactPageBody extends ConsumerWidget { - const ArtefactPageBody( - {super.key, required this.artefact, required this.artefactBuilds}); + const ArtefactPageBody({super.key, required this.artefact}); final Artefact artefact; - final List artefactBuilds; @override Widget build(BuildContext context, WidgetRef ref) { - return Row( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const PageFiltersView(searchHint: 'Search by environment name'), - const SizedBox(width: Spacing.level5), - Expanded( - child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const SizedBox(height: Spacing.level3), - Row( - crossAxisAlignment: CrossAxisAlignment.center, - textBaseline: TextBaseline.alphabetic, - children: [ - Text( - 'Environments (${artefact.completedTestExecutionsCount}/${artefact.allTestExecutionsCount})', - style: Theme.of(context).textTheme.headlineSmall, - ), - const Spacer(), - const RerunFilteredEnvironmentsButton(), - ], - ), - Expanded( - child: ListView.builder( - itemCount: artefactBuilds.length, - itemBuilder: (_, i) => Padding( - // Padding is to avoid scroll bar covering trailing buttons - padding: const EdgeInsets.only(right: Spacing.level3), - child: ArtefactBuildExpandable( - artefactBuild: artefactBuilds[i], + final artefactBuilds = ref.watch(ArtefactBuildsProvider(artefact.id)); + + return artefactBuilds.when( + data: (artefactBuilds) => Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const PageFiltersView(searchHint: 'Search by environment name'), + const SizedBox(width: Spacing.level5), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const SizedBox(height: Spacing.level3), + Row( + crossAxisAlignment: CrossAxisAlignment.baseline, + textBaseline: TextBaseline.alphabetic, + children: [ + Text( + 'Environments (${artefact.completedTestExecutionsCount}/${artefact.allTestExecutionsCount})', + style: Theme.of(context).textTheme.headlineSmall, + ), + const Spacer(), + const RerunFilteredEnvironmentsButton(), + ], + ), + Expanded( + child: ListView.builder( + itemCount: artefactBuilds.length, + itemBuilder: (_, i) => Padding( + // Padding is to avoid scroll bar covering trailing buttons + padding: const EdgeInsets.only(right: Spacing.level3), + child: ArtefactBuildExpandable( + artefactBuild: artefactBuilds[i], + ), ), ), ), - ), - ], + ], + ), ), - ), - ], + ], + ), + loading: () => const Center(child: YaruCircularProgressIndicator()), + error: (error, stackTrace) { + return Center(child: Text('Error: $error')); + }, ); } } diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index fa7ecc8d..d7f2344a 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -3,18 +3,14 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru/yaru.dart'; import '../../models/artefact.dart'; -import '../../models/artefact_build.dart'; import '../spacing.dart'; import '../user_avatar.dart'; import 'artefact_signoff_button.dart'; class ArtefactPageHeader extends ConsumerWidget { - const ArtefactPageHeader( - {Key? key, required this.artefact, required this.artefactBuilds}) - : super(key: key); + const ArtefactPageHeader({super.key, required this.artefact}); final Artefact artefact; - final List artefactBuilds; @override Widget build(BuildContext context, WidgetRef ref) { From 07490334c625b1d9a12b5652477d854793c24695 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Mon, 22 Jul 2024 15:16:32 +0200 Subject: [PATCH 11/15] Small improvements --- .../test_observer/controllers/artefacts/artefacts.py | 3 ++- frontend/benchmarks/common.dart | 2 ++ frontend/lib/providers/artefact_builds.dart | 10 +++++----- frontend/lib/ui/artefact_page/artefact_page.dart | 2 -- .../lib/ui/artefact_page/artefact_page_header.dart | 5 ++--- .../lib/ui/artefact_page/test_execution_pop_over.dart | 1 - 6 files changed, 11 insertions(+), 12 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index c5a4cd50..3873974d 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -115,7 +115,8 @@ def patch_artefact( artefact.status = request.status db.commit() - return artefact + test_executions_count_dict = _get_test_executions_count_dict(db, [artefact.id]) + return parse_artefact_orm_object(artefact, test_executions_count_dict) def _validate_artefact_status( diff --git a/frontend/benchmarks/common.dart b/frontend/benchmarks/common.dart index 28ef2a82..3706777f 100644 --- a/frontend/benchmarks/common.dart +++ b/frontend/benchmarks/common.dart @@ -51,6 +51,8 @@ class ApiRepositoryMock extends Mock implements ApiRepository { status: ArtefactStatus.undecided, stage: StageName.beta, bugLink: '', + allTestExecutionsCount: 1, + completedTestExecutionsCount: 0, ); return { diff --git a/frontend/lib/providers/artefact_builds.dart b/frontend/lib/providers/artefact_builds.dart index 109fb41f..658ec5a7 100644 --- a/frontend/lib/providers/artefact_builds.dart +++ b/frontend/lib/providers/artefact_builds.dart @@ -1,10 +1,8 @@ import 'package:riverpod_annotation/riverpod_annotation.dart'; -import '../models/artefact.dart'; import '../models/artefact_build.dart'; import '../models/family_name.dart'; import '../models/test_execution.dart'; -import '../routing.dart'; import 'api.dart'; import 'family_artefacts.dart'; @@ -36,9 +34,11 @@ class ArtefactBuilds extends _$ArtefactBuilds { final artefactBuilds = await future; final newCompletedTestExecutionsCount = artefactBuilds - .map((build) => build.testExecutions - .where((testExecution) => testExecution.reviewDecision.isNotEmpty) - .length) + .map( + (build) => build.testExecutions + .where((testExecution) => testExecution.reviewDecision.isNotEmpty) + .length, + ) .fold(0, (a, b) => a + b); ref diff --git a/frontend/lib/ui/artefact_page/artefact_page.dart b/frontend/lib/ui/artefact_page/artefact_page.dart index fd8e5122..6f82b4dc 100644 --- a/frontend/lib/ui/artefact_page/artefact_page.dart +++ b/frontend/lib/ui/artefact_page/artefact_page.dart @@ -2,7 +2,6 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru_widgets/yaru_widgets.dart'; -import '../../providers/artefact_builds.dart'; import '../../providers/family_artefacts.dart'; import '../../routing.dart'; import '../dialog_header.dart'; @@ -44,7 +43,6 @@ class ArtefactPage extends ConsumerWidget { data: (artefacts) { final artefact = artefacts[artefactId]; if (artefact == null) return _invalidArtefactErrorMessage; - return Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ diff --git a/frontend/lib/ui/artefact_page/artefact_page_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index d7f2344a..c9789d6b 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -1,5 +1,4 @@ import 'package:flutter/material.dart'; -import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:yaru/yaru.dart'; import '../../models/artefact.dart'; @@ -7,13 +6,13 @@ import '../spacing.dart'; import '../user_avatar.dart'; import 'artefact_signoff_button.dart'; -class ArtefactPageHeader extends ConsumerWidget { +class ArtefactPageHeader extends StatelessWidget { const ArtefactPageHeader({super.key, required this.artefact}); final Artefact artefact; @override - Widget build(BuildContext context, WidgetRef ref) { + Widget build(BuildContext context) { final assignee = artefact.assignee; final dueDate = artefact.dueDateString; diff --git a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart index acbd4280..15a04bf4 100644 --- a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart +++ b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart @@ -5,7 +5,6 @@ import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/family_name.dart'; import '../../models/test_execution.dart'; import '../../providers/artefact_builds.dart'; -import '../../routing.dart'; import '../spacing.dart'; class TestExecutionPopOver extends ConsumerStatefulWidget { From 23f9e9d3776b0906f3010682d5d5cf9ed3be1a6a Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Mon, 22 Jul 2024 15:38:10 +0200 Subject: [PATCH 12/15] Fix user avatar --- frontend/lib/ui/user_avatar.dart | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frontend/lib/ui/user_avatar.dart b/frontend/lib/ui/user_avatar.dart index e13611ce..5eb52f75 100644 --- a/frontend/lib/ui/user_avatar.dart +++ b/frontend/lib/ui/user_avatar.dart @@ -23,11 +23,12 @@ Color getDarkerColor(Color color, [double amount = 0.2]) { } class UserAvatar extends StatelessWidget { - const UserAvatar( - {super.key, - required this.user, - required this.allTestExecutionsCount, - required this.completedTestExecutionsCount}); + const UserAvatar({ + super.key, + required this.user, + required this.allTestExecutionsCount, + required this.completedTestExecutionsCount, + }); final User user; final int allTestExecutionsCount; @@ -59,8 +60,8 @@ class UserAvatar extends StatelessWidget { ), ), SizedBox( - width: 43.0, // specify the width - height: 43.0, // specify the height + width: 43.0, + height: 43.0, child: CircularProgressIndicator( color: getDarkerColor(userIdToColor(user.id)), backgroundColor: userIdToColor(user.id), From 3fc11e9c3d065ff7501ec725cd670fab6d0a9a1b Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Wed, 24 Jul 2024 16:23:37 +0200 Subject: [PATCH 13/15] Implement PR suggestions --- .../controllers/artefacts/artefacts.py | 34 ++--- .../controllers/artefacts/helpers.py | 140 ------------------ .../controllers/artefacts/models.py | 4 +- backend/test_observer/data_access/models.py | 22 +++ .../test_observer/data_access/repository.py | 8 +- .../controllers/artefacts/test_artefacts.py | 18 +-- frontend/lib/providers/artefact_builds.dart | 20 --- .../lib/providers/review_test_execution.dart | 50 +++++++ .../ui/artefact_page/artefact_page_body.dart | 2 +- .../test_execution_pop_over.dart | 6 +- 10 files changed, 100 insertions(+), 204 deletions(-) delete mode 100644 backend/test_observer/controllers/artefacts/helpers.py create mode 100644 frontend/lib/providers/review_test_execution.dart diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 3873974d..0f351ee6 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -20,11 +20,6 @@ from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.orm import Session, joinedload - -from test_observer.controllers.artefacts.helpers import ( - _get_test_executions_count_dict, - parse_artefact_orm_object, -) from test_observer.data_access import queries from test_observer.data_access.models import ( Artefact, @@ -49,7 +44,14 @@ def _get_artefact_from_db(artefact_id: int, db: Session = Depends(get_db)) -> Artefact: - a = db.get(Artefact, artefact_id) + a = ( + db.query(Artefact) + .options( + joinedload(Artefact.builds).joinedload(ArtefactBuild.test_executions), + ) + .filter(Artefact.id == artefact_id) + .one_or_none() + ) if a is None: msg = f"Artefact with id {artefact_id} not found" raise HTTPException(status_code=404, detail=msg) @@ -67,6 +69,7 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db db, family, load_stage=True, + load_test_executions=True, order_by_columns=order_by, ) else: @@ -75,26 +78,18 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db db, family, load_stage=True, + load_test_executions=True, order_by_columns=order_by, ) - test_executions_count_dict = _get_test_executions_count_dict( - db, [artefact.id for artefact in artefacts] - ) - - # Parse artefacts and add test execution counts - return [ - parse_artefact_orm_object(artefact, test_executions_count_dict) - for artefact in artefacts - ] + return artefacts @router.get("/{artefact_id}", response_model=ArtefactDTO) def get_artefact( - artefact: Artefact = Depends(_get_artefact_from_db), db: Session = Depends(get_db) + artefact: Artefact = Depends(_get_artefact_from_db), ): - test_executions_count_dict = _get_test_executions_count_dict(db, [artefact.id]) - return parse_artefact_orm_object(artefact, test_executions_count_dict) + return artefact @router.patch("/{artefact_id}", response_model=ArtefactDTO) @@ -115,8 +110,7 @@ def patch_artefact( artefact.status = request.status db.commit() - test_executions_count_dict = _get_test_executions_count_dict(db, [artefact.id]) - return parse_artefact_orm_object(artefact, test_executions_count_dict) + return artefact def _validate_artefact_status( diff --git a/backend/test_observer/controllers/artefacts/helpers.py b/backend/test_observer/controllers/artefacts/helpers.py deleted file mode 100644 index 8bf69aa7..00000000 --- a/backend/test_observer/controllers/artefacts/helpers.py +++ /dev/null @@ -1,140 +0,0 @@ -from sqlalchemy import Subquery, func -from sqlalchemy.orm import Session - -from test_observer.data_access import queries -from test_observer.controllers.artefacts.models import ArtefactDTO -from test_observer.data_access.models import ( - Artefact, - ArtefactBuild, - TestExecution, -) - - -def _get_test_execution_counts_subquery(db: Session) -> Subquery: - """ - Helper method to get a subquery that fetches the counts of both: - * total Test Executions related to the Artefact - * completed Test Executions related to the Artefact - - Completed Test Execution is defined as one having at least one - review decision - - Parameters: - db (Session): Database session object used to generate the query for - - Returns: - Subquery SQLAlchemy object to be used in the main query, where we can - merge Artefacts queries with other filters with the counts fetched - from this helper method - """ - # Define subquery to count all TestExecutions for each ArtefactBuild - all_tests = ( - db.query( - ArtefactBuild.id, - func.count(TestExecution.id).label("total"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .group_by(ArtefactBuild.id) - .subquery() - ) - - # Define subquery to count completed TestExecutions for each ArtefactBuild - completed_tests = ( - db.query( - ArtefactBuild.id, - func.count(TestExecution.id).label("completed"), - ) - .join(TestExecution, TestExecution.artefact_build_id == ArtefactBuild.id) - .filter(func.array_length(TestExecution.review_decision, 1) > 0) - .group_by(ArtefactBuild.id) - .subquery() - ) - - # Define the query to merge the subqueries for all and completed test executions - return ( - db.query( - all_tests.c.id, - func.coalesce(all_tests.c.total, 0).label("total"), - func.coalesce(completed_tests.c.completed, 0).label("completed"), - ) - .outerjoin(completed_tests, all_tests.c.id == completed_tests.c.id) - .subquery() - ) - - -def _get_test_executions_count_dict( - db: Session, artefact_ids: list[int] -) -> dict[int, dict[str, int]]: - """ - Helper method to fetch the counts for all/completed test executions - related to an artefact and return them as a dictionary where the key is - the artefact id - - Parameters: - db (Session): Database session object used to generate the query for - artefact_ids (list[int]): List of Artefact IDs to fetch the counts for - Returns: - Dictionary where the key is the Artefact ID and the value is a dictionary - with the counts of all and completed test executions - """ - test_execution_counts = _get_test_execution_counts_subquery(db) - - artefact_build_ids = [ - artefact_build.id - for artefact_build in db.scalars( - queries.latest_artefact_builds.where( - ArtefactBuild.artefact_id.in_(artefact_ids) - ) - ).unique() - ] - - # Execute the query and fetch all results - results = ( - db.query( - ArtefactBuild.artefact_id, - func.sum(test_execution_counts.c.total).label("total"), - func.sum(test_execution_counts.c.completed).label("completed"), - ) - .outerjoin( - test_execution_counts, - ArtefactBuild.id == test_execution_counts.c.id, - ) - .filter(test_execution_counts.c.id.in_(artefact_build_ids)) - .group_by(ArtefactBuild.artefact_id) - .all() - ) - - # Convert the results to a dictionary - return { - artefact_id: { - "total": int(total), - "completed": int(completed), - } - for artefact_id, total, completed in results - } - - -def parse_artefact_orm_object( - artefact: Artefact, counts_dict: dict[int, dict[str, int]] -) -> ArtefactDTO: - """ - Parses the Artefact ORM object to a DTO object and adds the counts of - all and completed test executions to the DTO object - - Parameters: - artefact (Artefact): Artefact ORM object to parse - counts_dict (dict[int, dict[str, int]]): Dictionary with the counts of - all and completed test executions for each Artefact ID - Returns: - ArtefactDTO object with the counts of all and completed test executions - added to the object - """ - parsed_artefact = ArtefactDTO.model_validate(artefact) - if counts_dict.get(artefact.id): - parsed_artefact.all_test_executions_count = counts_dict[artefact.id].get( - "total", 0 - ) - parsed_artefact.completed_test_executions_count = counts_dict[artefact.id].get( - "completed", 0 - ) - return parsed_artefact diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index ff7de385..fd7ab995 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -53,8 +53,8 @@ class ArtefactDTO(BaseModel): assignee: UserDTO | None due_date: date | None bug_link: str - all_test_executions_count: int = 0 - completed_test_executions_count: int = 0 + all_test_executions_count: int + completed_test_executions_count: int class EnvironmentDTO(BaseModel): diff --git a/backend/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index 1e0cf7f9..821322b6 100644 --- a/backend/test_observer/data_access/models.py +++ b/backend/test_observer/data_access/models.py @@ -18,6 +18,7 @@ # Nadzeya Hutsko # Omar Selo +from collections import defaultdict from datetime import date, datetime, timedelta from typing import TypeVar @@ -207,6 +208,27 @@ def is_kernel(self) -> bool: """Kernel artefacts start with 'linix-' or end with '-kernel'""" return self.name.startswith("linux-") or self.name.endswith("-kernel") + def _get_latest_builds(self) -> list["ArtefactBuild"]: + # Group builds by architecture + grouped_builds = defaultdict(list) + for build in self.builds: + grouped_builds[build.architecture].append(build) + + return [ + max(builds, key=lambda b: b.revision) for builds in grouped_builds.values() + ] + + @property + def all_test_executions_count(self) -> int: + return sum(len(ab.test_executions) for ab in self._get_latest_builds()) + + @property + def completed_test_executions_count(self) -> int: + return sum( + len([te for te in ab.test_executions if te.review_decision]) + for ab in self._get_latest_builds() + ) + class ArtefactBuild(Base): """A model to represent specific builds of artefact (e.g. arm64 revision 2)""" diff --git a/backend/test_observer/data_access/repository.py b/backend/test_observer/data_access/repository.py index bb96e636..df466b34 100644 --- a/backend/test_observer/data_access/repository.py +++ b/backend/test_observer/data_access/repository.py @@ -27,7 +27,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, joinedload -from .models import Artefact, DataModel, Family, Stage +from .models import Artefact, ArtefactBuild, DataModel, Family, Stage from .models_enums import FamilyName @@ -55,6 +55,7 @@ def get_artefacts_by_family( family_name: FamilyName, latest_only: bool = True, load_stage: bool = False, + load_test_executions: bool = False, order_by_columns: Iterable[Any] | None = None, ) -> list[Artefact]: """ @@ -126,6 +127,11 @@ def get_artefacts_by_family( if load_stage: query = query.options(joinedload(Artefact.stage)) + if load_test_executions: + query = query.options( + joinedload(Artefact.builds).joinedload(ArtefactBuild.test_executions) + ) + if order_by_columns: query = query.order_by(*order_by_columns) diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index b2610f3b..3ee1ad76 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -112,14 +112,7 @@ def test_get_artefact(test_client: TestClient, generator: DataGenerator): def test_get_artefact_test_execution_counts_only_latest_build( test_client: TestClient, generator: DataGenerator ): - u = generator.gen_user() - a = generator.gen_artefact( - "edge", - status=ArtefactStatus.APPROVED, - bug_link="localhost/bug", - due_date=date(2024, 12, 24), - assignee_id=u.id, - ) + a = generator.gen_artefact() ab = generator.gen_artefact_build(artefact=a, revision=1) e = generator.gen_environment() # Test Execution for the first artefact build @@ -145,14 +138,7 @@ def test_get_artefact_test_execution_counts( generator: DataGenerator, db_session: Session, ): - u = generator.gen_user() - a = generator.gen_artefact( - "edge", - status=ArtefactStatus.APPROVED, - bug_link="localhost/bug", - due_date=date(2024, 12, 24), - assignee_id=u.id, - ) + a = generator.gen_artefact() ab = generator.gen_artefact_build(a) e = generator.gen_environment() te = generator.gen_test_execution(ab, e) diff --git a/frontend/lib/providers/artefact_builds.dart b/frontend/lib/providers/artefact_builds.dart index 658ec5a7..780cf165 100644 --- a/frontend/lib/providers/artefact_builds.dart +++ b/frontend/lib/providers/artefact_builds.dart @@ -1,10 +1,8 @@ import 'package:riverpod_annotation/riverpod_annotation.dart'; import '../models/artefact_build.dart'; -import '../models/family_name.dart'; import '../models/test_execution.dart'; import 'api.dart'; -import 'family_artefacts.dart'; part 'artefact_builds.g.dart'; @@ -20,8 +18,6 @@ class ArtefactBuilds extends _$ArtefactBuilds { int testExecutionId, String reviewComment, List reviewDecision, - FamilyName familyName, - int artefactId, ) async { final api = ref.read(apiProvider); final testExecution = await api.changeTestExecutionReview( @@ -31,22 +27,6 @@ class ArtefactBuilds extends _$ArtefactBuilds { ); await _updateTestExecutions({testExecutionId}, (_) => testExecution); - - final artefactBuilds = await future; - final newCompletedTestExecutionsCount = artefactBuilds - .map( - (build) => build.testExecutions - .where((testExecution) => testExecution.reviewDecision.isNotEmpty) - .length, - ) - .fold(0, (a, b) => a + b); - - ref - .read(familyArtefactsProvider(familyName).notifier) - .updateCompletedTestExecutionsCount( - artefactId, - newCompletedTestExecutionsCount, - ); } Future rerunTestExecutions(Set testExecutionIds) async { diff --git a/frontend/lib/providers/review_test_execution.dart b/frontend/lib/providers/review_test_execution.dart new file mode 100644 index 00000000..26c0f14f --- /dev/null +++ b/frontend/lib/providers/review_test_execution.dart @@ -0,0 +1,50 @@ +import 'package:riverpod_annotation/riverpod_annotation.dart'; + +import '../models/family_name.dart'; +import '../models/test_execution.dart'; +import 'artefact_builds.dart'; +import 'family_artefacts.dart'; + +part 'review_test_execution.g.dart'; + +@riverpod +class ReviewTestExecution extends _$ReviewTestExecution { + @override + Future build() async { + return; + } + + Future reviewTestExecution( + int testExecutionId, + String reviewComment, + List reviewDecision, + FamilyName familyName, + int artefactId, + ) async { + await ref + .read(artefactBuildsProvider(artefactId).notifier) + .changeReviewDecision( + testExecutionId, + reviewComment, + reviewDecision, + ); + + final artefactBuilds = + ref.read(artefactBuildsProvider(artefactId)).requireValue; + + final newCompletedTestExecutionsCount = artefactBuilds + .map( + (build) => build.testExecutions + .where((testExecution) => testExecution.reviewDecision.isNotEmpty) + .length, + ) + .fold(0, (a, b) => a + b); + + await ref + .read(familyArtefactsProvider(familyName).notifier) + .updateCompletedTestExecutionsCount( + artefactId, + newCompletedTestExecutionsCount, + ); + } +} diff --git a/frontend/lib/ui/artefact_page/artefact_page_body.dart b/frontend/lib/ui/artefact_page/artefact_page_body.dart index 26a8c7f4..f7541588 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_body.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_body.dart @@ -34,7 +34,7 @@ class ArtefactPageBody extends ConsumerWidget { textBaseline: TextBaseline.alphabetic, children: [ Text( - 'Environments (${artefact.completedTestExecutionsCount}/${artefact.allTestExecutionsCount})', + 'Environments', style: Theme.of(context).textTheme.headlineSmall, ), const Spacer(), diff --git a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart index 15a04bf4..1357473b 100644 --- a/frontend/lib/ui/artefact_page/test_execution_pop_over.dart +++ b/frontend/lib/ui/artefact_page/test_execution_pop_over.dart @@ -4,7 +4,7 @@ import 'package:yaru_widgets/yaru_widgets.dart'; import '../../models/family_name.dart'; import '../../models/test_execution.dart'; -import '../../providers/artefact_builds.dart'; +import '../../providers/review_test_execution.dart'; import '../spacing.dart'; class TestExecutionPopOver extends ConsumerStatefulWidget { @@ -119,9 +119,7 @@ class TestExecutionPopOverState extends ConsumerState { const SizedBox(height: Spacing.level3), ElevatedButton( onPressed: () { - ref - .read(ArtefactBuildsProvider(widget.artefactId).notifier) - .changeReviewDecision( + ref.read(reviewTestExecutionProvider.notifier).reviewTestExecution( widget.testExecution.id, reviewCommentController.text, reviewDecisions, From 263a118e7dd300fb6ad39d29302aa009c442f82c Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Wed, 24 Jul 2024 17:01:41 +0200 Subject: [PATCH 14/15] Fix automated tests --- backend/tests/controllers/artefacts/test_artefacts.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 3ee1ad76..62434655 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -112,7 +112,7 @@ def test_get_artefact(test_client: TestClient, generator: DataGenerator): def test_get_artefact_test_execution_counts_only_latest_build( test_client: TestClient, generator: DataGenerator ): - a = generator.gen_artefact() + a = generator.gen_artefact("beta") ab = generator.gen_artefact_build(artefact=a, revision=1) e = generator.gen_environment() # Test Execution for the first artefact build @@ -138,7 +138,7 @@ def test_get_artefact_test_execution_counts( generator: DataGenerator, db_session: Session, ): - a = generator.gen_artefact() + a = generator.gen_artefact("beta") ab = generator.gen_artefact_build(a) e = generator.gen_environment() te = generator.gen_test_execution(ab, e) From 914ec36c94f7f1f33de45c60ae4be567801e0ad3 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski Date: Wed, 24 Jul 2024 17:10:29 +0200 Subject: [PATCH 15/15] Fix mypy errors --- backend/test_observer/data_access/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index 821322b6..62b150e3 100644 --- a/backend/test_observer/data_access/models.py +++ b/backend/test_observer/data_access/models.py @@ -215,7 +215,8 @@ def _get_latest_builds(self) -> list["ArtefactBuild"]: grouped_builds[build.architecture].append(build) return [ - max(builds, key=lambda b: b.revision) for builds in grouped_builds.values() + max(builds, key=lambda b: b.revision if b.revision else 0) + for builds in grouped_builds.values() ] @property