Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#11519 closed enhancement (fixed)

Allow default_handler to be set from the Basic Settings admin page

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: admin/web Version:
Severity: normal Keywords: default_handler
Cc: Jun Omae Branch:
Release Notes:

The [trac] default_handler option can be set from the Basic Settings admin page. A ConfigurationError is raised when navigating to the base url if [trac] default_handler is set to a class for which is_valid_default_handler is False.

API Changes:

The IRequestHandler class attribute is_valid_default_handler determines if the class can be used as a default handler, and defaults to True when not present. The function is_valid_default_hanlder should be used to determine whether an IRequestHandler object is a valid default handler.

Internal Changes:

Description

Some allowed values for default_handler are documented at TracIni#trac-section. However, plugins may add IRequestHandlers and some of the documented values may not be valid if the Components are disabled. We might consider trying to dynamically generate the list of IRequestHandlers for the documentation. However, a simple solution would be to add a select to the Basic Settings admin page and allow the default_handler to be set through the web ui.

Attachments (1)

t11519.diff (2.2 KB ) - added by Ryan J Ollos 10 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 by Ryan J Ollos, 11 years ago

Milestone: next-dev-1.1.x1.1.3
Owner: set to Ryan J Ollos
Status: newassigned

The IRequestHandlers in a default Trac installation includes several Components that aren't usable as a default_handler. The list of IRequestHandlers is:

['InterTracDispatcher', 'Chrome', 'LoginModule', 'AboutModule',
'AttachmentModule', 'AdminModule', 'PygmentsRenderer', 'PreferencesModule',
'SearchModule', 'BatchModifyModule', 'QueryModule', 'ReportModule',
'RoadmapModule', 'MilestoneModule', 'TicketModule', 'TimelineModule',
'BrowserModule', 'ChangesetModule', 'AnyDiffModule', 'LogModule',
'WikiRenderer', 'WikiModule']

Of those InterTracDispatcher, Chrome, LoginModule, PygmentsRenderer, BatchModifyModule and WikiRenderer aren't usable as a default_handler (and PreferencesModule needs the patch in #11531).

If BatchModifyModule and LoginModule were modified so that they would function correctly as a default_handler, then we could adopt a convention that IRequestHandlers ending in Module should function as a default_handler. However, we could probably come up with better ways of indicating that a Component was suitable for use as a `default_handler. Any good ideas?

Initial changes can be found in log:rjollos.git:t11519. Functional tests will be added later.

Is there a better way to get the list of enabled IRequestHandlers than instantiating a RequestDispatcher object?

Last edited 11 years ago by Ryan J Ollos (previous) (diff)

in reply to:  1 ; comment:2 by Peter Suter, 11 years ago

Replying to rjollos:

However, we could probably come up with better ways of indicating that a Component was suitable for use as a `default_handler. Any good ideas?

Similar to IWikiMacroProvider.is_inline() we could add (an optional) IRequestHandler.is_valid_default_handler().

Is there a better way to get the list of enabled IRequestHandlers than instantiating a RequestDispatcher object?

Could you just repeat the handlers = ExtensionPoint(IRequestHandler) declaration in BasicAdminPanel?

Last edited 11 years ago by Peter Suter (previous) (diff)

in reply to:  2 ; comment:3 by Ryan J Ollos, 11 years ago

Replying to psuter:

Similar to IWikiMacroProvider.is_inline() we could add (an optional) IRequestHandler.is_valid_default_handler().

I had been thinking of adding an attribute to the Interface definition. Perhaps default to False, and implementations can set the property to True if appropriate. Are there advantages to using a function rather than an attribute for this case?

One issue with the attribute is that it may not be obvious enough to the implementer of an IRequestHandler that they should consider whether to override the default value.

Could you just repeat the handlers = ExtensionPoint(IRequestHandler) declaration in BasicAdminPanel?

Yeah, that's a good point. A similar use is in trunk/trac/config.py@12432:725#L717.

in reply to:  3 comment:4 by Ryan J Ollos, 11 years ago

Replying to rjollos:

Perhaps default to False, and implementations can set the property to True if appropriate.

Defaulting to True might be more sensible, since most IRequestHandlers will probably be valid default_handlers.

in reply to:  3 comment:5 by Ryan J Ollos, 11 years ago

Replying to rjollos:

I had been thinking of adding an attribute to the Interface definition.

I guess we can't add it to the Interface since the Interfaces aren't base classes.

Like the required attribute, we could just treat it as having a default value if it is not defined, and add it to the class when the class should have a non-default value.

I think having a method is more clear. The latest proposed changes can be found in log:rjollos.git:t11519.2.

comment:6 by Peter Suter, 11 years ago

Right. Following the precedent of the required attribute would work as well I think.

The documentation sounds a bit misleading to me, given that the default is already True. A more detailed comment like for get_extra_mimetypes would be nice. Maybe also mention what is required of a default_handler.

in reply to:  6 comment:7 by Ryan J Ollos, 11 years ago

Replying to psuter:

Right. Following the precedent of the required attribute would work as well I think.

I'm reconsidering. It might be bit cleaner to use an attribute, and we can document it in the IRequestHandler interface documentation: trunk/trac/web/api.py@12527:51#L50.

I will give it some thought for another day or two, and wait to see if there is additional input on which direction to take.

I will also make sure to improve the documentation per your feedback. My understanding of the requirements for a default_handler are:

  • Returns a template representing an HTML document and the data dictionary needed to render the template (i.e. must satisfy the minimum requirements for an HTML document: html, head and body tags).
  • Does not require match_request to be called before process_request (as an example, PreferencesModule failed to meet this requirement prior to the fix in #11531: branches/1.0-stable/trac/web/main.py@12460:180-182#L171).
Last edited 11 years ago by Ryan J Ollos (previous) (diff)

in reply to:  2 ; comment:8 by Jun Omae, 11 years ago

Similar to IWikiMacroProvider.is_inline() we could add (an optional) IRequestHandler.is_valid_default_handler().

I like property rather than such a method, e.g. jquery_noconflict property of IRequestHandler class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652.

in reply to:  8 comment:9 by Ryan J Ollos, 11 years ago

Replying to jomae:

I like property rather than such a method, e.g. jquery_noconflict property of IRequestHandler class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652.

That is an interesting case. It seems like a good time to document jquery_noconflict in the IRequestHandler interface definition as well.

comment:10 by Ryan J Ollos, 11 years ago

Latest changes can be found in log:rjollos.git:t11519.3. is_valid_default_handler is now a property.

comment:11 by Jun Omae, 11 years ago

Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.

in reply to:  11 comment:12 by Ryan J Ollos, 11 years ago

Replying to jomae:

Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.

I was confused and had meant to use lazy there rather than property, but your changes are better. Thanks! I'll add functional tests in the coming days.

comment:13 by Ryan J Ollos, 11 years ago

I tried some different ways of isolating the code that checks whether default_handler is a valid value.

One was to define the class attribute as _default_handler, and have a class property named default_handler: [13144c03/rjollos.git]. Maybe that would be okay, but it doesn't look quite right. It would be nice if we could set a getter when the ExtensionOption is defined, but I haven't sorted out whether there is a way to make that work.

The other simple way to isolate the code would be to have a get_default_handler method: [9b8b200b/rjollos.git].

Is either of these two better than previous changes?

in reply to:  13 ; comment:14 by Peter Suter, 11 years ago

Both look ok to me.

Another alternative would be to leave default_handler as it is, and add the class property named valid_default_handler. (But if that's really better is debatable.)

in reply to:  14 comment:15 by Ryan J Ollos, 11 years ago

Replying to psuter:

Another alternative would be to leave default_handler as it is, and add the class property named valid_default_handler. (But if that's really better is debatable.)

I like valid_default_handler. I'll go ahead and make that change.

Last edited 11 years ago by Ryan J Ollos (previous) (diff)

comment:16 by Ryan J Ollos, 11 years ago

Milestone: 1.1.31.1.2

comment:17 by Ryan J Ollos, 11 years ago

If I had tested the changes I proposed in comment:13, I would have realized that using a property is problematic because a Request object is needed for constructing the href to the Basic Settings admin page.

I tried instead using self.env.href. That seems to work okay for the case that [trac] base_url is not set, because of this assignment. However, if [trac] base_url is set incorrectly, we land on the wrong page. I don't see any other cases of self.env.href used to generate an href, so I'm suspicious as to whether this is a good idea. Just the problem seen when [trac] base_url is set incorrectly might push us to instead prefer: [7903703f/rjollos.git].

get_valid_default_handler might be better anyway because in the future I'd like to propose adding default_handler as a session preference, and when get_valid_default_handler is modified to check the user's default_handler preference, it will need access to the Request object.

Proposed changes can be found in log:rjollos.git:t11519.5. I'm working on adding tests now.

comment:18 by Ryan J Ollos, 11 years ago

Functional test added in [b4735636/rjollos.git].

comment:19 by Ryan J Ollos, 11 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [12624].

comment:20 by Ryan J Ollos, 10 years ago

Olemis and I came up with the following refactoring after reviewing [12624] today:

  • trac/admin/web_ui.py

    diff --git a/trac/admin/web_ui.py b/trac/admin/web_ui.py
    index a07c7e9..6c85851 100644
    a b from trac.web import HTTPNotFound, IRequestHandler  
    4242from trac.web.chrome import add_notice, add_stylesheet, \
    4343                            add_warning, Chrome, INavigationContributor, \
    4444                            ITemplateProvider
     45from trac.web.api import is_valid_default_handler
    4546from trac.wiki.formatter import format_to_html
    4647
    4748try:
    class BasicsAdminPanel(Component):  
    212213            yield ('general', _("General"), 'basics', _("Basic Settings"))
    213214
    214215    def render_admin_panel(self, req, cat, page, path_info):
    215         valid_handlers = [h.__class__.__name__ for h in self.handlers
    216                           if getattr(h, 'is_valid_default_handler', True)]
     216        valid_handlers = [hdlr.__class__.__name__ for hdlr in self.handlers
     217                          if is_valid_default_handler(hdlr)]
    217218        if Locale:
    218219            locale_ids = get_available_locales()
    219220            locales = [Locale.parse(locale) for locale in locale_ids]
  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 8a9851a..8ace8fe 100644
    a b class IRequestHandler(Interface):  
    8787        """
    8888
    8989
     90def is_valid_default_handler(handler):
     91    return handler and getattr(handler, 'is_valid_default_handler', True)
     92
     93
    9094class IRequestFilter(Interface):
    9195    """Enable components to interfere with the processing done by the
    9296    main handler, either before and/or after it enters in action.
  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index c546dde..bdab9f0 100644
    a b class RequestDispatcher(Component):  
    267267
    268268    def get_valid_default_handler(self, req):
    269269        handler = self.default_handler
    270         if not handler or \
    271                 not getattr(handler, 'is_valid_default_handler', True):
     270        if not is_valid_default_handler(handler):
    272271            raise ConfigurationError(
    273272                tag_("%(handler)s is not a valid default handler. Please "
    274273                     "update %(option)s through the %(page)s page or by "

The aim is to isolate to a single location in the codebase the definition of the default value for the is_valid_default_handler attribute.

by Ryan J Ollos, 10 years ago

Attachment: t11519.diff added

comment:21 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Change from comment:20 committed in [12684].

comment:22 by Ryan J Ollos, 10 years ago

Another thing I just noticed is that get_valid_default_handler should probably be a private method: _get_valid_default_handler.

comment:23 by Ryan J Ollos, 10 years ago

Changed get_valid_default_handler to a private method in [12690].

comment:24 by Ryan J Ollos, 10 years ago

Made attribute/variable names more descriptive in [12752:12753].

comment:25 by Ryan J Ollos, 10 years ago

Another refactoring committed in [12836], and a defect fixed in [12837].

comment:26 by Ryan J Ollos, 10 years ago

Keywords: default_handler added

in reply to:  description comment:27 by Ryan J Ollos, 10 years ago

Replying to rjollos:

We might consider trying to dynamically generate the list of IRequestHandlers for the documentation.

This will be implemented in #11746.

comment:28 by Jun Omae, 10 years ago

Cc: Jun Omae added

Location of default handler setting in user preferences feels strange to me while I was translating message catalogs. Currently, settings in General panel is for user information. However, default handler setting is not for user information.

Help in General panel says;

This information is used to associate your login name with your email address and full name, which is used for email notification and RSS feeds, for example.

I suppose it would be better to be located to User Interface rather than General panel. Thoughts?

in reply to:  28 comment:29 by Ryan J Ollos, 10 years ago

Replying to jomae:

I suppose it would be better to be located to User Interface rather than General panel. Thoughts?

I chose General for lack of a better place. I agree that it's not ideal. #11597 is the ticket in which it was implemented.

User Interface doesn't feel like a better fit to me since it seems to be associated with visual presentation. I was hoping that a better location would be revealed as we add more preferences. #10543 could yield more preferences for a Navigation panel though I don't find the feature set compeling. A preference for mainnav ordering might be useful. Or we could do something like combine Keyboard Shortcuts with Default handler in a Navigation panel. Can we foresee any additional entries for Keyboard shortcuts? If not, then I'm not sure it deserves its own tab.

Maybe the thing to do is list preference we will likely add and try to come up with the best grouping that doesn't result in too many tabs.

comment:30 by Ryan J Ollos, 10 years ago

#11813 will consider moving the Default handler preference to another tab.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.