Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/817 copy another user rights for a new user #899

Merged
merged 11 commits into from
Nov 18, 2024
Merged

Feat/817 copy another user rights for a new user #899

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ff2d5c9
feat#817: Add UserCopyRight filter
helakaraa Oct 29, 2024
04bc18b
feat#817: Throw error if invited user email is already used.
helakaraa Oct 30, 2024
1a1383c
feat#817: modify sendInvitation method
helakaraa Oct 30, 2024
aa2a21b
feat#817: add test inviteUser
helakaraa Oct 31, 2024
932fca5
Revert "feat#817: add test inviteUser"
helakaraa Nov 6, 2024
a2ea411
Revert "feat#817: modify sendInvitation method"
helakaraa Nov 6, 2024
ab9579e
feat#817: select userRights in the frontend
helakaraa Nov 6, 2024
b1e9f73
feat#817: add toggle button : Copy rights from another user
helakaraa Nov 12, 2024
89fed1b
feat#817: codeReview modification code
helakaraa Nov 13, 2024
2ac4aac
feat#817: OnSubmit without Async
helakaraa Nov 15, 2024
382c2fe
feat#817: codeReview admin Right
helakaraa Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions izanami-frontend/src/pages/users.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,12 @@ export function Users() {
},
}
);
const inviteUserMutation = useMutation<
{ invitationUrl?: string } | null,
Error,
{ admin: boolean; email: string; rights: TRights; userToCopy: string }
>((data) => {
if (data.userToCopy) {
return queryUser(data.userToCopy)
.then((res) => {
data.rights = res.rights;
return createInvitation(data.email, data.admin, data.rights);
})
.catch((error) => {
throw new Error(error);
});
} else {
return createInvitation(data.email, data.admin, data.rights);
}
});
const inviteUserMutation = useMutation(
(data: { email: string; admin: boolean; rights: TRights }) =>
createInvitation(data.email, data.admin, data.rights)
);

const readUser = useMutation((user: string) => queryUser(user));
Copy link
Member

Choose a reason for hiding this comment

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

Since queryUser doesn't change anything server side it should be a query. However I would prefer not having a query at all, and chaining it with createInvitation as it was done before (sorry, I told you that userToCopy wasn't necessary anymore, however you'll need it to know it you need to queryUser or not).

Having this logic in mutation makes more sense to me, rather than bloating jsx with this logic.


function OperationToggleForm(props: {
bulkOperation: string;
Expand Down Expand Up @@ -319,7 +307,7 @@ export function Users() {
type: "bool",
},
rights: {
label: "",
label: () => "",
type: "object",
array: true,
visible: ({ rawValues }) => !rawValues.useCopyUserRights,
Expand Down Expand Up @@ -351,14 +339,34 @@ export function Users() {
),
},
}}
onSubmit={async (ctx) => {
onSubmit={(ctx) => {
const backendRights = rightStateArrayToBackendMap(ctx.rights);
const payload = {
let payload = {
rights: backendRights,
admin: ctx.admin,
email: ctx.email,
userToCopy: ctx.userToCopy?.value,
};
if (ctx.userToCopy) {
readUser
.mutateAsync(ctx.userToCopy.value)
.then((res: TUser) => {
payload = { ...payload, rights: res.rights };
return inviteUserMutation
.mutateAsync(payload)
.then((response) => {
if (response && response.invitationUrl) {
setCreationUrl(response.invitationUrl);
}
setCreating(false);
})
.catch((error) => {
throw new Error(error);
Copy link
Member

Choose a reason for hiding this comment

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

As I told you in my previous review, this kind of code is not very usefull, since you just rethrow the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this version :

const inviteUserMutation = useMutation(
    async (data: {
      email: string;
      admin: boolean;
      rights: TRights;
      userToCopy: string;
    }) => {
      if (data.userToCopy) {
        try {
          const res: TUser = await queryUser(data.userToCopy);
          data = { ...data, rights: res.rights };
        } catch (error) {
          throw new Error("Error fetching user rights: " + error);
        }
      }
      return createInvitation(data.email, data.admin, data.rights);
    }
  );
   onSubmit={async (ctx) => {
                  const backendRights = rightStateArrayToBackendMap(ctx.rights);
                  let payload = {
                    rights: backendRights,
                    admin: ctx.admin,
                    email: ctx.email,
                    userToCopy: ctx.userToCopy?.value,
                  };
                  return inviteUserMutation
                    .mutateAsync(payload)
                    .then((response) => {
                      if (response && response.invitationUrl) {
                        setCreationUrl(response.invitationUrl);
                      }
                      setCreating(false);
                    })
                    .catch((error) => {
                      throw new Error(error);
                    });
                }}

Copy link
Member

Choose a reason for hiding this comment

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

Its better, except that your code is using async/await (see my previous comment about that) and that it remains a catch that just rethrow the error (as I mentionned twice already, I don't see the point).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ptitFicus , I commit a modification without Async and double throwing error.

});
})
.catch((error: any) => {
throw new Error(error);
});
}
return inviteUserMutation
.mutateAsync(payload)
.then((response) => {
Expand Down