Is it possible for people to make their own attachment points? From the docs it's unclear whether this is possible and how to do this, maybe we could elaborate on this.

Should there be 2 spaces here?

Should there be 2 spaces here?

This could use some updating to mention how we can have non-UI actions and actions for the unified banner, maybe you could link to the AttachmentPoint class too.

This could all fit on one line I think.

Add -> None:

Update the description, action arg type and Raises section of this method to include how the given action can be a BaseReviewRequestMenuAction.

Missing a space between "reviewrequest".

I wonder if it would be worthwhile to make some mixins or decorators for some of the repeated access control logic we have in the should_render methods for these actions.

Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

Should we wrap reviewboard.actions.actions_registry in some Sphinx directive? Would :py:obj: work?

Could sort this alphabetically.

Typo in the description: "conistency" -> "consistency"

Maybe worth making all of these links/refs to the appropriate attributes/methods?

Super small nit, but I feel like this may read better with the IDs grouped together

Can we document this, with a Version Added?

While not a problem, we shouldn't need to do this, as initialize is intentionally empty by default.

Can we add a type/docs for this on the class body?

Changing the IDs here (and below) might break existing extensions.

What's the TODO?

Alignment issue.

This can probably use self.create_http_request(...)

This seems unrelated.

Everything in here but the perms and seems to return a bool result. Maybe we just want bool(...) for perms?

Same comment about perms here.
Also, can we add a blank line before return statements?

This isn't something we can reverse?

This doesn't have to be this change, but we should track it:
We could improve this through a ReviewRequest.has_diffsets() method that checks:

Is self._diffsets set (via get_diffsets()). If so, use that.
Is diffset_history loaded from the database? If so, use that diffsets relation.
Else, do DiffSet.objects.filter(history=self.diffset_history_id).exists()

Same comment about perms.

Super tiny nit for all these similar blocks. We have 3 lines that pull from context, and one line that doesn't, smack in the middle. It'd be nice to separate that one out.

Same here about perms.

Same comment about perms.

For these, it'd be nice to group related attributes. action_id and parent_id should probably go together. attachment and apply_to feel related.

Since extensions will most likely be using some subclass of this, we should ideally use that name.
Not sure what the best approach is to get that name, since type(self).__name__ will return the subclass.
But we probably should list that anyway in the error message, so there's a warning per-usage, instead of only one per-process (since warnings with the same text don't repeat).

Can you put this in alphabetical order?

Can this be self.child_actions = child_actions or []?

Can you put this in alphabetical order?

Can we state in the docs here and below that we're removing this in 7?

Missing a trailing period.

Missing a trailing period.

Missing a trailing period.

djblets is mixed in with django.

Older code, but I think this can be:

self.assertIs(
    self.action.should_render(context=request_context),
    getattr(self, 'read_only_always_show', False))


Though, maybe less obvious? I don't know.

Too much JS! deprecation_message :)
But with my other comments about fixing warnings to be per-class, this will need to go away and move into the tests.

These should be assertIsNone.
Same with many below.

This is hard to read. Can we go with keyword arguments here, like create_http_request does? (And same elsewhere.)
I almost wonder if we should ditch self.request and just have tests provide what they want in create_http_request, so it's all self-contained. But probably not worth it for this change.

Can we explicitly return a value if we don't return in the loop?
Right now we just get an implicit undefined.

While here, can we alphabetize these?

Can we add Version Added to each of these?

Can you put parens around the comparison? Helps with readability, given the =.

Extra blank line here.

Can we break this down into the multi-line format? Any time we have typing in params.
Same with others.

I don't think we need export {} anymore.

Can we alphabetize these?

actions should go after accounts, alphabetically.

Missing quotes around the {{action.get_dom_element_id}}.

No space needed before style.

Missing quotes around {{action.get_dom_element_id}}.

The change looks great.
I have a couple comments about documentation. I want to try to get our public docs a bit more fleshed out so it's easier to later track deprecations and usage and typing as we go forward.

Leftover debugging information.

We should keep to public before private.
Also, it'd be nice to have docs for the public attributes.

Can we document this as well?
And same with other public attributes.

Ship It!