I know its a WIP. Quick review, mostly style issues.

Alphabetical order.

Alphabetical order.

Is there a need for the intermediate variable "action"? I know it looks better, but you can just attach .save to the Action() init.

Same thing here.

Same thing.

the "review_published.send()" can be taken out of the if statement since it will be called anyway.

So:

verb = 'published a review'

if self.is_reply():
    verb = 'published a reply'

reply_published.send(sender=self.__class__, user=user, reply=self)

intermediate variable again.

I believe this fits on one line.

I don't see why this import was removed, was it unused?

Alphabetical order.

Keep this empty line before the return.

No need for this empty line.

No need for this empty line.

No need for this empty line.

No need for this empty line.

May I ask why you are using !important here?

No need for those empty lines.

No need for the empty lines here.

Indent all by one.

No need for this extra line.

indent "compressed_css"

indent "include" and "{{datagrid...}}"

A few more style issues.

No need for space between 'view' and :

Remove space before ':'

Space after ':'

Remove space before the ':'.

Indent inside the '{%', not outside.

i.e.:

{% for
{%  if
{%    definevar ...
{%  endif
{% endfor

One more level of indentation here.

Alphabetical order.

No need for this empty line.

Indent within the '{%'.

Indent within the '{%'.

Indent within the '{%'.

Quick review, mostly style.

Tiny optimization:

_request = self.request

Since self.request is being called three times within the method body.

Less .'s means faster code.

Add an empty line here.

Wouldn't it be easier to do this calculation before hand and leave the reasoning in a comment?

i.e.:

 = 31536000 # 365 * 24 * 60 * 60  (1 year)

Alphabetical order.

Translate: Review request.

Remove the trailing white space.

You can shift the the {% %} logic all the way to the left. That saves on white space being sent to the client.

Translate: Posted.

Translate: Updated.

Translate: Ago.

We're probably not doing great about alphabetical order here, but might as well place this above the others.

Just for readability, put each parameter on its own line (and start with the Action.objects.create) line.

Same as above.

Actually, given that we do this twice, can we just set a couple common variables and then create the action after this if/else?

Same here.

Same here.

Blank line after this. I prefer blank lines between blocks and other code (like around an if, for, etc.)

Was this intentional? It looks like a mistake.

Same.

Generally, (and I know a lot of older code doesn't do this), docs should be in this format:


    """One-line summary.

    Multi-line content.
    """

Or in this case, just """One-line summary."""

No blank line.

Instead of this, do this above:


@property
def request_display_id(self):
    ...

Are we expecting that the action feed will now be the default view?

I'd be a little concerned about that, as it changes everybody's workflow. Instead, I'd rather we keep the existing default, and then later we should add some easier way to select a default or quickly jump to the desired node from any page.

No blank line.

This should subclass from object.

Can you document why we need this?

In general, we should use "pk" instead of "id" when referring to the primary key of the model.

Same with the user above.

Add a trailing comma.

Alignment is off.

Space before {

All indentation should be 2 lines instead of 4.

Another case of {.

Same with others in this file.

No blank line.

No spaces around =

One space indentation.

No spaces inside {{..}}

Love it.

Let's go ahead and use 'action-feed'. '-' is nicer in URLs.

No blank line.

We should avoid queries on these objects. So instead, do:

if self.review_id:
    return self.review_id
elif self.changedesc_id:
    return self.changedesc_id

This should return something when neither of the above conditionals match.

No blank line.

Should also check review_id and changedesc_id, and return some default.

action-feed.

Add a trailing comma.

local_site_id

Indentation looks wrong.

This map should only be defined once, outside the function.

You should only have one var statement, formatted like:


var body = ...,
    request_url = ...;

Best as:

var changeitems = $('<ul/>')
    .appendTo(body);

This var needs to also be before. In JavaScript, all variables when compiling in the VM are moved to the top of the scope.

One var statemnet.

Best to append changeitem after you've completely populated it, at the end of the loop.

Best done as:

field_name = field_name_map[field] || field

Should be:

    changeitem.append($('<label/>').text(field_name));

This will do the right thing if there are characters in field_name that need escaping (such as when localized).

In general, instead of 'in', it's best to do field_changed.hasOwnProperty('added').

The reason being that if anything were to populate the base object with 'added' or whatever, this check could fail. By using hasOwnProperty, you ensure you're only looking up in field_changed. It also won't nest into the prototype, so it's a little quicker.

Hmm, not sure what you're doing here? I don't know that an object for added/removed is correct.

In JavaScript, always use braces.

Given that we do field_changed[type] a lot, I would just pull that value out once up-front and reuse it.

vars must be at the top of the scope.

===. Avoids a cast.

Build this with jQuery's methods to ensure there aren't HTML encoding issues.

No blank line.

Braces.

Why the length check?

One var, top of scope.

No blank lines.

For the field_changed length check, you'll want to pull the length out into a variable first. Saves on a recompute every iteration.

hasOwnProperty.

Build with jQuery.

hasOwnProperty.

Build with jQuery.

Blank line.

hasOwnProperty.

Alignment for multiline is wrong.

Should use === for comparison.

One set of var statements.

Maybe oldValue/newValue?

===

All this should be built with jQuery.

===

Alignment is wrong.

Move var out of the for, since it's really going to be promoted to the top of the "else if" block anyway.

Build with jQuery.

Make sure to always use braces.

If so, maybe we should log that.

Blank line.

Build with jQuery.

Blank line.

Alignment is off.

The whole function should build with jQuery.

Same.

One var.

Same.

Alignment is off.

$('<dl/>')
    .appendTo(body);

One var.

Move this into the declaration above, before my proposed appendTo.

Move var up, and pull the length out.

vars go first.

comment can probably just be defined in each loop that needs it.

Build with jQuery.

These comments apply to everywhere else in the JavaScript.

No blank line.

No blank line.

The resource needs a description. This will be auto-generated for our API docs page. Make sure to follow the styles for other resources.

I've noticed this a couple times now. Instead of repeating it, you should define a Manager class that the Action uses, which will define a function for this.

So:

class Action(...):
    ...
    objects = ActionManager()


Then in managers.py:

class ActionManager(...):
    def from_visits(self):
        # That query here

You can combine these.

You can do:

if field in ('screenshot_captions, 'file_captions'):

This looks fine with the exception being the one I noted about the Manager (I don't mind this being a separate cleanup change on top of this), and the unit test failures where Action.submitter doesn't allow null. That needs to be fixed before this goes in.

I'll be committing all these changes when done onto an action-feed branch, which will later be merged into master.

Continued with Review:3292.