Skip to content

do not register the comments controller if config.comments == false#5311

Closed
ryanoboril wants to merge 1 commit intoactiveadmin:masterfrom
ryanoboril:exclude_comments_from_routes
Closed

do not register the comments controller if config.comments == false#5311
ryanoboril wants to merge 1 commit intoactiveadmin:masterfrom
ryanoboril:exclude_comments_from_routes

Conversation

@ryanoboril
Copy link

Setting config.comments = false is supposed to completely disable comments functionality, but it doesn't exclude comments from the routes after calling ActiveAdmin.routes(self) and there is no way to remove a registered resource. This PR addresses this issue by preventing the Comments controller from being registered at all if config.comments is set to false.

@varyonic
Copy link
Contributor

varyonic commented Jan 8, 2018

See also #3695 and #5117. The intended behavior is that a default for config.comments may be set at the namespace level and overridden at the resource level.

@zorab47
Copy link
Contributor

zorab47 commented Feb 28, 2018

Ideally it would check to verify there are no ActiveAdmin resources that have comments enabled, right?

@Bialogs
Copy link

Bialogs commented Apr 23, 2018

Comment tables are also left in the database when setting comments to false...
Removing that table in conjunction with setting comments to false will obviously cause an application error when browsing to the route. Documentation should be updated to show that a migration to drop the comments table will finish completely disabling comments (to include removing?).

@javierjulio
Copy link
Member

I'm open to a configuration for removing the comments routes and not registering the controller if it's disabled but perhaps best revisited since the existing configuration also controls on what resources it does or doesn't show up. New suggestions can be brought up in #5117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants