[patch] MYPAGE_VIEW not registered properly, so doesn't show up in permission list

[dclark]

After putting the TracMyPage.py file in the plugins directory and restarting apache, trac does not seem to know about the "MYPAGE_VIEW" permission that the plugin needs in order to show up.

Workaround if you are fine with all users seeing the nav item is below; it just removes any reliance on permissions.

This is under trac v0.11.3.

--- /tmp/TracMyPage.py.orig     2009-03-07 06:06:08.000000000 -0500
+++ TracMyPage.py       2009-03-07 06:07:19.168416638 -0500
 -4,7 +4,6 @@
 from trac.web import IRequestHandler
 from trac.web.api import IRequestFilter
 from trac.web.chrome import INavigationContributor
-from trac.perm import IPermissionRequestor, IPermissionPolicy
 
 
 class MyPagePlugin(Component):
 -15,8 +14,7 @@
                return 'mypage'
 
        def get_navigation_items(self, req):
-               if req.perm.has_permission('MYPAGE_VIEW'):
-                       yield ('mainnav', 'mypage', html.A('My Page', href=req.href.me()))
+               yield ('mainnav', 'mypage', html.A('My Page', href=req.href.me()))
 
 
        # IRequestHandler methods

[Ryan J Ollos]

See also #5668.

[Ryan J Ollos]

From #5668:

I followed the directions to install this plugin as it seems like
it would meet my needs perfectly. However, when I try to add
MYPAGE_VIEW to my user role i get the following error: "Command
Failed: MYPAGE_VIEW is not a valid action"

[Steffen Hoffmann]

Replying to dclark:

After putting the TracMyPage.py file in the plugins directory and restarting apache, trac does not seem to know about the "MYPAGE_VIEW" permission that the plugin needs in order to show up.

Yes, this is the effect, since permission registration has not been done properly. And essentially it's the same as reported later in #5668, as advised before.

Workaround if you are fine with all users seeing the nav item is below; it just removes any reliance on permissions.

Thanks for taking care, but this is certainly just a hack on the hack.

I think a permission is still a valid approach to user(groups) selection, if not anyone should reach that private pages wiki name-space for some reason. I'm sure this could be fixed, an I'll look into it, since I require it to work too.

[Steffen Hoffmann]

Solution: IPermissionRequestor method get_permission_actions() must to return a list. A string 'MYPAGE_VIEW' would have been scattered into pieces creating permission 'A', 'E', 'E', 'G', ...

I'll attach a patch to fix not only this but a bunch of other issues, i.e.:

• options (for trac.ini section ![mypage] have not been registered properly, just requested (self.config.get()/self.config.getbool()), needs Option()/BoolOption() or similar inside the module
• no default value for url, define one with previous option registration
• IRequestHandler method process_request() had return only, if if clause evaluated to True, changed most probably unwanted indentation
• unneeded full import from trac.core, just import required methods for clarity
• setuptools complains about missing module __init__.py, create it
• fix line length (PEP8 requires < 80 chars) for practical reasons, i.e. nice diffs
• other rather small code improvements, that my depend on personal preferences

What ever you do with some/all of these recommendations is up to you. Please contect me directly, if you have bigger questions.

I've tested with Trac 0.12/0.13dev and finally got expected results. I know only too well about the req.redirect & Co as the source of much evil including redirection loops from AccountManagerPlugin's LoginModule, where I learned from kind advice by rblank and others.

I would vote strongly for switching from tab to space indentation. In fact it is much more common in Trac plugins, so reduces chances for mixed indentation (tab+spaces). I had a hard time removing all spaces I did enter casually before noticing tab indentation at all.

[Steffen Hoffmann]

fix IPermissionRequestor method output and more

[dgc]

Replying to hasienda:

Solution: IPermissionRequestor method get_permission_actions() must to return a list. A string 'MYPAGE_VIEW' would have been scattered into pieces creating permission 'A', 'E', 'E', 'G', ...

I'll attach a patch to fix not only this but a bunch of other issues, i.e.:

• options (for trac.ini section ![mypage] have not been registered properly, just requested (self.config.get()/self.config.getbool()), needs Option()/BoolOption() or similar inside the module
• no default value for url, define one with previous option registration
• IRequestHandler method process_request() had return only, if if clause evaluated to True, changed most probably unwanted indentation
• unneeded full import from trac.core, just import required methods for clarity
• setuptools complains about missing module __init__.py, create it
• fix line length (PEP8 requires < 80 chars) for practical reasons, i.e. nice diffs
• other rather small code improvements, that my depend on personal preferences

What ever you do with some/all of these recommendations is up to you. Please contect me directly, if you have bigger questions.

I've tested with Trac 0.12/0.13dev and finally got expected results. I know only too well about the req.redirect & Co as the source of much evil including redirection loops from AccountManagerPlugin's LoginModule, where I learned from kind advice by rblank and others.

I would vote strongly for switching from tab to space indentation. In fact it is much more common in Trac plugins, so reduces chances for mixed indentation (tab+spaces). I had a hard time removing all spaces I did enter casually before noticing tab indentation at all.

[dgc]

Gah. Sorry about the stupid all-quotes reply. I replied then noticed I wasn't logged in, so I logged in. Trac seemed to get al ajaxy and I thought my reply was still in the comment box, but what posted was the default reply text. Not sure whether that's a bug or I stopped paying attention. Anyway, my reply:

Replying to hasienda:

Solution: IPermissionRequestor method get_permission_actions() must to return a list. A string 'MYPAGE_VIEW' would have been scattered into pieces creating permission 'A', 'E', 'E', 'G', ...

Thanks, hasienda, for the contribution. I'll take a look a this when I have a chance. A few comments right away.

I'll attach a patch to fix not only this but a bunch of other issues, i.e.:

• options (for trac.ini section ![mypage] have not been registered properly, just requested (self.config.get()/self.config.getbool()), needs Option()/BoolOption() or similar inside the module

Sounds good.

• no default value for url, define one with previous option registration

Seems fine, no objection.

• IRequestHandler method process_request() had return only, if if clause evaluated to True, changed most probably unwanted indentation

Don't know what you mean about "probably unwarranted indentation". Python rarely allows unnecessary indentation. I will try to look at this but the jumbo patch makes this harder. (A series of feature/bugfix patches would have been easier to review.)

• unneeded full import from trac.core, just import required methods for clarity

I realize that this style is common in Trac plugins, but personally I dislike it and I think that it is much *less* clear. When I see DoSomething(foo) I wonder what the heck DoSomething is and where it came from. When I see trac.core.DoSomething(foo) I know exactly where to look. In other words, this style raises the requirement for familiarity with the Trac API. Since the Trac API documentation is so limited to begin with, this has even higher maintenance cost than it would under normal conditions.

• setuptools complains about missing module __init__.py, create it
• fix line length (PEP8 requires < 80 chars) for practical reasons, i.e. nice diffs

Seems good.

• other rather small code improvements, that my depend on personal preferences

uh, ok. Care to elaborate?

I've tested with Trac 0.12/0.13dev and finally got expected results. I know only too well about the req.redirect & Co as the source of much evil including redirection loops from AccountManagerPlugin's LoginModule, where I learned from kind advice by rblank and others.

Does it work in 0.11? I expect more people continue to run 0.11 than run 0.13dev.

I would vote strongly for switching from tab to space indentation. In fact it is much more common in Trac plugins, so reduces chances for mixed indentation (tab+spaces). I had a hard time removing all spaces I did enter casually before noticing tab indentation at all.

Thanks, but as long as I maintain this code independent of other plugins/software it will be tabs for structural indentation and spaces for alignment (to the right of the indentation). This is how I write all code that isn't under someone else's governance, and at the risk of bikeshedding I believe very strongly in its value.

I don't have a lot of time for Trac development or maintenance though. Trac is not a primary objective for me, and the project I use Trac for is itself a hobby project. If someone wants to take this over I would definitely consider a handoff. I'll wait a little while to decide in case there is any issue with it or contention for a maintainer.

[dgc]

Replying to dgc:

Gah. Sorry about the stupid all-quotes reply. I replied then noticed I wasn't logged in, so I logged in. Trac seemed to get al ajaxy and I thought my reply was still in the comment box, but what posted was the default reply text. Not sure whether that's a bug or I stopped paying attention.

Tested this. I think I just got temporarily stupid.

[Steffen Hoffmann]

Replying to dgc:

• IRequestHandler method process_request() had return only, if if clause evaluated to True, changed most probably unwanted indentation

Don't know what you mean about "probably unwarranted indentation". Python rarely allows unnecessary indentation. I will try to look at this but the jumbo patch makes this harder. (A series of feature/bugfix patches would have been easier to review.)

This may be one of that rare occasions. Anyway, sorry for the "jumbo" patch, but as it was not too much and nicely distributed over the whole file I saw less use in introducing a patch stack, where ofter order matters and you're less free to op-out without rewriting multiple other files. I didn't know in advance about you possible objections, and it was just a kind recommendation, and I already tried hard before to sound kindly facing the number of changes. And it's 0:12 am here now, so I didn't care too much about being as verbose as possible. Trying to be your friend, not your I-show-you-I-know-it-all teacher. ;-) Possibly you're a much better trained coder than me. It's not my profession at all, just self-education by studying Trac and Trac plugin sources and Python docs over a years time by now.

                        req.send_response(307)
                        req.send_header('Location', url)
                        req.end_headers()
-                       return
+               return


        # public methods

• unneeded full import from trac.core, just import required methods for clarity

I realize that this style is common in Trac plugins, but personally I dislike it and I think that it is much *less* clear. When I see DoSomething(foo) I wonder what the heck DoSomething is and where it came from. When I see trac.core.DoSomething(foo) I know exactly where to look. In other words, this style raises the requirement for familiarity with the Trac API. Since the Trac API documentation is so limited to begin with, this has even higher maintenance cost than it would under normal conditions.

But

from trac.core import *

looks not too clear to me as well, and this had been my major concern, not the trac.core.implements() syntax.

• other rather small code improvements, that my depend on personal preferences

uh, ok. Care to elaborate?

Sure, see here

shorter redirect

                url = self.mypage_url(req)
                self.log.debug('process_request: %s' % url)
                if req.path_info == '/me' or req.path_info == '/':
-                       req.send_response(307)
-                       req.send_header('Location', url)
-                       req.end_headers()
+                       req.redirect(req.href(url))
                        return

added, since following check expects string object and would bail out on None

        def mypage_url(self, req):
                url = self.config.get('mypage', 'url')
                self.log.debug('auth: %s' % req.authname)
+               if url is None:
+                       return req.abs_href()
                if '%' in url:
                        url = url % req.authname
                self.log.debug('url: %s' % url)

named arg looks clearer, so was introduced as recommended new url default above too, but that doesn't work with the simple % replacement notation

        # public methods
        def mypage_url(self, req):
                url = self.config.get('mypage', 'url')
-               self.log.debug('auth: %s' % req.authname)
-               if '%' in url:
-                       url = url % req.authname
+               user = req.authname
+               self.log.debug('auth: %s' % user)
+               if '%(user)s' in url:
+                       url = url.replace('%(user)s', user)
                self.log.debug('url: %s' % url)
                return url

I've tested with Trac 0.12/0.13dev and finally got expected results. I know only too well about the req.redirect & Co as the source of much evil including redirection loops from AccountManagerPlugin's LoginModule, where I learned from kind advice by rblank and others.

Does it work in 0.11? I expect more people continue to run 0.11 than run 0.13dev.

Sure, nothing specific to Trac > 0.11 added here, AFAIK. But most probably rjollos will confirm this soon.

I would vote strongly for switching from tab to space indentation. In fact it is much more common in Trac plugins, so reduces chances for mixed indentation (tab+spaces). I had a hard time removing all spaces I did enter casually before noticing tab indentation at all.

Thanks, but as long as I maintain this code independent of other plugins/software it will be tabs for structural indentation and spaces for alignment (to the right of the indentation). This is how I write all code that isn't under someone else's governance, and at the risk of bikeshedding I believe very strongly in its value.

I do appreciate you work. I did exactly recommend for personal preferences. But as explicitly mentioned in my previous comment, you are the maintainer. Only expect to need fixing of erroneously added spaces, even from experienced Trac developers.

I don't have a lot of time for Trac development or maintenance though. Trac is not a primary objective for me, and the project I use Trac for is itself a hobby project. If someone wants to take this over I would definitely consider a handoff. I'll wait a little while to decide in case there is any issue with it or contention for a maintainer.

Would never dream to push you out. You're more than welcome to continue your work. Trac and Trac plugin development is more of a hobby for most of us. The number of badly or unmaintained plugins is already killing 150 % of my spare time. Others will experience similarly "work" load here.

BTW: I've announced this patch in a number of other open tickets as well, since I feel you'll get rid of most, if not all of redirection nastiness with it. But YMMV, this is not easy stuff by no means.

[Steffen Hoffmann]

Did I mention? Thanks for noticing/handling/taking care about my patch in the first place.

Average patch/ticket lifetime on t-h.o is measured in years these days. Feels good to meet active, responsive maintainers like you.

[Steffen Hoffmann]

Ping. - Will you have some time for review, maybe between the years? I'd love to see some progress here. Thanks for taking care.