Summary

Add a base class for groups of actions.

Review Request #14643 — Created Oct. 19, 2025 and submitted Nov. 14, 2025, 5:07 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When actions were implemented, the only group of actions we needed were

menus. As we begin to use action for more purposes, we need other groups

of actions, meaning it's a good time to come up with a more general base

class for this.
This change introduces BaseGroupAction, which MenuAction is built

upon. This manages a group of actions, allowing for explicit ordering

and -- just like menus (though it would be up to an implementation how

it wants to handle this).
There's a JavaScript side as well, which is equivalent to what

MenuAction was before. MenuAction is now a very thin subclass over

GroupAction.
For testing, our formerly registry-specific set of test action classes

have been moved into a common module, along with some new actions for

groups.
On top of this, actions now have a parent registry, assigned when

registering and unassigned when unregistering. This is primarily for

unit test purposes, but removes the assumption of actions_registry for

child actions.

Testing Done

Unit tests pass.

Tested all the actions in the UI to make sure they still work and
still show the menu items in the right order.

Commits

Summary	ID
Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	2fba3d9d54d6468579af923f7c99c36db3f0942c

Summary

Add a base class for groups of actions.

When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.

2fba3d9d54d6468579af923f7c99c36db3f0942c

Issues

Description	From	Last Updated
'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 19, 2025, 1:07 a.m.
'django.http.HttpRequest' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 19, 2025, 1:07 a.m.
'django.test.client.RequestFactory' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 19, 2025, 1:07 a.m.
'reviewboard.actions.registry.ActionsRegistry' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 19, 2025, 1:07 a.m.
'reviewboard.actions.tests.base.TestAction' imported but unused Column: 1 Error code: F401	reviewbot	Oct. 19, 2025, 1:07 a.m.
Should probably be "child menu IDs" -> "child action IDs"	david	Nov. 6, 2025, 11:17 p.m.
A real exception seems better here too.	david	Nov. 6, 2025, 11:23 p.m.
It seems like this must have changed as you iterated. A dict doesn't make sense given the usage here -- …	david	Nov. 10, 2025, 2:04 a.m.
It seems like a real exception would be better than assert here.	david	Nov. 6, 2025, 11:23 p.m.
Should we instead just do del ... to remove the members?	david	Nov. 6, 2025, 11:24 p.m.
Leftover debug output.	david	Nov. 6, 2025, 11:24 p.m.
Seems like it might be more consistent/future-proof if we did: export interface MenuActionAttrs extends GroupActionAttrs {}	david	Nov. 10, 2025, 2:04 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/actions/tests/test_group_action.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'typing.TYPE_CHECKING' imported but unused

Column: 1
Error code: F401
```
reviewboard/actions/tests/test_group_action.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'django.http.HttpRequest' imported but unused

Column: 1
Error code: F401
```
reviewboard/actions/tests/test_group_action.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'django.test.client.RequestFactory' imported but unused

Column: 1
Error code: F401
```
reviewboard/actions/tests/test_group_action.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'reviewboard.actions.registry.ActionsRegistry' imported but unused

Column: 1
Error code: F401
```
reviewboard/actions/tests/test_group_action.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'reviewboard.actions.tests.base.TestAction' imported but unused

Column: 1
Error code: F401
```

Change Summary:

Removed unused imports.

Commits:

	Summary	ID
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	8993d10740361db62dcb0d8ac4abc5cd8cf30f3b
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	1ad611e3e26f6695432ed73492a262c2068e70f0

Diff:

Revision 2 (+886 -260)

Show changes

	reviewboard/actions/__init__.py
	reviewboard/actions/base.py
	reviewboard/actions/registry.py
	reviewboard/actions/tests/base.py
	reviewboard/actions/tests/test_group_action.py
	reviewboard/actions/tests/test_registry.py
	reviewboard/static/rb/js/common/actions/index.ts
	reviewboard/static/rb/js/common/actions/models/groupActionModel.ts
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/actions/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should probably be "child menu IDs" -> "child action IDs"
```
reviewboard/actions/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
A real exception seems better here too.
```

reviewboard/actions/base.py (Diff revision 2)

The issue has been dropped. Show all issues

It seems like this must have changed as you iterated. A dict doesn't make sense given the usage here -- you always set the values to True and never use them later, only using .keys(). Can you change this to use a set?

The naming here is also confusing. Can we call it something like unprocessed_child_ids?

chipx86 Nov. 6, 2025, 11:26 p.m.

This is effectively an ordered set. We want to quickly test for presence, but maintain order of keys.

This gets revised with better naming later. At this stage in development, I'd rather avoid the merge conflicts for now, if you don't mind a bit of temporary naming.

reviewboard/actions/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
It seems like a real exception would be better than assert here.
```

reviewboard/actions/tests/test_group_action.py (Diff revision 2)

The issue has been resolved. Show all issues

Should we instead just do del ... to remove the members?

reviewboard/actions/tests/test_group_action.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Leftover debug output.
```

reviewboard/static/rb/js/common/actions/models/menuActionModel.ts (Diff revision 2)

The issue has been resolved. Show all issues

Seems like it might be more consistent/future-proof if we did:

export interface MenuActionAttrs extends GroupActionAttrs {}

chipx86 Nov. 6, 2025, 11:26 p.m.

Agreed, but that triggers lint errors:

An interface declaring no members is equivalent to its supertype.

I wouldn't mind disabling that. Happy to switch it and just ignore the warning.

chipx86 Nov. 6, 2025, 11:26 p.m.
```
@typescript-eslint/no-empty-interface
```

david Nov. 8, 2025, 12:03 p.m.

The newer @typescript-eslint that we'll pick up with my changes replaces this rule with a more generic one, but I can configure that to allow this when we're extending an interface. I'll get a change up for that.

Change Summary:


Improved docs.
Added ActionError and raised it in place of a couple assertions.
Switched to del ... for removing test state.
Removed leftover debug output.
Switched to an empty interface for MenuActionAttrs.

Commits:

	Summary	ID
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	1ad611e3e26f6695432ed73492a262c2068e70f0
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	7ddde86011fa90fe21d8fdc7556a13f2dd29cd16

Diff:

Revision 3 (+906 -262)

Show changes

	reviewboard/actions/__init__.py
	reviewboard/actions/base.py
	reviewboard/actions/errors.py
	reviewboard/actions/registry.py
	reviewboard/actions/tests/base.py
	reviewboard/actions/tests/test_group_action.py
	reviewboard/actions/tests/test_registry.py
	reviewboard/static/rb/js/common/actions/index.ts
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added Raises sections for the new ActionError usage.

Commits:

	Summary	ID
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	7ddde86011fa90fe21d8fdc7556a13f2dd29cd16
	Add a base class for groups of actions. When actions were implemented, the only group of actions we needed were menus. As we begin to use action for more purposes, we need other groups of actions, meaning it's a good time to come up with a more general base class for this. This change introduces `BaseGroupAction`, which `MenuAction` is built upon. This manages a group of actions, allowing for explicit ordering and `--` just like menus (though it would be up to an implementation how it wants to handle this). There's a JavaScript side as well, which is equivalent to what `MenuAction` was before. `MenuAction` is now a very thin subclass over `GroupAction`. For testing, our formerly registry-specific set of test action classes have been moved into a common module, along with some new actions for groups. On top of this, actions now have a parent registry, assigned when registering and unassigned when unregistering. This is primarily for unit test purposes, but removes the assumption of `actions_registry` for child actions.	2fba3d9d54d6468579af923f7c99c36db3f0942c

Diff:

Revision 4 (+930 -262)

Show changes

	reviewboard/actions/__init__.py
	reviewboard/actions/base.py
	reviewboard/actions/errors.py
	reviewboard/actions/registry.py
	reviewboard/actions/tests/base.py
	reviewboard/actions/tests/test_group_action.py
	reviewboard/actions/tests/test_registry.py
	reviewboard/static/rb/js/common/actions/index.ts
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (3222270)