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.

Sign up for GitHub

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-321: Deprecate two enum choices #188

Merged
merged 4 commits into from
Jul 16, 2024
Merged

RTW-321: Deprecate two enum choices #188

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f80357
Deprecate two enum choices
andrejvelichkovski Jul 8, 2024
5861942
Use switch default
andrejvelichkovski Jul 8, 2024
44f24bb
Update frontend/lib/ui/artefact_page/test_execution_pop_over.dart
andrejvelichkovski Jul 16, 2024
17dfbbc
Explain long if condition on whether to show each checkbox
andrejvelichkovski Jul 16, 2024
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
10 changes: 10 additions & 0 deletions frontend/lib/models/test_execution.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ enum TestExecutionReviewDecision {
}
}

bool get isDeprecated {
switch (this) {
case approvedFaultyHardware:
case approvedUnstablePhysicalInfra:
return true;
Comment on lines +132 to +134
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry if I don't know how dart works, but shouldn't it look like this:

Suggested change
case approvedFaultyHardware:
case approvedUnstablePhysicalInfra:
return true;
case approvedFaultyHardware:
return true;
case approvedUnstablePhysicalInfra:
return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the Dart docs the empty case falls through, so in this case the logic for approvedFaultyHardware continues through and executes the same case as approvedUnstablePhysicalInfra.

default:
return false;
}
}

String toJson() {
switch (this) {
case rejected:
Expand Down
28 changes: 19 additions & 9 deletions frontend/lib/ui/artefact_page/test_execution_pop_over.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ class TestExecutionPopOverState extends ConsumerState<TestExecutionPopOver> {
Function(bool?)? getOnChangedCheckboxListTileFunction(
TestExecutionReviewDecision testExecutionReviewDecision,
) {
if ((testExecutionReviewDecision == TestExecutionReviewDecision.rejected &&
(_canReject)) ||
(testExecutionReviewDecision != TestExecutionReviewDecision.rejected &&
_canApprove)) {
if (!testExecutionReviewDecision.isDeprecated &&
((testExecutionReviewDecision == TestExecutionReviewDecision.rejected &&
_canReject) ||
(testExecutionReviewDecision !=
TestExecutionReviewDecision.rejected &&
_canApprove))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you simplify or make this if statement easier to understand somehow? one suggestion is to just create a variable whose name explains ((testExecutionReviewDecision == TestExecutionReviewDecision.rejected && _canReject) || (testExecutionReviewDecision != TestExecutionReviewDecision.rejected && _canApprove))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the second part in the new variable as suggested, and added a comment that explains what is happening here.

return (bool? value) {
setState(() {
if (reviewDecisions.contains(testExecutionReviewDecision)) {
Expand All @@ -53,6 +55,11 @@ class TestExecutionPopOverState extends ConsumerState<TestExecutionPopOver> {
return null;
}

bool shouldDisplayDecision(TestExecutionReviewDecision decision) {
return !decision.isDeprecated ||
(decision.isDeprecated && reviewDecisions.contains(decision));
andrejvelichkovski marked this conversation as resolved.
Show resolved Hide resolved
}

@override
void initState() {
super.initState();
Expand All @@ -79,12 +86,15 @@ class TestExecutionPopOverState extends ConsumerState<TestExecutionPopOver> {
Column(
children: TestExecutionReviewDecision.values
.map(
(e) => YaruCheckboxListTile(
value: reviewDecisions.contains(e),
onChanged: getOnChangedCheckboxListTileFunction(e),
title: Text(e.name),
),
(e) => shouldDisplayDecision(e)
? YaruCheckboxListTile(
value: reviewDecisions.contains(e),
onChanged: getOnChangedCheckboxListTileFunction(e),
title: Text(e.name),
)
: null,
)
.whereType<YaruCheckboxListTile>()
.toList(),
),
const SizedBox(height: Spacing.level4),
Expand Down
2 changes: 1 addition & 1 deletion frontend/lib/ui/artefact_page/test_execution_review.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class TestExecutionReviewButton extends StatelessWidget {
),
direction: PopoverDirection.bottom,
width: 500,
height: 500,
height: 400,
omar-selo marked this conversation as resolved.
Show resolved Hide resolved
arrowHeight: 15,
arrowWidth: 30,
);
Expand Down
Loading