-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Added a link to open new issue with prefilled details #3683
Added a link to open new issue with prefilled details #3683
Conversation
@@ -161,6 +162,11 @@ <h3>{% trans "Error" %}</h3> | |||
data-bind="text: error"> | |||
{{ build.error }} | |||
</p> | |||
<p> | |||
{% blocktrans with build_id=build.pk proj_name=build.project.name build_path=request.get_full_path user_name=request.user %} | |||
Report any build issues <a href="https://github.com/rtfd/readthedocs.org/issues/new?title=Build%20error%20with%20build%20id%20%23{{ build_id }}&body=%23%23%20Details%0A%0A*%20Project%20Url%3A%20https://readthedocs.org/projects/{{ proj_name }}/%0A*%20Build%20URL%20(if%20applicable)%3A%20https://readthedocs.org{{ build_path }}%0A*%20Read%20the%20Docs%20username%20(if%20applicable)%3A%20{{ user_name }}%0A%0A%23%23%20Expected%20Result%0A%0A*%20A%20description%20of%20what%20you%20wanted%20to%20happen*%0A%0A%23%23%20Actual%20Result%0A%0A*A%20description%20of%20what%20actually%20happened*%0A">here</a>. |
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.
Can you split this line for better readability (with each param per line o something like 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.
@stsewd I have made the changes!
b2bd677
to
100e2df
Compare
title=Build%20error%20with%20build%20id%20%23{{ build_id }} | ||
&body=%23%23%20Details%0A%0A | ||
*%20Project%20Url%3A%20https://readthedocs.org/projects/{{ proj_name }}/%0A | ||
*%20Build%20URL%20(if%20applicable)%3A%20https://readthedocs.org{{ build_path }}%0A |
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.
build_path
already includes the /
, right?
Project Url
, URL
uppercase (same as the template)
Also there is some way of django to do the encoding here? Something like https://github.com/rtfd/readthedocs.org/pull/3457/files#diff-02c668683a9a481865c186f12f7452e0R74
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.
Yup build_path
includes /
.
@stsewd I have encoded the URL. |
da61c8e
to
db0a618
Compare
I was thinking something inside the template, but let's wait to see what others think. |
db0a618
to
fcaf2d5
Compare
@stsewd I will try finding a way to encode it inside the template! |
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! I think this will need some refactoring and clean up before it can be merged, but I think the approach is mostly correct.
actual_res = "## Actual Result\n\n*A description of what actually happened*" | ||
body = body + quote(proj + build + uname + expec_res + actual_res) | ||
issue_url = issue + title + body | ||
context['issue_url'] = issue_url |
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.
This is tough to follow due to excessive variable usage. Perhaps this could be simplified into a single multi line string with interpolation through .format()
and a dictionary as input. This should feed into urlparse to actually create a URL
readthedocs/builds/views.py
Outdated
@@ -82,6 +83,17 @@ class BuildDetail(BuildBase, DetailView): | |||
def get_context_data(self, **kwargs): | |||
context = super(BuildDetail, self).get_context_data(**kwargs) | |||
context['project'] = self.project | |||
issue = "https://github.com/rtfd/readthedocs.org/issues/new?title=" |
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.
urlparse
is a better way to build a URL. There are a number of problems with building by hand through string concatenation.
@@ -161,6 +162,11 @@ <h3>{% trans "Error" %}</h3> | |||
data-bind="text: error"> | |||
{{ build.error }} | |||
</p> | |||
<p> | |||
{% blocktrans with url=issue_url %} | |||
Report any build issues <a href="{{ url }}">here</a>. |
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.
This should be surrounded in a template block so that we can avoid this block on our commercial hosting site. We don't want github issues from build errors there.
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.
@agjohnson I didn't get this one. Can you just make this a bit more clear? Your reason is perfectly fine, I agree with that too. But I didn't get how to do this one. Sorry I may be not familiar with this.
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.
Sorry, I should have clarified more here. You'll want to use a named block, something like:
{% block github_issue_link %}
{% blocktrans %}
...
{% endblocktrans %}
{% endblock %}
That way, we can override this on our commercial hosting and avoid a link to GitHub.
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.
Okay Thanks !
741eb44
to
1d09ed1
Compare
@agjohnson I have made the requested changes. Can you have a look once ? |
c99b41d
to
2096dc9
Compare
@agjohnson Any updates on this ? |
readthedocs/builds/views.py
Outdated
"?title={title}{build_id}" | ||
"&body={body}") | ||
|
||
body = ("# Details:\n\n" |
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.
Is better if you use the multistring syntax here ("""
), so it's more clear and you will not need to add \n
.
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.
Removing the \n
creates trouble when quote
is used to render url. Newlines and spaces are not converted properly. So, I used the \n
.
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 you're having trouble with spaces, consider https://docs.python.org/2/library/textwrap.html#textwrap.dedent
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 this @agjohnson, I have updated the string syntax!
readthedocs/builds/views.py
Outdated
'uname': self.request.user} | ||
|
||
scheme_dict = {'title': quote("Build error with build id #"), | ||
'build_id': self.request.path[-2:-1], |
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.
There is a better way to get the build_id
? The id isn't always 2 characters length.
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.
Fixed this!
readthedocs/builds/views.py
Outdated
"## Actual Result\n\n" | ||
"*A description of what actually happened*") | ||
|
||
body_dict = {'projname': self.project, |
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 thinks is more clear if you use this on the format method directly, since isn't used on other parts of the code.
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.
Done!
Marking partially blocked on #3399 |
9d51b39
to
e37a25f
Compare
I don't want to block this PR on this issue. I think it's related, but we are not putting effort on that just yet. We should merge this PR in the meanwhile and enjoy this benefit by helping users to provide all the information needed for proper debugging. This is how it looks like when a generic error happens. |
d8ab392
to
2f337dd
Compare
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.
This PR is ready to merge once tests pass.
This is blocked by #4543 (it uses urlparse) |
I don't want to block PRs on dropping Py2 support. Once merged, #4543 will automatically upgrade the code that's needed to upgrade using the script. |
Merging master to get tests passing, then we should merge. 👍 |
This PR fixes #3539. It includes a link to open a new issue.