Primerize the administration user form. wp/72005#23691
Conversation
dccd794 to
b1a1568
Compare
11aa8ee to
55474a1
Compare
b1a1568 to
9731068
Compare
55474a1 to
211e72c
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9731068 to
43e1795
Compare
|
Warning Flaky specs
|
32774c9 to
892a3db
Compare
HDinger
left a comment
There was a problem hiding this comment.
I admit, that it took my some time to get through this. At first, I thought the code is overly complex but then I realized the complex requirements these pages have and the code actually helped to understand all of that. Kudos @dfriquet for fighting through all of this 🙇 👏 The result looks so much better than before and is a really nice improvement 🌟 👍
I left some comments, feel free to reach out if you don't know how to solve things, especially with spacings and Primer stuff. I'd happy to help.
| form.group(hidden: true, data: { "admin--users-target": "authSourceFields" }) do |group| | ||
| group.text_field(name: :login, | ||
| label: User.human_attribute_name(:login), | ||
| required: true, | ||
| input_width: :medium) | ||
| end | ||
| end |
There was a problem hiding this comment.
Does this have to be a group? The Primer form DSL also offers f.hidden for hidden fields..
There was a problem hiding this comment.
Yes, it has to be a group because it is revealed/hidden by a Stimulus controller. I added a comment to explicit it.
| .avatars--current-gravatar, | ||
| .avatars--current-local-avatar | ||
| .avatars--layout | ||
| display: flex |
There was a problem hiding this comment.
In figma, the image was below the description text. Don't know how much of an issue that really is I admit 🤷 I like the text to the image as well 😅
There was a problem hiding this comment.
With that few text lines, I like the condensed version side-by-side instead of the really long line of text above the narrow image. I can send a screenshot to a designer, maybe?
There was a problem hiding this comment.
Yeah, we could bring it to the UX/UI open door. But it is definitely not a blocker for me
|
Btw, this also fixes https://community.openproject.org/wp/DREAM-559 👏 |
Merge AuthenticationSourceForm and PasswordForm into a single AuthenticationForm that renders every authentication scheme -- external provider, LDAP source select, password settings and the cannot-login notice -- inside one titled "Authentication" fieldset, replacing the blocks previously rendered outside the form.
- Guard the external identity URL display against a nil value. - Stop the avatar hint from mentioning Gravatar when it is disabled. - Show the user's status as a read-only line in the Account section instead of folding it into the section title. - Reword the untitled custom-field section label to "User attributes". - Drop the redundant "Create and continue" button (and the now-dead continue redirect in the controller). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dfriquet
left a comment
There was a problem hiding this comment.
Only one open point remains, quite tricky…
| form.group(hidden: true, data: { "admin--users-target": "authSourceFields" }) do |group| | ||
| group.text_field(name: :login, | ||
| label: User.human_attribute_name(:login), | ||
| required: true, | ||
| input_width: :medium) | ||
| end | ||
| end |
There was a problem hiding this comment.
Yes, it has to be a group because it is revealed/hidden by a Stimulus controller. I added a comment to explicit it.
| .avatars--current-gravatar, | ||
| .avatars--current-local-avatar | ||
| .avatars--layout | ||
| display: flex |
There was a problem hiding this comment.
With that few text lines, I like the condensed version side-by-side instead of the really long line of text above the narrow image. I can send a screenshot to a designer, maybe?
|
Warning Flaky specs
|
Does this work for you?
|
e75c7f2 to
34d635c
Compare
|
@HDinger And finally a last one.
|
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |



Ticket
https://community.openproject.org/wp/OP-19485
What are you trying to accomplish?
Convert the admin user create/edit form from the legacy form helpers to GitHub Primer.
Follow-up of approved PR #23454.
Screenshots
What approach did you choose and why?
Like for the advanced filters, this PR relies on form objects composed via
Primer::Forms::FormList.Users::FormComponentcoordinates the form (modeled onFilters::FilterFormComponent): it receives thesettings_primer_form_withbuilder, assembles aFormListfrom the inner forms it needs, and renders the read-only bits, the plugin hooks and the submit buttons around it;Users::Form::PreferencesForm(time zone, color mode, keyboard shortcuts) renders under the top-level :pref scope, not as an inner form;view_users_primer_formplugin hook for Primer builder and keepview_users_formworking via a TabularFormBuilder rendered in the form (deprecated, only when a listener exists);my/accountpage by tampering the form.Merge checklist