chipx86 got a fish trophy!

Summary

Move action activation and state management into the action models.

Review Request #14641 — Created Oct. 18, 2025 and updated Oct. 21, 2025, 7:59 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Blocks

14642

Reviewers

Groups

reviewboard

People

Description

Actions didn't have a standard way to perform an activation of the
action. The menu item action views had an activate() method tied to a
click handler, and subclasses implemented that, but this wasn't a
capability of ActionView. Quick Access actions happened to get the
same activation support by inheriting the same view for menu items, even
though buttons were used for display (creating a potential future
issue by themselves).

As part of the work to centralize actions, Action now has an
activate() method, which performs the operation backed by the action.
Although these are models, this method is allowed to contain or call
UI/view code.

ActionView has been updated to have an official activate() method
for compatibility, which defaults to calling model.activate().
Subclasses can always override this to put in logic between the parent
class's call and the model's activation logic.

ActionView has also been updated to rely more on Action for
visibility management (newly added for Quick Access actions in 7.1),
so all views for an action will behave the same way.

The existing action views have largely been replaced with models, which
in upcoming changes will be better utilized for both standard and Quick
Access actions. The old views have been removed, and probably aren't
worth retaining for any kind of API compatibility.

Testing Done

Unit tests pass.

Tested every single action, making sure states were updated,
visibility was reflected, and actions were activated correctly.

Commits

Summary	ID
Move action activation and state management into the action models. Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.	5efce448c830245f3ebd41bb3e97fdeb20b7deae

Summary

Move action activation and state management into the action models.

Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.

5efce448c830245f3ebd41bb3e97fdeb20b7deae

Issues

Description	From	Last Updated
This is missing a docstring, and maybe its better to have the assert be something like "<classname> must implement getLabelForVisibility".	maubin	Oct. 21, 2025, 3:48 p.m.
This can be removed.	maubin	Oct. 21, 2025, 3:48 p.m.
This can be removed.	maubin	Oct. 21, 2025, 3:48 p.m.
Seems like leftover debug code.	maubin	Oct. 21, 2025, 3:48 p.m.
Shouldn't we be triggering closeError on the reviewRequestEditor instance instead of on the action? I know in the previous code …	maubin	Oct. 21, 2025, 7:52 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Removed debug output.

Commits:

	Summary	ID
	Move action activation and state management into the action models. Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.	4aa77ebc8b47d05b6e748e3574c5cad666aaf69b
	Move action activation and state management into the action models. Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.	aaaacc3ebd53cf6c93e658d1fac7d43fa898c04d

Diff:

Revision 2 (+1298 -1214)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/static/rb/js/common/actions/models/actionModel.ts
	reviewboard/static/rb/js/common/actions/views/actionView.ts
	reviewboard/static/rb/js/common/actions/views/menuActionView.ts
	reviewboard/static/rb/js/common/models/userSessionModel.ts
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts
	reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts (Diff revision 2)

The issue has been resolved. Show all issues

This is missing a docstring, and maybe its better to have the assert be something like "<classname> must implement getLabelForVisibility".

reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts (Diff revision 2)

The issue has been resolved. Show all issues

This can be removed.

reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts (Diff revision 2)

The issue has been resolved. Show all issues

This can be removed.

reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts (Diff revision 2)

The issue has been resolved. Show all issues

Seems like leftover debug code.

reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts (Diff revision 2)

The issue has been dropped. Show all issues

Shouldn't we be triggering closeError on the reviewRequestEditor instance instead of on the action? I know in the previous code we were triggering this event on the action model, but I don't see how this gets bubbled up to ReviewRequestEditor which is the thing that actually listens to closeError events.

chipx86 Oct. 21, 2025, 3:50 p.m.

That is a very good point. Let me address that in its own change, since that's going to take some testing. I don't want to bury that in here.

maubin Oct. 21, 2025, 7:59 p.m.
```
Sounds good.
```

Change Summary:


Fixed the default label for the Archive/Mute actions.
Improved an assertion message.
Added a missing docstring.
Removed some old options doc strings.
Removed leftover debug code.

Commits:

	Summary	ID
	Move action activation and state management into the action models. Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.	aaaacc3ebd53cf6c93e658d1fac7d43fa898c04d
	Move action activation and state management into the action models. Actions didn't have a standard way to perform an activation of the action. The menu item action views had an `activate()` method tied to a click handler, and subclasses implemented that, but this wasn't a capability of `ActionView`. Quick Access actions happened to get the same activation support by inheriting the same view for menu items, even though buttons were used for display (creating a potential future issue by themselves). As part of the work to centralize actions, `Action` now has an `activate()` method, which performs the operation backed by the action. Although these are models, this method is allowed to contain or call UI/view code. `ActionView` has been updated to have an official `activate()` method for compatibility, which defaults to calling `model.activate()`. Subclasses can always override this to put in logic between the parent class's call and the model's activation logic. `ActionView` has also been updated to rely more on `Action` for visibility management (newly added for Quick Access actions in 7.1), so all views for an action will behave the same way. The existing action views have largely been replaced with models, which in upcoming changes will be better utilized for both standard and Quick Access actions. The old views have been removed, and probably aren't worth retaining for any kind of API compatibility.	5efce448c830245f3ebd41bb3e97fdeb20b7deae

Diff:

Revision 3 (+1342 -1214)

Show changes

	reviewboard/reviews/actions.py
	reviewboard/static/rb/js/common/actions/models/actionModel.ts
	reviewboard/static/rb/js/common/actions/views/actionView.ts
	reviewboard/static/rb/js/common/actions/views/menuActionView.ts
	reviewboard/static/rb/js/common/models/userSessionModel.ts
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/reviews/models/reviewRequestActionModels.ts
	reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!