What is the purpose of this evolution?
I might be missing something, but it seems like this (and the DeleteField below) can be removed from the final commit.

Should the scope of the imports be narrowed to the specific objects that are being used?

This line is over 80 characters long.

I feel like this line should be kept.

I feel like this would work better around page 1136 in the this.open callback. If you move it, I believe you can do away with the changes in comments_dlg.html.

While I do like documentation, I'm not entirely sure if this comment adds clarity.

The extra 2 lines at the end seem out of place.

You can do something like "var gOpenAnIssue = {{user.get_profile.open_an_issuelower}};

Hey Wilson,

It looks close - but I think you need to untangle your project from a few other commits, because I think there are some things in this diff that don't belong.

Let me know in the meeting today if you need assistance in sorting this out, and we can go over it after the meeting.

-Mike

Correct - we can just blow the always_issue.py evolution away.

Agreed.

As discussed in Jim's bug, I think we want to use:

issueField.checked = gOpenAnIssue;

This doesn't look like part of your project...

This doesn't look like part of your project...

This doesn't look like part of your project.

This doesn't look like part of your project.

This doesn't look like part of your project.

This doesn't look like part of your project.

This doesn't look like part of your project.

This doesn't look like part of your project.

Why is this in here?

This looks good.

This doesn't look like part of your project.

Just a few tips when posting review requests:

* Always have a descriptive summary and description. I should be able to have a sense as to what this change is about from the dashboard, and have full clarity when reading the description.

* View your diffs before publishing. If changes look tangled, get help first before posting.

* If something changes with your code (say, you've untangled the commits), update the description.

Just deleted the file. I was playing around with evolutions and realized I named the variable poorly.

Good call

Just added the spacer?

Not sure what I can do about it.

I need to find a way to change the initial state of the checkbox, not the final state as it closes

Tried:
{% if gOpenAnIssue %} check="checked" {% endif %}

Trailing white space and extra lines should be removed.

This evolution can be axed completely.

We can get rid of these extra newlines.

There's a function that opens the dialog box - you could put your code in there, to set the state of the checkbox each time the dialog opens.

Good job, just a few small things worth mentioning.

-Yazan

Empty line should be removed.

Maybe elaborate more in the label. "Default to opening an issue" isn't very intuitive.

Remove space after "checked".

ie: checked="checked"/>

I'm happy with this, and it seems to work.  Great job, Wilson!

-Mike

Wilson:

Hey - I think this patch has bitrotted - I can't apply and test it properly.

I went through the code again for one final pass through, and I may have found a problem.  Can you comment on the issue I've brought up?

Thanks,

-Mike

Hold on a second...

I think I missed this my first pass through.  What happens if we try editing a pre-existing comment that has an issue opened?

If our user default is to not open an issue, will the comment's issue checkbot be overridden?

If so, that's incorrect.  We should persist the original comments issue opened state.

Ship It!

Just one minor issue, Wilson.  With that fixed, I'll be ready to give my ship-it.

I think we want these formatted like they were before, and your evolution, since it was last, should be at the end, like:

'is_private',
'timezone',
'open_an_issue'

I'm good with this.  Thanks Wilson!