-
Notifications
You must be signed in to change notification settings - Fork 3
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-321: Deprecate two enum choices #188
Conversation
case approvedFaultyHardware: | ||
case approvedUnstablePhysicalInfra: | ||
return true; |
There was a problem hiding this comment.
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:
case approvedFaultyHardware: | |
case approvedUnstablePhysicalInfra: | |
return true; | |
case approvedFaultyHardware: | |
return true; | |
case approvedUnstablePhysicalInfra: | |
return true; |
There was a problem hiding this comment.
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
.
((testExecutionReviewDecision == TestExecutionReviewDecision.rejected && | ||
_canReject) || | ||
(testExecutionReviewDecision != | ||
TestExecutionReviewDecision.rejected && | ||
_canApprove))) { |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
Co-authored-by: Omar Abou Selo <omar.selo@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
This change deprecates the two following options from the Test Execution Review Decision:
The deprecation is only done on the frontend. To make sure everything is backwards compatible, we will still show the cases where these are selected, but they will be disabled and their selection wont be modifiable.
Resolved issues
RTW-321
Documentation
No documentation changes involved.
Web service API changes
No API changes involved.
Tests
The testing was performed manually. Firstly, we observe the case where one of the options was already selected:
We then observe that the choices are not rendered for new test executions