-
Notifications
You must be signed in to change notification settings - Fork 148
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
GH-318/fix create issue display error showing as JSON #333
GH-318/fix create issue display error showing as JSON #333
Conversation
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.
LGTM. Thanks for the quick contribution 👍
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! 🎉
Codecov Report
@@ Coverage Diff @@
## master #333 +/- ##
=======================================
Coverage 19.35% 19.35%
=======================================
Files 11 11
Lines 2770 2770
=======================================
Hits 536 536
Misses 2196 2196
Partials 38 38 Continue to review full report at Codecov.
|
Thanks @hanzei, @larkox, and @mickmister for reviewing this PR. This PR is a quick resolution for this issue, however, I believe that it will still need to be further examined and the changes from stringified error to parseable object error should happen in the function that receives the error, not in the method that renders it. Therefore, I believe that still needs to change for this issue to be complete, either in this PR, or if further consistency between error objects across the webapp and the server need to be examined then possibly in the scope of another issue. |
This fix seems fine for the scope of this ticket. Created a follow up ticket to address the broader issue #337 |
@@ -124,7 +124,7 @@ export default class CreateIssueModal extends PureComponent { | |||
if (error) { | |||
submitError = ( | |||
<p className='help-text error-text'> | |||
<span>{error}</span> | |||
<span>{JSON.parse(error).message}</span> |
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'm concerned that error
may sometimes be a string here. Maybe we should protect against that?
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.
If it is the case that error
is a string, then this will error out and cause WSOD. I suggest applying a fix in this component's handleCreate method instead of the render function, similar to the fix in the Attach Comment to Issue Modal:
Lines 61 to 66 in bc5a371
let errMessage = created.error.message; | |
if (created.error.response && | |
created.error.response.body && | |
created.error.response.body.message) { | |
errMessage = created.error.response.body.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.
@aidapira What are your thoughts on moving the fix out of the render
function and into handleCreate
?
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.
Hi @mickmister, that sounds like a great idea. Please give me a few days and I will commit the updates based on your comment.
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.
Tested and passed
- Reproduce this by attempting to create an issue in a repository where issue creation has been disabled
- Applied this PR and the json error is now replaced with friendlier
failed to create issue: Unknown status code 410
- Briefly regression tested the modal
LGTM!
Thanks @aidapira for this enhancement! Much appreciated.
This issue has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @hanzei |
@aidapira Do you have questions about the feedback above? #333 (comment) Let me know if you need any help. |
@hanzei Thank you so much for pointing my attention to @mickmister's comment. I will let you know after I give it a try! |
@aidapira Awesome, looking forward to your response 👍 |
@aidapira Thanks for updating the PR 👍 Heads up that there is a merge conflict to resolve. |
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.
Thanks @aidapira! Looks good, I just have two last requests
webapp/src/components/modals/attach_comment_to_issue/attach_comment_to_issue.jsx
Outdated
Show resolved
Hide resolved
webapp/src/components/modals/attach_comment_to_issue/attach_comment_to_issue.jsx
Outdated
Show resolved
Hide resolved
This PR has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @jfrerich |
Hi @aidapira! It seems that you are really close to completing this PR. Do you need any further assistance from one of our developers? There is also a merge conflict that will need to be addressed. |
@aidapira Are you planing to continue working on this PR? |
Hi @hanzei, thanks for mentioning me and giving me some time to complete this PR. I'm working on completing it now and would expect an update within the next week. |
…url and remove extra code on parse 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.
LGTM 👍 Thanks @aidapira!
@aidapira It looks like there are some conflicts with one of the files. Are you able to resolve the conflicts with master? |
Hi @aidapira, it looks like there is one remaining linting error here:
|
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.
Thanks for the contribution @aidapira!
Summary
The Create Issue error display showed as JSON and this ticket fixes it to display the message instead.
Ticket Link
#318