Skip to content

User management fields#2921

Open
brady-lamansky-gtt wants to merge 14 commits into
DiscipleTools:developfrom
brady-lamansky-gtt:user-management-fields
Open

User management fields#2921
brady-lamansky-gtt wants to merge 14 commits into
DiscipleTools:developfrom
brady-lamansky-gtt:user-management-fields

Conversation

@brady-lamansky-gtt

Copy link
Copy Markdown
Contributor

@cairocoder01 This resolves #2847 to replace user management fields w dt-web-components.
Please specifically look at the syntax and formatting of my changes to make sure it's what you're looking for!

@cairocoder01 cairocoder01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start. See the specific code comments, but there are also some functional issues:

  • When using the new user form, the submit doesn't work because it returns an API error
  • When changing fields of an existing user, they don't automatically save like they used to. We need to hook up the code to capture change events and trigger the API requests
  • Those will probably require js changes. Try to find the js code that was specific to the old form fields and remove it as you implement the new code for the components.

Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/user-management.js Outdated
Comment thread dt-users/template-user-management.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/user-management.js
Comment thread dt-users/user-management.js
@cairocoder01

Copy link
Copy Markdown
Collaborator
  • We've lost the spacing in between the fields and the following label. See image below. Green is good, red is bad and needs to match green.
image image

Comment thread dt-users/user-management.js

@cairocoder01 cairocoder01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better.

  • We still need the margin adjustments on the user management page for editing existing users.
  • I found a weird issue when editing users. I set the gender of a user to Female (had been blank), and then when I opened up any other user, it also showed Female even though they should have been blank. I'm having trouble reproducing it again after setting those values.
  • Related to that, as I was trying to reproduce it, I also had a situation where I changed another user to Male from blank, it saved, I closed to modal and then opened the same user, but the gender was blank again. If I refreshed the page, it correctly showed Male. I think whatever js is loading the modal had the original value instead of the updated one.

Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
Comment thread dt-users/template-new-user.php Outdated
@brady-lamansky-gtt

Copy link
Copy Markdown
Contributor Author

Just fixed those bugs! Let me know any other suggestions. For the Gender bug, I realize it was because I had 'select_cannot_be_empty' => true even though the field was supposed to be optional.

@cairocoder01 cairocoder01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed one commit to reset all of the component fields before loading in new values. It was causing an issue where saved icons were still displaying on a second user modal and the gender field wasn't cleared out if the value was "none" and the previous had a value.

As I was looking in the subassigned code, I see there's a feature to check the URL for a query param and preset the subassigned field (/user-management/add-user?contact_id=691). Let's make sure that still works. It happens when you're on a contact, open the Admin Actions dropdown, and select the option to make that contact into a user.

Comment thread dt-users/template-new-user.php
Comment on lines +93 to +101
<div class="user-field">
<?php DT_Components::render_connection( 'subassigned[query]', [
'subassigned[query]' => [
'name' => __( 'Contact to make a user (optional)', 'disciple_tools' ),
'default' => '',
'type' => 'connection',
'post_type' => 'contacts',
]
], [], [ 'placeholder' => __( 'Search multipliers and contacts', 'disciple_tools' ) ] ) ?>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much better code style. However, if I select a contact in this field and save the new user, it still creates a new contact instead of using this value.

Also, we should add 'allowAdd' => false to the params so that users can't add a new contact in the field.

Comment thread dt-users/template-user-management.php
Comment thread dt-users/user-management.js
@github-actions

Copy link
Copy Markdown

Code Review

High

1. Password field rendered as plain text — dt-users/template-new-user.php

The password field is now rendered with DT_Components::render_text('ddrowssap', ...), which outputs a <dt-text> web component (a text input). Unlike the removed <input type="password">, dt-text does not mask the characters the user types. When a user expands the hidden-fields panel and enters a password, it will be visible in clear text.

// dt-users/template-new-user.php (new code)
DT_Components::render_text( 'ddrowssap', [
    'ddrowssap' => [
        'name' => __( 'Password', 'disciple_tools' ),
        'type' => 'text',  // renders <dt-text>, not <input type="password">
        ...
    ]
], [], ... )

The original <input type="password" id="drowssap"> should either be kept as-is or replaced with a web component that supports type="password".


Medium

2. Optional fields silently dropped on new user creation — dt-users/template-new-user.php

The JS collects optional field values with:

const optionalFields = document.querySelectorAll('[data-optional=""]');

In the old template, fields like first_name, last_name, and site-defined fields all had data-optional directly on their <input> elements. The new DT_Components::render_text() / render_key_select() calls don't emit this attribute — shared_attributes() in dt-components.php only forwards placeholder from $params, not arbitrary attributes like data-optional. As a result, when creating a new user, first name, last name, gender, and any site-defined optional fields are silently dropped from the API payload. Only description (written out directly with data-optional in the HTML) is unaffected.

Fix: either pass data-optional through shared_attributes() for known params, or write those fields out as direct HTML elements with the attribute present.


3. Language names with apostrophes break the language dropdown — dt-users/user-management.js

write_language_dropdown() builds a <dt-single-select> element in a JS template literal:

return `<dt-single-select ... options='${JSON.stringify(options)}'></dt-single-select>`;

JSON.stringify does not escape single-quote characters in strings. If any native_name contains ' (e.g. the N'Ko script language), the JSON placed inside the single-quoted options='...' attribute will break HTML parsing, preventing the language dropdown from rendering.

PHP's render_key_select() avoids this correctly with esc_attr(json_encode(...)). The JS equivalent would be replacing single quotes in the serialized JSON with &#39; before inserting into the attribute, or switching to a value + JSON approach that doesn't rely on single-quoted HTML attribute syntax.


4. Dead typeahead initialization for the subassigned field — dt-users/user-management.js

Inside write_add_user(), the jQuery typeahead is still initialized targeting .js-typeahead-subassigned:

// subassigned check here
['subassigned'].forEach((field_id) => {
  $.typeahead({
    input: `.js-typeahead-${field_id}`,
    ...
  });
});

The <input class="js-typeahead-subassigned"> element was removed from template-new-user.php and replaced with a <dt-connection> web component. The typeahead will silently find no matching elements. The developer left a // subassigned check here comment indicating this needs follow-up. If the dt-connection component does not emit an event that the existing corresponds_to_contact tracking captures, the "link to existing contact" option on user creation may be broken.


5. ComponentService initialized with stale user context in add-user flow — dt-users/user-management.js

write_add_user() now creates a ComponentService using window.current_user_lookup:

function write_add_user() {
    const componentService = new window.DtWebComponents.ComponentService(
      'users',
      window.current_user_lookup,   // may be null, or a previously-opened user's ID
      window.wpApiShare.nonce,
    );
    componentService.attachLoadEvents();

write_add_user() is called when the "Add User" button is clicked. At that point window.current_user_lookup is either undefined (first action on the page) or set to a user who was previously opened. If attachLoadEvents() wires up change-save listeners that fire API requests against that stale ID, edits in the new-user form could silently update an unrelated existing user instead of being held until after the new user is created.


Summary

The PR is not ready to merge. The password-masking regression is a clear security issue, and the optional-fields bug means profile data is silently dropped for every new user created through this form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace components: user management

3 participants