From a339e5b58714309bca3071fb09eb5c449c7c5782 Mon Sep 17 00:00:00 2001 From: Andrej Velichkovski <33193463+andrejvelichkovski@users.noreply.github.com> Date: Fri, 26 Jul 2024 09:23:12 +0200 Subject: [PATCH] RTW-313: Implement percentage completed UI (#189) * Initial implementation of UI for showing percentage completed * Fix error when loading artefact builds * Update code to show exact counts * Fix import issues * Fix mypy errors * Fix issue with tests * Add doc-string to new helper methods * Implement new logic for fetching * Fix ruff issue * Remove unnecessary changes on frontend * Small improvements * Fix user avatar * Implement PR suggestions * Fix automated tests * Fix mypy errors --- .../controllers/artefacts/artefacts.py | 15 ++++- .../controllers/artefacts/models.py | 4 ++ backend/test_observer/data_access/models.py | 23 +++++++ .../test_observer/data_access/repository.py | 8 ++- .../controllers/artefacts/test_artefacts.py | 59 ++++++++++++++++++ frontend/benchmarks/common.dart | 2 + frontend/lib/models/artefact.dart | 4 ++ frontend/lib/providers/family_artefacts.dart | 14 +++++ .../lib/providers/review_test_execution.dart | 50 ++++++++++++++++ .../artefact_page/artefact_page_header.dart | 8 ++- .../test_execution_pop_over.dart | 11 ++-- .../artefact_page/test_execution_review.dart | 2 + frontend/lib/ui/dashboard/artefact_card.dart | 8 ++- frontend/lib/ui/user_avatar.dart | 60 +++++++++++++++---- frontend/test/dummy_data.dart | 2 + 15 files changed, 250 insertions(+), 20 deletions(-) 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 eb8f0348..0f351ee6 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -44,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) @@ -62,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: @@ -70,6 +78,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, ) @@ -77,7 +86,9 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db @router.get("/{artefact_id}", response_model=ArtefactDTO) -def get_artefact(artefact: Artefact = Depends(_get_artefact_from_db)): +def get_artefact( + artefact: Artefact = Depends(_get_artefact_from_db), +): return artefact diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 8f22450e..fd7ab995 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 @@ -51,6 +53,8 @@ class ArtefactDTO(BaseModel): assignee: UserDTO | None due_date: date | None bug_link: str + 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..62b150e3 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,28 @@ 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 if b.revision else 0) + 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 308f9ea9..62434655 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,62 @@ 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_only_latest_build( + test_client: TestClient, generator: DataGenerator +): + 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 + 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, + db_session: Session, +): + a = generator.gen_artefact("beta") + 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 +325,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, } 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/models/artefact.dart b/frontend/lib/models/artefact.dart index 6e029212..91e1bdc4 100644 --- a/frontend/lib/models/artefact.dart +++ b/frontend/lib/models/artefact.dart @@ -22,6 +22,10 @@ class Artefact with _$Artefact { required String repo, required ArtefactStatus status, required StageName stage, + @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/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/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_header.dart b/frontend/lib/ui/artefact_page/artefact_page_header.dart index a36bf08b..c9789d6b 100644 --- a/frontend/lib/ui/artefact_page/artefact_page_header.dart +++ b/frontend/lib/ui/artefact_page/artefact_page_header.dart @@ -15,13 +15,19 @@ class ArtefactPageHeader extends StatelessWidget { Widget build(BuildContext context) { final assignee = artefact.assignee; final dueDate = artefact.dueDateString; + 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, + allTestExecutionsCount: artefact.allTestExecutionsCount, + completedTestExecutionsCount: artefact.completedTestExecutionsCount, + ), const SizedBox(width: Spacing.level4), if (dueDate != null) Text( 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..1357473b 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,9 @@ 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 '../../providers/review_test_execution.dart'; import '../spacing.dart'; class TestExecutionPopOver extends ConsumerStatefulWidget { @@ -11,10 +12,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(); @@ -116,12 +119,12 @@ 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, + 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, diff --git a/frontend/lib/ui/dashboard/artefact_card.dart b/frontend/lib/ui/dashboard/artefact_card.dart index 7827c5e5..e722d491 100644 --- a/frontend/lib/ui/dashboard/artefact_card.dart +++ b/frontend/lib/ui/dashboard/artefact_card.dart @@ -63,7 +63,13 @@ class ArtefactCard extends ConsumerWidget { fontColor: YaruColors.red, ), const Spacer(), - if (assignee != null) UserAvatar(user: assignee), + if (assignee != null) + UserAvatar( + user: assignee, + allTestExecutionsCount: artefact.allTestExecutionsCount, + completedTestExecutionsCount: + artefact.completedTestExecutionsCount, + ), ], ), ], diff --git a/frontend/lib/ui/user_avatar.dart b/frontend/lib/ui/user_avatar.dart index 1c4b4fac..5eb52f75 100644 --- a/frontend/lib/ui/user_avatar.dart +++ b/frontend/lib/ui/user_avatar.dart @@ -15,23 +15,61 @@ 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.allTestExecutionsCount, + required this.completedTestExecutionsCount, + }); final User user; + final int allTestExecutionsCount; + final int completedTestExecutionsCount; + + double get ratioCompleted { + if (allTestExecutionsCount == 0) { + return 0.0; + } + return completedTestExecutionsCount / allTestExecutionsCount; + } @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: $completedTestExecutionsCount / $allTestExecutionsCount (${(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, + height: 43.0, + child: CircularProgressIndicator( + color: getDarkerColor(userIdToColor(user.id)), + backgroundColor: userIdToColor(user.id), + value: ratioCompleted, + semanticsLabel: 'Circular progress indicator', + ), + ), + ], ), ); } diff --git a/frontend/test/dummy_data.dart b/frontend/test/dummy_data.dart index 18737701..d05a2e56 100644 --- a/frontend/test/dummy_data.dart +++ b/frontend/test/dummy_data.dart @@ -24,6 +24,8 @@ const dummyArtefact = Artefact( stage: StageName.beta, assignee: dummyUser, bugLink: '', + allTestExecutionsCount: 1, + completedTestExecutionsCount: 0, ); const dummyEnvironment = Environment(