-
Notifications
You must be signed in to change notification settings - Fork 469
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
Adds error check on Experiment results page #1507
Conversation
Your preview environment pr-1507-bttf has been deployed. Preview environment endpoints are available at: |
…ngoose update - we can no longer pass in undefined, it has to be null or an empty string.
@@ -118,6 +118,7 @@ export async function updateReport( | |||
$set: { | |||
...updates, | |||
dateUpdated: new Date(), | |||
error: updates.error || "", |
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 don't think this is right place for this. If someone just edits the report name, this will now delete the error. I think this belongs in the ReportQueryRunner class instead. That's where we call updateReport
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.
Ah, yea - good call. Updated.
Features and Changes
On the experiment report page, we're now checking if a report has an error via
report.error
and if so, displaying a red warning icon. On hover, we display a tooltip that displays the error string.Additionally, on the report id page
[rid].tsx
we're also surfacing any error message on the report itself, rather than making the user open the modal to see if there is an error.Also included in this PR is an update to how we're handling the
updateReport
method within theReportModel
. Similar to this PR Mongoose doesn't acceptundefined
when calling$set
and spreading in the updates. As a result, if a report had an error, but the error was resolved, it wasn't actually getting reset. With this update, we're now successfully resetting an error message if it's been resolved.Testing
Screenshots