Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTW-313: Implement percentage completed UI #189

Merged
merged 17 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions backend/test_observer/controllers/artefacts/artefacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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:
Expand All @@ -70,14 +78,17 @@ 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,
)

return artefacts


@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


Expand Down
4 changes: 4 additions & 0 deletions backend/test_observer/controllers/artefacts/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@


class UserDTO(BaseModel):
model_config = ConfigDict(from_attributes=True)

id: int
launchpad_handle: str
launchpad_email: str
Expand All @@ -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):
Expand Down
23 changes: 23 additions & 0 deletions backend/test_observer/data_access/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
# Nadzeya Hutsko <[email protected]>
# Omar Selo <[email protected]>

from collections import defaultdict
from datetime import date, datetime, timedelta
from typing import TypeVar

Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omar-selo can you please help me verify that it is fine to assign revision to 0 if it is None. Mypy was complaining because the key here can be null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine

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)"""
Expand Down
8 changes: 7 additions & 1 deletion backend/test_observer/data_access/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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]:
"""
Expand Down Expand Up @@ -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)

Expand Down
59 changes: 59 additions & 0 deletions backend/tests/controllers/artefacts/test_artefacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
# Nadzeya Hutsko <[email protected]>
from datetime import date, timedelta

from sqlalchemy.orm import Session

from fastapi.testclient import TestClient

from test_observer.data_access.models import TestExecution
Expand Down Expand Up @@ -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,
}
]

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
}


Expand Down
2 changes: 2 additions & 0 deletions frontend/benchmarks/common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ class ApiRepositoryMock extends Mock implements ApiRepository {
status: ArtefactStatus.undecided,
stage: StageName.beta,
bugLink: '',
allTestExecutionsCount: 1,
completedTestExecutionsCount: 0,
);

return {
Expand Down
4 changes: 4 additions & 0 deletions frontend/lib/models/artefact.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 14 additions & 0 deletions frontend/lib/providers/family_artefacts.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,18 @@ class FamilyArtefacts extends _$FamilyArtefacts {
final previousState = await future;
state = AsyncData({...previousState, artefact.id: artefact});
}

Future<void> 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),
});
}
}
}
50 changes: 50 additions & 0 deletions frontend/lib/providers/review_test_execution.dart
Original file line number Diff line number Diff line change
@@ -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<void> build() async {
return;
}

Future<void> reviewTestExecution(
int testExecutionId,
String reviewComment,
List<TestExecutionReviewDecision> 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,
);
}
}
8 changes: 7 additions & 1 deletion frontend/lib/ui/artefact_page/artefact_page_header.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
11 changes: 7 additions & 4 deletions frontend/lib/ui/artefact_page/test_execution_pop_over.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@ 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 {
const TestExecutionPopOver({
super.key,
required this.testExecution,
required this.artefactId,
required this.family,
});

final TestExecution testExecution;
final int artefactId;
final FamilyName family;

@override
TestExecutionPopOverState createState() => TestExecutionPopOverState();
Expand Down Expand Up @@ -116,12 +119,12 @@ class TestExecutionPopOverState extends ConsumerState<TestExecutionPopOver> {
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);
},
Expand Down
2 changes: 2 additions & 0 deletions frontend/lib/ui/artefact_page/test_execution_review.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -46,6 +47,7 @@ class TestExecutionReviewButton extends StatelessWidget {
bodyBuilder: (context) => TestExecutionPopOver(
testExecution: testExecution,
artefactId: artefactId,
family: family,
),
direction: PopoverDirection.bottom,
width: 500,
Expand Down
8 changes: 7 additions & 1 deletion frontend/lib/ui/dashboard/artefact_card.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
],
),
],
Expand Down
Loading
Loading