[Active users][Settings][Kebab] Add 'Reset password' option

[carma12]

The 'Reset password' option should take the new password values associated with the user account.

[pvoborni]

Thanks for the PR. I've provided comments. The most important one is probably the "not visible error message". Remaining is more in an area of maintainability.

[pvoborni]

This can be improved later. But from UX perspective I'd rather see: Reset the password for user $UID. The reason is that in the "settings" page the user is not selected and it might be also a confirmation to the admin what user it applies to.

Alternatively, I'd remove the "description" altogether as it, in current form, doesn't add more info than what is already showed in the title.

CC @bdellasc

[carma12]

I agree with your point, I think is more useful to refer to the current user (especially when this is an operation that is not made in bulk).

I can either change the title to the one that you are proposing (Reset the password for user $UID) or leave it as it is from now until we can discuss this with @bdellasc . I personally prefer the latter because it can bring an interesting UX perspective :)

[pvoborni]

Both works for me. So we can go with your preferred.

[pvoborni]

The use of any[] looks as anti pattern to me, makes it harder to read, especially from usage perspective - where to use the call one must look at the implementation to know what the payload should look like.

I'd expect:

export interface PasswordChangePayload {
  uid: string;
  password: string;
}

changePassword: build.mutation<FindRPCResponse, PasswordChangePayload>({
      query: (pwPayload) => {
        const params = [
          [pwPayload.uid],
          {
            password: pwPayload.password,
            version: API_VERSION_BACKUP,
          },
        ];

        return getCommand({
          method: "passwd",
          params: params,
        });
      },
    }),

The question is whether the response should be FindRPCResponse. It moves the responsibility of parsing API result to the dialog instead of the rpc. E.g. it can return something like:

export interface OperationResult {
  success: boolean;
  // this would contain the success message obtain from API (result/result/summary) 
  // or the error  message (error/message).
  message: string; 
  error: ErrorResult; // this might not be needed
}

[carma12]

I have adapted the code to pass the PasswordChangePayload as a parameter (will push the changes in a minute).

About using the OperationResult interface instead of the FindRPCResponse - Is not a bad idea, but seems quite incomplete and redundant to me. From my point of view, the findRPCResponse can provide further information. about the status of the operation (failed/successful), the result received (sometimes multiple data), and even the number of returned items.

If we want to simplify the current FindRPCResponse I would add more elements to the interface. Something like:

export interface OperationResult {
  // If the operation failed or succeed
  success: boolean;
  // Result data if operation succeeds
  resultData: ObjectResult | undefined; 
  // Success/error messages (is it possible to receive multiple errors?)
  message: string;
}

This also brings another question (API-wise): when making an API call, is it possible to receive a valid result (E.g.: result.results.result is not undefined) and still get an error at the same time (result.error)?

[pvoborni]

The idea was to "simplify" the Password dialog logic by getting data with well defined meaning instead of a general answer and thus needed to know what to look for in the answer. Or said differently, it`s about which component should have the message result parsing logic.

But the PR is ok even as it is.

[carma12]

@pvoborni - I have already updated the code based on your feedback.

[carma12]

@pvoborni - The code has been updated based on your last comment.

[pvoborni]

LGTM