#7436 /auth/preferences cleanup

v1.2.0
closed
nobody
42cc (433)
General
2015-08-20
2014-06-03
No
  • Add a section at the top of readonly information, show:
    • Username: {{username}}
    • <a href="{{user.private_project().url()}}">My profile and my projects</a>
    • <a href="{{user.private_project().url()}}admin/overview">Set avatar icon</a>
  • password_change_form and upload_key_form shouldn't be tied to the theme. Move the forms to a better place (auth something..)
  • Make every part of the page (password_change_form and upload_key_form, Display Name, Page Size, Email, User Messages, etc in a unique jinja block. Instead of "if" statements that check auth.method or the theme, every section on the page should be contained in a block so that a simple template override can be set up for user_prefs.html and any individual block can be made to be empty (or something else)
  • Improve the layout between each section as best you can. And within the email block especially. It seems like everything is just run together. It is extra confusing that there a multiple separate forms with their own save buttons. I think dedicated forms for pwd change and ssh keys makes sense, but it is confusing to have so many forms on the page. I am open to suggestions for the multiple forms issue. Best idea I have so far is to have a <hr> between each form, to visually show the separations. (Ideally would have it all auto-save ajax or all one single form, but those would be big projects).

Related

Tickets: #7432

Discussion

  • Dave Brondsema

    Dave Brondsema - 2014-06-03

    Hm, after looking at the controllers... seeing that auth.py def update has an "if" statement based on auth.method to match the "if" stmt in the template, I think the jinja blocks won't be sufficient. Controllers need to disable features not just the HTML form (so that a sneaky user can't simulate a form submit).

    Setting up blocks to allow for overriding still is nice, but I'm now thinking that we should have config options similar to the existing auth.allow_user_to_disable_account for each of the sections and "if" statements in each jinja block and in the relevant controller methods.

     
  • Igor Bondarenko

    Igor Bondarenko - 2014-06-05

    For now I'm heading for something like this to visually separate forms.

    "Email" part needs to be adjusted yet, especially extremely counterintuitive "Claim Address" & "Save Changes" behavior (I think, one button should handle both cases, but still don't sure exactly how). Or maybe separate "email" and "general settings" (display name, page size, etc) into two forms? But it would require controller refactoring and I don't sure if you up to such changes or want only cosmetic changes like above for now.

     
  • Dave Brondsema

    Dave Brondsema - 2014-06-05

    I like it. I haven't used a fieldset in a long time, but they're good! I think leaving general settings and email together is acceptable. But as you work through the buttons within the Email section, if it really warrants separating email out from the other settings, then feel free to do that.

     
  • Igor Bondarenko

    Igor Bondarenko - 2014-06-06

    Closed #599. je/42cc_7436

    I've left functionality for emails section as is, but changed layout a bit, I think now it's more clear.

    Notes:

    • New option: auth.allow_edit_prefs

    • upload_key_form doesn't work and didn't worked before, since corresponding auth provider methods are not implemented for local provider. Don't sure why it was shown in a first place. Maybe it's better to hide it, then?

    • password_change_form changes password even if you enter invalid old password. It was fixed in [#7432].

    • I've rebased [#7432] to this branch and updated code a bit, since it uses password change form, which was moved.

     

    Related

    Tickets: #7432

  • Igor Bondarenko

    Igor Bondarenko - 2014-06-06
    • status: in-progress --> code-review
     
  • Dave Brondsema

    Dave Brondsema - 2014-06-06
    • status: code-review --> in-progress
    • QA: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2014-06-06
    • auth.allow_edit_prefs should default to True in the code.
    • These were most likely pre-existing, but can you fix them up now too?
      • If you hit "Enter" from the Display Name field (rather than hitting Save below) you get an error.
      • hitting 'Claim New Address' without entering an address ends up with a weird address record. Invalid addresses (i.e. not email address format) are incorrectly permitted too.
    • Yes, the ssh key form should be hidden by default. I think it was intended to be used with this configuration https://forge-allura.apache.org/docs/scm_host_ssh.html Can you add blocks & config options for it, and the password change form, and the user messages section? (And you can remove {% if c.password_change_form %} then too, I think its unnecessary). Default the ssh key form to False and the rest to True.
     
  • Igor Bondarenko

    Igor Bondarenko - 2014-06-12

    Closed #601. Updated je/42cc_7436

     
  • Igor Bondarenko

    Igor Bondarenko - 2014-06-12
    • status: in-progress --> code-review
     
  • Dave Brondsema

    Dave Brondsema - 2014-06-12
    • status: code-review --> closed
    • Milestone: limbo --> forge-jun-13
     
  • Dave Brondsema

    Dave Brondsema - 2015-01-05
    • Milestone: unreleased --> asf_release_1.2.0