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

Nicer bug report emails #2143

Merged
merged 12 commits into from Apr 14, 2016

Conversation

Projects
None yet
3 participants
@erasche
Copy link
Member

commented Apr 13, 2016

The current bug report emails are pretty dull, which is really totally OK. My client recently tried to render them as HTML. I realised that I might actually like HTML bug report emails. This was not a good use of my time, probably. ;)

  • HTML support to the email function
  • fancier HTML bug reports
  • properly sanitized (all HTML characters are URL encoded with cgi.escape, this is enough, right?)

Images:

utvalg_703
utvalg_704
utvalg_705

erasche added some commits Apr 12, 2016

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@erasche There are 2 small flake8 issues to fix.

@martenson martenson removed the minor label Apr 13, 2016

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

minor is a special tag used to generate release notes. It should only be applied to pull requests made by committers that fix functionality modified during the same release cycle. Such fixes are unimportant for release notes.

erasche added some commits Apr 13, 2016

@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

Thanks @martenson. I swear, I'll never be able to use that tag correctly. Pushed a change to fix flake8 stuff.

to = listify( to )
msg = email_mime_text.MIMEText( body.encode( 'ascii', 'replace' ) )
if html is None:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 13, 2016

Member

if html: would also include the case in which html is the empty string.

Edit: need also to invert the commands in the if/else branches

This comment has been minimized.

Copy link
@erasche

erasche Apr 13, 2016

Author Member

was about to say, surely you mean if not html

if config.smtp_server is None:
log.error( "Mail is not configured for this Galaxy instance." )
log.info( msg )
return

if html is not None:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 13, 2016

Member

if html:

erasche added some commits Apr 13, 2016

@@ -1244,7 +1244,7 @@ def send_mail( frm, to, subject, body, config, html=None ):
log.info( msg )
return

if html is not None:
if not html:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 13, 2016

Member

if html:

@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

Whoever merges this should feel free to squash it since there are a dozen tiny, useless commits here.

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

ha, we have the :squash: button now!
(no squash icon though :/ )

<tbody>
<tr><td>Dataset</td><td>${dataset_id} (${dataset_id_encoded})</td></tr>
<tr><td>History</td><td><a href="${history_view_link}">${history_id} (${history_id_encoded})</a></td></tr>
<tr><td>Failed Job</td><td>${hid}: ${history_item_name}</td></tr>

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 13, 2016

Member

What about adding also (${hda_id_encoded}) after ${history_item_name} for consistency with the non-HTML report?

This comment has been minimized.

Copy link
@erasche

erasche Apr 13, 2016

Author Member

Because the TXT report wasn't consistent at all. Had I spotted that I would've added it.

Txt report:

reference to dataset id ${dataset_id} (${dataset_id_encoded}) from history id ${history_id} (${history_id_encoded}) ... history containing the related history item (${hda_id_encoded})

But yes, I can patch this.


self.report = string.Template( error_report_template ).safe_substitute( report_variables )
# Escape the message for use in the HTML report
report_variables['message'] = cgi.escape(report_variables['message'])

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 13, 2016

Member

I suspect that message is not the only variable which can be considered (at least partially) under the user control: history_item_name, job_command_line, job_stderr, job_stdout, job_traceback

@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

@martenson I've generated 4 * 3 (4-1 because travis runs quickly) extra builds due to back and forth on review process, none of which need to run. Can those be killed somehow so I'm not wasting so bloody much job space/electricity/etc.

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@erasche you can kill them in Jenkins while logged in

@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

@martenson cheers.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

Speaking of squashing, @nsoranzo could I persuade you to "squash" your review comments a bit more for me in the future (into fewer temporally distributed review sessions)? There has been significant back and forth on this one and that's been a source of frustration for me due to the large amount of context switching each time a new review comment is made (open terminal, patch code, test, flake8, commit, sign, push), especially given how tiny this PR is.

I recognise that repeatedly looking at code is useful and produces new, valuable insights, I don't mean to offend you with this. I just want to express that this has been an unpleasant PR review process for me and document why I'm feeling that, in hopes we as a group don't scare away potential contributors due to experiences getting code merged.

@nsoranzo

This comment has been minimized.

Copy link

commented on lib/galaxy/tools/errors.py in dd05f0a Apr 13, 2016

You are escaping message twice.

This comment has been minimized.

Copy link
Owner Author

replied Apr 13, 2016

Dangit, you're right. Thought I'd deleted that line.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

@erasche I am sorry that this was unpleasant for you, but I don't think that a span of around 2 hours for 5 comments is a long time. Please take into account that:

  • there is no support in the GitHub interface for the type of review submission you are asking for
  • as you probably know, I do code review examining thoroughly the code and comments and this takes a lot of time
  • I also context switch a lot between code review, emails, support on galaxyproject IRC channel and day job duties
  • there is absolutely no rush to fix review comments: instead of committing and pushing right after receiving the notification, just wait a bit for us to finish, check your code for other places a review comment may apply, review your changes (I spotted problems in 2 of your fix commits) and then commit.

Finally, a general comment. In the last few months, I've received 3 public complains about my code reviews from different committers: too nitpicking, too early, now too sparse in time. I appreciate that people don't like every detail to be scrutinized, but I have a scientific education and I am not willing to compromise on the code quality or treat people differently.
I think that we are not scaring contributors, but the few (probably 4-5) active reviewers. I suggest that we should actively encourage committers to do more code review (which is not just pressing +1) and appreciate more the work of the ones that do it.

@nsoranzo nsoranzo merged commit 87e2a6c into galaxyproject:dev Apr 14, 2016

4 checks passed

api test Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished.
Details
toolshed test Build finished.
Details
@erasche

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

@nsoranzo all fair! Thanks for the response / merge. :)

@erasche erasche deleted the erasche:nicer-emails branch Dec 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.