From dd6a2cba3d01e3d44dcdfc57f53d849109ce611c Mon Sep 17 00:00:00 2001 From: Donkoko Date: Fri, 16 Feb 2024 14:46:27 +0200 Subject: [PATCH 1/4] improving asset search performance - added option to get paginated assets either via the AssetSearchView or directly from assets. This is in order to get small performance improvements in certain views --- app/modules/asset/service.server.ts | 198 +++++++++++++++++- .../bookings.$bookingId.add-assets.tsx | 1 + .../locations.$locationId.add-assets.tsx | 1 + 3 files changed, 198 insertions(+), 2 deletions(-) diff --git a/app/modules/asset/service.server.ts b/app/modules/asset/service.server.ts index 1e3383139..ba5980955 100644 --- a/app/modules/asset/service.server.ts +++ b/app/modules/asset/service.server.ts @@ -108,7 +108,11 @@ export async function getAsset({ return asset; } -export async function getAssets({ +/** + * Fetches assets from AssetSearchView + * This is used to have a more advanced search however its less performant + */ +export async function getAssetsFromView({ organizationId, page = 1, perPage = 8, @@ -296,6 +300,185 @@ export async function getAssets({ return { assets: assetSearch.map((a) => a.asset), totalAssets }; } +/** + * Fetches assets directly from asset table + */ +export async function getAssets({ + organizationId, + page = 1, + perPage = 8, + search, + categoriesIds, + tagsIds, + bookingFrom, + bookingTo, + hideUnavailable, + unhideAssetsBookigIds, // works in conjuction with hideUnavailable, to show currentbooking assets +}: { + organizationId: Organization["id"]; + + /** Page number. Starts at 1 */ + page: number; + + /** Assets to be loaded per page */ + perPage?: number; + + search?: string | null; + + categoriesIds?: Category["id"][] | null; + tagsIds?: Tag["id"][] | null; + hideUnavailable?: Asset["availableToBook"]; + bookingFrom?: Booking["from"]; + bookingTo?: Booking["to"]; + unhideAssetsBookigIds?: Booking["id"][]; +}) { + const skip = page > 1 ? (page - 1) * perPage : 0; + const take = perPage >= 1 && perPage <= 100 ? perPage : 20; // min 1 and max 25 per page + + /** Default value of where. Takes the assetss belonging to current user */ + let where: Prisma.AssetWhereInput = { organizationId }; + + /** If the search string exists, add it to the where object */ + if (search) { + const words = search.trim().replace(/ +/g, " "); //replace multiple spaces into 1 + where.title = words; + } + + if (categoriesIds && categoriesIds.length > 0) { + if (categoriesIds.includes("uncategorized")) { + where.OR = [ + { + categoryId: { + in: categoriesIds, + }, + }, + { + categoryId: null, + }, + ]; + } else { + where.categoryId = { + in: categoriesIds, + }; + } + } + const unavailableBookingStatuses = [ + BookingStatus.DRAFT, + BookingStatus.RESERVED, + BookingStatus.ONGOING, + ]; + if (hideUnavailable) { + //not disabled for booking + where.availableToBook = true; + //not assigned to team meber + where.custody = null; + if (bookingFrom && bookingTo) { + //reserved during that time + where.bookings = { + none: { + ...(unhideAssetsBookigIds?.length && { + id: { notIn: unhideAssetsBookigIds }, + }), + status: { in: unavailableBookingStatuses }, + OR: [ + { + from: { lte: bookingTo }, + to: { gte: bookingFrom }, + }, + { + from: { gte: bookingFrom }, + to: { lte: bookingTo }, + }, + ], + }, + }; + } + } + if (hideUnavailable === true && (!bookingFrom || !bookingTo)) { + throw new ShelfStackError({ + message: "booking dates are needed to hide unavailable assets", + }); + } + if (bookingFrom && bookingTo) { + where.availableToBook = true; + } + + if (tagsIds && tagsIds.length > 0) { + where.tags = { + some: { + id: { + in: tagsIds, + }, + }, + }; + } + + const [assets, totalAssets] = await db.$transaction([ + /** Get the assets */ + db.asset.findMany({ + skip, + take, + where, + include: { + category: true, + tags: true, + location: { + select: { + name: true, + }, + }, + custody: { + select: { + custodian: { + select: { + name: true, + user: { + select: { + profilePicture: true, + }, + }, + }, + }, + }, + }, + ...(bookingTo && bookingFrom + ? { + bookings: { + where: { + status: { in: unavailableBookingStatuses }, + OR: [ + { + from: { lte: bookingTo }, + to: { gte: bookingFrom }, + }, + { + from: { gte: bookingFrom }, + to: { lte: bookingTo }, + }, + ], + }, + take: 1, //just to show in UI if its booked, so take only 1, also at a given slot only 1 booking can be created for an asset + select: { + from: true, + to: true, + status: true, + id: true, + name: true, + }, + }, + } + : {}), + }, + orderBy: { createdAt: "desc" }, + }), + + /** Count them */ + db.asset.count({ where }), + ]); + + return { assets, totalAssets }; +} + export async function createAsset({ title, description, @@ -818,12 +1001,18 @@ export const getPaginatedAndFilterableAssets = async ({ organizationId, excludeCategoriesQuery = false, excludeTagsQuery = false, + excludeSearchFromView = false, }: { request: LoaderFunctionArgs["request"]; organizationId: Organization["id"]; extraInclude?: Prisma.AssetInclude; excludeCategoriesQuery?: boolean; excludeTagsQuery?: boolean; + /** + * Set to true if you want the query to be performed by directly accessing the assets table + * instead of the AssetSearchView + */ + excludeSearchFromView?: boolean; }) => { const searchParams = getCurrentSearchParams(request); const { @@ -842,7 +1031,12 @@ export const getPaginatedAndFilterableAssets = async ({ const cookie = await updateCookieWithPerPage(request, perPageParam); const { perPage } = cookie; - const { assets, totalAssets } = await getAssets({ + let getFunction = getAssetsFromView; + if (excludeSearchFromView) { + getFunction = getAssets; + } + + const { assets, totalAssets } = await getFunction({ organizationId, page, perPage, diff --git a/app/routes/_layout+/bookings.$bookingId.add-assets.tsx b/app/routes/_layout+/bookings.$bookingId.add-assets.tsx index 9aa9b347c..623cd586d 100644 --- a/app/routes/_layout+/bookings.$bookingId.add-assets.tsx +++ b/app/routes/_layout+/bookings.$bookingId.add-assets.tsx @@ -61,6 +61,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { organizationId, excludeCategoriesQuery: true, excludeTagsQuery: true, + excludeSearchFromView: true, }); const modelName = { diff --git a/app/routes/_layout+/locations.$locationId.add-assets.tsx b/app/routes/_layout+/locations.$locationId.add-assets.tsx index 182082527..fa5b28637 100644 --- a/app/routes/_layout+/locations.$locationId.add-assets.tsx +++ b/app/routes/_layout+/locations.$locationId.add-assets.tsx @@ -46,6 +46,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { organizationId, excludeCategoriesQuery: true, excludeTagsQuery: true, + excludeSearchFromView: true, }); const modelName = { From 60a82f1040f29bcd50fe66fe429d9043385b5297 Mon Sep 17 00:00:00 2001 From: Donkoko Date: Mon, 19 Feb 2024 12:48:53 +0200 Subject: [PATCH 2/4] fixing asset status bug - we had a case where removing a asset with status CHECKED_OUT from any booking, would reset its status back to AVAILABLE: https://github.com/Shelf-nu/shelf.nu/issues/703#issuecomment-1944315975. This has been fixed by implementing an extra check --- app/modules/booking/service.server.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/modules/booking/service.server.ts b/app/modules/booking/service.server.ts index d9dac4c9d..7719acf91 100644 --- a/app/modules/booking/service.server.ts +++ b/app/modules/booking/service.server.ts @@ -503,13 +503,24 @@ export const removeAssets = async ( /** When removing an asset from a booking we need to make sure to set their status back to available * This is needed because the user is allowed to remove an asset from a booking that is ongoing, which means the asset status will be CHECKED_OUT * So we need to set it back to AVAILABLE + * We only do that if the booking we removed it from is ongoing or overdue. + * Reason is that the user can add an asset to a draft booking and remove it and that will reset its status back to available, which shouldnt happen + * https://github.com/Shelf-nu/shelf.nu/issues/703#issuecomment-1944315975 + * * Because prisma doesnt support transactional execution of nested queries, we need to do them in 2 steps, because if the disconnect runs first, * the updateMany will not find the assets in the booking anymore and wont update them */ - await db.asset.updateMany({ - where: { id: { in: assetIds } }, - data: { status: AssetStatus.AVAILABLE }, - }); + + if ( + b.status === BookingStatus.ONGOING || + b.status === BookingStatus.OVERDUE + ) { + await db.asset.updateMany({ + where: { id: { in: assetIds } }, + data: { status: AssetStatus.AVAILABLE }, + }); + } + return b; }; From 021d3e4ea6daf3429ab2478757e9a9a0cdbc20e7 Mon Sep 17 00:00:00 2001 From: Donkoko Date: Mon, 19 Feb 2024 16:21:38 +0200 Subject: [PATCH 3/4] bookings fixes - sending separate emails to all admins if a booking has been made by self service user - Made dynamic footers of emails based on weather its for admin or normal user - Changed subject of reservation email to make it more clear - Fixed a password-is-too short error. Error was just palceholder --- app/emails/bookings-updates-template.tsx | 40 ++++------------ app/emails/components/footers.tsx | 53 +++++++++++++++++++++ app/modules/booking/email-helpers.ts | 2 +- app/modules/booking/service.server.ts | 32 ++++++++++++- app/modules/organization/service.server.ts | 24 ++++++++++ app/routes/_auth+/join.tsx | 8 +++- app/routes/_layout+/bookings.$bookingId.tsx | 5 +- 7 files changed, 127 insertions(+), 37 deletions(-) create mode 100644 app/emails/components/footers.tsx diff --git a/app/emails/bookings-updates-template.tsx b/app/emails/bookings-updates-template.tsx index d55808453..444cf2313 100644 --- a/app/emails/bookings-updates-template.tsx +++ b/app/emails/bookings-updates-template.tsx @@ -1,8 +1,6 @@ import { Button, Html, - Text, - Link, Head, render, Container, @@ -11,6 +9,7 @@ import { import type { ClientHint } from "~/modules/booking/types"; import { getDateTimeFormatFromHints } from "~/utils/client-hints"; import { SERVER_URL } from "~/utils/env"; +import { AdminFooter, UserFooter } from "./components/footers"; import { LogoForEmail } from "./logo"; import { styles } from "./styles"; import type { BookingForEmail } from "./types"; @@ -21,6 +20,7 @@ interface Props { assetCount: number; hints: ClientHint; hideViewButton?: boolean; + isAdminEmail?: boolean; } export function BookingUpdatesEmailTemplate({ @@ -29,6 +29,7 @@ export function BookingUpdatesEmailTemplate({ hints, assetCount, hideViewButton = false, + isAdminEmail = false, }: Props) { const fromDate = getDateTimeFormatFromHints(hints, { dateStyle: "short", @@ -95,34 +96,11 @@ export function BookingUpdatesEmailTemplate({ )} - - This email was sent to{" "} - - {booking.custodianUser!.email} - {" "} - because it is part of the workspace{" "} - - "{booking.organization.name}" - - .
If you think you weren’t supposed to have received this email - please{" "} - - contact the owner - {" "} - of the workspace. - - - {" "} - © 2024 Shelf.nu - + {isAdminEmail ? ( + + ) : ( + + )} ); @@ -138,6 +116,7 @@ export const bookingUpdatesTemplateString = ({ assetCount, hints, hideViewButton = false, + isAdminEmail = false, }: Props) => render( ); diff --git a/app/emails/components/footers.tsx b/app/emails/components/footers.tsx new file mode 100644 index 000000000..ac6c1d9b3 --- /dev/null +++ b/app/emails/components/footers.tsx @@ -0,0 +1,53 @@ +import { Text, Link } from "@react-email/components"; +import type { BookingForEmail } from "../types"; + +/** Footer used when sending normal user emails */ +export const UserFooter = ({ booking }: { booking: BookingForEmail }) => ( + <> + + This email was sent to{" "} + + {booking.custodianUser!.email} + {" "} + because it is part of the workspace{" "} + + "{booking.organization.name}" + + .
If you think you weren’t supposed to have received this email + please{" "} + + contact the owner + {" "} + of the workspace. + + + {" "} + © 2024 Shelf.nu + + +); + +/** Footer used when sending admin user emails */ +export const AdminFooter = ({ booking }: { booking: BookingForEmail }) => ( + <> + + This email was sent to you because you are the OWNER or ADMIN of the + workspace{" "} + + "{booking.organization.name}" + + .
If you think you weren’t supposed to have received this email + please contact support. + + + {" "} + © 2024 Shelf.nu + + +); diff --git a/app/modules/booking/email-helpers.ts b/app/modules/booking/email-helpers.ts index 81227c50a..75dc4f44d 100644 --- a/app/modules/booking/email-helpers.ts +++ b/app/modules/booking/email-helpers.ts @@ -83,7 +83,7 @@ export const assetReservedEmailContent = ({ to, bookingId, assetsCount, - emailContent: `Booking confirmation for ${custodian}.`, + emailContent: `Booking reservation for ${custodian}.`, }); /** diff --git a/app/modules/booking/service.server.ts b/app/modules/booking/service.server.ts index 7719acf91..22de08ff5 100644 --- a/app/modules/booking/service.server.ts +++ b/app/modules/booking/service.server.ts @@ -21,6 +21,7 @@ import { sendCheckinReminder, } from "./email-helpers"; import type { ClientHint, SchedulerData } from "./types"; +import { getOrganizationAdminsEmails } from "../organization"; /** Includes needed for booking to have all data required for emails */ export const bookingIncludeForEmails = { @@ -96,7 +97,8 @@ export const upsertBooking = async ( | "custodianUserId" > & { assetIds: Asset["id"][] } >, - hints: ClientHint + hints: ClientHint, + isSelfService: boolean = false ) => { const { assetIds, @@ -247,11 +249,37 @@ export const upsertBooking = async ( }); let html = bookingUpdatesTemplateString({ booking: res, - heading: `Booking confirmation for ${custodian}`, + heading: `Booking reservation for ${custodian}`, assetCount: res.assets.length, hints, }); + /** Here we need to check if the custodian is different than the admin and send email to the admin in case they are different */ + if (isSelfService) { + const adminsEmails = await getOrganizationAdminsEmails({ + organizationId: res.organizationId, + }); + + const adminSubject = `Booking reservation request (${res.name}) by ${custodian} - shelf.nu`; + + /** Pushing admins emails to promises */ + promises.push( + sendEmail({ + to: adminsEmails.join(","), + subject: adminSubject, + text, + /** We need to invoke this function separately for the admin email as the footer of emails is different */ + html: bookingUpdatesTemplateString({ + booking: res, + heading: `Booking reservation for ${custodian}`, + assetCount: res.assets.length, + hints, + isAdminEmail: true, + }), + }) + ); + } + if (data.status === BookingStatus.COMPLETE) { subject = `Booking completed (${res.name}) - shelf.nu`; text = completedBookingEmailContent({ diff --git a/app/modules/organization/service.server.ts b/app/modules/organization/service.server.ts index 45d7bea6b..fcb2c6b37 100644 --- a/app/modules/organization/service.server.ts +++ b/app/modules/organization/service.server.ts @@ -183,3 +183,27 @@ export const getUserOrganizations = async ({ userId }: { userId: string }) => { return userOrganizations; }; + +export const getOrganizationAdminsEmails = async ({ + organizationId, +}: { + organizationId: string; +}) => { + const admins = await db.userOrganization.findMany({ + where: { + organizationId, + roles: { + hasSome: [OrganizationRoles.OWNER, OrganizationRoles.ADMIN], + }, + }, + select: { + user: { + select: { + email: true, + }, + }, + }, + }); + + return admins.map((a) => a.user.email); +}; diff --git a/app/routes/_auth+/join.tsx b/app/routes/_auth+/join.tsx index cb1e6923f..d678db16a 100644 --- a/app/routes/_auth+/join.tsx +++ b/app/routes/_auth+/join.tsx @@ -41,8 +41,12 @@ const JoinFormSchema = z .string() .email("invalid-email") .transform((email) => email.toLowerCase()), - password: z.string().min(8, "password-too-short"), - confirmPassword: z.string().min(8, "password-too-short"), + password: z + .string() + .min(8, "Your password is too short. Min 8 characters are required."), + confirmPassword: z + .string() + .min(8, "Your password is too short. Min 8 characters are required."), redirectTo: z.string().optional(), }) .superRefine(({ password, confirmPassword }, ctx) => { diff --git a/app/routes/_layout+/bookings.$bookingId.tsx b/app/routes/_layout+/bookings.$bookingId.tsx index 8c5edff23..12fec54ea 100644 --- a/app/routes/_layout+/bookings.$bookingId.tsx +++ b/app/routes/_layout+/bookings.$bookingId.tsx @@ -214,6 +214,7 @@ export async function action({ request, params }: ActionFunctionArgs) { intent2ActionMap[intent] ); const id = getRequiredParam(params, "bookingId"); + const isSelfService = role === OrganizationRoles.SELF_SERVICE; switch (intent) { case "save": @@ -278,7 +279,8 @@ export async function action({ request, params }: ActionFunctionArgs) { case "reserve": await upsertBooking( { id, status: BookingStatus.RESERVED }, - getClientHint(request) + getClientHint(request), + isSelfService ); sendNotification({ title: "Booking reserved", @@ -296,7 +298,6 @@ export async function action({ request, params }: ActionFunctionArgs) { } ); case "delete": - const isSelfService = role === OrganizationRoles.SELF_SERVICE; if (isSelfService) { /** * When user is self_service we need to check if the booking belongs to them and only then allow them to delete it. From c8cce34282e6d11cd0e2a78c9c9fe0aa0c087070 Mon Sep 17 00:00:00 2001 From: Donkoko Date: Mon, 19 Feb 2024 16:37:08 +0200 Subject: [PATCH 4/4] fixing search - When searching and status was set to all, we didnt handle the case so it was breaking as ALL is not a real status --- app/components/errors/index.tsx | 8 +++++--- app/utils/list.ts | 6 +++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/components/errors/index.tsx b/app/components/errors/index.tsx index ac6a940d6..85630f7da 100644 --- a/app/components/errors/index.tsx +++ b/app/components/errors/index.tsx @@ -66,9 +66,11 @@ export const ErrorBoundryComponent = ({ {error.message} + ) : ( + "Please try again and if the issue persists, contact support" + ) } /> ); diff --git a/app/utils/list.ts b/app/utils/list.ts index bf9888368..5c0ba13b4 100644 --- a/app/utils/list.ts +++ b/app/utils/list.ts @@ -18,7 +18,11 @@ export const getParamsValues = (searchParams: URLSearchParams) => ({ ? searchParams.get("hideUnavailable") == "true" : undefined, unhideAssetsBookigIds: searchParams.getAll("unhideAssetsBookigIds") || [], - status: (searchParams.get("status") || null) as BookingStatus | null, + + status: + searchParams.get("status") === "ALL" // If the value is "ALL", we just remove the param + ? null + : (searchParams.get("status") as BookingStatus | null), }); /** Generates prev & next links */