bloodhound-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Olemis Lang <ole...@gmail.com>
Subject Re: First installation... my humble review
Date Tue, 12 Nov 2013 20:12:46 GMT
On Tue, Nov 12, 2013 at 11:58 AM, Gary Martin <gary.martin@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message