-
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
feat: Allows testers' results to be publicly viewable #1209
Conversation
…n a user is not signed in but viewing a user's run
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 is awesome!
I tested manually and tried to see if I could break it. I was unable to. It does feel a bit weird to get the form view when viewing an unsubmitted test. I get that it isn't actually editable but just feels strange. Maybe it would feel better if the icon on the left in the navigator didn't switch to the "in progress" icon. No strong feelings here though.
I left minor organizational comments inline.
disabled={!nonSelfAssignedRuns.length} | ||
> | ||
<FontAwesomeIcon icon={faFileImport} /> | ||
View Results for... |
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.
It looks like only the label is different so a possible simplification could be:
const dropdownLabel = isAdmin ? "Open run as..." : "View Results for...";
<Dropdown focusFirstItemOnShow>
<Dropdown.Toggle
variant="secondary"
disabled={!nonSelfAssignedRuns.length}
>
<FontAwesomeIcon icon={faFileImport} />
{dropdownLabel}
</Dropdown.Toggle>
<Dropdown.Menu role="menu">
{nonSelfAssignedRuns.map(testPlanRun => (
<Dropdown.Item
key={testPlanRun.id}
role="menuitem"
href={`/run/${testPlanRun.id}?user=${testPlanRun.tester.id}`}
>
{testPlanRun.tester.username}
</Dropdown.Item>
))}
</Dropdown.Menu>
</Dropdown>
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.
Another good catch! Done in 643c123
@@ -84,7 +88,10 @@ const UnexpectedBehaviorsFieldset = ({ | |||
name={`problem-${commandIndex}`} | |||
autoFocus={isSubmitted && unexpectedBehaviors.failChoice.focus} | |||
defaultChecked={unexpectedBehaviors.failChoice.checked} | |||
onClick={unexpectedBehaviors.failChoice.click} | |||
onClick={e => { | |||
if (readOnly) e.preventDefault(); |
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.
It might be nice to wrap this behavior in a handler function
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 e62a595
client/components/TestRun/index.jsx
Outdated
// we are ready enough to show the page and all the buttons when the above code is | ||
// pageReady and we have an anon view, bot test run, or a test result to display | ||
const completeRender = pageReady && (!isSignedIn || currentTest.testResult); | ||
// const enableInteractions = isSignedIn || !isViewingRun; |
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.
Left behind?
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 catch!
@stalgiag agreed with this, it does seem a little odd. Following this is a screenshot I've done to represent the case where there is "missing" results for that tester's test in the test navigator using a gray question mark with an updated gray border. Also in the screenshot, is a highlighted text to show some description around the current state of the test if viewing as a user that's not admin / signed out (although I'm more weary about adding new language). I was thinking it could be something like:
What do you think? |
That looks great! I think 'unopened' works well enough. |
…n admin / signed out user through the test navigator and heading message
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.
Looks great! Thanks for going above and beyond in addressing the feedback.
Address #1204.