-
-
Think I'd prefer different text here, but I'm having trouble thinking of something. Checkboxes should use sentence casing though. The one here should be more like: "Blocks submission" or "Blocks code submission."
-
reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1) I'd prefer "blocksSubmission" everywhere in the change instead of "blockit". It's more descriptive that way.
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) Now that we have two places that influence the button (this and the textarea's keyup handler), we need to have a single function that manages the state of the function. It should take into account both the text and this checkbox, and both the keyup handler and blockCheck.click should call it.
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) Can't we test this.blockIt directly? Shouldn't need to parse it I think. We should make sure we never are able to set anything but 0 or 1. Also, space after "if"
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) Indentation level is off. Should only be indented 4 spaces. Make sure you're using spaces and not tabs everywhere. Also, class should be set using attr(), and each thing in the chain should be on its own line.
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) Whole thing can be shortened to: $("#id_blockit")[0].checked = (comment.blockIt == 1);
-
reviewboard/reviews/models.py (Diff revision 1) Will need an evolution file for this. See the existing ones in the evolution/ directories. The text in the first parameter should be human-readable. Same as on the comment box.
-
reviewboard/templates/reviews/comments_dlg.html (Diff revision 1) The <input /> tag should be on its own line, and there should be a space before /> The indentation of these lines should be one space only.
-
reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 1) Indented too far. What is "gr" ?
-
reviewboard/templates/reviews/review_draft_inline_form.html (Diff revision 1) Should be aligned with the <pre> above. Make sure you're using spaces and not tabs.
-
reviewboard/templates/reviews/review_draft_inline_form.html (Diff revision 1) I imagine instead of JavaScript, you could just test against the template variable using template tags and set the attribute right on the Ship It checkbox during creation.
-
-
reviewboard/webapi/json.py (Diff revision 1) Should just be: blockIt = (request.POST['blockit'] == "1")
Comments can be marked as "Blocking" for submission
Review Request #1285 — Created Nov. 30, 2009 and discarded
Information | |
---|---|
trimbo | |
Review Board | |
Reviewers | |
reviewboard | |
jparise |
Serious problems with reviewed code sometimes need a way to indicate that parts must be fixed before a checkin, this code change attempts to create a solution for that by allowing a reviewer to mark an inline comment as "submission blocking". A the review requester can still select "submitted" if comments have been marked as submission blocking but never fixed. We did not want to prevent that because it creates a potential workflow issue. This has bugs right now but I wanted to get feedback as soon as possible. Bugs I'm trying to verify/fix: * Existing comments, when clicked on, do not indicate they were marked as blocking in the pop up blue dialog box. * "Ship it!" is still available to a reviewer who has marked a comment as blocking. It should not be available to be chosen if that reviewer marked something as submission blocking.
Locally against alpha 1.1 tip.