Code review comment for lp:~mbp/launchpad/612171-diff-generation-spam

On 11-10-13 07:49 PM, Martin Pool wrote:
> Oh, perhaps I was unclear, I'm not suppressing mail for all user errors.
> That would be bad.

I'm glad you're not suppressing all user errors. The point of a user error is that the user should be notified. If it's not something the user should be notified about, then it shouldn't be a user error, and conversely, no user errors should be suppressed.

You are suppressing errors about pending writes, but you are also suppressing errors about missing source and target revisions. I believe that kind of error is something the user should be notified about, because either they made a mistake, or they need to push to the source or target.

The obvious solution is to raise a different exception when there are pending writes.

> I'm logging all user errors

That's great; I have a branch to do that, also: lp:~abentley/launchpad/log-user-errors

I haven't submitted mine yet, because it doesn't have tests. Unfortunately, it appears you haven't tested that, either. The bug is #850974: No indication when a job resulted in a user error

> Then for this particular error, diff generation failure, which we may agree
> is not really a user error, I'm suppressing the mail. Perhaps I should also
> formally reclassify it.

You should definitely formally reclassify it. I think you should do that by raising a different exception for pending writes, but if you want to reclassify all UpdatePreviewDiffNotReady exceptions, you can just delete UpdatePreviewDiffJob.user_error_types

It appears that you've deleted the original merge proposal, which makes it hard to continue the conversation. In the future, please consider resubmitting in these circumstances. Resubmitting will allow you to change the target branch.