Register and "Forgot your password?" links can no longer be enabled

[lkraav]

i know "ugly" is subjective, this ticket is mainly a tracking vehicle for my to-be attempt towards something "better".

sorta "ugly":

sorta "pretty":

all ideas welcome.

browse through ​"good looking login form" google images search perhaps.

[lkraav]

my bad, pressed submit before selecting correct component.

[Steffen Hoffmann]

Patch welcome. Until then I'll keep this with lower priority, since now right now we're able to deliver a seamless replacement for the stock Trac login form, and this is a valuable feature as well.

[lkraav]

my suggestion is to copy ​https://secure.gitorious.org/login with some trac styling. imho trac's look needs an facelift itself.

[Steffen Hoffmann]

I've chosen priority for good measure, but I'm open to suggestions.

Gitorious.org looks nice, may be little too over-styled to my eyes. Especially I like the way to switch between direkt login and OpenID form. But we'll need the functionality first (see #173), a long standing feature request.

To give an impression for my take on a small Trac login re-style, I attach a current​ and future​ screenshot. This is a quick, purely fictional graphical hack done with The GIMP. More suggestions and patches welcome (indeed, real code rules).

[Steffen Hoffmann]

screenshot of current AccountManagerPlugin login, already localized with German header and texts

[Steffen Hoffmann]

fictional screenshot of future login form, done with The GIMP - no real code by now

[Steffen Hoffmann]

The most important improvement to me would be the move of the "Register" link from meta-nav down below the [ Login ] button, where it joins the "Forgot password" link nicely. So there are all the click-able items in one place and especially registration is much better accessible than today.

Despite of a bigger login button this is more or less what I'd implement, if no more suggestions are made.

[lkraav]

your re-design is quite nice already the only thing i'd suggest is to style the input button as well, but then again that might require to re-style all trac's input buttons for consistency.. which i have done in my custom themes :)

[Steffen Hoffmann]

Ok, thanks for the thumbs-up. As mentioned before, the button style is of much too less concern to me now, might even be a wontfix, since this could be done by other means (Trac style hacks).

Push priority back to normal, since now we've got some noteworthy UI improvements.

[Steffen Hoffmann]

I've started to work on this issue some days ago. Following patches attached will show some iterations. Comments welcome.

[Steffen Hoffmann]

1st version: preliminary realisation of central box, still unclean and highly browser-dependend text input position

[Steffen Hoffmann]

BTW: How do you like the slightly shaded background? I've tested with really dark ones as well, but didn't like the big brightness jump then.

I'll need to take extra care for some special configurations, i.e. when moving the "Register" link from meta-nav to the new central box. This is still on my ToDo list, but when the login form isn't used but registration is enabled, the link should remain in the old place.

[lkraav]

hasienda, do you mind attaching a screenshot with your patches? i'm currently in the middle of stuff and it can take a while to apply patches, with a screenshot i could provide some feedback almost immediately.

[Steffen Hoffmann]

1st version: a screenshot from development system

[Steffen Hoffmann]

Replying to lkraav:

hasienda, do you mind attaching a screenshot with your patches? i'm currently in the middle of stuff and it can take a while to apply patches, with a screenshot i could provide some feedback almost immediately.

Thanks for tacking care. Meanwhile I'm struggling with HTML and CSS. Refusing to add something complicated like browser-dependent style I'll continue to experiment a little. Patch code has been tested with Opera9 and FF3.0, but IE6/7/8 still pending. As I said before, especially the position of text input and related labels looks not ideally. Depending on the browser it's little more left or right - text boxes are never ideally centered.

[Steffen Hoffmann]

Replying to lkraav:

![...] with a screenshot i could provide some feedback almost immediately.

Ping. Any comments? Would love to hear from you.

I've already put it in production for some weeks now. No complaints, no applause, no feedback at all - just went through. I this a good sign? Think so. While I made quite slow progress «under the hood» towards a version, that could be considered technically final too, a least the design seem acceptable. So I'll commit it soon.

[Steffen Hoffmann]

(In [9577]) AccountManagerPlugin: Change login form, refs #6821.

While this is quite simple, improvements towards a cleaner UI are always a good thing. Login page CSS style had to go into an own file to separate it and especially prevent rather unique settings like background color from being applied to other pages too. I've even prepared, but not finished yet moving 'Register' link next to the 'Forgot password' link, if AcctMgr's LoginModule is enabled.

[Steffen Hoffmann]

(In [9584]) AccountManagerPlugin: Move 'Register' link next to 'Forgot password' link, closes #6821.

The 'move' will only happen, if both AcctMgr's LoginModule and RegistrationModule are enabled and IPasswordStore is writeable. This is the announced follow-up to changeset [9577].

[dmcr@…]

Actually, the test is backward, and the code refuses to enable registration if the AcctMgr's LoginModule is enabled. Code is currently as follows:

    def get_navigation_items(self, req):
        if not self._enable_check() or LoginModule(self.env).enabled:
            return
        if req.authname == 'anonymous':
            yield 'metanav', 'register', tag.a(_("Register"),
                                               href=req.href.register())

So the function returns if the RegistrationModule is not enabled OR the LoginModule IS enabled, and therefore no registration link is displayed.

Should be:

    def get_navigation_items(self, req):
        if not (self._enable_check() and LoginModule(self.env).enabled):
            return
        if req.authname == 'anonymous':
            yield 'metanav', 'register', tag.a(_("Register"),
                                               href=req.href.register())

The same problem exists with the "Forgot your password?" code:

    def get_navigation_items(self, req):
        if not self.reset_password_enabled or LoginModule(self.env).enabled:
            return
        if req.authname == 'anonymous':
            yield 'metanav', 'reset_password', tag.a(
                _("Forgot your password?"), href=req.href.reset_password())

It needs the same fix.

Dennis

[Steffen Hoffmann]

I love testing of all the changes done recently. But wouldn't it be wiser to not capture this enhancement ticket for a bug report, that most probably will need more discussion than you'll have expected? Anyway, you've already chosen, and I'll not wast my time reverting it. In the future just think it twice, please. :-)

Replying to dmcr@Princeton.EDU:

Actually, the test is backward, and the code refuses to enable registration if the AcctMgr's LoginModule is enabled. Code is currently as follows: ![...] So the function returns if the RegistrationModule is not enabled OR the LoginModule IS enabled, and therefore no registration link is displayed.

Did you spend some time reading about the work done to solve this ticket? So I may have failed in documenting the purpose of this code, that looks "backward" to you - it's actually to really NOT show the "Register" link in the meta-navigation bar, since we aim at showing it in the login box now. At least the first part is done on purpose and I fail to see an error here.

Your proposed fix would just revert to the old behavior. I know, I've done a quite radical change here, but this would be better subject to a discussion on the mailing list first.

The same problem exists with the "Forgot your password?" code: ![...] It needs the same fix.

Sorry, again I feel you failed to see, that this is just one side of moving both to another place. Take a look at the aforementioned changeset to see it all. At best I would accept a report against documentation. I know this is still pending to show up in the wiki, but a reminder is always a good thing, nothing more, right? You're free to sue me, but close this one (provided, there is nothing else to complain about) and open a new ticket, please. ;-)

Nevertheless there is another bug that causes to let the "Register" link appear (inside the new login box) even if registration is disabled. But again this is a different story and I'm prepared to hunt it ASAP.

[dmcr@…]

My apologies! I coopted this ticket because I thought (mistakenly, apparently) that changeset 9584, which closed this ticket, had a bug in it. I wasn't sure of the correct protocol in such a case, and thought reopening the ticket might provide more continuity. So I did actually think about it, but seem to have come to the wrong conclusion.

I also appologize that I did not look closer at the other related changes to that changeset. The self-registration link worked "correctly" (i.e., the old way) on my test server, and then when I installed the plugin on my production server, I got your latest changes, and suddenly, the self-registration link was gone. Not noticing it elsewhere and being under a deadline, I dug into the code, found the line that removed the link, and assumed it was a simple logic error.

I have no objection to moving to a new interface for doing self-registration. So I'll now take a look at the whole picture, and once I can get it to work the new way, I'll close the ticket.

Thanks for the considerate explanation.

Dennis

[Steffen Hoffmann]

Replying to dmcr@Princeton.EDU:

My apologies! ![...]

Np, didn't hurt me. Trying to have continuous subject is certainly a valid reason, and I already wondered, if you had such an idea in mind. Please come up with any other findings at me directly, via tickets or just to discuss. You'll find that I'll try to push AcctMgr forward as good as I can.

[Mitar]

OK, this should really be documented better. I have now spend few hours searching for a reason why there is no Register option in the menu. And especially, this should be optional. I really liked the way it was before and our visitors are also used seeing that option there.

[Steffen Hoffmann]

Replying to mitar:

OK, this should really be documented better.

Sure. But feel free to start it, it's a wiki.

I have now spend few hours searching for a reason why there is no Register option in the menu.

Uh, sorry.

And especially, this should be optional. I really liked the way it was before and our visitors are also used seeing that option there.

So this is a vote to make this optional. Looked like an enhancement, but to more experienced users it is certainly a disruption. OTOH, who is registering that often? I'm still convinced, you'll spot the "Register" link in the central login box easier than before (in the meta-nav), no?

[Mitar]

Hm, then it should be written "Login or Register". How would user know that it has to go to login page to be able to register? And then we are back to the problem that it looks ugly. ;-)

And current login.html is still strange. I am attaching mine which looks very nice to me. ;-)

[Mitar]

I had to gzip it because of the spam checker

[Steffen Hoffmann]

Replying to mitar:

Hm, then it should be written "Login or Register". How would user know that it has to go to login page to be able to register? And then we are back to the problem that it looks ugly. ;-)

As long as it's not the default page, yes, I understand your reasoning. So a duplicated link might be preferred? I'll have to think this over again.

And current login.html is still strange. I am attaching mine which looks very nice to me. ;-)

I do appreciate a discussion about good style, even if we should agree that personal style is always disputable. We might still find something with ergonomically and logically better placement. Just take my code as a proposal that was good enough as long as nobody else did care about this ticket and the idea in general. But be more specific about exactly what looks "strange" to you, please.

So here we go. Let's compete on the best (read: cleanest) UI design in good Trac tradition ("low fuzz, no nerds"). I'm looking forward to even more contributions. Take your time. I've got the impression that maybe it's worth to spend time on some more iterations, and I'll hold back that part from an upcoming release, until we've found some sort of agreement.

BTW, your attached compressed HTML is unreadable here even after decompressing with gunzip. Is this an UTF-8 encoded file? What other magic do I need to apply here? ;-) Why not a plain PNG image?

[Mitar]

OK, sorry, maybe I was really not precise with what I meant by good login page style. ;-)

So what I did was to make it the same as Trac is otherwise designed. For example, a div with "buttons" class around button. I moved "forget link" to "hint" class. I also added one div around hidden field, to make it XHTML valid. By doing this I got a look-and-feel of Trac which is something I see as an improvement. So the layout (hints under the input boxes and not on the right, as it is currently for forget and register) is according to how other Trac forms do it. Probably also error box should be moved to the top, as it is normal for other forms (it was so before, now somebody moved it down).

I do not understand what is wrong with attachment. It works for me. Download raw and then apply gzip -d. And then it opens normally? I attached code because my changes are mostly in the structure. The style is left to Trac to fill in. In this way it is also easier to theme whole Trac installation because by changing CSS the login form also changes accordingly, correctly, in style.

[Mitar]

Or, you can check it ​here live.

[Steffen Hoffmann]

Replying to mitar:

Or, you can check it ​here live.

I see, so that's nothing fancy, just the common style.

I'd really like to open the HTML attachment, but it's still just garbage here. Anyway I'll review your comments above and get back later. Thanks for sharing your ideas.

[Mitar]

Yes, I believe "fancy" stuff should be done with CSS and for whole Trac installation. Not just one plugin. So by default it should meld in nicely with official Trac look&feel. By my opinion.

I am attaching a patch with href HTML attribute changed into hXXXref. So please find&replace.

[Mitar]

Find&replace hXXXref with href

[oxullo@…]

Replying to hasienda:

Replying to mitar:

Hm, then it should be written "Login or Register". How would user know that it has to go to login page to be able to register? And then we are back to the problem that it looks ugly. ;-)

As long as it's not the default page, yes, I understand your reasoning. So a duplicated link might be preferred? I'll have to think this over again.

After naively reopened #8697, I'm moving here to leave my 2c. Having the links on the login form is super fine from my personal standpoint. Anyway I believe that having at least Register repeated on metanav could be an easy way for visitors to see that they could register by their own. This feature is quite important for the site I'm running trac on, that's the reason I saw it as a missing feature and then thought it was a regression issue.

OTOH, who is registering that often? I'm still convinced, you'll spot the "Register" link in the central login box easier than before (in the meta-nav), no?

I agree with mitar: a visitor is not brought to think a site may accept registrations unless s/he is compelled to try any way to access it.

hasienda, thanks for your work and your patience. Very much appreciated.

[Steffen Hoffmann]

I admit I've learned a lot through the discussion here.

A good share of all users may like the Trac style as it is, ancient or not. So it's probably be a better option to enhance current login form template structure, so that a contrib CSS file would be sufficient to actually switch the style towards the current design (or anything else).

[dmcr@…]

I agree that the home page needs something to show that self-registration is enabled. This does not have to be a duplicate link. Just having the URL text state 'Login or Register' instead of just 'Login' could be enough.

But I'm not fussy about the exact way of informing the user. A long as there is *something* that would tell him that registration is a possibility, that is all that is needed I believe. I don't think most users would normally think to click on 'Login' to find the registration link, unless they've been given a hint.

Dennis

[lkraav]

hi hasienda

i implemented AccountManagerPlugin trunk today for a client and noticed AMP css files were giving 404s. it turns out all your get_htdocs_dirs functions return [] for some reason, instea of:

    def get_htdocs_dirs(self):
        """Return the absolute path of a directory containing additional
        static resources (such as images, style sheets, etc).
        """
        return [('acct_mgr', resource_filename(__name__, 'htdocs'))]

patching it up as such myself brought the now awesomely prettified login form back.

[Steffen Hoffmann]

Replying to lkraav:

![...] it turns out all your get_htdocs_dirs functions return [] for some reason

Well I came across this only recently by myself during code review. So I think I know the reason, but not, why only some users are affected (I didn't find this by testing the code for months). The reason for the duplicate definition is most probably my previously too limited understanding of Trac components (here: ITemplateProvider) or just too busy copy-n-paste. Thanks for taking care.

[Steffen Hoffmann]

In [10113]: AccountManagerPlugin: Add missing ITemplateProvider content to LoginModule, refs #6821. (added manually as it didn't appear here automatically for some reason)

[David.Byrne@…]

It appears that the registration_enabled flag isn't being set correctly. I have the RegistrationModule disabled, but the login form still shows the Register link. When I investigated this, I found that the check doesn't seem to be coded correctly. The variable 'registration_enabled' maps to RegistrationModule(self.env)._enable_check(). _enable_check checks to see if the AccountManager supports 'set_password' and verifies that the password store is writable. I think it should at least check to see if the RegistrationModule is enabled similar to how the 'reset_password_enabled' looks at self.env.is_component_enabled(AccountModule) and self.reset_password and self._write_check(), etc.

[Steffen Hoffmann]

Replying to David.Byrne@us.xchanging.com:

![...] I think it should at least check to see if the RegistrationModule is enabled similar to how the 'reset_password_enabled' looks at self.env.is_component_enabled(AccountModule) and self.reset_password and self._write_check(), etc.

Right, but I discovered, that even env.is_component_enabled is not correct/sufficient under all possible circumstances. Another solution is there, but a bit too fresh in my eyes. I'll provide it after some more testing for hidden issues of that changes.

[Steffen Hoffmann]

A minor complication on the way to more flexible styling was, that attributes of the body HTML tag from a Genshi template are overwritten, if includes are used.

      xmlns:xi="http://www.w3.org/2001/XInclude">
  <xi:include href="layout.html" />

To create a CSS selector exactly for login page's body I had to work-around this using jQuery's addClass().

[Steffen Hoffmann]

Replying to mitar:

Yes, I believe "fancy" stuff should be done with CSS and for whole Trac installation. Not just one plugin. So by default it should meld in nicely with official Trac look&feel. By my opinion.

I am attaching a patch with href HTML attribute changed into hXXXref. So please find&replace.

Ok, I finally could open the URL provided. Thanks for sharing your thought regarding customization. I'll largely follow this idea and separate the styles into CSS file in contrib directory. I'm interested to hear from you about the rework published within the next days.

[Mitar]

Great.

[Steffen Hoffmann]

(In [10304]) AccountManagerPlugin: Fix _enable_check() in RegistrationModule refs #6821 and #8663.

The functions name has been misleading, as it has been more like a sibling of _write_check() in AccountModule.

[Steffen Hoffmann]

(In [10306]) AccountManagerPlugin: Do the real check, and compatible to Trac 0.11 too, refs #6821 and #8663.

Plugin components can be activated implicitly, if the code is located in the plugins directory of the enironment. Method ComponentManager.is_component_enabled returns None for such a case, so it's not at all a reliable method to detect component activation.

Special thanks to David Byrne for testing and for critical hints on the issue.

[Steffen Hoffmann]

(In [10307]) AccountManagerPlugin: Revive the classic Trac login form style, refs #6821.

Following user feedback the classic login form is the default again and CSS style definitions for this template are not included but contributed separately.

To get the new-style login form now you need to set login_opt_list = True in your trac.ini and make sure, CSS styles for the login page get loaded, since this is no longer done by AcctMgr itself.

I'm grateful for all the feedback given on the preliminary new-style login form. Thank to all participants. A visible result is, that the "Register" link is no longer moved but copied into the central login form box to double the chance of being recognized by an unaware visitor. Thanks especially to Mitar for some valuable template improvements, i.e. for fixing XHTML compliance of the hidden field in login.html.

[Steffen Hoffmann]

(In [10393]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.

[Steffen Hoffmann]

(In [10395]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.