Skip to content

[dashboard] Send error body with feedback #12992

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

Jump to bottom
Merged
merged 1 commit into from
Sep 15, 2022
Merged

[dashboard] Send error body with feedback #12992

merged 1 commit into from
Sep 15, 2022

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Sep 15, 2022 •
edited
Loading

Description

This makes sure an error message and/or body gets sent along with a feedback.

WITH ERROR WITHOUT
Screenshot 2022-09-15 at 13 24 16 Screenshot 2022-09-15 at 13 24 25

Related Issue(s)

Fixes #10402

How to test

  1. Trigger an error, for example by trying to run a private workspace that you don't have access to, e.g. my repo.
  2. Visit Segment and filter for feedback_submitted.
  3. See the properties error_body and error_message.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview
  • /werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe

@laushinka laushinka mentioned this pull request Sep 15, 2022
Closed
2 tasks
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-lau-error-feedback-10402.1 because the annotations in the pull request description changed
(with .werft/ from main)

@laushinka laushinka marked this pull request as ready for review September 15, 2022 11:25
@laushinka laushinka requested a review from a team September 15, 2022 11:25
@laushinka laushinka requested a review from jldec September 15, 2022 11:25
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 15, 2022
andrew-farries
andrew-farries reviewed Sep 15, 2022
View reviewed changes
components/dashboard/src/Analytics.tsx
Comment on lines +44 to +45
error_object?: StartWorkspaceError;
error_message?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason these props have to be snake-cased rather than camel-cased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really! Before it was error_rendered which I confirmed with @jakobhero. Let me see what the convention is in Segment.

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 see it's a mix 😅 Some are ui_experiments, button_type, while others are workspaceInstanceId or clientKind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asked here

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought this interface defined props for some component, but it doesn't 😊 It's fine how it is if Segment needs it that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

short answer: snake case is preferred downstream, but using camel case is perfectly fine as segment will automatically change the prop names to snake case before pushing downstream - more details here

andrew-farries
andrew-farries reviewed Sep 15, 2022
View reviewed changes
components/dashboard/src/feedback-form/FeedbackComponent.tsx Outdated
Comment on lines 41 to 42
error_object: props.error_object || undefined,
error_message: props.error_message,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not introduced in this PR, but this object can be explicitly typed as EventProperties .

@laushinka laushinka force-pushed the lau/error-feedback-10402 branch from dd67d45 to 1c9fc54 Compare September 15, 2022 12:37
@laushinka laushinka force-pushed the lau/error-feedback-10402 branch from 1c9fc54 to b8e1702 Compare September 15, 2022 12:58
@mads-hartmann
Copy link
Contributor

/hold the build is failing at the moment so adding hold to not block merge queue

@laushinka
Copy link
Contributor Author

/hold the build is failing at the moment so adding hold to not block merge queue

Thanks @mads-hartmann! Will fix it

@laushinka laushinka force-pushed the lau/error-feedback-10402 branch from b8e1702 to 044a163 Compare September 15, 2022 13:17
@laushinka
Copy link
Contributor Author

laushinka commented Sep 15, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-lau-error-feedback-10402.6
(with .werft/ from main)

@laushinka
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit f5c9181 into main Sep 15, 2022
@roboquat roboquat deleted the lau/error-feedback-10402 branch September 15, 2022 13:32
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobhero jakobhero jakobhero left review comments

@andrew-farries andrew-farries andrew-farries approved these changes

@jldec jldec Awaiting requested review from jldec

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send error message with feedback
5 participants
@laushinka @mads-hartmann @andrew-farries @jakobhero @roboquat