Skip to content
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

Exclude stack trace from being included in the new GitHub issues request #9523

Merged
merged 2 commits into from
Feb 27, 2019

Conversation

alfarok
Copy link
Contributor

@alfarok alfarok commented Feb 26, 2019

Purpose

DYN-1491

This PR removes the stack trace data that got appended to the GitHub new issues url. This was recently introduced in #9393 and has not yet been shipped (only lives in Master not 2.1). There are several issues with making an authenticated request in a browser window in which we are not prepared to handle. More details can be found in #9519 and earlier linked PRs.

It has always been the case that users who do not have a GitHub account cannot report issues. We still gain several improvements with #9393 but automatically copying the stack trace needs to be removed until the team has the bandwidth to implement a more robust solution.

As previously discussed a future solution could be to use a GitHub app. The app could handle/post all crash reports tagging the user by their GitHub id but would be optional allowing all users to report issues. Many in the AEC industry don't use or know much about GitHub.

image

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@QilongTang @mjkkirschner

FYIs

@radumg

@alfarok alfarok changed the title excluded stack trace from un-authenticated get request Exclude stack trace from being included in the new GitHub issues request Feb 26, 2019
@QilongTang
Copy link
Contributor

How do we determine the issue tile? I may have asked before but forgot how we define that currently

@mjkkirschner
Copy link
Member

@alfarok do tests need to be updated?

@alfarok
Copy link
Contributor Author

alfarok commented Feb 27, 2019

@QilongTang The title is set here which attempts to append the current Dynamo version.

@QilongTang
Copy link
Contributor

@alfarok Thanks. If there is no unit tests to be updated like @mjkkirschner asked, LGTM

@alfarok
Copy link
Contributor Author

alfarok commented Feb 27, 2019

@mjkkirschner @QilongTang should be good to go now. I didn't remove the stack trace parameter from the BuildMarkdownContent function so it can still be implemented in the future and just commented out 1 assertion that verified this content was present.

@alfarok
Copy link
Contributor Author

alfarok commented Feb 27, 2019

All tests passing on the self-serve, merging

@alfarok alfarok added the LGTM Looks good to me label Feb 27, 2019
@alfarok alfarok merged commit beafb90 into DynamoDS:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants