feat: add export service hours feature and migrate email, http -> https #61
Conversation
WalkthroughThe project has been updated with various enhancements, focusing on improving error handling, simplifying code logic, and upgrading Docker configurations. Key modifications include refining the initialization of the transporter in the backend, implementing optional chaining in several frontend components, and streamlining Dockerfiles for both the frontend and backend by upgrading base images and consolidating commands. Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Out of diff range and nitpick comments (1)
interapp-backend/api/routes/endpoints/exports/exports.ts (1)
Line range hint
14-47: Ensure that error handling is robust, especially for cases where the XLSX generation fails or the user lacks necessary permissions.- res.status(200).send(exports); + try { + res.status(200).send(exports); + } catch (error) { + res.status(500).send({ message: 'Failed to generate export file.' }); + }
| ENV NODE_ENV development | ||
| ENV WATCHPACK_POLLING true | ||
| RUN npm install --frozen-lockfile | ||
|
|
There was a problem hiding this comment.
Consider using npm ci instead of npm install for more reliable builds based on package-lock.json.
interapp-frontend/src/app/exports/ServiceHoursExportsForm/ServiceHoursExportsForm.tsx
Outdated
Show resolved
Hide resolved
| const normaliseServiceExportsProps = ( | ||
| props: UserFriendlyServiceExportsProps, | ||
| ): ServiceExportsProps => { | ||
| let type: ServiceExportsProps['type']; | ||
| let order: ServiceExportsProps['order']; | ||
|
|
||
| switch (props.type) { | ||
| case 'User ID': | ||
| type = 'user_id'; | ||
| break; | ||
| case 'Username': | ||
| type = 'username'; | ||
| break; | ||
| case 'CCA Hours': | ||
| type = 'service_hours'; | ||
| break; | ||
| } | ||
|
|
||
| switch (props.order) { | ||
| case 'Ascending': | ||
| order = 'ASC'; | ||
| break; | ||
| case 'Descending': | ||
| order = 'DESC'; | ||
| break; | ||
| } | ||
|
|
||
| return { type, order }; | ||
| }; |
There was a problem hiding this comment.
Refactor the normaliseServiceExportsProps function to improve readability and maintainability.
The current implementation uses two separate switch statements to map user-friendly properties to API-friendly properties. This could be simplified by using a mapping object or a function that handles both properties at once, reducing the amount of code and improving readability.
interapp-frontend/src/app/exports/ServiceHoursExportsForm/ServiceHoursExportsForm.tsx
Show resolved
Hide resolved
interapp-frontend/src/app/exports/AttendanceExportsForm/AttendanceExportsForm.tsx
Show resolved
Hide resolved
| const handleSubmit = async (values: AttendanceExportsProps) => { | ||
| const transformedValues = { | ||
| id: values.names.map((name) => serviceData[name]), | ||
| start_date: values.range[0]?.toISOString(), | ||
| end_date: values.range[1]?.toISOString(), | ||
| }; | ||
|
|
||
| const apiClient = new APIClient().instance; | ||
| setLoading(true); | ||
| const response = await apiClient.get('/exports', { | ||
| params: transformedValues, | ||
| responseType: 'arraybuffer', | ||
| }); | ||
| setLoading(false); | ||
|
|
||
| const error = generateErrorFromResponse(response); | ||
| if (error) { | ||
| notifications.show(error); | ||
| return; | ||
| } | ||
|
|
||
| downloadFile(response.data as ArrayBuffer, response.headers as DownloadFileHeaders); | ||
|
|
||
| notifications.show({ | ||
| title: 'Success', | ||
| message: 'Export has been created', | ||
| color: 'green', | ||
| }); |
There was a problem hiding this comment.
Optimize state management during API calls in handleSubmit.
Similar to the previous file, consider using a finally block to ensure that the loading state is always reset, even if an error occurs during the API call.
- setLoading(true);
- const response = await apiClient.get('/exports', {
- params: transformedValues,
- responseType: 'arraybuffer',
- });
- setLoading(false);
+ try {
+ setLoading(true);
+ const response = await apiClient.get('/exports', {
+ params: transformedValues,
+ responseType: 'arraybuffer',
+ });
+ } finally {
+ setLoading(false);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const handleSubmit = async (values: AttendanceExportsProps) => { | |
| const transformedValues = { | |
| id: values.names.map((name) => serviceData[name]), | |
| start_date: values.range[0]?.toISOString(), | |
| end_date: values.range[1]?.toISOString(), | |
| }; | |
| const apiClient = new APIClient().instance; | |
| setLoading(true); | |
| const response = await apiClient.get('/exports', { | |
| params: transformedValues, | |
| responseType: 'arraybuffer', | |
| }); | |
| setLoading(false); | |
| const error = generateErrorFromResponse(response); | |
| if (error) { | |
| notifications.show(error); | |
| return; | |
| } | |
| downloadFile(response.data as ArrayBuffer, response.headers as DownloadFileHeaders); | |
| notifications.show({ | |
| title: 'Success', | |
| message: 'Export has been created', | |
| color: 'green', | |
| }); | |
| const handleSubmit = async (values: AttendanceExportsProps) => { | |
| const transformedValues = { | |
| id: values.names.map((name) => serviceData[name]), | |
| start_date: values.range[0]?.toISOString(), | |
| end_date: values.range[1]?.toISOString(), | |
| }; | |
| const apiClient = new APIClient().instance; | |
| try { | |
| setLoading(true); | |
| const response = await apiClient.get('/exports', { | |
| params: transformedValues, | |
| responseType: 'arraybuffer', | |
| }); | |
| } finally { | |
| setLoading(false); | |
| } | |
| const error = generateErrorFromResponse(response); | |
| if (error) { | |
| notifications.show(error); | |
| return; | |
| } | |
| downloadFile(response.data as ArrayBuffer, response.headers as DownloadFileHeaders); | |
| notifications.show({ | |
| title: 'Success', | |
| message: 'Export has been created', | |
| color: 'green', | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
docker-compose.prod.yml (1)
Line range hint
55-55: The indentation for thevolumesblock under theschedulerservice is incorrect.- - ./dump:/tmp/dump - - ./minio-dump:/tmp/minio-dump + - ./dump:/tmp/dump + - ./minio-dump:/tmp/minio-dump
| # set timeout to 8 minutes | ||
| RUN echo 'Acquire::http::Timeout "480";' > /etc/apt/apt.conf.d/99timeout && \ | ||
| echo 'Acquire::ftp::Timeout "480";' >> /etc/apt/apt.conf.d/99timeout && \ | ||
| apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get clean |
There was a problem hiding this comment.
Set an 8-minute timeout for APT operations.
Ensure that versions of packages are pinned to avoid potential inconsistencies during deployment.
- apt-get install -y --fix-missing postgresql-client-16 tzdata
+ apt-get install -y --fix-missing postgresql-client-16=16.0 tzdata=2023aConsider using --no-install-recommends to avoid installing unnecessary packages and ensure that APT lists are cleaned up after installation to reduce the image size.
- apt-get install -y --fix-missing postgresql-client-16 tzdata
+ apt-get install -y --no-install-recommends --fix-missing postgresql-client-16 tzdata && rm -rf /var/lib/apt/lists/*Tools
Hadolint
[info] 17-17: Avoid additional packages by specifying
--no-install-recommends(DL3015)
[warning] 17-17: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
[info] 17-17: Delete the apt-get lists after installing something (DL3009)
Consider setting SHELL option -o pipefail for the RUN command that uses pipes. This will ensure that the failure of a command in a pipeline causes the pipeline to fail.
+ SHELL ["/bin/bash", "-o", "pipefail"]
RUN echo 'Acquire::http::Timeout "480";' > /etc/apt/apt.conf.d/99timeout && \
echo 'Acquire::ftp::Timeout "480";' >> /etc/apt/apt.conf.d/99timeout && \
apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get cleanCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # set timeout to 8 minutes | |
| RUN echo 'Acquire::http::Timeout "480";' > /etc/apt/apt.conf.d/99timeout && \ | |
| echo 'Acquire::ftp::Timeout "480";' >> /etc/apt/apt.conf.d/99timeout && \ | |
| apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get clean | |
| SHELL ["/bin/bash", "-o", "pipefail"] | |
| RUN echo 'Acquire::http::Timeout "480";' > /etc/apt/apt.conf.d/99timeout && \ | |
| echo 'Acquire::ftp::Timeout "480";' >> /etc/apt/apt.conf.d/99timeout && \ | |
| apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get clean |
Tools
Hadolint
[info] 17-17: Avoid additional packages by specifying
--no-install-recommends(DL3015)
[warning] 17-17: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
[info] 17-17: Delete the apt-get lists after installing something (DL3009)
| depends_on: | ||
| - backend | ||
| - postgres | ||
|
|
There was a problem hiding this comment.
Remove trailing spaces to maintain clean and consistent formatting.
- image: sebassnoob/interapp-nginx:latest
+ image: sebassnoob/interapp-nginx:latestCommittable suggestion was skipped due to low confidence.
Tools
yamllint
[error] 121-121: trailing spaces (trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (4)
interapp-frontend/prod.Dockerfile (2)
1-1: Use a specific version of thenode:22-alpineimage.To ensure consistent builds, specify a version for the
node:22-alpineimage.-FROM node:22-alpine +FROM node:22-alpine3.14
8-8: Consider usingnpm cifor more reliable builds.
npm ciinstalls dependencies based onpackage-lock.json, ensuring consistent builds.-RUN npm install --frozen-lockfile && \ +RUN npm ci && \interapp-backend/scheduler/prod.Dockerfile (1)
16-19: Set HTTP and FTP timeout settings.Timeouts for HTTP and FTP have been set to 480 seconds. This can help in environments with slower connections.
interapp-frontend/src/components/Navbar/NavbarNotifications/NavbarNotifications.tsx (1)
Line range hint
170-180: Enhance keyboard interaction for accessibility.The use of optional chaining improves null safety, and
tabIndex={0}enhances accessibility. Consider supportingEnterandSpacekeys in theonKeyDownhandler to fully mimic button behavior.Apply this diff to enhance the
onKeyDownhandler:onKeyDown={(e) => { - if (e.key === 'F1') setOpened(!opened); + if (e.key === 'F1' || e.key === 'Enter' || e.key === ' ') setOpened(!opened); }}
| RUN bun install --frozen-lockfile && \ | ||
| # install curl, lsb-release and gnupg | ||
| apt-get update && apt-get install -y curl gnupg lsb-release && apt-get clean && \ |
There was a problem hiding this comment.
Consolidate installation steps.
The installation of curl, gnupg, and lsb-release has been consolidated. Consider using --no-install-recommends to avoid unnecessary packages.
apt-get update && apt-get install -y --no-install-recommends curl gnupg lsb-release && apt-get clean && \Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN bun install --frozen-lockfile && \ | |
| # install curl, lsb-release and gnupg | |
| apt-get update && apt-get install -y curl gnupg lsb-release && apt-get clean && \ | |
| RUN bun install --frozen-lockfile && \ | |
| # install curl, lsb-release and gnupg | |
| apt-get update && apt-get install -y --no-install-recommends curl gnupg lsb-release && apt-get clean && \ |
| # set timeout to 8 minutes | ||
| echo 'Acquire::http::Timeout "480";' > /etc/apt/apt.conf.d/99timeout && \ | ||
| echo 'Acquire::ftp::Timeout "480";' >> /etc/apt/apt.conf.d/99timeout && \ | ||
| apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get clean && \ |
There was a problem hiding this comment.
Pin package versions and use --no-install-recommends.
Consider pinning package versions and using --no-install-recommends to avoid unnecessary packages and ensure consistent builds.
apt-get install -y --no-install-recommends --fix-missing postgresql-client-16=16.0 tzdata=2023a && apt-get clean && \Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apt-get update && apt-get install -y --fix-missing postgresql-client-16 tzdata && apt-get clean && \ | |
| apt-get update && apt-get install -y --no-install-recommends --fix-missing postgresql-client-16=16.0 tzdata=2023a && apt-get clean && \ |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
as per title