-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update status dialog for minimum exact AT versions #1065
Conversation
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.
@alflennik this looks great and certainly another big step towards supporting #791 and #792!
My main hang up is with the newly named TestPlanReportStatus
type. I think it may cause some confusion so would like for an alternate but if I'm alone in my thought on it, it does not have to be considered as blocking since it can be easily refactored afterwards if needed.
counts.missing !== 0 && | ||
counts.completed === 0 && | ||
counts.inProgress === 0 |
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.
nit: assigning these boolean results to values may help with readability and future maintenance if needed.
An existing or missing TestPlanReport that can be collected for a given | ||
TestPlanVersion. | ||
""" | ||
type TestPlanReportStatus { |
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.
Don't think I'm a huge fan of the name TestPlanReportStatus
here. The way "status" has been used for test plan reports in the past, and even how it is used to currently with other entities makes me feel like there's a chance that this may bring some confusion.
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.
For the name I was riffing off of the "TestPlanReportStatusDialog". I was considering several alternatives but nothing felt quite as semantic. Let me know if you think of anything better, I'm always game to improve it
Whether the TestPlanReport is actually required during the given | ||
TestPlanVersion phase. | ||
""" | ||
isRequired: Boolean! |
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.
Nice!
@@ -1,60 +0,0 @@ | |||
const requiredReports = { |
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've wondered how well this could scale. Removing it seems like a plus!
server/resolvers/TestPlanVersion/testPlanReportStatusesResolver.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,179 @@ | |||
const atResolver = require('../TestPlanReport/atResolver'); |
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.
This was a lot but the tests definitely make understanding this much easier!
…r.js Co-authored-by: Howard Edwards <[email protected]>
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.
As my major bit of feedback was on the name used here, TestPlanReportStatus
, I don't think it's anything this particular PR has to be held up on.
Definitely something that can be revised in the future if needed.
This includes work to support #791 and #792. Includes the following changes: * #1055 * #1001 * #1065 * #1052 * #1087 * #1098 * #1092 * #1131 * #1124 --------- Co-authored-by: Howard Edwards <[email protected]> Co-authored-by: Paul Clue <[email protected]> Co-authored-by: alflennik <[email protected]>
Updates the TestPlanStatusDialog to support minimum and exact AtVersions, which required extensive changes to the algorithm that populates the content of that dialog.
One difference worth highlighting is that I updated the sorting of the dialog to sort by AT before sorting by "required" - the dialog feels a lot more organized when all of the reports are gathered together for one AT, especially because recommended reports can have a huge number of rows in the dialog.
One limitation of this PR is the fact that the automatic switch from minimumAtVersion to exactAtVersion which occurs when a testPlanVersion goes from candidate to recommended has not yet been implemented, mostly because this PR is already bulging at the seams and also because the PR enabling it has not yet been merged into #1055. That seems like a good follow-on task to this one.