Warning modal for creating duplicate test plan runs #901
Conversation
| return ( | ||
| <BasicModal | ||
| show={errorMessage} | ||
| title={'Conflicting Report Found'} |
There was a problem hiding this comment.
No need to use interpolation for static values.
| title={'Conflicting Report Found'} | |
| title='Conflicting Report Found' |
| 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?' | ||
| } |
There was a problem hiding this comment.
The project doesn't enforce a maximum line length, but it appears to (mostly) split strings at the 100-column boundary. @howard-e @alflennik @stalgiag is there any consensus on this?
There was a problem hiding this comment.
It's certainly been mentioned a few times now, in GitHub threads and in sync discussions. I've created an issue to now track that discussion, #924.
There was a problem hiding this comment.
Hey we found out that the project formats code at column 80. I split the strings there.
| if (hasAutomationSupport) { | ||
| setShowConfirmation(true); | ||
| const conflictingReportExists = | ||
| existingTestPlanReports.some(report => { |
There was a problem hiding this comment.
existingTestPlanReports is potentially falsey, so we should use optional chaining here, too. It might also be good to perform this calculation outside the handler just like we do for hasAutomationSupport.
| 'browser and test plan version. Would you like to return ' + | ||
| 'the existing report back to the test queue?' | ||
| } | ||
| closeLabel={'Cancel'} |
There was a problem hiding this comment.
| closeLabel={'Cancel'} | |
| closeLabel="Cancel" |
|
Thanks, @Paul-Clue! Would you mind taking a look at the test failures? |
|
Hey, @jugglinmike, The tests were failing because I moved the |
|
That explains why the If we address the "undefined requirements" case only by relocating the calculation to the event handler, then there's a risk that the code still manages to run before the value is available (possibly due to some uncommon network delay, for instance). Also, implicitly relying on the delay represents a refactoring hazard for future contributors. (What I mean by this is that I think folks would be justified if they assumes that the new logic would be safe to relocate anywhere in the component. We know that assumption is false because we're in the thick of it, but if that future contributor acted on their assumption, they could unknowingly introduce a bug.) And to that latter concern, if we use optional chaining, then the new code will be safe to use anywhere in the component. That means we could place it up near the top like before, after all. I can think of a couple reasons why that's still desirable:
If you want to get fancy, you could also wrap this calculation in a call to |
|
Hey, @jugglinmike, I missed that I can add optional chaining to the |
see issue #870
This PR is creates a modal that issues a warning to a user that is trying to add a new test plan run to the test queue with the same AT / browser combination and is the same version of an already completed test plan run. The modal warns the user that by creating a duplicate test plan run, the finalized test plan with results will be removed from the results page.
This behavior (completed test results being removed form the results page) is no longer acceptable, according to the issue. This PR serves as a temporary solution for the main issue at hand.