You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bloodhound.apache.org by Olivier Mauras <ol...@core-hosting.net> on 2013/11/06 16:06:48 UTC

First installation... my humble review

Hello,

I met Gary Martin this morning on #trac and discussing quickly about 
the look & feel of Bloodhound i ended up installing it on a test server.
Here's a compilation of the points that i find "lacking":

1/ Blocking points for a production adoption - indeed that's only my 
opinion

   * Product owner should automatically have admin rights to this 
product.
   I plan on using Bloodhound in a multi product / multi users setup.
   I can't imagine it working without delegating the product admin 
rights to their owners

   * Products should be able to have their own source repo
   If rights are delegated, I don't see any valid reason for owners to 
not be able to add their own repos

2/ Confusing points

   * Tickets tab being the dashboard
   Being a new user, I find it really confusing for the "Tickets" tab 
for being the dashboard and consequently the only access to products 
list.
   It would be way more intuitive - and faster - to keep the products 
list dropdown at the left of the breadcrumb.

   * Not knowing where you are
   This is indeed linked to the previous point, wiki and source pages 
don't show the product in breadcrumb

3/ Eye candy

   * Source browser CSS may not be the easiest to read
   All lines to the same color make it difficult to catch things
   Age color highlight is too light and/or too close in colors

   * There's place on the mainnav tabs
   As there's place available and that one can enable timeline and 
roadmap from base.ini why not add them to mainnav() to not have then in 
"More" tab?

4/ Graphic glitch

   https://bh-demo1.apache.org/products/%40/roadmap
   As you can see here milestone bars are not displaying correctly.



I think that's all I have for now, thanks for taking time to read me.

Olivier

Re: First installation... my humble review

Posted by Joachim Dreimann <jo...@wandisco.com>.

> On 12 Nov 2013, at 16:58, Gary Martin <ga...@wandisco.com> wrote:
> 
> Hi,
> 
> Just wondering how people felt about Olivier's contribution so far and the idea in general. Obviously the code does need a bit more work as we probably need to have product repositories isolated from the global environment.
> 
> While I think we definitely should maintain global repositories that products can link to, the ability to keep a repository private to a specific product, even in the admin, makes a lot of sense to me.
> 

I agree.

- Joe

> Cheers,
>    Gary
> 
>> On 07/11/13 16:40, Olivier Mauras wrote:
>> 
>> Please find attached two patches.
>> First one grants product owner admin rights to his products, and change the repository management part to the one used by global one. This makes the product owner to create/delete/alias repositories.
>> For the moment it's global, it would require some changes at DB level to make the repositories isolated per product. I think a new table - "product_repositories" - with a new "DbRepositoryProvider" class would be the less intrusive...
>> 
>> The second small patch modifies the theme to get "Source" tab to point to product/<id>/browser instead of getting the wiki.
>> This indeed gives a "No node /" error as i haven't yet found my way in the code that would point the product browser to the repository.
>> 
>> Sorry for the nasty two patches, i worked on installed app instead of the code... This is all based on the latest stable 0.7.0.
>> 
>> Hope this will help.
>> 
>> Olivier
>> 
>> 
>> bh_p1.patch
>> 
>> 
>> diff --git a/product_admin.py b/product_admin.py
>> index 9c0fc42..df69e04 100644
>> --- a/product_admin.py
>> +++ b/product_admin.py
>> @@ -18,6 +18,8 @@
>>    """Admin panels for product management"""
>>  +from genshi.builder import tag
>> +
>>  from trac.admin.api import IAdminCommandProvider, AdminCommandError,\
>>      AdminCommandManager
>>  from trac.admin.console import TracAdmin, TRAC_VERSION
>> @@ -27,9 +29,9 @@ from trac.config import *
>>  from trac.perm import PermissionSystem
>>  from trac.resource import ResourceNotFound
>>  from trac.ticket.admin import TicketAdminPanel, _save_config
>> -from trac.util import lazy
>> -from trac.util.text import print_table, to_unicode, printerr, printout
>> -from trac.util.translation import _, N_, gettext, ngettext
>> +from trac.util import lazy, as_bool
>> +from trac.util.text import print_table, to_unicode, printerr, printout, breakable_path, normalize_whitespace
>> +from trac.util.translation import _, N_, gettext, ngettext, tag_
>>  from trac.web.api import HTTPNotFound, IRequestFilter, IRequestHandler
>>  from trac.web.chrome import Chrome, add_notice, add_warning
>>  @@ -37,9 +39,10 @@ from multiproduct.env import ProductEnvironment
>>  from multiproduct.model import Product
>>  from multiproduct.perm import sudo
>>  -import multiproduct.versioncontrol
>> +#import multiproduct.versioncontrol
>>  import trac.versioncontrol.admin
>> -from trac.versioncontrol import DbRepositoryProvider, RepositoryManager
>> +from trac.versioncontrol import DbRepositoryProvider, RepositoryManager, \
>> +                                is_default
>>  from multiproduct.util import ReplacementComponent
>>    #--------------------------
>> @@ -335,8 +338,7 @@ class ProductAdminModule(Component):
>>          """
>>          if isinstance(self.env, ProductEnvironment) and \
>>                  handler is AdminModule(self.env) and \
>> -                not req.perm.has_permission('TRAC_ADMIN') and \
>> -                req.perm.has_permission('PRODUCT_ADMIN'):
>> +                not req.perm.has_permission('PRODUCT_ADMIN'):
>>              # Intercept admin request
>>              return self
>>          return handler
>> @@ -410,9 +412,27 @@ class ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>>        def get_admin_panels(self, req):
>>          if 'VERSIONCONTROL_ADMIN' in req.perm:
>> -            yield ('versioncontrol', _('Version Control'), 'repository',
>> -                   _('Repository Links') if isinstance(self.env, ProductEnvironment)
>> -                    else _('Repositories'))
>> +            yield ('versioncontrol', _('Version Control'), 'repository',
>> +                _('Repositories'))
>> +
>> +    def _extend_info(self, reponame, info, editable):
>> +        """Extend repository info for rendering."""
>> +        info['name'] = reponame
>> +        if info.get('dir') is not None:
>> +            info['prettydir'] = breakable_path(info['dir']) or ''
>> +        info['hidden'] = as_bool(info.get('hidden'))
>> +        #info['editable'] = editable
>> +        info['editable'] = True
>> +        if not info.get('alias'):
>> +            try:
>> +                self.log.debug(reponame)
>> +                repos = RepositoryManager(self.env.parent).get_repository(reponame)
>> +                youngest_rev = repos.get_youngest_rev()
>> +                info['rev'] = youngest_rev
>> +                info['display_rev'] = repos.display_rev(youngest_rev)
>> +            except Exception:
>> +                pass
>> +        return info
>>        def render_admin_panel(self, req, category, page, path_info):
>>          if not isinstance(self.env, ProductEnvironment):
>> @@ -420,41 +440,143 @@ class ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>>                  req, category, page, path_info)
>>            req.perm.require('VERSIONCONTROL_ADMIN')
>> -        db_provider = self.env[DbRepositoryProvider]
>> -
>> -        if req.method == 'POST' and db_provider:
>> -            if req.args.get('remove'):
>> -                repolist = req.args.get('sel')
>> -                if repolist:
>> -                    if isinstance(repolist, basestring):
>> -                        repolist = [repolist, ]
>> -                    for reponame in repolist:
>> -                        db_provider.unlink_product(reponame)
>> -            elif req.args.get('addlink') is not None and db_provider:
>> -                reponame = req.args.get('repository')
>> -                db_provider.link_product(reponame)
>> -            req.redirect(req.href.admin(category, page))
>> -
>> -        # Retrieve info for all product repositories
>> -        rm_product = RepositoryManager(self.env)
>> -        rm_product.reload_repositories()
>> -        all_product_repos = rm_product.get_all_repositories()
>> -        repositories = dict((reponame, self._extend_info(
>> -                                reponame, info.copy(), True))
>> -                            for (reponame, info) in
>> -                                all_product_repos.iteritems())
>> -        types = sorted([''] + rm_product.get_supported_types())
>> -
>> -        # construct a list of all repositores not linked to this product
>>          rm = RepositoryManager(self.env.parent)
>>          all_repos = rm.get_all_repositories()
>> -        unlinked_repositories = dict([(k, all_repos[k]) for k in
>> -            sorted(set(all_repos) - set(all_product_repos))])
>> +        db_provider = self.env[DbRepositoryProvider]
>> +
>> +        if path_info:
>> +            # Detail view
>> +            self.log.debug(path_info)
>> +            reponame = path_info if not is_default(path_info) else ''
>> +            info = all_repos.get(reponame)
>> +            if info is None:
>> +                raise TracError(_("Repository '%(repo)s' not found",
>> +                                  repo=path_info))
>> +            if req.method == 'POST':
>> +                if req.args.get('cancel'):
>> +                    req.redirect(req.href.admin(category, page))
>> +
>> +                elif db_provider and req.args.get('save'):
>> +                    # Modify repository
>> +                    changes = {}
>> +                    for field in db_provider.repository_attrs:
>> +                        value = normalize_whitespace(req.args.get(field))
>> +                        if (value is not None or field == 'hidden') \
>> +                                and value != info.get(field):
>> +                            changes[field] = value
>> +                    if 'dir' in changes \
>> +                            and not self._check_dir(req, changes['dir']):
>> +                        changes = {}
>> +                    if changes:
>> +                        db_provider.modify_repository(reponame, changes)
>> +                        add_notice(req, _('Your changes have been saved.'))
>> +                    name = req.args.get('name')
>> +                    resync = tag.tt('trac-admin $ENV repository resync "%s"'
>> +                                    % (name or '(default)'))
>> +                    if 'dir' in changes:
>> +                        msg = tag_('You should now run %(resync)s to '
>> +                                   'synchronize Trac with the repository.',
>> +                                   resync=resync)
>> +                        add_notice(req, msg)
>> +                    elif 'type' in changes:
>> +                        msg = tag_('You may have to run %(resync)s to '
>> +                                   'synchronize Trac with the repository.',
>> +                                   resync=resync)
>> +                        add_notice(req, msg)
>> +                    if name and name != path_info and not 'alias' in info:
>> +                        cset_added = tag.tt('trac-admin $ENV changeset '
>> +                                            'added "%s" $REV'
>> +                                            % (name or '(default)'))
>> +                        msg = tag_('You will need to update your post-commit '
>> +                                   'hook to call %(cset_added)s with the new '
>> +                                   'repository name.', cset_added=cset_added)
>> +                        add_notice(req, msg)
>> +                    if changes:
>> +                        req.redirect(req.href.admin(category, page))
>> +
>> +            Chrome(self.env).add_wiki_toolbars(req)
>> +            data = {'view': 'detail', 'reponame': reponame}
>>  -        data = {'types': types, 'default_type': rm_product.repository_type,
>> -                'repositories': repositories,
>> -                'unlinked_repositories': unlinked_repositories}
>> -        return 'repository_links.html', data
>> +        else:
>> +            if req.method == 'POST':
>> +                # Add a repository
>> +                if db_provider and req.args.get('add_repos'):
>> +                    name = req.args.get('name')
>> +                    type_ = req.args.get('type')
>> +                    # Avoid errors when copy/pasting paths
>> +                    dir = normalize_whitespace(req.args.get('dir', ''))
>> +                    if name is None or type_ is None or not dir:
>> +                        add_warning(req, _('Missing arguments to add a '
>> +                                            'repository.'))
>> +                    elif self._check_dir(req, dir):
>> +                        db_provider.add_repository(name, dir, type_)
>> +                        name = name or '(default)'
>> +                        add_notice(req, _('The repository "%(name)s" has been '
>> +                                          'added.', name=name))
>> +                        resync = tag.tt('trac-admin $ENV repository resync '
>> +                                        '"%s"' % name)
>> +                        msg = tag_('You should now run %(resync)s to '
>> +                                   'synchronize Trac with the repository.',
>> +                                    resync=resync)
>> +                        add_notice(req, msg)
>> +                        cset_added = tag.tt('trac-admin $ENV changeset '
>> +                                            'added "%s" $REV' % name)
>> +                        msg = tag_('You should also set up a post-commit hook '
>> +                                   'on the repository to call %(cset_added)s '
>> +                                   'for each committed changeset.',
>> +                                   cset_added=cset_added)
>> +                        add_notice(req, msg)
>> +                        req.redirect(req.href.admin(category, page))
>> +
>> +                # Add a repository alias
>> +                elif db_provider and req.args.get('add_alias'):
>> +                    name = req.args.get('name')
>> +                    alias = req.args.get('alias')
>> +                    if name is not None and alias is not None:
>> +                        db_provider.add_alias(name, alias)
>> +                        add_notice(req, _('The alias "%(name)s" has been '
>> +                                          'added.', name=name or '(default)'))
>> +                        req.redirect(req.href.admin(category, page))
>> +                    add_warning(req, _('Missing arguments to add an '
>> +                                       'alias.'))
>> +
>> +                # Refresh the list of repositories
>> +                elif req.args.get('refresh'):
>> +                    req.redirect(req.href.admin(category, page))
>> +
>> +                # Remove repositories
>> +                elif db_provider and req.args.get('remove'):
>> +                    sel = req.args.getlist('sel')
>> +                    if sel:
>> +                        for name in sel:
>> +                            db_provider.remove_repository(name)
>> +                        add_notice(req, _('The selected repositories have '
>> +                                          'been removed.'))
>> +                        req.redirect(req.href.admin(category, page))
>> +                    add_warning(req, _('No repositories were selected.'))
>> +
>> +            data = {'view': 'list'}
>> +
>> +        # Force repo refresh - should already happen :/
>> +        rm.reload_repositories()
>> +
>> +        #self.log.debug(all_repos)
>> +
>> +        db_repos = {}
>> +        if db_provider is not None:
>> +            db_repos = dict(db_provider.get_repositories())
>> +
>> +        # Prepare common rendering data
>> +        repositories = dict((reponame, self._extend_info(reponame, info.copy(),
>> +                                                         reponame in db_repos))
>> +                            for (reponame, info) in all_repos.iteritems())
>> +
>> +        #self.log.debug(repositories)
>> +        types = sorted([''] + rm.get_supported_types())
>> +        data.update({'types': types, 'default_type': rm.repository_type,
>> +                     'repositories': repositories})
>> +
>> +        return 'admin_repositories.html', data
>>    trac.versioncontrol.admin.RepositoryAdminPanel = ProductRepositoryAdminPanel
>>  
>> 
>> bh_p2.patch
>> 
>> 
>> diff --git a/theme.py b/theme.py
>> index 6c7f81b..8686a26 100644
>> --- a/theme.py
>> +++ b/theme.py
>> @@ -443,8 +443,7 @@ class BloodhoundTheme(ThemeBase):
>>              bm = self.env[BrowserModule]
>>              if bm and not list(bm.get_navigation_items(req)):
>>                  yield ('mainnav', 'browser',
>> -                       tag.a(_('Browse Source'),
>> -                             href=req.href.wiki('TracRepositoryAdmin')))
>> +                       tag.a(_('Browse Source'), href=req.href.browser()))
>>      class QuickCreateTicketDialog(Component):
> 

Re: First installation... my humble review

Posted by Olemis Lang <ol...@gmail.com>.
On Tue, Nov 12, 2013 at 11:58 AM, Gary Martin <ga...@wandisco.com>wrote:

> Hi,
>

:)


>
> Just wondering how people felt about Olivier's contribution so far and the
> idea in general. Obviously the code does need a bit more work as we
> probably need to have product repositories isolated from the global
> environment.
>

IMO what we need is UI to hide the underlying complexity of product-repos
links and give product admins the chance to create / fork / import their
own repos thus giving them the illusion of creating new repos in a given
product context .


>
> While I think we definitely should maintain global repositories that
> products can link to,


+


> the ability to keep a repository private to a specific product, even in
> the admin, makes a lot of sense to me.
>

+

... now ... below I add my comment about both patches


>
> Cheers,
>     Gary
>
>
> On 07/11/13 16:40, Olivier Mauras wrote:
>
>>
>> Please find attached two patches.
>> First one grants product owner admin rights to his products,
>>
>
Like I just said this should be the case in current implementation via
permission policies .


> and change the repository management part to the one used by global one.
>> This makes the product owner to create/delete/alias repositories.
>>
>
I really do not want this to happen because this implies that product
admins will have knowledge of file-system path and access to perform some
actions beyond web UI , which I do not recommend in many kinds of
deployments , especially those similar to forges where most products are
unrelated and many people actually manage their own products .

@olivier : firstly ... have you run the test suite after with your patches
applied ? What are the results ?

I have other objections to the first patch that I'll mention below .

[...]

>   The second small patch modifies the theme to get "Source" tab to point
>> to product/<id>/browser instead of getting the wiki.
>>
>
JFTR , even if your patch is ok , this is a design decision so needs to be
discussed a bit further .


> This indeed gives a "No node /" error as i haven't yet found my way in the
>> code that would point the product browser to the repository.
>>
>> Sorry for the nasty two patches, i worked on installed app instead of the
>> code... This is all based on the latest stable 0.7.0.
>>
>> Hope this will help.
>>
>> Olivier
>>
>>
>> bh_p1.patch
>>
>>
>> diff --git a/product_admin.py b/product_admin.py
>> index 9c0fc42..df69e04 100644
>> --- a/product_admin.py
>> +++ b/product_admin.py
>>
> [...]

> @@ -335,8 +338,7 @@ class ProductAdminModule(Component):
>>           """
>>           if isinstance(self.env, ProductEnvironment) and \
>>                   handler is AdminModule(self.env) and \
>> -                not req.perm.has_permission('TRAC_ADMIN') and \
>> -                req.perm.has_permission('PRODUCT_ADMIN'):
>> +                not req.perm.has_permission('PRODUCT_ADMIN'):
>>               # Intercept admin request
>>               return self
>>           return handler
>>
>
I'm strongly against this change ... This is aimed at granting
PRODUCT_ADMIN users with access to admin panels even if they lack
TRAC_ADMIN permission . What's the goal of your modifications ?


> @@ -410,9 +412,27 @@ class
>> ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>>         def get_admin_panels(self, req):
>>           if 'VERSIONCONTROL_ADMIN' in req.perm:
>> -            yield ('versioncontrol', _('Version Control'), 'repository',
>> -                   _('Repository Links') if isinstance(self.env,
>> ProductEnvironment)
>> -                    else _('Repositories'))
>> +            yield ('versioncontrol', _('Version Control'), 'repository',
>> +                _('Repositories'))
>> +
>> +    def _extend_info(self, reponame, info, editable):
>> +        """Extend repository info for rendering."""
>> +        info['name'] = reponame
>> +        if info.get('dir') is not None:
>> +            info['prettydir'] = breakable_path(info['dir']) or ''
>> +        info['hidden'] = as_bool(info.get('hidden'))
>> +        #info['editable'] = editable
>> +        info['editable'] = True
>> +        if not info.get('alias'):
>> +            try:
>> +                self.log.debug(reponame)
>> +                repos =
>> RepositoryManager(self.env.parent).get_repository(reponame)
>> +                youngest_rev = repos.get_youngest_rev()
>> +                info['rev'] = youngest_rev
>> +                info['display_rev'] = repos.display_rev(youngest_rev)
>> +            except Exception:
>> +                pass
>> +        return info
>>
>         def render_admin_panel(self, req, category, page, path_info):
>>           if not isinstance(self.env, ProductEnvironment):
>>
>
Does this belong in Bloodhound theme template override functions i.e.
bhtheme.theme ?


> @@ -420,41 +440,143 @@ class
>> ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>>
>
>  [...]

I'm not quite sure ... but maybe I did not understand all the details
involved .

[...]

-- 
Regards,

Olemis - @olemislc

Re: First installation... my humble review

Posted by Gary Martin <ga...@wandisco.com>.
Hi,

Just wondering how people felt about Olivier's contribution so far and 
the idea in general. Obviously the code does need a bit more work as we 
probably need to have product repositories isolated from the global 
environment.

While I think we definitely should maintain global repositories that 
products can link to, the ability to keep a repository private to a 
specific product, even in the admin, makes a lot of sense to me.

Cheers,
     Gary

On 07/11/13 16:40, Olivier Mauras wrote:
>
> Please find attached two patches.
> First one grants product owner admin rights to his products, and 
> change the repository management part to the one used by global one. 
> This makes the product owner to create/delete/alias repositories.
> For the moment it's global, it would require some changes at DB level 
> to make the repositories isolated per product. I think a new table - 
> "product_repositories" - with a new "DbRepositoryProvider" class would 
> be the less intrusive...
>
> The second small patch modifies the theme to get "Source" tab to point 
> to product/<id>/browser instead of getting the wiki.
> This indeed gives a "No node /" error as i haven't yet found my way in 
> the code that would point the product browser to the repository.
>
> Sorry for the nasty two patches, i worked on installed app instead of 
> the code... This is all based on the latest stable 0.7.0.
>
> Hope this will help.
>
> Olivier
>
>
> bh_p1.patch
>
>
> diff --git a/product_admin.py b/product_admin.py
> index 9c0fc42..df69e04 100644
> --- a/product_admin.py
> +++ b/product_admin.py
> @@ -18,6 +18,8 @@
>   
>   """Admin panels for product management"""
>   
> +from genshi.builder import tag
> +
>   from trac.admin.api import IAdminCommandProvider, AdminCommandError,\
>       AdminCommandManager
>   from trac.admin.console import TracAdmin, TRAC_VERSION
> @@ -27,9 +29,9 @@ from trac.config import *
>   from trac.perm import PermissionSystem
>   from trac.resource import ResourceNotFound
>   from trac.ticket.admin import TicketAdminPanel, _save_config
> -from trac.util import lazy
> -from trac.util.text import print_table, to_unicode, printerr, printout
> -from trac.util.translation import _, N_, gettext, ngettext
> +from trac.util import lazy, as_bool
> +from trac.util.text import print_table, to_unicode, printerr, printout, breakable_path, normalize_whitespace
> +from trac.util.translation import _, N_, gettext, ngettext, tag_
>   from trac.web.api import HTTPNotFound, IRequestFilter, IRequestHandler
>   from trac.web.chrome import Chrome, add_notice, add_warning
>   
> @@ -37,9 +39,10 @@ from multiproduct.env import ProductEnvironment
>   from multiproduct.model import Product
>   from multiproduct.perm import sudo
>   
> -import multiproduct.versioncontrol
> +#import multiproduct.versioncontrol
>   import trac.versioncontrol.admin
> -from trac.versioncontrol import DbRepositoryProvider, RepositoryManager
> +from trac.versioncontrol import DbRepositoryProvider, RepositoryManager, \
> +                                is_default
>   from multiproduct.util import ReplacementComponent
>   
>   #--------------------------
> @@ -335,8 +338,7 @@ class ProductAdminModule(Component):
>           """
>           if isinstance(self.env, ProductEnvironment) and \
>                   handler is AdminModule(self.env) and \
> -                not req.perm.has_permission('TRAC_ADMIN') and \
> -                req.perm.has_permission('PRODUCT_ADMIN'):
> +                not req.perm.has_permission('PRODUCT_ADMIN'):
>               # Intercept admin request
>               return self
>           return handler
> @@ -410,9 +412,27 @@ class ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>   
>       def get_admin_panels(self, req):
>           if 'VERSIONCONTROL_ADMIN' in req.perm:
> -            yield ('versioncontrol', _('Version Control'), 'repository',
> -                   _('Repository Links') if isinstance(self.env, ProductEnvironment)
> -                    else _('Repositories'))
> +            yield ('versioncontrol', _('Version Control'), 'repository',
> +                _('Repositories'))
> +
> +    def _extend_info(self, reponame, info, editable):
> +        """Extend repository info for rendering."""
> +        info['name'] = reponame
> +        if info.get('dir') is not None:
> +            info['prettydir'] = breakable_path(info['dir']) or ''
> +        info['hidden'] = as_bool(info.get('hidden'))
> +        #info['editable'] = editable
> +        info['editable'] = True
> +        if not info.get('alias'):
> +            try:
> +                self.log.debug(reponame)
> +                repos = RepositoryManager(self.env.parent).get_repository(reponame)
> +                youngest_rev = repos.get_youngest_rev()
> +                info['rev'] = youngest_rev
> +                info['display_rev'] = repos.display_rev(youngest_rev)
> +            except Exception:
> +                pass
> +        return info
>   
>       def render_admin_panel(self, req, category, page, path_info):
>           if not isinstance(self.env, ProductEnvironment):
> @@ -420,41 +440,143 @@ class ProductRepositoryAdminPanel(ReplacementComponent, trac.versioncontrol.admi
>                   req, category, page, path_info)
>   
>           req.perm.require('VERSIONCONTROL_ADMIN')
> -        db_provider = self.env[DbRepositoryProvider]
> -
> -        if req.method == 'POST' and db_provider:
> -            if req.args.get('remove'):
> -                repolist = req.args.get('sel')
> -                if repolist:
> -                    if isinstance(repolist, basestring):
> -                        repolist = [repolist, ]
> -                    for reponame in repolist:
> -                        db_provider.unlink_product(reponame)
> -            elif req.args.get('addlink') is not None and db_provider:
> -                reponame = req.args.get('repository')
> -                db_provider.link_product(reponame)
> -            req.redirect(req.href.admin(category, page))
> -
> -        # Retrieve info for all product repositories
> -        rm_product = RepositoryManager(self.env)
> -        rm_product.reload_repositories()
> -        all_product_repos = rm_product.get_all_repositories()
> -        repositories = dict((reponame, self._extend_info(
> -                                reponame, info.copy(), True))
> -                            for (reponame, info) in
> -                                all_product_repos.iteritems())
> -        types = sorted([''] + rm_product.get_supported_types())
> -
> -        # construct a list of all repositores not linked to this product
>           rm = RepositoryManager(self.env.parent)
>           all_repos = rm.get_all_repositories()
> -        unlinked_repositories = dict([(k, all_repos[k]) for k in
> -            sorted(set(all_repos) - set(all_product_repos))])
> +        db_provider = self.env[DbRepositoryProvider]
> +
> +        if path_info:
> +            # Detail view
> +            self.log.debug(path_info)
> +            reponame = path_info if not is_default(path_info) else ''
> +            info = all_repos.get(reponame)
> +            if info is None:
> +                raise TracError(_("Repository '%(repo)s' not found",
> +                                  repo=path_info))
> +            if req.method == 'POST':
> +                if req.args.get('cancel'):
> +                    req.redirect(req.href.admin(category, page))
> +
> +                elif db_provider and req.args.get('save'):
> +                    # Modify repository
> +                    changes = {}
> +                    for field in db_provider.repository_attrs:
> +                        value = normalize_whitespace(req.args.get(field))
> +                        if (value is not None or field == 'hidden') \
> +                                and value != info.get(field):
> +                            changes[field] = value
> +                    if 'dir' in changes \
> +                            and not self._check_dir(req, changes['dir']):
> +                        changes = {}
> +                    if changes:
> +                        db_provider.modify_repository(reponame, changes)
> +                        add_notice(req, _('Your changes have been saved.'))
> +                    name = req.args.get('name')
> +                    resync = tag.tt('trac-admin $ENV repository resync "%s"'
> +                                    % (name or '(default)'))
> +                    if 'dir' in changes:
> +                        msg = tag_('You should now run %(resync)s to '
> +                                   'synchronize Trac with the repository.',
> +                                   resync=resync)
> +                        add_notice(req, msg)
> +                    elif 'type' in changes:
> +                        msg = tag_('You may have to run %(resync)s to '
> +                                   'synchronize Trac with the repository.',
> +                                   resync=resync)
> +                        add_notice(req, msg)
> +                    if name and name != path_info and not 'alias' in info:
> +                        cset_added = tag.tt('trac-admin $ENV changeset '
> +                                            'added "%s" $REV'
> +                                            % (name or '(default)'))
> +                        msg = tag_('You will need to update your post-commit '
> +                                   'hook to call %(cset_added)s with the new '
> +                                   'repository name.', cset_added=cset_added)
> +                        add_notice(req, msg)
> +                    if changes:
> +                        req.redirect(req.href.admin(category, page))
> +
> +            Chrome(self.env).add_wiki_toolbars(req)
> +            data = {'view': 'detail', 'reponame': reponame}
>   
> -        data = {'types': types, 'default_type': rm_product.repository_type,
> -                'repositories': repositories,
> -                'unlinked_repositories': unlinked_repositories}
> -        return 'repository_links.html', data
> +        else:
> +            if req.method == 'POST':
> +                # Add a repository
> +                if db_provider and req.args.get('add_repos'):
> +                    name = req.args.get('name')
> +                    type_ = req.args.get('type')
> +                    # Avoid errors when copy/pasting paths
> +                    dir = normalize_whitespace(req.args.get('dir', ''))
> +                    if name is None or type_ is None or not dir:
> +                        add_warning(req, _('Missing arguments to add a '
> +                                            'repository.'))
> +                    elif self._check_dir(req, dir):
> +                        db_provider.add_repository(name, dir, type_)
> +                        name = name or '(default)'
> +                        add_notice(req, _('The repository "%(name)s" has been '
> +                                          'added.', name=name))
> +                        resync = tag.tt('trac-admin $ENV repository resync '
> +                                        '"%s"' % name)
> +                        msg = tag_('You should now run %(resync)s to '
> +                                   'synchronize Trac with the repository.',
> +                                    resync=resync)
> +                        add_notice(req, msg)
> +                        cset_added = tag.tt('trac-admin $ENV changeset '
> +                                            'added "%s" $REV' % name)
> +                        msg = tag_('You should also set up a post-commit hook '
> +                                   'on the repository to call %(cset_added)s '
> +                                   'for each committed changeset.',
> +                                   cset_added=cset_added)
> +                        add_notice(req, msg)
> +                        req.redirect(req.href.admin(category, page))
> +
> +                # Add a repository alias
> +                elif db_provider and req.args.get('add_alias'):
> +                    name = req.args.get('name')
> +                    alias = req.args.get('alias')
> +                    if name is not None and alias is not None:
> +                        db_provider.add_alias(name, alias)
> +                        add_notice(req, _('The alias "%(name)s" has been '
> +                                          'added.', name=name or '(default)'))
> +                        req.redirect(req.href.admin(category, page))
> +                    add_warning(req, _('Missing arguments to add an '
> +                                       'alias.'))
> +
> +                # Refresh the list of repositories
> +                elif req.args.get('refresh'):
> +                    req.redirect(req.href.admin(category, page))
> +
> +                # Remove repositories
> +                elif db_provider and req.args.get('remove'):
> +                    sel = req.args.getlist('sel')
> +                    if sel:
> +                        for name in sel:
> +                            db_provider.remove_repository(name)
> +                        add_notice(req, _('The selected repositories have '
> +                                          'been removed.'))
> +                        req.redirect(req.href.admin(category, page))
> +                    add_warning(req, _('No repositories were selected.'))
> +
> +            data = {'view': 'list'}
> +
> +        # Force repo refresh - should already happen :/
> +        rm.reload_repositories()
> +
> +        #self.log.debug(all_repos)
> +
> +        db_repos = {}
> +        if db_provider is not None:
> +            db_repos = dict(db_provider.get_repositories())
> +
> +        # Prepare common rendering data
> +        repositories = dict((reponame, self._extend_info(reponame, info.copy(),
> +                                                         reponame in db_repos))
> +                            for (reponame, info) in all_repos.iteritems())
> +
> +        #self.log.debug(repositories)
> +        types = sorted([''] + rm.get_supported_types())
> +        data.update({'types': types, 'default_type': rm.repository_type,
> +                     'repositories': repositories})
> +
> +        return 'admin_repositories.html', data
>   
>   trac.versioncontrol.admin.RepositoryAdminPanel = ProductRepositoryAdminPanel
>   
>
>
> bh_p2.patch
>
>
> diff --git a/theme.py b/theme.py
> index 6c7f81b..8686a26 100644
> --- a/theme.py
> +++ b/theme.py
> @@ -443,8 +443,7 @@ class BloodhoundTheme(ThemeBase):
>               bm = self.env[BrowserModule]
>               if bm and not list(bm.get_navigation_items(req)):
>                   yield ('mainnav', 'browser',
> -                       tag.a(_('Browse Source'),
> -                             href=req.href.wiki('TracRepositoryAdmin')))
> +                       tag.a(_('Browse Source'), href=req.href.browser()))
>   
>   
>   class QuickCreateTicketDialog(Component):
>


Re: First installation... my humble review

Posted by Olivier Mauras <ol...@mauras.ch>.
 

On 2013-11-07 17:39, Joachim Dreimann wrote: 

> On 6 November 2013
18:15, Gary Martin <ga...@wandisco.com> wrote:
> 
>> Hi Olivier,
Thanks for bringing all that up! On 06/11/13 15:06, Olivier Mauras
wrote: 
>> 
>>> Hello, I met Gary Martin this morning on #trac and
discussing quickly about the look & feel of Bloodhound i ended up
installing it on a test server. Here's a compilation of the points that
i find "lacking": 1/ Blocking points for a production adoption - indeed
that's only my opinion * Product owner should automatically have admin
rights to this product. I plan on using Bloodhound in a multi product /
multi users setup. I can't imagine it working without delegating the
product admin rights to their owners
>> I am pretty happy with named
owners of products, or more generally resources, being given rights
associated with admin of the resource. It might be useful if the
permissions admin settings for a product reflected that in some sense.

>> 
>>> * Products should be able to have their own source repo If
rights are delegated, I don't see any valid reason for owners to not be
able to add their own repos
>> I think that there may be some good
use-cases for this, particularly when combined with the rights to access
the product's admin pages. For instance it could be that a repository
for a product should never even be known about in another product. The
current solution forcing repository definitions globally and allowing
local links means that the admin pages would have to be restricted to
those that are effectively allowed access to all. It could instead be
delegated. In the end, there is still the current requirement that
repositories have to be on disk and a trac-admin command has to be run
by someone with appropriate access to the server but that doesn't fully
justify the restrictions.
> 
> Tabs should remember the place you were
last within the tab context, so
> that you don't lose context every time
to switch tabs. Currently tabs
> always return you back to the landing
page for that tab, which is a poor
> experience in my opinion.
> 
>
Thanks for your feedback!
> 
> Joe
> 
>>> * Not knowing where you are
This is indeed linked to the previous point, wiki and source pages don't
show the product in breadcrumb
>> I would definitely argue for extending
the breadcrumb to go across all pages for this purpose. 
>> 
>>> 3/ Eye
candy * Source browser CSS may not be the easiest to read All lines to
the same color make it difficult to catch things Age color highlight is
too light and/or too close in colors * There's place on the mainnav tabs
As there's place available and that one can enable timeline and roadmap
from base.ini why not add them to mainnav() to not have then in "More"
tab?
>> The timeline has the virtue that it crosses all aspects of the
system. There is probably a case for having a link, particularly if
there are pages that do not have activity areas.

Please find attached
two patches.
First one grants product owner admin rights to his
products, and change the repository management part to the one used by
global one. This makes the product owner to create/delete/alias
repositories. 
For the moment it's global, it would require some changes
at DB level to make the repositories isolated per product. I think a new
table - "product_repositories" - with a new "DbRepositoryProvider" class
would be the less intrusive...

The second small patch modifies the
theme to get "Source" tab to point to product/<id>/browser instead of
getting the wiki.
This indeed gives a "No node /" error as i haven't yet
found my way in the code that would point the product browser to the
repository.

Sorry for the nasty two patches, i worked on installed app
instead of the code... This is all based on the latest stable
0.7.0.

Hope this will help.

Olivier 
 

Re: First installation... my humble review

Posted by Joachim Dreimann <jo...@wandisco.com>.
On 6 November 2013 18:15, Gary Martin <ga...@wandisco.com> wrote:

> Hi Olivier,
>
> Thanks for bringing all that up!
>
>
>
> On 06/11/13 15:06, Olivier Mauras wrote:
>
>> Hello,
>>
>> I met Gary Martin this morning on #trac and discussing quickly about the
>> look & feel of Bloodhound i ended up installing it on a test server.
>> Here's a compilation of the points that i find "lacking":
>>
>> 1/ Blocking points for a production adoption - indeed that's only my
>> opinion
>>
>>   * Product owner should automatically have admin rights to this product.
>>   I plan on using Bloodhound in a multi product / multi users setup.
>>   I can't imagine it working without delegating the product admin rights
>> to their owners
>>
>
> I am pretty happy with named owners of products, or more generally
> resources, being given rights associated with admin of the resource. It
> might be useful if the permissions admin settings for a product reflected
> that in some sense.
>
>
>
>>   * Products should be able to have their own source repo
>>   If rights are delegated, I don't see any valid reason for owners to not
>> be able to add their own repos
>>
>
> I think that there may be some good use-cases for this, particularly when
> combined with the rights to access the product's admin pages. For instance
> it could be that a repository for a product should never even be known
> about in another product. The current solution forcing repository
> definitions globally and allowing local links means that the admin pages
> would have to be restricted to those that are effectively allowed access to
> all. It could instead be delegated.
>
> In the end, there is still the current requirement that repositories have
> to be on disk and a trac-admin command has to be run by someone with
> appropriate access to the server but that doesn't fully justify the
> restrictions.
>
>
>
>> 2/ Confusing points
>>
>>   * Tickets tab being the dashboard
>>   Being a new user, I find it really confusing for the "Tickets" tab for
>> being the dashboard and consequently the only access to products list.
>>   It would be way more intuitive - and faster - to keep the products list
>> dropdown at the left of the breadcrumb.
>>
>
Tabs should remember the place you were last within the tab context, so
that you don't lose context every time to switch tabs. Currently tabs
always return you back to the landing page for that tab, which is a poor
experience in my opinion.

Thanks for your feedback!

Joe


>
>>   * Not knowing where you are
>>   This is indeed linked to the previous point, wiki and source pages
>> don't show the product in breadcrumb
>>
>
> I would definitely argue for extending the breadcrumb to go across all
> pages for this purpose.
>
>
>
>> 3/ Eye candy
>>
>>   * Source browser CSS may not be the easiest to read
>>   All lines to the same color make it difficult to catch things
>>   Age color highlight is too light and/or too close in colors
>>
>>   * There's place on the mainnav tabs
>>   As there's place available and that one can enable timeline and roadmap
>> from base.ini why not add them to mainnav() to not have then in "More" tab?
>>
>
> The timeline has the virtue that it crosses all aspects of the system.
> There is probably a case for having a link, particularly if there are pages
> that do not have activity areas.
>
>
>
>> 4/ Graphic glitch
>>
>>   https://bh-demo1.apache.org/products/%40/roadmap
>>   As you can see here milestone bars are not displaying correctly.
>>
>
> There is an argument for dashboards to be replacements for the roadmap.
> Dashboards will provide more information and they are not yet available in
> an equivalent style. This might be enough of an argument to get the roadmap
> up to scratch as, at least in a sense, it is a little more focused a view
> than the dashboard provides.
>
> Cheers,
>     Gary
>

Re: First installation... my humble review

Posted by Gary Martin <ga...@wandisco.com>.
Hi Olivier,

Thanks for bringing all that up!


On 06/11/13 15:06, Olivier Mauras wrote:
> Hello,
>
> I met Gary Martin this morning on #trac and discussing quickly about 
> the look & feel of Bloodhound i ended up installing it on a test server.
> Here's a compilation of the points that i find "lacking":
>
> 1/ Blocking points for a production adoption - indeed that's only my 
> opinion
>
>   * Product owner should automatically have admin rights to this product.
>   I plan on using Bloodhound in a multi product / multi users setup.
>   I can't imagine it working without delegating the product admin 
> rights to their owners

I am pretty happy with named owners of products, or more generally 
resources, being given rights associated with admin of the resource. It 
might be useful if the permissions admin settings for a product 
reflected that in some sense.

>
>   * Products should be able to have their own source repo
>   If rights are delegated, I don't see any valid reason for owners to 
> not be able to add their own repos

I think that there may be some good use-cases for this, particularly 
when combined with the rights to access the product's admin pages. For 
instance it could be that a repository for a product should never even 
be known about in another product. The current solution forcing 
repository definitions globally and allowing local links means that the 
admin pages would have to be restricted to those that are effectively 
allowed access to all. It could instead be delegated.

In the end, there is still the current requirement that repositories 
have to be on disk and a trac-admin command has to be run by someone 
with appropriate access to the server but that doesn't fully justify the 
restrictions.

>
> 2/ Confusing points
>
>   * Tickets tab being the dashboard
>   Being a new user, I find it really confusing for the "Tickets" tab 
> for being the dashboard and consequently the only access to products 
> list.
>   It would be way more intuitive - and faster - to keep the products 
> list dropdown at the left of the breadcrumb.
>
>   * Not knowing where you are
>   This is indeed linked to the previous point, wiki and source pages 
> don't show the product in breadcrumb

I would definitely argue for extending the breadcrumb to go across all 
pages for this purpose.

>
> 3/ Eye candy
>
>   * Source browser CSS may not be the easiest to read
>   All lines to the same color make it difficult to catch things
>   Age color highlight is too light and/or too close in colors
>
>   * There's place on the mainnav tabs
>   As there's place available and that one can enable timeline and 
> roadmap from base.ini why not add them to mainnav() to not have then 
> in "More" tab?

The timeline has the virtue that it crosses all aspects of the system. 
There is probably a case for having a link, particularly if there are 
pages that do not have activity areas.

>
> 4/ Graphic glitch
>
>   https://bh-demo1.apache.org/products/%40/roadmap
>   As you can see here milestone bars are not displaying correctly.

There is an argument for dashboards to be replacements for the roadmap. 
Dashboards will provide more information and they are not yet available 
in an equivalent style. This might be enough of an argument to get the 
roadmap up to scratch as, at least in a sense, it is a little more 
focused a view than the dashboard provides.

Cheers,
     Gary

Re: First installation... my humble review

Posted by Olivier Mauras <ol...@mauras.ch>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/14/2013 12:44 PM, Gary Martin wrote:
> On 14/11/13 07:04, Olivier Mauras wrote:
>> I thought about all this. To be less intrusive and still support the way
>> BH was intended to, how about:
>> - - Keep the repository creation on global admin side
>> - - Add option in repository admin page to enable it or not for a given
>> product - Could be enabled for more than one repository
>> - - Product would see only its enabled repositories
>>
>> This would make much less modifications to BH core. How does that sound?
>>
>>
>> Olivier
>
> Yes, I think this helps as it focuses on the more important part of
the proposal.
>
> It would also be good to be able to choose between enabling for all
products, and restricting to a set. We will probably also want to either
force the choice when a repository is added or to severely restrict
access until a choice is made.
>
> Cheers,
>     Gary
That's what i had in mind :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJShLkJAAoJEJXQwVHPrdN8IhgP/jaOZ9J4pu5TmVyxBIQwdKWH
VeWVF5FVqtKzkF5XwRcWrSiuMXwM0JcQ1N7TNak0brlloyFiv99LfWF8L1N69bvl
MLbMQ8+8GS9DTwbWrqOpRhW6ynoZ2yaaOPS1wC6oCceQrK6iN9PUX/SMU78dbyQT
Ey+RFmbQqD9FYr5d9GtPpbMSnI2PPjzrUs53SY9LS2M4H25uOLzJ1Qd2AZMSl4qk
pQzjPTilWS8Ecc7IC58DyGxdhLSsAzH0R8m0pcKFwFUBZeSVQNe4JdeTp9rvcRnv
xlS9x1z67xrBWsV2FeoP1ok1Yx1gey4Lb+37I6VpKuLZX/h5EGe8fOvvy6ddhFeR
nZS+Z1xeoX++ubxHS4xZdAUllBlqAAute1qHdXUHw2mUVldn5WTWB8SdNPl3FjXm
3ChtTtoqhLa47SJKLQg3JOZVb6mpXXuVq8/Mlur2poO9f4o8BQKX/jXXCyCB5CKL
Z+RzDTsZ5geWVJvegj5Zutl8RP8ZWtESiLKU1eI6pBCuve8ZDRxRiFYnJUEowrwb
XsgDssBPTjZRgX4xXIAXJZu1Uo9BSD+I4f7bi6sheDDMn9U9vVHySiPExyqrcvUm
VLONLgnB08TJ0/8TcyW9yw3F1xksUmLpTZ1be6eMIRxOSuCUna726oRSZud76ZzE
wvwPAApqDkzuQFd1BaG1
=JfiK
-----END PGP SIGNATURE-----


Re: First installation... my humble review

Posted by Gary Martin <ga...@wandisco.com>.
On 14/11/13 07:04, Olivier Mauras wrote:
> I thought about all this. To be less intrusive and still support the way
> BH was intended to, how about:
> - - Keep the repository creation on global admin side
> - - Add option in repository admin page to enable it or not for a given
> product - Could be enabled for more than one repository
> - - Product would see only its enabled repositories
>
> This would make much less modifications to BH core. How does that sound?
>
>
> Olivier

Yes, I think this helps as it focuses on the more important part of the 
proposal.

It would also be good to be able to choose between enabling for all 
products, and restricting to a set. We will probably also want to either 
force the choice when a repository is added or to severely restrict 
access until a choice is made.

Cheers,
     Gary

Re: First installation... my humble review

Posted by Olivier Mauras <ol...@mauras.ch>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/13/2013 09:13 PM, Olemis Lang wrote:
>
> A final precision on this subject ...
>
> On Wed, Nov 13, 2013 at 3:08 PM, Olemis Lang <olemis@gmail.com
<ma...@gmail.com>> wrote:
>
>
>
>     On Wed, Nov 13, 2013 at 3:44 AM, Olivier Mauras <olivier@mauras.ch
<ma...@mauras.ch>> wrote:
>
>         On 2013-11-12 21:41, Olemis Lang wrote:
>
>             
>
> [...]
>
>
>         ... but yes , there is a reason and it's due to the Trac vs
product admin
>         roles mentioned above . Trac repository connectors operate on
repos cloned
>         in the local file systems (or equivalent ;) therefore adding a
new one
>         happens outside the web site boundaries is more like a task of
site admins
>
>         I don't think this matters much. Even if the product owner
doesn't have access to filesystem this shouldn't prevent him to enter a
path given to him by the "bloodhound server admin"
>
>
>     I do think this matters . I'm not very fond of publishing server
paths (... neither do the admins in my team ;). By establishing a
convention (e.g. /opt/vcs/<vcs_type>/<repos_name> where vcs_type = hg |
svn | git ... ). The product admin wouldn't even need to worry about
server-side filesystem paths ; they'll only need to know the vcs type
and repos name . That's just an example ...
>
>
> This should not be interpreted as a limitation of Bloodhound core but
the lack of a repository provider providing support for such a FS layout ...
> 
>
>
>     In any case the current implementation still implies that only
TRAC_ADMIN users may create new repositories, and always outside web UI
. IMO it would be nice to integrate those operations too in forthcoming
releases for an enhanced user experience . Nonetheless in order to do so
new (or enhanced) entry points will be needed .
>
>
> ... as opposite to this , which requires an upgrade .
>
> --
> Regards,
>
> Olemis - @olemislc
>
I thought about all this. To be less intrusive and still support the way
BH was intended to, how about:
- - Keep the repository creation on global admin side
- - Add option in repository admin page to enable it or not for a given
product - Could be enabled for more than one repository
- - Product would see only its enabled repositories

This would make much less modifications to BH core. How does that sound?


Olivier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJShHYPAAoJEJXQwVHPrdN8NKoP/33cIkTw550QdvldTpAZy7eX
UNOimeK1n2RNRggWpZZ/jN5MDXbRq3nhdMgw7d3Cc/TfJmZa/L+mNk+VFdM9YQz3
i6o3MSaLeDIBMZ7MgbNJWdzk8mpwQDw1oIKhsHXsc8nH05sybY0F//qjt6rgQjbP
+emHzNJOhBUcTJn+5pq+IACYffV/TFBuBa12sW9TcMc4t44mxWn4nC7qvYeTQu2V
9+E14eQH+zKVXSb39yx/gE0RMBS+ugC1OmZ7JL4JdkxztOmDfb4ZSMWohzd9gN1/
Ax4T+W0Otxq/JfN2Aelk5nzVAkf7bBp4b1HVH93HupQAd6dlFkrFEgzBI6S9/p19
IygjwmWrLpros+YeKwWyTtfKWhfsfhzwyOACe6EKz+V/XEUyiJ0lWs7nV3TVFhq3
nFwF4DLtDcynVv5+2xzBrNPlvyNKICVF9YqV2RuH3boqpfXy0K8L3uhDQB6Lo1qt
eaDMO1QKh9zQCNv/8K9+018OXggJtv/PcQW9/Lz2i44402QH4Tc7xWt7FQCA7Mh1
dmwnlBX3e083YJAIajdp0dwHtx7KI8aP8mse5e6u44aD3MmcaFPkS9sNKZeAt0sv
9tCUdqUkACCz2ATN1OIgt+TN5dRz0QKgOgEm35buFIqrMeh/xleW+Quf5STPufuW
WuO/e54IomJ0MYPZhF2u
=ypqy
-----END PGP SIGNATURE-----


Re: First installation... my humble review

Posted by Olemis Lang <ol...@gmail.com>.
A final precision on this subject ...

On Wed, Nov 13, 2013 at 3:08 PM, Olemis Lang <ol...@gmail.com> wrote:

>
>
> On Wed, Nov 13, 2013 at 3:44 AM, Olivier Mauras <ol...@mauras.ch> wrote:
>
>> On 2013-11-12 21:41, Olemis Lang wrote:
>>
>>>
>>
>> [...]

>
>  ... but yes , there is a reason and it's due to the Trac vs product admin
>> roles mentioned above . Trac repository connectors operate on repos cloned
>> in the local file systems (or equivalent ;) therefore adding a new one
>> happens outside the web site boundaries is more like a task of site admins
>>
>>  I don't think this matters much. Even if the product owner doesn't have
>> access to filesystem this shouldn't prevent him to enter a path given to
>> him by the "bloodhound server admin"
>>
>
> I do think this matters . I'm not very fond of publishing server paths
> (... neither do the admins in my team ;). By establishing a convention
> (e.g. /opt/vcs/<vcs_type>/<repos_name> where vcs_type = hg | svn | git ...
> ). The product admin wouldn't even need to worry about server-side
> filesystem paths ; they'll only need to know the vcs type and repos name .
> That's just an example ...
>

This should not be interpreted as a limitation of Bloodhound core but the
lack of a repository provider providing support for such a FS layout ...


>
> In any case the current implementation still implies that only TRAC_ADMIN
> users may create new repositories, and always outside web UI . IMO it would
> be nice to integrate those operations too in forthcoming releases for an
> enhanced user experience . Nonetheless in order to do so new (or enhanced)
> entry points will be needed .
>

... as opposite to this , which requires an upgrade .

-- 
Regards,

Olemis - @olemislc

Re: First installation... my humble review

Posted by Olemis Lang <ol...@gmail.com>.
On Wed, Nov 13, 2013 at 3:44 AM, Olivier Mauras <ol...@mauras.ch> wrote:

> On 2013-11-12 21:41, Olemis Lang wrote:
>
>> Unless I missed something or a regression has been introduced in /trunk
>> that's exactly the case . This is enforced by the multi-product
>> permission
>> policy [1]_
>>
>>  I join you a screenshot that shows that it's not.
>

hmmm ... I did not received the screenshot .


> And to my understanding it is because of not returning the handler if "not
> req.perm.has_permission('TRAC_ADMIN')" in the product admin.
> By removing this check product panel admin becomes available to the owner
> _without_ giving access to other admin panels. At least that's what my
> tests are showing :)


If you please could write a test case for this and show that it fails,
that'd be nice to follow from there .


>  Nevertheless there's a difference between Trac and product admin role .
>> The
>> former are site admins , i.e. they have access to the file system , sudo
>> etc ... whereas the later only manage product resources e.g. tickets ,
>> wiki
>> , ... If you could list all the instances we fail at doing so we'll be
>> looking forward to improve them asap
>>
>>  I see the repositories as a product ressource
>

I do not (generally speaking ;) , indeed the same repository may be shared
and connected to multiple products , which is the case for e.g. library
code . For instance , Bloodhound mirror is available in
blood-hound.netenvironment and it actually does not belong in any
product , but should be
linked to all products because, in the end, all packages developed in there
depend upon BH core .


>  they can ...
>>
>>  Not really they can only link to a globally available repository...
> which beats the product isolation.


yes and no ... depends on your viewpoint and use cases. If you mean that
there's room for enhancements , yes , definitely ; but repositories

 ... but yes , there is a reason and it's due to the Trac vs product admin
> roles mentioned above . Trac repository connectors operate on repos cloned
> in the local file systems (or equivalent ;) therefore adding a new one
> happens outside the web site boundaries is more like a task of site admins
>
>  I don't think this matters much. Even if the product owner doesn't have
> access to filesystem this shouldn't prevent him to enter a path given to
> him by the "bloodhound server admin"
>

I do think this matters . I'm not very fond of publishing server paths (...
neither do the admins in my team ;). By establishing a convention (e.g.
/opt/vcs/<vcs_type>/<repos_name> where vcs_type = hg | svn | git ... ). The
product admin wouldn't even need to worry about server-side filesystem
paths ; they'll only need to know the vcs type and repos name . That's just
an example ...

In any case the current implementation still implies that only TRAC_ADMIN
users may create new repositories, and always outside web UI . IMO it would
be nice to integrate those operations too in forthcoming releases for an
enhanced user experience . Nonetheless in order to do so new (or enhanced)
entry points will be needed .

-- 
Regards,

Olemis - @olemislc

Re: First installation... my humble review

Posted by Joachim Dreimann <jo...@wandisco.com>.
On 13 November 2013 08:44, Olivier Mauras <ol...@mauras.ch> wrote:

> On 2013-11-12 21:41, Olemis Lang wrote:
>
>> Unless I missed something or a regression has been introduced in /trunk
>> that's exactly the case . This is enforced by the multi-product
>> permission
>> policy [1]_
>>
>>  I join you a screenshot that shows that it's not.
> And to my understanding it is because of not returning the handler if "not
> req.perm.has_permission('TRAC_ADMIN')" in the product admin.
> By removing this check product panel admin becomes available to the owner
> _without_ giving access to other admin panels. At least that's what my
> tests are showing :)
>
>
>  Nevertheless there's a difference between Trac and product admin role .
>> The
>> former are site admins , i.e. they have access to the file system , sudo
>> etc ... whereas the later only manage product resources e.g. tickets ,
>> wiki
>> , ... If you could list all the instances we fail at doing so we'll be
>> looking forward to improve them asap
>>
>>  I see the repositories as a product ressource
>

Agreed, I think that's common in production environments.

- Joe


>
>  they can ...
>>
>>  Not really they can only link to a globally available repository...
> which beats the product isolation.
>
>
>  ... but yes , there is a reason and it's due to the Trac vs product admin
>> roles mentioned above . Trac repository connectors operate on repos cloned
>> in the local file systems (or equivalent ;) therefore adding a new one
>> happens outside the web site boundaries is more like a task of site admins
>>
>>  I don't think this matters much. Even if the product owner doesn't have
> access to filesystem this shouldn't prevent him to enter a path given to
> him by the "bloodhound server admin"
>
> Regards,
> Olivier
>

Re: First installation... my humble review

Posted by Olivier Mauras <ol...@mauras.ch>.
On 2013-11-12 21:41, Olemis Lang wrote:
> Unless I missed something or a regression has been introduced in 
> /trunk
> that's exactly the case . This is enforced by the multi-product
> permission
> policy [1]_
>
I join you a screenshot that shows that it's not.
And to my understanding it is because of not returning the handler if 
"not req.perm.has_permission('TRAC_ADMIN')" in the product admin.
By removing this check product panel admin becomes available to the 
owner _without_ giving access to other admin panels. At least that's 
what my tests are showing :)

> Nevertheless there's a difference between Trac and product admin role 
> . The
> former are site admins , i.e. they have access to the file system , 
> sudo
> etc ... whereas the later only manage product resources e.g. tickets 
> , wiki
> , ... If you could list all the instances we fail at doing so we'll 
> be
> looking forward to improve them asap
>
I see the repositories as a product ressource

> they can ...
>
Not really they can only link to a globally available repository... 
which beats the product isolation.

> ... but yes , there is a reason and it's due to the Trac vs product 
> admin
> roles mentioned above . Trac repository connectors operate on repos 
> cloned
> in the local file systems (or equivalent ;) therefore adding a new 
> one
> happens outside the web site boundaries is more like a task of site 
> admins
>
I don't think this matters much. Even if the product owner doesn't have 
access to filesystem this shouldn't prevent him to enter a path given to 
him by the "bloodhound server admin"

Regards,
Olivier

Re: First installation... my humble review

Posted by Olemis Lang <ol...@gmail.com>.
On Wed, Nov 6, 2013 at 10:06 AM, Olivier Mauras <ol...@core-hosting.net>wrote:

> Hello,
>

:)
Below you'll see my quick comments ... I'll fast forward to recent messages
in this thread .

[...]

>
> Here's a compilation of the points that i find "lacking":
>
> 1/ Blocking points for a production adoption - indeed that's only my
> opinion
>
>   * Product owner should automatically have admin rights to this product.
>

Unless I missed something or a regression has been introduced in /trunk
that's exactly the case . This is enforced by the multi-product permission
policy [1]_

  I plan on using Bloodhound in a multi product / multi users setup.
>   I can't imagine it working without delegating the product admin rights
> to their owners
>

Nevertheless there's a difference between Trac and product admin role . The
former are site admins , i.e. they have access to the file system , sudo
etc ... whereas the later only manage product resources e.g. tickets , wiki
, ... If you could list all the instances we fail at doing so we'll be
looking forward to improve them asap


>
>   * Products should be able to have their own source repo
>

they can ...


>   If rights are delegated, I don't see any valid reason for owners to not
> be able to add their own repos
>

... but yes , there is a reason and it's due to the Trac vs product admin
roles mentioned above . Trac repository connectors operate on repos cloned
in the local file systems (or equivalent ;) therefore adding a new one
happens outside the web site boundaries is more like a task of site admins
.

Of course , I see your point and you are right . The current implementation
has to be improved . Nonetheless AFAIK there are no interfaces in Trac
providing entry points for creating / forking new repos (which is a
backend-specific task ...)


> 2/ Confusing points
>
>   * Tickets tab being the dashboard
>   Being a new user, I find it really confusing for the "Tickets" tab for
> being the dashboard and consequently the only access to products list.
>   It would be way more intuitive - and faster - to keep the products list
> dropdown at the left of the breadcrumb.
>
>   * Not knowing where you are
>   This is indeed linked to the previous point, wiki and source pages don't
> show the product in breadcrumb
>

This depends ... in deployments based on sub-domains , there's no strong
reason to highlight an specific product context .That might be helpful
though .


>
> 3/ Eye candy
>
>   * Source browser CSS may not be the easiest to read
>   All lines to the same color make it difficult to catch things
>   Age color highlight is too light and/or too close in colors
>
>   * There's place on the mainnav tabs
>   As there's place available and that one can enable timeline and roadmap
> from base.ini why not add them to mainnav() to not have then in "More" tab?
>

They should also fit in screen for smartphones and tablets ...


>
> 4/ Graphic glitch
>
>   https://bh-demo1.apache.org/products/%40/roadmap
>   As you can see here milestone bars are not displaying correctly.
>

AFAICR the roadmap view has been phased out , cmiiw .

[...]

.. [1]
https://issues.apache.org/bloodhound/browser/trunk/bloodhound_multiproduct/multiproduct/perm.py#L47

-- 
Regards,

Olemis - @olemislc

Re: First installation... my humble review

Posted by Ryan Ollos <ry...@wandisco.com>.
On Wed, Nov 6, 2013 at 8:54 AM, Olivier Mauras <ol...@mauras.ch> wrote:

> [..]
> Syntax
> highlighting is ok, i'm talking about the "inside directory"
> browsing.
> Having all lines to the same colors makes it less easier to
> catch things when browsing a big file/directory
> list.
> http://trac.edgewall.org/browser [2] for example default trac
> alternate line colors.
>

Thanks for the suggestion. It was added in http://svn.apache.org/r1545216

Re: First installation... my humble review

Posted by Olivier Mauras <ol...@mauras.ch>.
 

On 2013-11-06 18:43, Ryan Ollos wrote: 

> On Wed, Nov 6, 2013 at
7:06 AM, Olivier Mauras <ol...@core-hosting.net>wrote:
> 
>> Hello, I
met Gary Martin this morning on #trac and discussing quickly about the
look & feel of Bloodhound i ended up installing it on a test server.
Here's a compilation of the points that i find "lacking": 1/ Blocking
points for a production adoption - indeed that's only my opinion *
Product owner should automatically have admin rights to this product. I
plan on using Bloodhound in a multi product / multi users setup. I can't
imagine it working without delegating the product admin rights to their
owners * Products should be able to have their own source repo If rights
are delegated, I don't see any valid reason for owners to not be able to
add their own repos 2/ Confusing points * Tickets tab being the
dashboard Being a new user, I find it really confusing for the "Tickets"
tab for being the dashboard and consequently the only access to products
list. It would be way more intuitive - and faster - to keep the products
list dropdown at the left of the breadcrumb. * Not knowing where you are
This is indeed linked to the previous point, wiki and source pages don't
show the product in breadcrumb 3/ Eye candy * Source browser CSS may not
be the easiest to read All lines to the same color make it difficult to
catch things Age color highlight is too light and/or too close in colors
* There's place on the mainnav tabs As there's place available and that
one can enable timeline and roadmap from base.ini why not add them to
mainnav() to not have then in "More" tab? 4/ Graphic glitch
https://bh-demo1.apache.org/products/%40/roadmap [1] As you can see here
milestone bars are not displaying correctly. I think that's all I have
for now, thanks for taking time to read me. Olivier
> 
> Thank you for
the feedback.
> * Your points about the navigation being confusing are
relate-able. We
> need to improve in this area.
> * You say that all
lines in the source browser are the same color. This
> could be due to
Pygments not being installed correctly. Do you have a
> "Syntax
Highlighting" tab on the prefs page (/prefs/pygments)? It could
> also
be due to the mimetype on a file being set to text/plain. The mimetype
>
should be autodiscovered for common file extensions if not set on the
file.
> * I agree that we should offer more flexibility for users to
customize the
> mainnav. This will be improved in #636.
> * The roadmap
needs a Bootstrap template, which is why it is not visually
> appealing.
Some would say that the Roadmap is replaced by the Dashboard,
> but I
think we should make the Roadmap usable for those that choose to use
>
it.
> 
> https://issues.apache.org/bloodhound/ticket/636

Syntax
highlighting is ok, i'm talking about the "inside directory"
browsing.
Having all lines to the same colors makes it less easier to
catch things when browsing a big file/directory
list.
http://trac.edgewall.org/browser [2] for example default trac
alternate line colors. 
 

Links:
------
[1]
https://bh-demo1.apache.org/products/%40/roadmap
[2]
http://trac.edgewall.org/browser

Re: First installation... my humble review

Posted by Ryan Ollos <ry...@wandisco.com>.
On Wed, Nov 6, 2013 at 7:06 AM, Olivier Mauras <ol...@core-hosting.net>wrote:

> Hello,
>
> I met Gary Martin this morning on #trac and discussing quickly about the
> look & feel of Bloodhound i ended up installing it on a test server.
> Here's a compilation of the points that i find "lacking":
>
> 1/ Blocking points for a production adoption - indeed that's only my
> opinion
>
>   * Product owner should automatically have admin rights to this product.
>   I plan on using Bloodhound in a multi product / multi users setup.
>   I can't imagine it working without delegating the product admin rights
> to their owners
>
>   * Products should be able to have their own source repo
>   If rights are delegated, I don't see any valid reason for owners to not
> be able to add their own repos
>
> 2/ Confusing points
>
>   * Tickets tab being the dashboard
>   Being a new user, I find it really confusing for the "Tickets" tab for
> being the dashboard and consequently the only access to products list.
>   It would be way more intuitive - and faster - to keep the products list
> dropdown at the left of the breadcrumb.
>
>   * Not knowing where you are
>   This is indeed linked to the previous point, wiki and source pages don't
> show the product in breadcrumb
>
> 3/ Eye candy
>
>   * Source browser CSS may not be the easiest to read
>   All lines to the same color make it difficult to catch things
>   Age color highlight is too light and/or too close in colors
>
>   * There's place on the mainnav tabs
>   As there's place available and that one can enable timeline and roadmap
> from base.ini why not add them to mainnav() to not have then in "More" tab?
>
> 4/ Graphic glitch
>
>   https://bh-demo1.apache.org/products/%40/roadmap
>   As you can see here milestone bars are not displaying correctly.
>
>
>
> I think that's all I have for now, thanks for taking time to read me.
>
> Olivier
>

Thank you for the feedback.
 * Your points about the navigation being confusing are relate-able. We
need to improve in this area.
 * You say that all lines in the source browser are the same color. This
could be due to Pygments not being installed correctly. Do you have a
"Syntax Highlighting" tab on the prefs page (/prefs/pygments)? It could
also be due to the mimetype on a file being set to text/plain. The mimetype
should be autodiscovered for common file extensions if not set on the file.
 * I agree that we should offer more flexibility for users to customize the
mainnav. This will be improved in #636.
 * The roadmap needs a Bootstrap template, which is why it is not visually
appealing. Some would say that the Roadmap is replaced by the Dashboard,
but I think we should make the Roadmap usable for those that choose to use
it.

https://issues.apache.org/bloodhound/ticket/636