Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: More UI friendly errors#1994

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 23 commits intomainfromstevenmasley/ui_errors
Jun 3, 2022
Merged

chore: More UI friendly errors#1994

Emyrk merged 23 commits intomainfromstevenmasley/ui_errors
Jun 3, 2022

Conversation

Emyrk
Copy link
Member

@EmyrkEmyrk commentedJun 2, 2022
edited
Loading

If you see a type in this PR, feel free to just commit the spelling fix to the branch

I might have missed something. And errors can be improved from here.

Some detail about postgres failing was a hardcoded error to show what it looks like in the cli.
Screenshot from 2022-06-03 09-36-27

NOTE

All these errors were my best effort giving like 20 seconds per error. I imagine we can improve from here, and as we come across anything vague, we can do that. This is just the baseline work for the UI team to be unblocked.

I don't think I made anything more vague. So anything not specific is status quo, just with the new format.

Future work

  • Implement UI to read these
  • Add a linter to help enforce friendly errors

Mainly capitlization + messages prefix error
@EmyrkEmyrk marked this pull request as ready for reviewJune 3, 2022 14:37
Comment on lines 55 to 67
// Message is for general user-friendly error messages. This message will
// be shown at the top/bottom of a form, or in a toast on the UI.
Message string `json:"message"`
// Internal has the technical error information (err.Error()). These details
// might come from external packages and might not be user friendly.
// Do not populate this error field with any sensitive information or
// any errors that may be a security implication. These details are still
// available to more technical users.
Internal string `json:"internal"`
// Errors are form field-specific friendly error messages. They will be
// shown on a form field in the UI. These can also be used to add additional
// context if there is a set of errors in the primary 'Message'.
Errors []Error `json:"errors,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I think this will be difficult to understand at a glance how I'd display an error to the user.

Here's an alternative structure that just tweaks the names a bit and adds examples, but could help:

typeResponsestruct {// An actionable message that depicts actions the request took. Examples:// - "A user has been created."// - "Failed to create a user."Messagestring`json:"message"`// A debug message that provides further insight into why the action failed.// - "database: too many open connections"// - "stat: too many open files"Detailstring`json:"detail"`}

I don't know what to nameErrors. since it's form-specific I'd lean towardsValidations maybe?

I'm curious for your take@presleyp, you're a magician with words! 🪄

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kira-Pilot suggestedVerbose instead ofInternal and I like that;Detail works too.

I likeValidations better thanErrors - more specific! Right now I feel like it requires an explanatory comment on the frontend.

But I know@Emyrk doesn't want this PR to be open for long, maybe these tweaks can be a followup.

kylecarbs reacted with hooray emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I can change that now, that's easy with IDE tools

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Emyrkand others added5 commitsJune 3, 2022 10:08
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Copy link
Contributor

@presleyppresleyp left a comment

Choose a reason for hiding this comment

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

There are a few actual comments in there mixed in with the committable suggestions, the most important of which IMO is how to handle cases where the message is redundant with the field error. But I'm cool with merge after the committable suggestions are in, we can be iterative.

Copy link
Contributor

@greyscaledgreyscaled left a comment

Choose a reason for hiding this comment

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

LGTM with@presleyp 's suggestions

httpapi.Write(rw,http.StatusForbidden, httpapi.Response{
Message:err.Error(),
})
httpapi.Forbidden(rw)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: what allowed us to make this change? Does it mean that we no longer send those randomMessage errors on forbidden routes?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@vapurrmaid for security reasons all forbidden messages should be identical. If they were different, then the different errors allow the end user to gain information.


There is a line of "security" vs "usability" that we will need to decide on these endpoints. As it's unhelpful by design.

@misskniss
Copy link

@Emyrk I like this. And we should pull in the changes suggested by@presleyp here.

Copy link
Contributor

@f0sself0ssel left a comment

Choose a reason for hiding this comment

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

very much welcome this addition

@EmyrkEmyrkenabled auto-merge (squash)June 3, 2022 21:40
@EmyrkEmyrk merged commitc9a4642 intomainJun 3, 2022
@EmyrkEmyrk deleted the stevenmasley/ui_errors branchJune 3, 2022 21:48
kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
* chore: More UI friendly errorsMainly capitlization + messages prefix error
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@presleyppresleyppresleyp approved these changes

@johnstcnjohnstcnjohnstcn left review comments

@kylecarbskylecarbskylecarbs left review comments

@greyscaledgreyscaledgreyscaled left review comments

@khorne3khorne3khorne3 left review comments

@f0sself0sself0ssel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

8 participants
@Emyrk@misskniss@presleyp@johnstcn@kylecarbs@greyscaled@khorne3@f0ssel

[8]ページ先頭

©2009-2025 Movatter.jp