Hey Jesus,

A few general comments:

1) For HTML and Django templates, we tend to only use 2 spaces for indenting per level
2) You're going to need to axe the trailing whitespace (see the big red areas in the diff)

I'm a little confused about the logic for the discard banner. Notice that it's contained within a conditional which *ensures* that review_request.status == "S".

Remember - we want all three banners (submitted, discard, and draft) rendered in the template (so no wrapping conditionals around those). Instead, we want to control the *visibility* of those banners by putting the conditionals within the style attribute of the DIVs, to set display:none; if the block should not be displayed.

Keep digging,

-Mike

Can you combine these two lines?

Please combine these too.

This change isn't correct--we already were hiding it using CSS. We don't want it to be present in the case where the user isn't the owner/can-edit for the review.

You removed the spaces inside the {%. Please don't 

Please put the spaces back before the else.

There's no reason for this change.

This still isn't correct. It's still including the draft banner in the page for users who should never be able to see it (and doing so with two style="display: none" attributes).

What happens if you pass this success function to gReviewRequest.close on line ~2619 in reviews.js, where we attach the click handler for #btn-review-request-discard and #discard-review-request-link?

I think you may have found a bug in datastore.js - at this line:

https://github.com/reviewboard/reviewboard/blob/master/reviewboard/static/rb/js/datastore.js#L660

if (!options.success) is evaluating to true, even if a function is passed.

We can fix that by using hasOwnProperty. So change that line to be:

if (!options.hasOwnProperty("success")) {
  // ...
}

That makes the banner go away for me.

This semicolon is breaking reviews.js - you need to remove it.

Hey Jesus,

This looks pretty good! Just one major point below, and a question.

-Mike

Why was this removed?

You need to put the other id "#discard-review-request-link" back into this query.

Hi Jesus,

I'd like to see a complete, detailed, step-by-step set of tests in Testing Done that describes every permutation of states you're working with. We need to know what testing and that testing needs to occur with every draft, since this is something very core that we can't get wrong.

This isn't needed. Keep it as it was. You only really care about hasOwnProperty when you want to be sure that this specific base-level instance has it and not a prototype backing it.

Space before "{"

Should use gDraftBanner.

It'd be nice to have a variable at the top like gDraftBanner for this.

This seems wrong. Note that there's two conditions for showing this form:

1) It has a pending draft.
2) The user viewing it has permissions to edit this review request.

#1 is dynamic. That state will change when there's an edit or when a review request is reopened from a discarded state.

#2 is not dynamic. This will never change while the page is open. So that original check should remain, and if the user doesn't have permission to edit the review request, this form should never be in the HTML.

The first conditional is checking against 'P', so I don't see how the newly added check against 'D' can ever be false.

I'd also like to see detailed testing showing things are working for when the review request is in the following states:

* Brand new, not yet published
* Published and unchanged
* Published with a change causing a draft
* Discarded
* Roepened after discarded
* Submitted
* Reopened after submitted

nits - space before the equals signs here.

The style is a bit off here - should look like:

gReviewRequest.publish({
    buttons: gDraftBannerButtons,
    success: function() {
        gDraftBanner.hide();
    }
});

Maybe I missed something, but why are we generating this updated timestamp on the client side, instead of reading it from the returned object once the API call is successful?

space before =

Spaces after the ifs, and on either side of the =='s, and before the open braces.

Also, I think you can simplify your replacements here like this:

if (newState) {
  $("span.status").text(newState);
} else {
  $("span.status").text("updated");
}

In an effort to prepare for our eventual localization, we probably want to abstract the English text into some constants that can be easily flipped.

So, instead of hiding each editicon, how about shutting down their editable-fieldness?

This would mean changing inlineEditor so that it can be disabled.

Same as above.

Same comments as above - only in this case, we'd be enabling the inlineEditor.

Instead of changing the ID and adding this, let's just keep the old ID.

What is this element? I can't find any elements with this ID anywhere...

Not entirely sure this is necessary - see below.

Why is this ID changing?

Did I miss something? Why are we adding this new ability?

Please remove this extra line.

Hm, so I'm afraid how how fragile this structure is for l10n. Not sure how important that is at this stage... David or Christian?

This looks really, really wrong. That's three things named the same ID, with tiny variations. That's just plain confusing.

If you really need these things to share these styles, you should use a class - not a series of IDs.

Style is a bit off here.

success: function() {
    gDraftBanner.hide();
}

I think I'd feel better if we didn't depend so much on whether the banners were visible, and just checked the review request object to see if it was in a state that allowed for editable fields.

This is really confusing - why are we still creating a client-side timestamp here? Why can't we retrieve this information from the review request instead?

This can be condensed to:

$("span.status").text(newState ? newState : "updated");

Whoa - hold it...why are you passing "disable" as a string? I thought disable was a function that we'd call on inlineEditor widgets.

Same as above.

Also, this function looks *extremely* similar to the one on line 2503. We might want to DRY* it up.

* Don't repeat yourself.

I really don't think this is how we want to be enabling and disabling inlineEditors...

Why was this ID changed?

Jesus - is this still a WIP patch, or can we start deeply reviewing it?

Swap these. var should go first. Blank line after the var.

loaded should be last. It signifies that we've finished loading.

I believe this can possibly fetch fields that aren't on the review request. I think what you want is .review-request .main

I'm certain this doesn't work. You're never terminating the publish() command. That makes me think that this code path was never tested.

There's no way any of this was ever tested.

You're using Django template tags in here, but we never go through Django. That clearly can never work.

What are you attempting to do here? Why can't we reuse the banner we'd otherwise have? That should just be in the HTML and shown.

Indentation problems.

Should be alphabetical.

Space after the ","

You have indentation problems here.

We only call this once, so I don't think it's worth having a function for it. Just do this logic where it's needed.

Same comments here.

You can chain this:

$('time.timesince')
    .attr('datetime', gReviewRequest.last_updated)
    .timesince();

We're not actually closing it. I'd call this "onReviewRequestClosed" instead, since it's really the handler after we've closed.

Can you keep the queries grouped and the non-queries grouped?

No need for ".editable"

onReviewRequestReopened

What's the "updated" all about?

There can only be one element with a given ID. This query is not reliable. You may need to change button IDs to classes and make sure nothing breaks.

You're missing a ;

Same comment as above.

Space before "("

The ID is the most specific. No need to append the class names.

Needs to wrap to < 80 chars. Do:

$('#submitted-description')
    .reviewCloseCommentEditor(...);

I think what you really want is:

$('.review-request .editable').reviewRequestFieldEditor();

Keep the blank line.

You're defining this button ID multiple times, which won't work. You need to switch to classes and update call sites.

Levels of indentation look wrong. The last endif should only have one space, and the first here should have two.

Make sure the template tags above only indent by 1 space each time and that they're all correct.

One space before "if"