-
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
Copy results when adding to Test Queue #978
Copy results when adding to Test Queue #978
Conversation
server/graphql-schema.js
Outdated
@@ -1016,6 +1016,7 @@ const graphqlSchema = gql` | |||
testPlanVersionId: ID! | |||
atId: ID! | |||
browserId: ID! | |||
testPlanVersionDataToIncludeId: ID |
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.
The part that confuses me is that it reads like we're copying data from a TestPlanVersion to a TestPlanReport, which superficially doesn't make sense, even if there's an algorithm happening under the hood. I find myself wondering if it would be possible to call this something like copyResultsFromTestPlanReportId
. That seems more direct and intuitive to me. Do you think that would be workable?
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.
Fine with me! Done in bffd224.
@@ -142,31 +183,72 @@ function AddTestToQueueWithConfirmation({ | |||
}; | |||
|
|||
const renderErrorDialog = () => { |
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.
Seems like we need to rename the dialog now since the new message doesn't seem like an error anymore!
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.
Good point, done in bffd224. Let me know if you'd like the workshop the name further.
'completely new report?'; | ||
actions = [ | ||
{ | ||
label: 'Create new report', |
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.
How about "Create empty report"?
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.
Sounds good, done in bffd224
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'm a big fan of this change, it makes copying results way easier and in my testing it behaved flawlessly. I appreciated that we were able to reuse so much existing functionality. I was able to successfully copy results from a previous final report in a different phase as well as copy results from reports still in the test queue, which is awesome. The system behaves identically to now when adding reports where no previous data exists. Great work!
Problem
Currently, the only way reports are copied from an older test plan version's report(s) into a newer test plan version's report(s) is by the implicit action of just using the
Advance to <Phase>
button on the DataManagement page. In the instance where an update has been missed (because of the current state of the data) or if a copied report was removed or otherwise, it would mean that the ability to now copy those older results which matched for the same Test Plan + AT + Browser is functionally lost.Solution
This PR allows a test admin to choose whether to copy those results or create a new report when adding to the Test Queue if older results are found for the valid combination, using a dialog.
This addresses the following from #935:
Dependent on #976