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

Add Minimum / Exact AT Version to Test Plan Reports #997

Closed
wants to merge 6 commits into from
Closed

Add Minimum / Exact AT Version to Test Plan Reports #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e563863
Add minimum or exact at version to reports
alflennik Apr 3, 2024
f233b0f
Quick tweak
alflennik Apr 3, 2024
5d7ef54
Revert home copy change
alflennik Apr 3, 2024
408b3f1
Remove unused field from createTestPlanReport
alflennik Apr 3, 2024
1d140c0
Fix undefined var
alflennik Apr 3, 2024
9695e4f
Prevent API from creating duplicate reports
alflennik Apr 3, 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
38 changes: 21 additions & 17 deletions client/components/AddTestToQueueWithConfirmation/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,24 +149,29 @@ function AddTestToQueueWithConfirmation({
content={
'The report could not be created because an existing ' +
'report was found on the reports page with the same AT, ' +
'browser and test plan version. Would you like to return ' +
'the existing report back to the test queue?'
'browser and test plan version. Work is currently ' +
'underway to remove this limitation.'
Comment on lines +152 to +153
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder if we don't actually want to include this change in main until the rest of #791 and #792 is also ready. So we can instead treat this as a current dev "base"? Do you have any thoughts on that?

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 do think it might become necessary at some point to have a branch for trend report changes. However personally I feel like this code is still mergeable to main, and we haven't arrived at the "ok now we need a separate branch" point.

Other than removing the "proceed" button, I don't think there's much impact on the user experience. In the event the admins do want to move a report from the reports page back to the test queue, we can accommodate that through the API.

That's my perspective but I'm happy to defer to whatever you think is better.

Do you think removing the "work is underway" sentence would mitigate your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I'm okay with not yet considering this the "breaking point", so it could go to main but not just yet for 2 reasons:

  1. This particular feature has been used in recent times to make use of the current results copy featureset, in that it was unexpectedly only working if the results to be copied was in a report currently on the Test Queue, so this allowed a report to be pulled back. This won't be an issue once the enhancements identified in Improving the test results copy functionality #935 are also a part of main (PRs Support results being copied even when commands have been added or removed between versions #967, Copy results from advanced phase #976 and Copy results when adding to Test Queue #978) though.
  2. There is a planned release early next week and this new restriction hasn't yet been discussed. So this allows us the chance to notify them that this restriction is coming and so it also doesn't have to be forcibly excluded during the next push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@howard-e do you want to merge it into a branch with a nice name like "trends"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

}
closeLabel="Cancel"
staticBackdrop={true}
actions={[
{
label: 'Proceed',
onClick: async () => {
setErrorMessage(false);
if (hasAutomationSupport) {
setShowConfirmation(true);
} else {
await addTestToQueue();
}
}
}
]}
// The proceed button is disabled because it will now create
// duplicate testPlanReports, and support has not yet been
// implemented across the full frontend to account for
// duplicates.
Comment on lines +157 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to defend against this on the backend as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I added temporary code to enforce that.


// actions={[
// {
// label: 'Proceed',
// onClick: async () => {
// setErrorMessage(false);
// if (hasAutomationSupport) {
// setShowConfirmation(true);
// } else {
// await addTestToQueue();
// }
// }
// }
// ]}
useOnHide
handleClose={async () => {
setErrorMessage(false);
Expand All @@ -186,8 +191,7 @@ function AddTestToQueueWithConfirmation({
}
});
const testPlanReport =
res?.data?.findOrCreateTestPlanReport?.populatedData
?.testPlanReport ?? null;
res?.data?.createTestPlanReport?.testPlanReport ?? null;
tpr = testPlanReport;
}, 'Adding Test Plan to Test Queue');
setShowConfirmation(true);
Expand Down
19 changes: 7 additions & 12 deletions client/components/TestQueue/queries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,29 +223,24 @@ export const ADD_TEST_QUEUE_MUTATION = gql`
$atId: ID!
$browserId: ID!
) {
findOrCreateTestPlanReport(
createTestPlanReport(
input: {
testPlanVersionId: $testPlanVersionId
atId: $atId
browserId: $browserId
}
) {
populatedData {
testPlanReport {
testPlanReport {
id
at {
id
at {
id
}
browser {
id
}
}
testPlanVersion {
browser {
id
}
}
created {
locationOfData
testPlanVersion {
id
}
}
}
Expand Down
45 changes: 10 additions & 35 deletions server/graphql-schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,8 @@ const graphqlSchema = gql`
testPlanVersionId: ID!
atId: ID!
browserId: ID!
exactAtVersionId: ID
minimumAtVersionId: ID
}

"""
Expand Down Expand Up @@ -1230,15 +1232,6 @@ const graphqlSchema = gql`
"""
unmarkAsFinal: PopulatedData!
"""
Update the report to a specific TestPlanVersion id.
"""
updateTestPlanReportTestPlanVersion(
"""
The TestPlanReport to update.
"""
input: TestPlanReportInput!
): PopulatedData!
"""
Move the vendor review status from READY to IN PROGRESS
or IN PROGRESS to APPROVED
"""
Expand Down Expand Up @@ -1331,25 +1324,6 @@ const graphqlSchema = gql`
retryCanceledCollections: CollectionJob!
}

"""
Generic response to findOrCreate mutations, which allow you to dictate an
expectation of what you want to exist, and it will be made so. It allows you
to check whether new database records were created.
"""
type FindOrCreateResult {
"""
The data that was found or created, as well as any implicit
associations. For example, if you find or create a TestPlanReport, this
will include the TestPlanReport as well as the TestPlanVersion and
TestPlan.
"""
populatedData: PopulatedData!
"""
There will be one array item per database record created.
"""
created: [PopulatedData]!
}

type Mutation {
"""
Get the available mutations for the given AT.
Expand All @@ -1364,17 +1338,18 @@ const graphqlSchema = gql`
"""
browser(id: ID!): BrowserOperations!
"""
Adds a report with the given TestPlanVersion, AT and Browser, and a
state of "DRAFT", resulting in the report appearing in the Test Queue.
In the case an identical report already exists, it will be returned
without changes and without affecting existing results.
Adds an empty report to the test queue, a container for related test
results. Each report must be scoped to a specific TestPlanVersion, AT
and Browser. Optionally, either a minimum or exact AT version
requirement can be included to constrain the versions testers are
allowed to use to run the tests.
"""
findOrCreateTestPlanReport(
createTestPlanReport(
"""
The TestPlanReport to find or create.
The TestPlanReport to create.
"""
input: TestPlanReportInput!
): FindOrCreateResult!
): PopulatedData!
"""
Get the available mutations for the given TestPlanReport.
"""
Expand Down
Loading
Loading