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.

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: add creation user required step #102

Merged
merged 7 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
54 changes: 54 additions & 0 deletions src/app/components/RequiredSteps.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
'use client';
import { Grid } from '@mui/material';
import { usePathname } from 'next/navigation';
import React from 'react';

import { AddUserDialog } from './dialogs/AddUserDialog';
import { useAuth } from '../context/AuthProvider';
import { apiUser } from '../services/endpoints/user.class';

export const RequiredSteps = ({ children }: { children: React.ReactNode }) => {
const { user, isAuthenticated } = useAuth();
const pathname = usePathname();

const [isCreateUserDialogOpened, setIsCreateUserDialogOpened] =
React.useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @yassinedorbozgithub, I suggest we import everything we need from react once and for all, to keep the code aligned with other files.

Suggested change
React.useState(false);
useState(false);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure we will do that

const [hasUser, setHasUser] = React.useState(false);
const isAnonymous = !isAuthenticated && pathname === '/viewer';

console.log(pathname);
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right the mentioned code need to be removed, nice catch


React.useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
React.useEffect(() => {
useEffect(() => {

const fetchUser = async (id: string) =>
apiUser
.getUser(id)
.then(({ data }) => {
setHasUser(true);
return data;
})
.catch((e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @yassinedorbozgithub, Or maybe you prefer to log the error instead?

Suggested change
.catch((e) => {
.catch(() => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @amalessid, i think that we can remove e prop like you suggest

setHasUser(false);
setIsCreateUserDialogOpened(true);
});

if (user?.id) fetchUser(user.id);
}, []);

return (
<Grid minHeight={'calc(100vh - 137px)'}>
{isAnonymous || hasUser ? (
children
) : (
<AddUserDialog
open={isCreateUserDialogOpened}
onClose={() => {
setIsCreateUserDialogOpened(false);
}}
callback={() => {
setHasUser(true);
}}
/>
Comment on lines +40 to +48
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve AddUserDialog prop naming and callback handling.

The current implementation of AddUserDialog props could be improved for clarity and maintainability.

Consider the following changes:

  1. Rename the callback prop to something more descriptive, like onUserCreated.
  2. Use a single function to handle both dialog closing and user creation callback.

Here's a suggested refactor:

 <AddUserDialog
   open={isCreateUserDialogOpened}
-  onClose={() => {
-    setIsCreateUserDialogOpened(false);
-  }}
-  callback={() => {
+  onClose={() => setIsCreateUserDialogOpened(false)}
+  onUserCreated={() => {
     setHasUser(true);
+    setIsCreateUserDialogOpened(false);
   }}
 />

This change improves the readability and maintainability of the component.

Committable suggestion was skipped due to low confidence.

)}
</Grid>
);
};
120 changes: 120 additions & 0 deletions src/app/components/dialogs/AddUserDialog.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
'use client';

import { KeyboardBackspace, Send } from '@mui/icons-material';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { KeyboardBackspace, Send } from '@mui/icons-material';
import { Send } from '@mui/icons-material';

import {
Dialog,
DialogContent,
Grid,
Alert,
TextField,
Button,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Button,

} from '@mui/material';
import { FC, useEffect, useState } from 'react';
import { useForm } from 'react-hook-form';

import { useAuth } from '@/app/context/AuthProvider';
import { apiUser } from '@/app/services/endpoints/user.class';
import { CreateUserDto } from '@/domain/dtos/user';

import { StyledButton } from '../StyledButton';
import { StyledDialogActions } from '../StyledDialogActions';
import { StyledDialogContent } from '../StyledDialogContent';
import { StyledDialogTitle } from '../StyledDialogTitle';

interface AddUserDialogProps {
open?: boolean;
onClose?: () => void;
callback?: () => void;
}
Comment on lines +17 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refining the AddUserDialogProps interface.

While the current interface allows for flexibility, consider the following improvements for better type safety:

  1. Make the open prop required, as it's crucial for controlling the dialog's visibility.
  2. Consider making onClose required to ensure proper handling of dialog closure.

Example refactored interface:

interface AddUserDialogProps {
  open: boolean;
  onClose: () => void;
  callback?: () => void;
}

This change would enforce better prop usage and potentially prevent runtime errors.


export const AddUserDialog: FC<AddUserDialogProps> = ({
open,
onClose,
callback,
}) => {
const { session } = useAuth();
const [hasError, setHasError] = useState(false);
const [disableButton, setDisableButton] = useState(false);

const {
reset,
register,
formState: { errors },
resetField,
handleSubmit,
} = useForm<CreateUserDto>();
Comment on lines +32 to +38
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance type safety for the useForm hook.

To improve type safety and autocompletion, consider using a more specific type for the useForm hook. Instead of CreateUserDto, which might include fields not present in the form, create a dedicated interface for the form fields.

Example:

interface AddUserFormData {
  userId: string;
  patientId: string;
}

const {
  reset,
  register,
  formState: { errors },
  resetField,
  handleSubmit,
} = useForm<AddUserFormData>();

This change will ensure that the form fields and their types are explicitly defined, improving type checking and developer experience.

Comment on lines +23 to +38
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance type safety for the useForm hook.

To improve type safety and autocompletion, consider using a more specific type for the useForm hook. Instead of CreateUserDto, which might include fields not present in the form, create a dedicated interface for the form fields.

Example:

interface AddUserFormData {
  userId: string;
  patientId: string;
}

const {
  reset,
  register,
  formState: { errors },
  resetField,
  handleSubmit,
} = useForm<AddUserFormData>();

This change will ensure that the form fields and their types are explicitly defined, improving type checking and developer experience.


const onSubmitForm = async (data: CreateUserDto) => {
setDisableButton(true);
try {
await apiUser
.createUser(data)
.then(() => {
onClose?.();
callback?.();
})
.catch(() => {
setHasError(true);
setDisableButton(false);
});
} catch (error) {
console.error('Failed to create user:', error);
}
};
Comment on lines +40 to +56
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and form state management in onSubmitForm.

Consider the following improvements:

  1. Provide more specific error messages:

    .catch((error) => {
      setHasError(true);
      setDisableButton(false);
      console.error('Failed to create user:', error.message);
    });
  2. Reset the form state on successful submission:

    .then(() => {
      reset(); // Reset form fields
      onClose?.();
      callback?.();
    })
  3. Use a more descriptive error message in the catch block:

    } catch (error) {
      console.error('Unexpected error during user creation:', error);
      setHasError(true);
      setDisableButton(false);
    }

These changes will improve error handling, provide better feedback to users, and ensure the form is in a clean state after successful submission.


useEffect(() => {
if (open) {
reset();
setHasError(false);
setDisableButton(false);
}
}, [open]);

useEffect(() => {
if (session?.token?.sub)
resetField('userId', { defaultValue: session.token.sub });
}, [session?.token?.sub]);
Comment on lines +66 to +69
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add a safety check for the session token in the useEffect hook.

To prevent potential runtime errors, consider adding a safety check:

useEffect(() => {
  if (session?.token?.sub) {
    resetField('userId', { defaultValue: session.token.sub });
  } else {
    console.warn('Session token is undefined. Unable to set userId.');
    // Optionally, set a default value or handle this case as needed
  }
}, [session?.token?.sub, resetField]);

This change will make the code more robust and prevent potential issues if the session token is unexpectedly undefined.


return (
<Dialog open={!!open} fullWidth maxWidth="xs">
<form onSubmit={handleSubmit(onSubmitForm)}>
<StyledDialogTitle>Create a user</StyledDialogTitle>
<DialogContent style={{ padding: '5px 8px' }}>
<StyledDialogContent>
{hasError && (
<Grid pb={2}>
<Alert severity="error">The creation of a user is failed</Alert>
</Grid>
)}
<TextField
label="User id"
error={!!errors.userId}
disabled
required
inputProps={{ readOnly: true }}
{...register('userId', {})}
/>
<TextField
type="number"
label="National identity number *"
{...register('patientId', {
setValueAs: (value) => value || undefined,
})}
/>
Comment on lines +90 to +96
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation rules for the patientId field.

The patientId field currently lacks validation rules. Consider adding appropriate validation:

<TextField
  type="number"
  label="National identity number *"
  error={!!errors.patientId}
  helperText={errors.patientId?.message}
  {...register('patientId', {
    required: 'National identity number is required',
    validate: (value) => {
      // Add custom validation logic here
      // For example, check if it's a valid length for a national ID
      return value.length === 10 || 'Invalid national identity number';
    },
    setValueAs: (value) => value || undefined,
  })}
/>

This change will ensure that the patientId field is properly validated before form submission, improving data integrity and user experience.

</StyledDialogContent>
</DialogContent>
<StyledDialogActions sx={{ justifyContent: 'end' }}>
<StyledButton
type="submit"
color="success"
variant="contained"
disabled={disableButton}
>
<Send sx={{ paddingRight: '4px', marginRight: '4px' }} />
Create
</StyledButton>
</StyledDialogActions>
</form>
</Dialog>
);
};
5 changes: 3 additions & 2 deletions src/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { Grid } from '@mui/material';
import { ThemeProvider } from '@mui/material/styles';
import { AppRouterCacheProvider } from '@mui/material-nextjs/v14-appRouter';
import type { Metadata } from 'next';
import { getServerSession } from 'next-auth';
import React from 'react';

import { authOptions } from './api/auth/authOptions';
import Footer from './components/Footer';
import Header from './components/Header';
import { RequiredSteps } from './components/RequiredSteps';
import AuthProvider from './context/AuthProvider';
import theme from './theme';

Expand All @@ -32,7 +33,7 @@ export default async function RootLayout({
<ThemeProvider theme={theme}>
<AuthProvider session={session}>
<Header />
<Grid minHeight={'calc(100vh - 137px)'}>{children}</Grid>
<RequiredSteps>{children}</RequiredSteps>
<Footer />
</AuthProvider>
</ThemeProvider>
Expand Down
10 changes: 10 additions & 0 deletions src/app/services/api.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
CreateSHLinkEndpointDto,
} from '@/domain/dtos/shlink-endpoint';
import { SHLinkQRCodeRequestDto } from '@/domain/dtos/shlink-qrcode';
import { CreateUserDto, UserDto } from '@/domain/dtos/user';
import { type TBundle } from '@/types/fhir.types';

export interface IApi {
Expand Down Expand Up @@ -87,6 +88,15 @@ export interface IPathMapTypes {
update: never;
delete: never;
};
[EPath.users]: {
create: {
req: CreateUserDto;
res: UserDto;
};
read: UserDto;
update: never;
delete: never;
};
[EPath.viewer]: {
create: {
req: SHLinkRequestDto;
Expand Down
34 changes: 34 additions & 0 deletions src/app/services/endpoints/user.class.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { type AxiosInstance } from 'axios';

import { BaseApi, instance } from '../api.class';
import {
EPath,
type IPathMapTypes,
type TBaseApiProps,
type TPathToOperations,
} from '../api.types';

export class User<
TPath extends keyof IPathMapTypes,
TOperations extends TBaseApiProps<TOperations> = TPathToOperations<TPath>,
> extends BaseApi<TOperations> {
constructor(protected readonly instance: AxiosInstance) {
super(instance);
}

async createUser(data: TOperations['create']['req']) {
return await this.create({
url: `/${EPath.users}`,
data,
});
}

async getUser(userId: string) {
return await this.get({
url: `/${EPath.users}/${userId}`,
});
}
}

export const createApiUser = () => new User<EPath.users>(instance);
export const apiUser = createApiUser();
Loading