Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#12248 closed enhancement (fixed)

Hide repository admin panel if no connectors enabled

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2
Component: admin/web Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Repository admin page is hidden if there are no repository connectors enabled.
  • Synchronization is skipped for repository with an invalid connector.
API Changes:
Internal Changes:

Description

The proposed change is to have the Manage Repositories admin panel visible only when at least one repository connector is enabled (comment:2:ticket:11713).

Attachments (0)

Change History (11)

comment:2 by Ryan J Ollos, 6 years ago

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

Committed to trunk in [14684].

comment:3 by Ryan J Ollos, 6 years ago

Resolution: fixed
Status: closedreopened

Functional tests are failing.

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

comment:4 by Ryan J Ollos, 6 years ago

Resolution: fixed
Status: reopenedclosed

Failing functional tests fixed in [14687].

comment:5 by Jun Omae, 6 years ago

Since [14684], repository admin page wouldn't be shown if no repository connectors.

If a repository with sync_per_request is defined, I get warning Can't synchronize with repository .... However, it is unable to disable sync_per_request caused by invisible repository admin page and no repository connectors.

I consider we should show repository admin page if Trac environment has repositories and no repository connectors. Thoughts?

  • trac/versioncontrol/admin.py

    diff --git a/trac/versioncontrol/admin.py b/trac/versioncontrol/admin.py
    index 57e119e53..829b86b44 100644
    a b class RepositoryAdminPanel(Component):  
    179179    # IAdminPanelProvider methods
    180180
    181181    def get_admin_panels(self, req):
    182         types = RepositoryManager(self.env).get_supported_types()
    183         if types and 'VERSIONCONTROL_ADMIN' \
    184                       in req.perm('admin', 'versioncontrol/repository'):
     182        rm = RepositoryManager(self.env)
     183        if 'VERSIONCONTROL_ADMIN' in \
     184                req.perm('admin', 'versioncontrol/repository') and \
     185                (rm.get_supported_types() or rm.get_all_repositories()):
    185186            yield ('versioncontrol', _('Version Control'), 'repository',
    186187                   _('Repositories'))
    187188

comment:6 by Ryan J Ollos, 6 years ago

Good find. I think we should definitely deal with this corner-case. What about skipping repository synchronization in RepositoryManager.pre_process_request?

  • trac/versioncontrol/api.py

    diff --git a/trac/versioncontrol/api.py b/trac/versioncontrol/api.py
    index a6fd337..3aadbf6 100644
    a b class RepositoryManager(Component):  
    356356
    357357    def pre_process_request(self, req, handler):
    358358        from trac.web.chrome import Chrome, add_warning
    359         if handler is not Chrome(self.env):
     359        if handler is not Chrome(self.env) and self.get_supported_types():
    360360            for repo_info in self.get_all_repositories().values():
    361361                if not as_bool(repo_info.get('sync_per_request')):
    362362                    continue

comment:7 by Jun Omae, 6 years ago

I think it would be better to skip sync if the repository type isn't supported for each repository with sync_per_request.

  • trac/versioncontrol/api.py

    diff --git a/trac/versioncontrol/api.py b/trac/versioncontrol/api.py
    index a6fd3377a..94e7385d5 100644
    a b class RepositoryManager(Component):  
    357357    def pre_process_request(self, req, handler):
    358358        from trac.web.chrome import Chrome, add_warning
    359359        if handler is not Chrome(self.env):
     360            supported_types = set(self.get_supported_types())
    360361            for repo_info in self.get_all_repositories().values():
    361362                if not as_bool(repo_info.get('sync_per_request')):
    362363                    continue
     364                rtype = repo_info.get('type') or self.default_repository_type
     365                if rtype not in supported_types:
     366                    continue
    363367                start = time_now()
    364368                repo_name = repo_info['name'] or '(default)'
    365369                try:

comment:8 by Ryan J Ollos, 6 years ago

Checking supported type for each repository sounds good. I'd also be okay with equivalent functionality that uses exception handling.

  • trac/versioncontrol/api.py

    diff --git a/trac/versioncontrol/api.py b/trac/versioncontrol/api.py
    index a6fd337..5400eeb 100644
    a b class InvalidRepository(TracError):  
    4040    """Exception raised when a repository is invalid."""
    4141
    4242
     43class InvalidConnector(TracError):
     44    """Exception raised when a repository connector is invalid."""
     45
     46
    4347class IRepositoryConnector(Interface):
    4448    """Provide support for a specific version control system."""
    4549
    class RepositoryManager(Component):  
    365369                try:
    366370                    repo = self.get_repository(repo_info['name'])
    367371                    repo.sync()
     372                except InvalidConnector:
     373                    continue
    368374                except TracError as e:
    369375                    add_warning(req,
    370376                        _("Can't synchronize with repository \"%(name)s\" "
    class RepositoryManager(Component):  
    565571        :return: if no corresponding repository was defined,
    566572                 simply return `None`.
    567573
     574        :raises InvalidConnector: if the repository connector cannot be
     575                                  opened.
    568576        :raises InvalidRepository: if the repository cannot be opened.
    569577        """
    570578        reponame = reponame or ''
    class RepositoryManager(Component):  
    768776            if prio >= 0: # no error condition
    769777                return connector
    770778            else:
    771                 raise TracError(
     779                raise InvalidConnector(
    772780                    _('Unsupported version control system "%(name)s"'
    773781                      ': %(error)s', name=rtype,
    774782                      error=to_unicode(connector.error)))
    775783        else:
    776             raise TracError(
     784            raise InvalidConnector(
    777785                _('Unsupported version control system "%(name)s": '
    778786                  'Can\'t find an appropriate component, maybe the '
    779787                  'corresponding plugin was not enabled? ', name=rtype))

comment:9 by Jun Omae, 6 years ago

That changes look good to me!

comment:10 by Ryan J Ollos, 6 years ago

Thanks, I'll look at adding a test for the changes.

comment:11 by Ryan J Ollos, 6 years ago

Release Notes: modified (diff)

Fixed on 1.2-stable in r15123, merged to trunk in r15124.

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.