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

Added a link to open new issue with prefilled details #3683

Merged
merged 10 commits into from
Jan 22, 2019

Conversation

bansalnitish
Copy link
Contributor

This PR fixes #3539. It includes a link to open a new issue.

@@ -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>.
Copy link
Member

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)?

Copy link
Contributor Author

@bansalnitish bansalnitish Feb 26, 2018

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!

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
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup build_path includes /.

@bansalnitish
Copy link
Contributor Author

@stsewd I have encoded the URL.

@stsewd
Copy link
Member

stsewd commented Feb 26, 2018

I was thinking something inside the template, but let's wait to see what others think.

@bansalnitish
Copy link
Contributor Author

@stsewd I will try finding a way to encode it inside the template!

Copy link
Contributor

@agjohnson agjohnson left a 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
Copy link
Contributor

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

@@ -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="
Copy link
Contributor

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>.
Copy link
Contributor

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.

Copy link
Contributor Author

@bansalnitish bansalnitish Feb 28, 2018

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.

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay Thanks !

@bansalnitish bansalnitish force-pushed the preFilledIssue branch 13 times, most recently from 741eb44 to 1d09ed1 Compare February 28, 2018 21:09
@bansalnitish
Copy link
Contributor Author

@agjohnson I have made the requested changes. Can you have a look once ?

@bansalnitish
Copy link
Contributor Author

@agjohnson Any updates on this ?

"?title={title}{build_id}"
"&body={body}")

body = ("# Details:\n\n"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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!

'uname': self.request.user}

scheme_dict = {'title': quote("Build error with build id #"),
'build_id': self.request.path[-2:-1],
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this!

"## Actual Result\n\n"
"*A description of what actually happened*")

body_dict = {'projname': self.project,
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@agjohnson agjohnson added PR: work in progress Pull request is not ready for full review Status: blocked Issue is blocked on another issue and removed PR: ready for review labels Mar 16, 2018
@agjohnson
Copy link
Contributor

Marking partially blocked on #3399

@humitos
Copy link
Member

humitos commented Jan 9, 2019

Marking partially blocked on #3399

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.

captura de pantalla_2019-01-09_17-21-34

@humitos humitos removed PR: work in progress Pull request is not ready for full review Status: blocked Issue is blocked on another issue labels Jan 9, 2019
@humitos humitos requested a review from a team January 9, 2019 16:30
@readthedocs readthedocs deleted a comment from todo bot Jan 19, 2019
@readthedocs readthedocs deleted a comment from todo bot Jan 19, 2019
Copy link
Member

@humitos humitos left a 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.

@stsewd
Copy link
Member

stsewd commented Jan 21, 2019

This is blocked by #4543 (it uses urlparse)

@humitos
Copy link
Member

humitos commented Jan 21, 2019

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.

@humitos humitos mentioned this pull request Jan 21, 2019
12 tasks
@ericholscher
Copy link
Member

Merging master to get tests passing, then we should merge. 👍

@ericholscher ericholscher merged commit 3b7ff83 into readthedocs:master Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link to open an issue from a failed Build with pre-filled details
6 participants