[Active users][Settings][Account settings] Adapt fields and sync data #135

carma12 · 2023-08-03T14:01:42Z

The 'Account settings' section has different types of UI components. Only the 'Text input' ones (given by the IpaTextInput component) were adapted to retrieve the data from the metadata and update this value. This has been adapted and now more elements are capable to do that. Those are:

Text inputs accompanied with 'Add' and 'Delete' buttons
- Field(s): 'Principal alias' (krbprincipalname)
Dropdown selectors
- Field(s): 'Radius proxy configuration' (ipatokenradiusconfiglink) and 'External IdP configuration' (ipaidpconfiglink)
Data-time selectors
- Field(s): 'Kerberos principal expiration (UTC) (krbprincipalexpiration)
Group of checkboxes
- Field(s): 'User authentication types' (ipauserauthtype)

Also, the following fields have been adapted to use the IpaTextInput component:

'User login' (uid)
'Password' (has_password)
'Password expiration' (krbpasswordexpiration)
'UID' (uidnumber)
'GID' (gidnumber)
'Login shell' (loginshell)
'Home directory' (homedirectory)
'Radius proxy username' (ipatokenradiususername)
'External IdP user identifier' (ipaidpsub)

Signed-off-by: Carla Martinez [email protected]

The functionality to take data directly from the metadata needs to be implemented for the Text inputs with buttons. This should be applied to the following field: 'Principal alias' (`krbprincipalname`). Signed-off-by: Carla Martinez <[email protected]>

The functionality for the group of checkboxes needs to be implemented and applied to the following field: 'User authentication types' (`ipauserauthtype`). Signed-off-by: Carla Martinez <[email protected]>

The Select[1] components need to be adapted to take data from the metadata. This will be applied for the fields 'Radius proxy configuration' and 'External IdP configuration'. [1]- http://v4-archive.patternfly.org/v4/components/select Signed-off-by: Carla Martinez <[email protected]>

The Date-time picker[1] component needs to be adapted to take the data from the metadata and keep managing its functionality while using other components/wrappers. This is applied to the following field: 'Kerberos principal expiration (UTC). [1] - http://v4-archive.patternfly.org/v4/components/date-picker Signed-off-by: Carla Martinez <[email protected]>

The following fields need to be adapted to use the `IpaTextInput` component: - 'User login' (`uid`) - 'Password' (`has_password`) - 'Password expiration' (`krbpasswordexpiration`) - 'UID' (`uidnumber`) - 'GID' (`gidnumber`) - 'Login shell' (`loginshell`) - 'Home directory' (`homedirectory`) - 'Radius proxy username' (`ipatokenradiususername`) - 'External IdP user identifier' (`ipaidpsub`) Signed-off-by: Carla Martinez <[email protected]>

pvoborni

The MR is humongous having it split into multiple would be better.

Providing review after reading first 2 patches. The rest will come later.

We can discuss how and when to address them as some could be done in separate MRs so that this MR won't become sn impossible task.

pvoborni · 2023-08-21T11:53:08Z

src/components/UsersSections/UsersAccountSettings.tsx

-                aria-label="user login"
-                isDisabled
+              <IpaTextInput
+                name={"uid"}


I think it would be valid and probably better also notation: name="uid" as this is a constant thus no need to wrap it with {}.

But I understand why it is used as we used it also in Identity section.

pvoborni · 2023-08-21T12:04:09Z

src/utils/ipaObjectUtils.ts

+  newValue: string | string[],
+  paramName: string
+) => {
+  if (!isSimpleValue(paramName)) {


import { isSimpleValue } from "./userUtils"; should not be used in ipaObjectUtils module as that implementation is limited to user object thus would be unusable for other object types.

In this form, this method should be in userUtils module.

To be generic for other objects, we would need to find a different way, e.g. pass the isSimpleValue method to the updateIpaObject or something else.

pvoborni · 2023-08-21T12:06:57Z

src/components/layouts/Calendar/DataTimePickerLayout.tsx

@@ -14,6 +14,7 @@ interface PropsToDataTimePicker {
  toggleIndicator?: React.ElementType | null;
  toggleOnToggle?: (value: boolean, event: any) => void;
  toggleStyle?: React.CSSProperties | undefined;
+  // isDisabled: boolean;


Changes in this file looks like forgotten leftovers.

pvoborni · 2023-08-21T12:18:44Z

src/utils/utils.tsx

+  const minutes = date.substring(10, 12);
+  const seconds = date.substring(12, 14);
+
+  const dateFormat = new Date(


I don't think call create UTC Date. See in Individual date and time component values https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date#parameters You can also check the old UI implementation https://github.com/freeipa/freeipa/blob/master/install/ui/src/freeipa/datetime.js#L44

pvoborni · 2023-08-21T12:24:04Z

src/utils/utils.tsx

+//   less than 10.
+// - E.g.: 3 --> 03
+export const getFullDay = (day: Date) => {
+  return (day.getDate() < 10 ? "0" : "") + day.getDate();


I think something line the following would also work: String(day.getDate()).padStart(2, "0") (untested)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/String
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart

pvoborni · 2023-08-21T12:29:32Z

src/utils/utils.tsx

+  const minutes = getFullMinutes(date);
+  const seconds = getFullSeconds(date);
+
+  return (


As indicated by the Z at the end, the idea is to return UTC date. But the method used are not using UTC dates. The getXXXmethods should be using the UTC variants getUTCXXX. E.g. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getUTCFullYear

This probably works only because similar mistake is done in parsing part. But it might have side effect in displaying dates.

pvoborni · 2023-08-21T13:44:20Z

src/components/Form/IpaCalendar.tsx

+  React.useEffect(() => {
+    if (props.ipaObject !== undefined) {
+      // Parse ipaObject into 'User' type to access the 'datetime' parameter
+      const user = props.ipaObject as unknown as User;


IpaCalendar component should not have any reference to a specific object type (in this case user). The raw value should be obtained the same way as in text input, e.g.:

const { required, readOnly, value, onChange } = getParamProperties(props);

After that you can extract the base64 value and change it into Date object. There is also no need to do it in an effect as it is simple conversion of value from props.

But the question is whether we should be doing this parsing here. I think that the conversion from base64 to Date or string (depending what we find more useful) should happen in apiToUser method in userUtils or any similar method that is used in rpc for other object types.

The reason is that in ipaObject should be of the same type/structure after changing it. Or in other words, it should be possible to set it to the same value and be regarded as a same value. Atm the source is object with base64 string and after modification it is string, thus this doesn't apply.

pvoborni

Added review for "selectors".

pvoborni · 2023-08-21T15:49:32Z

src/components/UsersSections/UsersAccountSettings.tsx

+
+  useEffect(() => {
+    const idpList: string[] = [];
+    props.idpConf.map((item) => {


There should be an additional "empty" item which allows users to remove radius proxy or display nothing when a user doesn't have a proxy defined.

Atm, after adding a radius proxy to IPA, this control shows that the users have it even though they don't.

pvoborni · 2023-08-21T15:53:08Z

src/components/UsersSections/UsersAccountSettings.tsx

@@ -197,6 +202,59 @@ const UsersAccountSettings = (props: PropsToUsersAccountSettings) => {
    </Button>,
  ];

+  // Dropdown 'Radius proxy configuration'
+  const [isRadiusConfOpen, setIsRadiusConfOpen] = useState(false);


Most of this logic could be generalized and be part of IpaSelect - thus reusable for other parts. The only specific part is translation of radius proxy objects into list of strings. The same applies for idp config.

pvoborni · 2023-08-21T15:55:15Z

src/components/UsersSections/UsersAccountSettings.tsx

+  const [radiusConfSelected, setRadiusConfSelected] = useState(
+    ipaObject.ipatokenradiusconfiglink
+  );
+  const [radiusConfOptions, setRadiusConfOptions] = useState<string[]>([]);


This doesn't need to be a state. And the conversion from radiusProxyConf doesn't need to be in an effect -> simplification.

pvoborni · 2023-08-21T16:00:38Z

src/hooks/useUserSettingsData.tsx

@@ -23,6 +30,8 @@ type UserSettingsData = {
  certData?: Record<string, unknown>;
  refetch: () => void;
  modifiedValues: () => Partial<User>;
+  radiusServer: RadiusServer[];


Shouldn't the name be plural for both radius proxies and idp servers? As it is an array.

pvoborni · 2023-08-21T16:07:18Z

src/hooks/useUserSettingsData.tsx

@@ -102,6 +148,54 @@ const useUserSettingsData = (userId: string): UserSettingsData => {
    setModified(modified);
  }, [user, userFullData]);

+  // Detect any change for 'radiusServer'
+  useEffect(() => {


I don't understand why this // Detect any change for 'radiusServer' and // Detect any change for 'idpServer' code exist. E.g. selected radius proxy should be part of ipatokenradiusconfiglink attribute of user object. Detection of its change is already implemented in the // Detect any change in 'originalUser' and 'user' objects.

The loaded lists of radius proxies and idp servers are just information to show as options in the select.

pvoborni · 2023-08-21T16:12:58Z

src/hooks/useUserSettingsData.tsx

  const [modified, setModified] = useState(false);

  // Data displayed and modified by the user
  const [user, setUser] = useState<Partial<User>>({});
+  const [radiusServer, setRadiusServer] = useState<RadiusServer[]>([]);


const [radiusServer, setRadiusServer] = useState<RadiusServer[]>([]); const [idpServer, setIdpServer] = useState<IDPServer[]>([]);

should not be needed as it is just copying info from their "data" counterparts.

But the hook should return the data counterparts the same way as it is doing for e.g. metadata.

pvoborni · 2023-08-21T16:17:41Z

src/components/UsersSections/UsersAccountSettings.tsx

@@ -197,6 +202,59 @@ const UsersAccountSettings = (props: PropsToUsersAccountSettings) => {
    </Button>,
  ];

+  // Dropdown 'Radius proxy configuration'
+  const [isRadiusConfOpen, setIsRadiusConfOpen] = useState(false);
+  const [radiusConfSelected, setRadiusConfSelected] = useState(


The "selected" state is not needed as the value is already part of user/ipaObject.

pvoborni · 2023-08-21T16:34:31Z

Actually I think that each commit should be a separate MR so that we can merge one by one. Without it, it may takes us forever to merge this. This is especially true for the last checkboxes and krbprincipal aliases. I don't want to comment these 2 here as it would be better to have the commentary to be in its own place.

carma12 · 2023-08-25T09:09:01Z

As agreed, this PR will contain the functionality related to the Datetime selector, as most of the comments are focused on that one. The rest of the commit messages will be addressed in different PRs. These are the ones created so far:

Text inputs: [Active users][Settings][Account settings] Add functionality for Text inputs #136
Selectors: [Active users][Settings][Account settings] Add functionality for Selectors #138
Checkboxes list: [Active users][Settings][Account settings] Add functionality for checkbox list #139

I will be adding the rest of the PRs when they are created.

mreynolds389 · 2023-09-07T14:04:52Z

src/components/Form/IpaTextInputFromList.tsx

+                  objectName="user"
+                  metadata={props.metadata}
+                  idx={idx}
+                  readOnly={true} // This field is always read-only


Very minor issue, but I think you get a build warning when a prop is explicitly set to true or false. A boolean prop is set or it isn't. Again, if it's not generating a warning then don't worry about it, but I've seen this trigger annoying messages. :-)

pvoborni · 2023-09-18T10:18:04Z

@carma12 this PR can be closed, right? As I think all the parts were already merged separately.

carma12 · 2023-09-18T10:58:38Z

@pvoborni - Correct. This PR has been split to manage the changes in the following PRs:

Text inputs: [Active users][Settings][Account settings] Add functionality for Text inputs #136
Selectors: [Active users][Settings][Account settings] Add functionality for Selectors #138
List of checkboxes: [Active users][Settings][Account settings] Add functionality for checkbox list #139
Text inputs with buttons: [Active users][Settings][Account settings] Add functionality for text inputs with buttons #140
Date time selector: [Active users][Settings][Account settings] Add functionality to Date time selector #143

Hence, closing this one.

NOTE: The modals will be handled in a different PR as well.

carma12 added the WIP Work in Progress (do not merge) label Aug 3, 2023

carma12 force-pushed the account-settings-sync-fields branch 4 times, most recently from 88755ec to 371416a Compare August 7, 2023 08:10

carma12 mentioned this pull request Aug 8, 2023

Add onChange function for checkboxes #116

Closed

carma12 force-pushed the account-settings-sync-fields branch 2 times, most recently from f1ace78 to 8bf91c4 Compare August 10, 2023 06:55

carma12 changed the title ~~'Account settings' sync fields~~ Active users][Settings][Account settings] Adapt fields and sync data Aug 10, 2023

carma12 changed the title ~~Active users][Settings][Account settings] Adapt fields and sync data~~ [Active users][Settings][Account settings] Adapt fields and sync data Aug 10, 2023

carma12 requested a review from pvoborni August 10, 2023 07:22

carma12 force-pushed the account-settings-sync-fields branch 2 times, most recently from 29a4160 to 47c8def Compare August 10, 2023 14:23

carma12 force-pushed the account-settings-sync-fields branch 2 times, most recently from df737a7 to d6254ec Compare August 11, 2023 08:23

Functionality for checkbox list

0ae1535

The functionality for the group of checkboxes needs to be implemented and applied to the following field: 'User authentication types' (`ipauserauthtype`). Signed-off-by: Carla Martinez <[email protected]>

carma12 force-pushed the account-settings-sync-fields branch from d6254ec to adb4c1f Compare August 11, 2023 08:38

carma12 added 3 commits August 11, 2023 10:46

carma12 force-pushed the account-settings-sync-fields branch from adb4c1f to e615727 Compare August 11, 2023 08:46

carma12 removed the WIP Work in Progress (do not merge) label Aug 11, 2023

pvoborni requested changes Aug 21, 2023

View reviewed changes

pvoborni self-assigned this Aug 21, 2023

pvoborni requested changes Aug 21, 2023

View reviewed changes

mreynolds389 reviewed Sep 7, 2023

View reviewed changes

carma12 closed this Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Active users][Settings][Account settings] Adapt fields and sync data #135

[Active users][Settings][Account settings] Adapt fields and sync data #135

carma12 commented Aug 3, 2023 •

edited

Loading

pvoborni left a comment

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni left a comment

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni Aug 21, 2023

pvoborni commented Aug 21, 2023

carma12 commented Aug 25, 2023 •

edited

Loading

mreynolds389 Sep 7, 2023

pvoborni commented Sep 18, 2023

carma12 commented Sep 18, 2023 •

edited

Loading

[Active users][Settings][Account settings] Adapt fields and sync data #135

[Active users][Settings][Account settings] Adapt fields and sync data #135

Conversation

carma12 commented Aug 3, 2023 • edited Loading

pvoborni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvoborni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvoborni commented Aug 21, 2023

carma12 commented Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

pvoborni commented Sep 18, 2023

carma12 commented Sep 18, 2023 • edited Loading

carma12 commented Aug 3, 2023 •

edited

Loading

carma12 commented Aug 25, 2023 •

edited

Loading

carma12 commented Sep 18, 2023 •

edited

Loading