Add Donation Receipt functionality and UI improvements #2416
Conversation
…footer sections for donation receipt page
…s with translations
…hen downloadUrl is available.
…ameters (dtn, year, challenge) - Implemented logic to fetch donation receipt data when valid query parameters are present. - Added type checks for dtn, year, and challenge before making the API request. - Updated the donation receipt data upon successful fetch. - Handled errors in case of failed API request.
…ontact support section
- added next-intl keys for "donationAmount" and "name" with currency and type-based rendering. - defined proper types for Donor, Donation, and ReceiptData for stricter TypeScript typing. - moved fetchReceiptData logic from ReceiptDataSection to DonationReceiptLayout for better separation of concerns. - simplified UI by removing firstName and lastName fields, displaying only Name. - added skeleton structure for the receipt verification page to enhance loading experience.
- added cursor pointer style to RedirectArrowButton for better usability. - Implemented the `modify-user-data` route.
- rename `donation-receipts` to `donation-receipt`. - use `useCallback` for the routing function to optimize performance.
…ents - added `formatReceiptData` helper to transform `ReceiptDataAPI` into `ReceiptData` with strict type validation. - Updated `ReceiptData` to include `operation` and `issuedDonations` fields. - Improved type safety for `DonorView`, `AddressView`, and donation-related properties. - Refactored components to align with the updated ReceiptData structure and context logic.
- Rename hasUserDataChanged to hasDonorDataChanged
…ataSection - Relocated the `confirmDonorData` logic to improve component structure and ensure relevant logic resides within `ReceiptDataSection`. -Introduce spinner for loading state.
mohitb35
changed the base branch from
tax-receipt/modify-donor-data-form
to
develop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
src/features/user/Settings/EditProfile/AddressManagement/AddAddress.tsx (1)
60-118: 🛠️ Refactor suggestionUpdate dependency array in useCallback.
The dependency array for
addAddressis missingsetUser, which is used inside the callback. This could lead to stale closures.[ contextLoaded, user, token, country, logoutUser, handleError, setIsLoading, setIsModalOpen, postAuthenticatedRequest, primaryAddressChecked, updateUserAddresses, + setUser, + setErrors, + tenantConfig.id, + setAddressAction, ]
🧹 Nitpick comments (53)
public/assets/images/icons/ContactIcon.tsx (1)
3-28: Add accessibility attributes to SVG iconThe SVG icon looks good, but it's missing some important accessibility attributes:
<svg width="45" + height="45" viewBox="0 0 45 45" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + role="img" + focusable="false" >Adding these attributes will:
- Ensure proper rendering with explicit height
- Improve accessibility by properly hiding the icon from screen readers
- Prevent focus when tabbing through interactive elements
src/features/user/DonationReceipt/microComponents/ReceiptVerificationErrors.tsx (1)
6-13: Enhance accessibility for error message displayThe component correctly displays error messages, but could benefit from improved accessibility for screen readers.
Consider adding ARIA attributes to identify this component as an error message:
- <div className={styles.donationReceiptLayout}> + <div className={styles.donationReceiptLayout}> <div className={styles.donationReceiptContainer}> - <p className={styles.baseErrorMessage}>{message}</p> + <p className={styles.baseErrorMessage} role="alert" aria-live="assertive">{message}</p> <VerifyReceiptFooter /> </div> </div>Adding
role="alert"andaria-live="assertive"will ensure screen readers announce the error message immediately when it appears.src/features/user/DonationReceipt/microComponents/SupportAssistanceInfo.tsx (2)
16-29: Improve contact information implementationThe email implementation could be enhanced for better maintainability and usability.
Consider these improvements:
- Extract the hardcoded email address to a constant or configuration value:
+const SUPPORT_EMAIL = '[email protected]'; // Then in your component - href="mailto:[email protected]" + href={`mailto:${SUPPORT_EMAIL}`}
- Add a visual indicator for the link opening in a new tab:
<a className="planet-links" href="mailto:[email protected]" target="_blank" rel="noopener noreferrer" + aria-label={`${chunk} (opens in a new tab)`} > - {chunk} + {chunk} <span className="visually-hidden">(opens in a new tab)</span> </a>
30-34: Make phone number clickable for mobile usersThe phone number display could be enhanced for better mobile usability.
Consider wrapping the phone number in a clickable telephone link:
<span> {tReceipt.rich('phoneDetails', { - strong: (chunk) => <strong>{chunk}</strong>, + strong: (chunk) => ( + <a href={`tel:${chunk.replace(/\s+/g, '')}`} className="planet-links"> + <strong>{chunk}</strong> + </a> + ), })} </span>This assumes the phone number is in the
strongchunk. If not, adjust accordingly.src/features/user/DonationReceipt/microComponents/VerifyReceiptHeader.tsx (1)
11-25: Use more semantic HTML for header elementsThe component uses a generic
sectionto contain header elements, but more semantic HTML would improve accessibility.Consider using the
headerelement instead ofsectionfor better semantics:- <section className={styles.verifyReceiptHeader}> + <header className={styles.verifyReceiptHeader}> {operation === RECEIPT_STATUS.DOWNLOAD ? ( <h2>{tReceipt('downloadTaxReceipt')}</h2> ) : ( <> <h2>{tReceipt('verifyTaxHeaderPrimary')}</h2> <h3>{tReceipt('verifyTaxHeaderSecondary')}</h3> </> )} - </section> + </header>This more clearly communicates to assistive technologies that this is a header section of the document.
pages/sites/[slug]/[locale]/verify-receipt-data.tsx (2)
65-67: Add error handling for missing tenant configurationThe code could handle the case where tenant configuration might not be retrieved successfully.
Consider adding logging or more explicit fallback handling:
const tenantConfig = (await getTenantConfig(context.params?.slug as string)) ?? defaultTenant; + + // Log when falling back to default tenant + if (!context.params?.slug || !(await getTenantConfig(context.params.slug))) { + console.warn(`Tenant config not found for slug: ${context.params?.slug}, using default tenant`); + }This provides better visibility when the default tenant is used as a fallback.
68-71: Add error handling for message loadingThe message loading has no error handling if files are missing or invalid.
Add try-catch block around message loading:
- const messages = await getMessagesForPage({ - locale: context.params?.locale as string, - filenames: ['common', 'me', 'country', 'donationReceipt'], - }); + let messages; + try { + messages = await getMessagesForPage({ + locale: context.params?.locale as string, + filenames: ['common', 'me', 'country', 'donationReceipt'], + }); + } catch (error) { + console.error(`Error loading messages for locale: ${context.params?.locale}`, error); + // Fall back to default locale or empty messages + messages = await getMessagesForPage({ + locale: 'en', + filenames: ['common', 'me', 'country', 'donationReceipt'], + }); + }This ensures the page won't fail if there are issues loading translation files.
global.d.ts (1)
1-1: ESLint rule disabled - understand the reason?You've disabled the
@typescript-eslint/consistent-type-importsrule. While this doesn't affect functionality, it would be helpful to understand if this is a project-wide decision or specific to this file.src/features/user/Settings/EditProfile/AddressManagement/microComponents/PrimaryAddressToggle.tsx (1)
1-30: LGTM! Well-structured component with clear props interface.The
PrimaryAddressTogglecomponent is well implemented with appropriate type definitions, proper use of the internationalization hook, and clean state management through props.Consider adding aria attributes to improve accessibility:
<NewToggleSwitch checked={primaryAddressChecked} onChange={(e) => setPrimaryAddressChecked(e.target.checked)} + aria-label={t('addressManagement.actions.setAsPrimaryAddress')} />src/features/user/Settings/EditProfile/AddressManagement/index.tsx (1)
151-153: Update dependencies in useMemo dependency array.The
renderModalContentfunction's dependency array still includessetUserAddresses, but this is no longer used in the function. This could cause unnecessary re-renders.}, [ setIsModalOpen, - setUserAddresses, selectedAddressForAction, updateUserAddresses, primaryAddress, billingAddress, addressAction, setAddressAction, ]);pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx (1)
20-20: Remove or uncomment unused import.There's a commented-out import for
AccessDeniedLoaderthat should either be removed if not needed or uncommented if it will be used.-// import AccessDeniedLoader from '../../../../../../src/features/common/ContentLoaders/Projects/AccessDeniedLoader';src/utils/apiRequests/api.ts (1)
259-289: LGTM with minor style inconsistenciesThe
putRequestfunction implementation is well-structured and consistent with other request functions. However, there are two minor style inconsistencies:
- There's an empty line at line 261 that doesn't appear in the similar
postRequestfunction- The catch variable name is
errorwhile other similar functions useerrThese don't affect functionality but could be standardized for better code consistency.
export function putRequest<T>({ tenant, url, data }: PutRequestOptions) { const lang = localStorage.getItem('language') || 'en'; - return new Promise<T>((resolve, reject) => { (async () => { try { const res = await fetch(process.env.API_ENDPOINT + url, { method: 'PUT', body: JSON.stringify(data), headers: { 'Content-Type': 'application/json', 'tenant-key': `${tenant}`, 'X-SESSION-ID': await getsessionId(), 'x-locale': lang, }, }); if (!res.ok) { throw new APIError(res.status, await res.json()); } if (res.status === 204) { resolve(true as T); } else { resolve(await res.json()); } - } catch (error) { - reject(error); + } catch (err) { + reject(err); } })(); }); }src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx (1)
43-48: Consider extracting address validation logicThe address validation logic is embedded directly in the component. For better maintainability and reusability, consider extracting this into a separate utility function that can be shared across components.
+ // In a separate utility file + export const isAddressInvalid = (address: AddressType) => { + return !address.address1 || !address.zipCode || !address.city || !address.country; + }; // Then in this component <DonorDetails donor={donor} address={address} - isAddressInvalid={ - !address.address1 || - !address.zipCode || - !address.city || - !address.country - } + isAddressInvalid={isAddressInvalid(address)} />src/features/user/DonationReceipt/microComponents/ReceiptActions.tsx (1)
53-54: Remove unnecessary curly braces around string literalThe
target={'_blank'}uses unnecessary curly braces around a string literal. For consistency and better readability, prefertarget="_blank".href={downloadUrl} - target={'_blank'} + target="_blank"src/features/user/DonationReceipt/microComponents/DonationsTable.tsx (2)
16-56: Improve accessibility by using semantic HTML table elementsThe component uses div/span elements with ARIA roles to simulate a table structure, which is less accessible than using native HTML table elements.
Consider refactoring to use proper HTML table elements:
-<div className={styles.donationsTable}> - <div className={styles.header} role="row"> - <span role="columnheader"> - {tReceipt('donationDetails.referenceNumber')} - </span> - <span className={styles.amountDonated} role="columnheader"> - {tReceipt('donationDetails.amountDonated')} - </span> - <span className={styles.paymentDate} role="columnheader"> - {tReceipt('donationDetails.paymentDate')} - </span> - </div> - <ul role="table"> +<div className={styles.donationsTable}> + <table> + <thead> + <tr className={styles.header}> + <th>{tReceipt('donationDetails.referenceNumber')}</th> + <th className={styles.amountDonated}>{tReceipt('donationDetails.amountDonated')}</th> + <th className={styles.paymentDate}>{tReceipt('donationDetails.paymentDate')}</th> + </tr> + </thead> + <tbody>And change the list items to table rows:
- {donations?.map(({ reference, currency, amount, paymentDate }) => { - return ( - <li className={styles.record} key={reference} role="row"> - <span className={styles.reference} role="cell"> - {reference} - </span> - <span className={styles.amount} role="cell"> - {tReceipt('donationDetails.donationAmount', { - currency, - amount, - })} - </span> - <time className={styles.date} dateTime={paymentDate} role="cell"> - {formatDate(paymentDate)} - </time> - </li> - ); - })} - </ul> + {donations?.map(({ reference, currency, amount, paymentDate }) => { + return ( + <tr className={styles.record} key={reference}> + <td className={styles.reference}>{reference}</td> + <td className={styles.amount}> + {tReceipt('donationDetails.donationAmount', { + currency, + amount, + })} + </td> + <td className={styles.date}> + <time dateTime={paymentDate}>{formatDate(paymentDate)}</time> + </td> + </tr> + ); + })} + </tbody> + </table>
29-46: Add defensive check for empty donations arrayThe component directly maps over the donations array without checking if it's empty, which could result in an empty table with just headers.
Consider adding a conditional check to display a message when no donations are available:
<ul role="table"> + {donations.length === 0 ? ( + <li className={styles.emptyState}> + {tReceipt('donationDetails.noDonations')} + </li> + ) : ( {donations?.map(({ reference, currency, amount, paymentDate }) => { return ( <li className={styles.record} key={reference} role="row"> <span className={styles.reference} role="cell"> {reference} </span> <span className={styles.amount} role="cell"> {tReceipt('donationDetails.donationAmount', { currency, amount, })} </span> <time className={styles.date} dateTime={paymentDate} role="cell"> {formatDate(paymentDate)} </time> </li> ); })} + )} </ul>src/features/user/DonationReceipt/DonationReceipts.tsx (2)
47-48: Improve error handling with user feedbackCurrently, the component redirects to the homepage on any error without providing feedback to the user.
Consider showing an error message before redirecting or providing a more specific error page:
} catch (error) { handleError(error as APIError); + // Display an error message or toast notification + // Consider using a delay before redirecting redirect('/'); }
89-94: Add loading text for screen readers with skeleton loaderThe skeleton loader is visual-only without text for screen readers.
Add an aria-label or screen reader text to improve accessibility:
if (!donationReceipts) return ( <section className={styles.donorContactManagementLayout}> - <Skeleton height={600} width={524} /> + <Skeleton height={600} width={524} aria-label="Loading donation receipts" /> + <span className="sr-only">Loading donation receipts</span> </section> );src/features/user/DonationReceipt/microComponents/DonationInfoPopover.tsx (2)
12-12: Add JSDoc comment to document the Donation typeThe Donation type represents a union type but lacks documentation.
Add a JSDoc comment to clarify the purpose and usage of this type:
+/** + * Represents either an issued or unissued donation object from the API + */ type Donation = UnissuedDonationApi | IssuedDonationApi;
62-67: Add error handling for the donations map operationThe component directly maps over donations without checking for empty arrays or null values.
Add a check for empty arrays and handle potential errors when accessing paymentDate:
<tbody> - {donations.map((item) => ( + {donations.length === 0 ? ( + <tr> + <td colSpan={2}>No donations to display</td> + </tr> + ) : donations.map((item) => ( <tr key={getDonationReference(item)}> <td>{getDonationReference(item)}</td> - <td>{formatDate(item.paymentDate)}</td> + <td>{item.paymentDate ? formatDate(item.paymentDate) : 'Date unavailable'}</td> </tr> - ))} + ))} </tbody>pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx (2)
29-32: Remove commented code and use translation for page titleThere's a commented title tag and the current title is hardcoded without using translations.
Remove the commented code and use the translation function:
<Head> - {/*<title>{t('receipts')}</title>*/} - <title>Receipts</title> + <title>{tReceipt('receipts')}</title> </Head>This requires adding the translation hook at the top of the component:
const tReceipt = useTranslations('DonationReceipt');
38-55: Consider improving the static path generation strategyThe current implementation uses 'blocking' for fallback, which might lead to slower page loads.
Consider using 'true' with a loading state for a better user experience:
return { paths, - fallback: 'blocking', + fallback: true, };Then handle the fallback state in the component:
import { useRouter } from 'next/router'; export default function DonationReceiptPage({ pageProps }: Props) { const router = useRouter(); const { setTenantConfig } = useTenant(); // Show a loading state while the page is being generated if (router.isFallback) { return <LoadingComponent />; } // Rest of the component... }src/features/user/DonationReceipt/microComponents/DonorAddressList.tsx (2)
46-55: Ensure consistency with formattedAddress parametersYour
getFormattedAddressutility is called withnullas the third parameter, but the dependency array doesn't match all the used variables. The country translation is dependent ontCountrywhich isn't included in dependencies.const formattedAddress = useMemo( () => getFormattedAddress( address.zipCode, address.city, null, tCountry(address.country.toLowerCase() as Lowercase<CountryCode>) ), - [zipCode, city, country] + [zipCode, city, country, tCountry] );
58-63: Add fallback address selection logicThe current implementation only auto-selects an address if it's the primary address. Consider adding a fallback to select the first available address if no primary address exists to ensure the user always has an address selected.
src/features/user/DonationReceipt/DonorContactManagement.tsx (2)
93-103: Ensure address properties are typed properlyThe address view construction uses nullish coalescing with empty strings, which can lead to empty fields in the form. Consider using
undefinedinstead to allow form validation to catch missing required fields, or add validation before submission.
169-169: Provide a meaningful fallback componentUsing
<div />as a fallback in the modal whenrenderModalContent()returns null isn't descriptive. Consider creating a more meaningful empty state component or adding an explanatory message.- {renderModalContent() || <div />} + {renderModalContent() || <div className={styles.emptyModalState}>No address action selected</div>}pages/sites/[slug]/[locale]/profile/donation-receipt/index.tsx (1)
33-50: Consider supporting multiple locales in getStaticPathsCurrently, only the 'en' locale is hardcoded in the static paths generation. This might limit internationalization if other locales are supported by the application.
export const getStaticPaths: GetStaticPaths = async () => { const subDomainPaths = await constructPathsForTenantSlug(); + const supportedLocales = ['en', 'de', 'es', 'fr']; // Add all supported locales const paths = subDomainPaths?.map((path) => { - return { - params: { - slug: path.params.slug, - locale: 'en', - }, - }; + return supportedLocales.map((locale) => ({ + params: { + slug: path.params.slug, + locale, + }, + })); - }) ?? []; + }).flat() ?? []; return { paths, fallback: 'blocking', }; };src/utils/apiRequests/apiClient.ts (1)
33-41: Simplify URL construction logicThe URL construction logic can be simplified for better readability and maintenance.
// Construct full URL const queryString = options.queryParams ? '?' + getQueryString(options.queryParams) : ''; - const fullUrl = options.url.startsWith('http') - ? options.url - : `${baseUrl}${options.url.startsWith('/') ? '' : '/'}${ - options.url - }${queryString}`; + const fullUrl = options.url.startsWith('http') + ? `${options.url}${queryString}` + : `${baseUrl}${options.url.startsWith('/') ? options.url : `/${options.url}`}${queryString}`;src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx (1)
48-58: Improve error handling and provide more user-friendly feedback.Currently, errors from the API are handled by redirecting to the home page or marking the receipt as invalid. Consider adding more detailed user feedback for different error scenarios.
} catch (err) { const errorResponse = err as APIError; setErrors(handleError(errorResponse)); if (errorResponse.statusCode === 400) { setIsReceiptInvalid(true); + // Log specific validation error details for debugging + console.debug('Receipt validation failed:', errorResponse.message); } else { + // Provide more context about the error before redirecting + console.error('Failed to fetch receipt data:', errorResponse.message); redirect('/'); } }src/features/user/DonationReceipt/DonationReceiptWrapper.tsx (2)
55-116: Improve error handling and refactor the complex confirmReceiptData function.The current implementation logs errors to the console but doesn't provide user feedback. Also, the function is quite complex and could be broken down into smaller, more focused functions.
Consider:
- Extracting the API call logic into separate functions for verify and issue operations
- Implementing a more robust error handling mechanism that provides feedback to the user
- Adding loading indicators or feedback during the confirmation process
Example refactoring approach:
const confirmReceiptData = async () => { const donor = getDonor(); const address = getAddress(); const addressGuid = getAddressGuid(); const donationUids = getDonationUids(); if (!donor || !address || !receiptData) { console.error('❌ Missing required data for confirmation.'); + // Show user feedback + // e.g., setError('Missing required data for confirmation'); return; } setIsLoading(true); try { + let response = null; + if (operation === RECEIPT_STATUS.VERIFY) { + response = await handleVerifyOperation(receiptData, addressGuid, verificationDate); + } else if (operation === RECEIPT_STATUS.ISSUE) { + response = await handleIssueOperation(addressGuid, donationUids, verificationDate); + } - let response = null; - const verificationDate = new Date() - .toISOString() - .slice(0, 19) - .replace('T', ' '); - if (operation === RECEIPT_STATUS.VERIFY) { - const payload = { - dtn: receiptData.dtn, - challenge: receiptData.challenge, - year: receiptData.year, - // set to the current date time - verificationDate, - ...(addressGuid ? { receiptAddress: addressGuid } : {}), - }; - - response = addressGuid - ? await putApi<IssuedReceiptDataApi>( - '/app/donationReceipt/verify', - payload - ) - : await putApiAuthenticated<IssuedReceiptDataApi>( - '/app/donationReceipt/verify', - payload - ); - } else if (operation === RECEIPT_STATUS.ISSUE) { - const payload = { - receiptAddress: addressGuid, - donationUids: JSON.stringify(donationUids), - verificationDate, - }; - - response = await postApiAuthenticated<IssuedReceiptDataApi>( - '/app/donationReceipts', - payload - ); - } if (response) { initForVerification(response, null); } else { console.error('❌ Failed to process receipt.'); + // Show user feedback + // e.g., setError('Failed to process receipt'); } } catch (error) { console.error('❌ Error during receipt operation:', error); + // Show user feedback + // e.g., setError(`Error during receipt operation: ${error.message}`); } finally { setIsLoading(false); } }; + // Helper function for verification operation + const handleVerifyOperation = async (receiptData, addressGuid, verificationDate) => { + const payload = { + dtn: receiptData.dtn, + challenge: receiptData.challenge, + year: receiptData.year, + verificationDate, + ...(addressGuid ? { receiptAddress: addressGuid } : {}), + }; + + return addressGuid + ? await putApi<IssuedReceiptDataApi>('/app/donationReceipt/verify', payload) + : await putApiAuthenticated<IssuedReceiptDataApi>('/app/donationReceipt/verify', payload); + }; + + // Helper function for issuance operation + const handleIssueOperation = async (addressGuid, donationUids, verificationDate) => { + const payload = { + receiptAddress: addressGuid, + donationUids: JSON.stringify(donationUids), + verificationDate, + }; + + return await postApiAuthenticated<IssuedReceiptDataApi>('/app/donationReceipts', payload); + };
70-73: Improve date formatting for better readability.The current date formatting is quite verbose and hard to read. Consider using a dedicated date formatting utility or the Intl.DateTimeFormat API.
- const verificationDate = new Date() - .toISOString() - .slice(0, 19) - .replace('T', ' '); + // Format date in SQL format (YYYY-MM-DD HH:MM:SS) + const now = new Date(); + const verificationDate = new Intl.DateTimeFormat('en-CA', { + year: 'numeric', + month: '2-digit', + day: '2-digit', + hour: '2-digit', + minute: '2-digit', + second: '2-digit', + hour12: false, + }) + .format(now) + .replace(/(\d+)\/(\d+)\/(\d+), (\d+:\d+:\d+)/, '$3-$1-$2 $4');Alternatively, consider using a library like date-fns for more robust date handling.
src/features/user/DonationReceipt/DonationReceiptValidator.ts (1)
18-22: Consider more robust address validation.The current implementation only checks for the existence of required fields but doesn't validate their format or content. For a production application, consider adding more robust validation for addresses.
For example, you could check:
- If zipCode follows country-specific format
- If country is a valid country code
- If city and address1 have reasonable lengths
const validateAddress = (address: AddressView | null): boolean => { - return Boolean( - address?.address1 && address?.city && address?.zipCode && address?.country - ); + if (!address) return false; + + // Check required fields exist + if (!address.address1 || !address.city || !address.zipCode || !address.country) { + return false; + } + + // Add additional validation as needed: + // - Check if zipCode follows country-specific format + // - Validate country is a valid country code + // - Verify city and address1 have reasonable lengths + + return true; };src/features/user/Settings/EditProfile/AddressManagement/AddAddress.tsx (1)
81-96: Consider extracting address update logic to a separate function.The logic for updating addresses when a new address is added is complex and could be clearer if extracted to a separate function.
+ // Helper function to update addresses when adding a new one + const getUpdatedAddresses = ( + currentAddresses: Address[] = [], + newAddress: Address + ): Address[] => { + // If the new address is primary, update all existing primary addresses to OTHER + const updatedExistingAddresses = + newAddress.type === ADDRESS_TYPE.PRIMARY + ? currentAddresses.map((addr) => + addr.type === ADDRESS_TYPE.PRIMARY + ? { ...addr, type: ADDRESS_TYPE.OTHER } + : addr + ) + : currentAddresses; + + // Add the new address to the list + return [...updatedExistingAddresses, newAddress]; + }; setUser((prev) => { if (!prev) return null; - const updatedAddresses = - res.type === ADDRESS_TYPE.PRIMARY - ? prev.addresses.map((addr) => - addr.type === ADDRESS_TYPE.PRIMARY - ? { ...addr, type: ADDRESS_TYPE.OTHER } - : addr - ) - : prev.addresses; - + const updatedAddresses = getUpdatedAddresses(prev.addresses, res); + return { ...prev, - addresses: [...updatedAddresses, res], + addresses: updatedAddresses, }; });src/features/user/DonationReceipt/microComponents/DonorContactForm.tsx (3)
103-107: Extract address handling logic to a separate method.The address handling logic could be encapsulated in a separate method for better maintainability.
+ const handleAddNewAddress = () => { + setIsModalOpen(true); + setAddressAction(ADDRESS_ACTIONS.ADD); + };
74-74: Consider providing feedback when user is null.The early return when
useris null is good for defensive coding, but consider adding a fallback UI or logging to help debug why a user might be missing.- if (!user) return null; + if (!user) { + console.warn('DonorContactForm: No user data provided'); + return <div className={styles.donationReceiptSpinner}>Loading user data...</div>; + }
107-107: Consider memoizing the address limit check.The
hasReachedAddressLimitcalculation could be memoized to avoid recalculation on every render.- const hasReachedAddressLimit = user.addresses.length >= MAX_ADDRESS_LIMIT; + const hasReachedAddressLimit = useMemo( + () => user.addresses.length >= MAX_ADDRESS_LIMIT, + [user.addresses.length] + );src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressForm.tsx (1)
7-7: Consider using a consistent import style for types.The addition of the AddressType import is good, but consider using a consistent import style for all types (either individual imports or grouped imports).
src/features/user/DonationReceipt/microComponents/DonorDetails.tsx (3)
51-53: Improve type safety in country translation handling.The country name retrieval could be more safely typed to avoid potential type errors.
- const countryName = country - ? tCountry(country.toLowerCase() as Lowercase<CountryCode>) - : ''; + const countryName = country + ? tCountry(country.toLowerCase() as Lowercase<CountryCode>) + : '';
62-64: Use consistent address formatting.Consider using the getFormattedAddress utility for the entire address to ensure consistency.
- <address> - {address1}, {cityStatePostalString} - </address> + <address> + {address1} + {address1 && cityStatePostalString && <span>, </span>} + {cityStatePostalString} + </address>
16-21: Consider making ErrorMessage more reusable.The ErrorMessage component is well-implemented but could be made more reusable by accepting additional props for customization.
-const ErrorMessage = ({ message }: { message: string }) => ( +const ErrorMessage = ({ + message, + className = '', + icon = <MissingDataIcon /> +}: { + message: string; + className?: string; + icon?: React.ReactNode; +}) => ( - <div className={styles.errorMessageContainer}> - <MissingDataIcon /> + <div className={`${styles.errorMessageContainer} ${className}`}> + {icon} <span className={styles.errorMessage}>{message}</span> </div> );src/features/common/Layout/DonationReceiptContext.tsx (1)
128-128: Remove debug log statement.Leaving
console.login production code may lead to unintended information exposure and clutter. Consider removing or replacing it with a more structured logging approach (if needed).- console.log(state, '==2');src/hooks/useServerApi.ts (1)
80-80: Confirm usage of HTTP 498 as an invalid token status code.HTTP 498 is non-standard. Ensure the backend or edge services consistently return this code for invalid tokens and that clients are aware of it. If not, consider standardizing on an official status code.
src/features/user/Settings/EditProfile/AddressManagement/EditAddress.tsx (1)
83-99: Potential refactor to unify primary address assignment.When applying the newly updated primary address to
res, you also reset any previously primary address within the user’s address list. Consider extracting this logic into a helper function or clarifying the block with descriptive comments. This can improve maintainability and highlight the constraint of one primary address at a time.src/features/user/DonationReceipt/DonationReceipt.module.scss (4)
391-402: Consider usingoverflow: autoinstead ofoverflow: scroll.The
.donationReceiptsclass usesoverflow: scrollwhich forces scrollbars to appear even when not needed in some browsers. Usingoverflow: autowould display scrollbars only when content overflows, creating a cleaner UI when scrolling isn't necessary..donationReceipts { max-width: 524px; height: 494px; - overflow: scroll; + overflow: auto; width: 100%; display: flex; flex-direction: column; gap: 21px; margin-top: 21px; scrollbar-width: none; /* Hides scrollbar in Firefox */ -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ }
361-363: Fix conflicting margin declarations.There are two margin declarations, with the second one (
margin: 42px 12px 0px;) overriding the first (margin-top: 68px;). This appears redundant and potentially confusing..donorContactFormAction { display: flex; justify-content: space-between; gap: 12px; - margin-top: 68px; margin: 42px 12px 0px; button { flex: 1; }
56-57: Extract color values to variables for better maintainability.Several hardcoded color values are used throughout the stylesheet (
#fff,#ccc, rgba values). Consider extracting these to SCSS variables or using existing theme variables for better consistency and easier theme management.Also applies to: 113-116, 200-202, 412-413
177-184: Add focus styles for interactive elements.The styling for interactive elements (links, buttons) doesn't include focus states, which is important for accessibility. Consider adding focus styles to ensure keyboard navigation is visually apparent.
a, button { width: 100%; + &:focus-visible { + outline: 2px solid var(--primary-color); + outline-offset: 2px; + } } svg { width: 13px; }src/features/user/DonationReceipt/transformers.tsx (3)
50-60: Add fallback for empty user name.When transforming a user profile to a donor view, there's a risk of creating an empty name when both
firstnameandlastnameare undefined. Although.trim()is used, it would be better to provide a fallback.export const transformProfileToDonorView = (user: User): DonorView => ({ name: user.type === 'individual' - ? `${user.firstname ?? ''} ${user.lastname ?? ''}`.trim() + ? `${user.firstname ?? ''} ${user.lastname ?? ''}`.trim() || 'Unknown' : user.name ?? null, tin: user.tin ?? null, type: user.type === 'individual' || user.type === 'organization' ? user.type : null, });
13-29: Consider consolidating similar transformation functions.The
transformIssuedDonationandtransformUnissuedDonationfunctions are nearly identical except for the reference field source. Consider refactoring to reduce duplication.-export const transformIssuedDonation = ( - item: IssuedDonationApi -): DonationView => ({ - reference: item.reference, - amount: item.amount, - currency: item.currency, - paymentDate: item.paymentDate, -}); - -export const transformUnissuedDonation = ( - item: UnissuedDonationApi -): DonationView => ({ - reference: item.uid, - amount: item.amount, - currency: item.currency, - paymentDate: item.paymentDate, -}); +export const transformDonation = <T extends { + amount: number; + currency: string; + paymentDate: string; + reference?: string; + uid?: string; +}>( + item: T, + referenceField: 'reference' | 'uid' = 'reference' +): DonationView => ({ + reference: item[referenceField] as string, + amount: item.amount, + currency: item.currency, + paymentDate: item.paymentDate, +}); + +export const transformIssuedDonation = ( + item: IssuedDonationApi +): DonationView => transformDonation(item, 'reference'); + +export const transformUnissuedDonation = ( + item: UnissuedDonationApi +): DonationView => transformDonation(item, 'uid');
74-76: Use structured logging for production code.Using
console.warnis fine for development but may not be ideal for production. Consider implementing a more structured logging approach that can be configured for different environments.if (!primaryAddress) { - console.warn('No primary address found for user.'); + // Use a configurable logger service instead + // loggerService.warn('No primary address found for user.'); return null; }src/features/user/DonationReceipt/donationReceiptTypes.ts (3)
34-34: Simplify the Operation type definition.The current definition of the
Operationtype uses a complex syntax. For better readability, consider using a simple union type based on the values fromRECEIPT_STATUS.-export type Operation = (typeof RECEIPT_STATUS)[keyof typeof RECEIPT_STATUS]; +export type Operation = 'verify' | 'download' | 'issue';
36-36: Improve documentation for API interfaces.The comment "// latest API" is vague. Provide more specific documentation about API versioning or the purpose of these interfaces for better maintainability.
-// latest API +// API interfaces for donation receipt service - V2 endpoints
52-67: Group related properties in complex interfaces.Interfaces like
IssuedReceiptDataApiandReceiptDatahave many properties. Consider organizing related properties into nested objects for better structure and readability.export interface IssuedReceiptDataApi { + // Basic receipt information amount: number; currency: string; reference: string; dtn: string; + + // Contextual information challenge: string; country: string; donationCount: number; year: string; tinIsRequired: boolean | null; + + // Dates paymentDate: string; verificationDate: string | null; + + // Related entities donations: IssuedDonationApi[]; donor: DonorApi; downloadUrl: string; }Also applies to: 69-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package.jsonis excluded by!**/*.jsonpublic/static/locales/en/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/en/me.jsonis excluded by!**/*.json
📒 Files selected for processing (53)
global.d.ts(3 hunks)i18n.ts(2 hunks)pages/_app.tsx(2 hunks)pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx(1 hunks)pages/sites/[slug]/[locale]/profile/donation-receipt/index.tsx(1 hunks)pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx(1 hunks)pages/sites/[slug]/[locale]/verify-receipt-data.tsx(1 hunks)public/assets/images/icons/ContactIcon.tsx(1 hunks)public/assets/images/icons/DonorAddressCheckIcon.tsx(1 hunks)public/assets/images/icons/MissingDataIcon.tsx(1 hunks)public/assets/images/icons/RedirectRightArrowIcon.tsx(1 hunks)src/features/common/Layout/DonationReceiptContext.tsx(1 hunks)src/features/common/Layout/UserLayout/UserLayout.module.scss(1 hunks)src/features/common/Layout/UserLayout/UserLayout.tsx(1 hunks)src/features/common/Layout/UserPropsContext.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceiptAuthenticated.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceiptUnauthenticated.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceiptValidator.ts(1 hunks)src/features/user/DonationReceipt/DonationReceiptWrapper.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceipts.tsx(1 hunks)src/features/user/DonationReceipt/DonorContactManagement.tsx(1 hunks)src/features/user/DonationReceipt/donationReceiptTypes.ts(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfoPopover.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationReceipt.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationsTable.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonorAddressList.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonorContactForm.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonorDetails.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/EditPermissionDenied.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/ReceiptActions.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/ReceiptVerificationErrors.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/StyledCheckbox.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/SupportAssistanceInfo.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/VerifyReceiptFooter.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/VerifyReceiptHeader.tsx(1 hunks)src/features/user/DonationReceipt/transformers.tsx(1 hunks)src/features/user/Settings/EditProfile/AddressManagement/AddAddress.tsx(7 hunks)src/features/user/Settings/EditProfile/AddressManagement/AddressManagement.module.scss(1 hunks)src/features/user/Settings/EditProfile/AddressManagement/EditAddress.tsx(6 hunks)src/features/user/Settings/EditProfile/AddressManagement/index.tsx(2 hunks)src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressForm.tsx(5 hunks)src/features/user/Settings/EditProfile/AddressManagement/microComponents/PrimaryAddressToggle.tsx(1 hunks)src/hooks/useServerApi.ts(1 hunks)src/utils/addressManagement.ts(4 hunks)src/utils/apiRequests/api.ts(2 hunks)src/utils/apiRequests/apiClient.ts(1 hunks)src/utils/apiRequests/impersonation.tsx(1 hunks)src/utils/language/getMessagesForPage.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/features/common/Layout/UserLayout/UserLayout.module.scss
- src/features/user/DonationReceipt/microComponents/StyledCheckbox.tsx
- src/features/user/DonationReceipt/DonationReceiptAuthenticated.tsx
- public/assets/images/icons/RedirectRightArrowIcon.tsx
- src/utils/addressManagement.ts
- public/assets/images/icons/DonorAddressCheckIcon.tsx
- public/assets/images/icons/MissingDataIcon.tsx
🧰 Additional context used
🧠 Learnings (3)
src/features/user/Settings/EditProfile/AddressManagement/AddressManagement.module.scss (1)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2286
File: src/features/user/Settings/EditProfile/AddressManagement/AddressManagement.module.scss:51-69
Timestamp: 2024-12-12T06:09:14.416Z
Learning: In `src/features/user/Settings/EditProfile/AddressManagement/AddressManagement.module.scss`, the user prefers to ignore accessibility suggestions related to the kebab menu button's touch target size and contrast.
src/features/user/Settings/EditProfile/AddressManagement/index.tsx (2)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2286
File: src/features/user/Settings/EditProfile/AddressManagement/index.tsx:52-65
Timestamp: 2024-12-12T06:50:16.513Z
Learning: In future reviews, do not suggest adding a retry mechanism to the `updateUserAddresses` function in `src/features/user/Settings/EditProfile/AddressManagement/index.tsx`.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2286
File: src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressList.tsx:25-34
Timestamp: 2024-12-12T06:18:38.174Z
Learning: In the `AddressList` component (`src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressList.tsx`), the `addresses` prop is always defined, so null checks before mapping the `addresses` array are not required.
src/features/user/Settings/EditProfile/AddressManagement/EditAddress.tsx (1)
Learnt from: sunilsabatp
PR: Plant-for-the-Planet-org/planet-webapp#2286
File: src/features/user/Settings/EditProfile/AddressManagment/EditAddress.tsx:0-0
Timestamp: 2024-12-12T06:59:25.623Z
Learning: In 'src/features/user/Settings/EditProfile/AddressManagement/EditAddress.tsx', the `updateAddress` function relies on the React useForm hook for validation, so manual validation before the API call is unnecessary.
🔇 Additional comments (43)
src/utils/apiRequests/impersonation.tsx (1)
1-4: Type definition looks good.Clean, minimalistic type definition for impersonation data with required fields.
src/features/common/Layout/UserLayout/UserLayout.tsx (1)
270-274: Navigation integration is properly implemented.The new donation receipts menu item is correctly added to the payments section, following the existing pattern for submenu items and utilizing the translation system.
src/utils/language/getMessagesForPage.ts (1)
13-13: Internationalization support properly added.The addition of 'donationReceipt' to the TRANSLATION_FILE_NAMES type correctly enables internationalization for the new feature.
src/features/common/Layout/UserPropsContext.tsx (1)
97-97: Proper cleanup on logout.Good addition to clear the donation receipt context from session storage during logout, maintaining application state integrity.
src/features/user/Settings/EditProfile/AddressManagement/AddressManagement.module.scss (2)
138-146: LGTM! Toggle container styling matches app design patternsThe new toggle container styling is well-structured with appropriate spacing, colors and border radius that match existing app styling patterns.
152-152: Reduction in spacing improves UI densityReducing the margin-top from 25px to 15px provides a more compact layout that maintains good visual hierarchy while improving space efficiency.
src/features/user/DonationReceipt/microComponents/EditPermissionDenied.tsx (1)
1-21: LGTM! Clean permission error component with internationalization supportThis component is well-structured with proper use of:
- Internationalization through
useTranslations- Rich text formatting for emphasis
- Modular design with reusable footer component
The error message implementation with rich text formatting is particularly well done, allowing for proper translation while maintaining formatting.
i18n.ts (2)
16-17: LGTM! Properly integrated donation receipt translationsThe donation receipt translations are correctly added to the user messages object, following the established pattern in the file.
55-56: LGTM! Default translations properly configuredThe English default translations for donation receipts are correctly added to the default messages object, ensuring proper fallback behavior.
global.d.ts (2)
13-14: LGTM! New donation receipt type added correctly.The
MessagesDonationReceipttype is defined following the same pattern as other message types in the file.
49-49: LGTM! Type integration done properly.The
MessagesDonationReceipttype is correctly integrated into theMessagesintersection type.src/features/user/Settings/EditProfile/AddressManagement/index.tsx (2)
86-88: LGTM! Props updated for better component structure.The changes properly update the
AddAddresscomponent to useupdateUserAddressesinstead ofsetUserAddressesand add theshowPrimaryAddressToggleprop. This aligns with the PR's refactoring goals.
98-99: LGTM! Consistent prop addition across components.Adding
showPrimaryAddressToggletoEditAddressmaintains consistency with how theAddAddresscomponent is configured.src/features/user/DonationReceipt/microComponents/DonationReceipt.tsx (1)
1-39: LGTM! Well-structured donation receipt component.The
DonationReceiptcomponent is well-designed with a clear separation of concerns - header, data section, and footer. The props interface is comprehensive and the component fits well into the broader donation receipt functionality described in the PR objectives.src/features/user/DonationReceipt/microComponents/VerifyReceiptFooter.tsx (1)
1-29: LGTM! Component structure and implementation looks good.The component is well-structured and implements internationalization correctly using the
useTranslationshook. The layout is clean with appropriate section and div elements for organizing content.pages/_app.tsx (2)
55-55: Appropriate import for the new context provider.The import statement for
DonationReceiptProvideris correctly placed with other context provider imports.
306-313: Well-positioned context provider in component hierarchy.The
DonationReceiptProvideris correctly positioned in the component tree, wrapping both the conditionalMapHoldercomponent and thepageContent. This ensures all components that need access to donation receipt data can consume this context.pages/sites/[slug]/[locale]/profile/donation-receipt/donor-contact-management.tsx (2)
33-50: LGTM! Well-implemented static paths generation.The
getStaticPathsimplementation correctly constructs paths for tenant slugs, with appropriate fallback strategy set to 'blocking'.
57-74: LGTM! Well-implemented static props retrieval.The
getStaticPropsfunction correctly retrieves tenant configuration and internationalized messages based on the provided context.src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx (2)
33-40: LGTM! Conditionally renders action button based on receipt type.The component appropriately shows a verification and download button only when the receipt type is not PENDING.
41-47: LGTM! Good use of conditional rendering for pending receipts.The component correctly displays a badge for pending receipts with proper internationalization that includes the donation count.
src/utils/apiRequests/api.ts (1)
51-54: Looks good - new interface follows established patternThe new
PutRequestOptionsinterface is well-defined and follows the same pattern as other request option interfaces in the file.src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx (2)
9-14: Props interface is well-definedThe interface clearly defines the required props with appropriate types.
50-61: Good conditional rendering based on loading stateThe component properly handles the loading state by conditionally rendering either the ReceiptActions or a loading spinner.
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (2)
29-41: Well-implemented popover state managementGood use of
useStateanduseCallbackhooks for managing the popover state. The callbacks are correctly memoized with empty dependency arrays since they don't depend on any external variables.
51-69: Good conditional rendering based on donation countThe component intelligently renders different UI elements based on whether there is a single donation or multiple donations, providing appropriate user interaction for each case.
src/features/user/DonationReceipt/microComponents/ReceiptActions.tsx (2)
22-23: Good use of derived stateCreating a derived boolean for
showDownloadButtonimproves readability and helps avoid repetition in the conditional rendering.
25-62: Well-structured conditional rendering for different statesThe component does a good job of handling different states (unverified receipt, verified with download URL, verified without download URL) with clear conditional rendering and appropriate UI elements for each case.
src/features/user/DonationReceipt/DonorContactManagement.tsx (1)
20-20: Remove TODO comment before productionThis TODO comment should be addressed or removed before merging to production.
pages/sites/[slug]/[locale]/profile/donation-receipt/index.tsx (1)
63-66: LGTM: Appropriate i18n file inclusionThe inclusion of 'donationReceipt' in the messages filenames is appropriate for the new feature being added.
src/features/user/DonationReceipt/DonationReceiptValidator.ts (4)
8-16: LGTM - TIN validation logic is clear and well-documented.The function correctly handles both authenticated and unauthenticated scenarios, and the documentation explains the logic well.
24-31: LGTM - Ownership validation is clear and handles edge cases.The function properly handles all scenarios: when no user is logged in, when no email is provided, and when comparing emails.
33-40: LGTM - Issued receipt validation combines TIN and address validations well.The function effectively combines the two validation functions to create a comprehensive validation for issued receipts.
42-54: LGTM - Unissued receipt validation adds address GUID check to the base validations.The function builds on the existing validations and adds a check for the address GUID, which is required for unissued receipts.
src/features/user/Settings/EditProfile/AddressManagement/AddAddress.tsx (1)
77-96: 🛠️ Refactor suggestionAdd null check for addresses before mapping.
When updating the user state, you're mapping over
prev.addressesbut not checking if it exists. This could cause errors ifprev.addressesis undefined.setUser((prev) => { if (!prev) return null; const updatedAddresses = - res.type === ADDRESS_TYPE.PRIMARY - ? prev.addresses.map((addr) => - addr.type === ADDRESS_TYPE.PRIMARY - ? { ...addr, type: ADDRESS_TYPE.OTHER } - : addr - ) - : prev.addresses; + res.type === ADDRESS_TYPE.PRIMARY && prev.addresses + ? prev.addresses.map((addr) => + addr.type === ADDRESS_TYPE.PRIMARY + ? { ...addr, type: ADDRESS_TYPE.OTHER } + : addr + ) + : prev.addresses || []; return { ...prev, addresses: [...updatedAddresses, res], }; });⛔ Skipped due to learnings
Learnt from: mohitb35 PR: Plant-for-the-Planet-org/planet-webapp#2286 File: src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressList.tsx:25-34 Timestamp: 2024-12-12T06:18:38.174Z Learning: In the `AddressList` component (`src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressList.tsx`), the `addresses` prop is always defined, so null checks before mapping the `addresses` array are not required.src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx (1)
15-60: Good implementation of conditional rendering based on verification status.The component correctly handles different states based on the receipt verification status, providing appropriate actions for each case.
src/features/user/DonationReceipt/microComponents/DonorContactForm.tsx (1)
46-61: Well-structured reusable FormInput component.The FormInput component effectively encapsulates the common functionality for form inputs, making the code more maintainable and consistent.
src/features/user/Settings/EditProfile/AddressManagement/microComponents/AddressForm.tsx (1)
237-243: Good implementation of conditional primary address toggle.The conditional rendering of the PrimaryAddressToggle component is implemented correctly, showing the toggle only when appropriate (when
showPrimaryAddressToggleis true and the address is not already primary).src/features/user/DonationReceipt/microComponents/DonorDetails.tsx (1)
86-106: Well-structured component with good error handling.The DonorDetails component effectively manages different subcomponents and handles error states appropriately to provide a good user experience.
src/features/common/Layout/DonationReceiptContext.tsx (1)
77-86: Consider potential privacy implications of storing PII in session storage.This context state may contain personally identifiable information such as donor addresses, emails, and other sensitive data. Storing such data in session storage can be a privacy risk if a user leaves their device unattended. Consider encrypting these values or finding a more secure storage mechanism if compliance and privacy requirements demand it.
src/hooks/useServerApi.ts (1)
121-121: Verify compatibility of hard-coded fallback API version.The
{ 'x-accept-versions': '1.0.3' }fallback might be outdated or inconsistent with newer endpoints. Ensure that your backend supports the declared version and that any version mismatches won't break clients.src/features/user/Settings/EditProfile/AddressManagement/EditAddress.tsx (2)
52-57: Good approach for handling the primary address toggle.Automatically checking whether the address is primary in the
useEffectis a neat way to set the toggle state whenever the chosen address changes. This logic is simple and clear.
66-68: Ensuring correct primary address assignment.Using
primaryAddressChecked ? ADDRESS_TYPE.PRIMARY : selectedAddressForAction?.typemaintains consistent address typing. This is a valid approach for toggling primary vs. non-primary states.
| export const getStaticPaths: GetStaticPaths = async () => { | ||
| const subDomainPaths = await constructPathsForTenantSlug(); | ||
|
|
||
| const paths = | ||
| subDomainPaths?.map((path) => { | ||
| return { | ||
| params: { | ||
| slug: path.params.slug, | ||
| locale: 'en', | ||
| }, | ||
| }; | ||
| }) ?? []; | ||
|
|
||
| return { | ||
| paths, | ||
| fallback: 'blocking', | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make locale handling more dynamic in static paths generation
The current implementation hardcodes 'en' as the locale in getStaticPaths, which limits internationalization flexibility.
Consider making the locale dynamic instead of hardcoded:
export const getStaticPaths: GetStaticPaths = async () => {
const subDomainPaths = await constructPathsForTenantSlug();
+ const supportedLocales = ['en', 'de', 'es']; // Add your supported locales or fetch them from config
const paths =
subDomainPaths?.map((path) => {
- return {
- params: {
- slug: path.params.slug,
- locale: 'en',
- },
- };
+ return supportedLocales.map((locale) => ({
+ params: {
+ slug: path.params.slug,
+ locale,
+ },
+ }));
- }) ?? [];
+ }).flat() ?? [];
return {
paths,
fallback: 'blocking',
};
};This creates path combinations for all supported locales and slugs, allowing proper static generation for multiple languages.
📝 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.
| export const getStaticPaths: GetStaticPaths = async () => { | |
| const subDomainPaths = await constructPathsForTenantSlug(); | |
| const paths = | |
| subDomainPaths?.map((path) => { | |
| return { | |
| params: { | |
| slug: path.params.slug, | |
| locale: 'en', | |
| }, | |
| }; | |
| }) ?? []; | |
| return { | |
| paths, | |
| fallback: 'blocking', | |
| }; | |
| }; | |
| export const getStaticPaths: GetStaticPaths = async () => { | |
| const subDomainPaths = await constructPathsForTenantSlug(); | |
| const supportedLocales = ['en', 'de', 'es']; // Add your supported locales or fetch them from config | |
| const paths = | |
| subDomainPaths?.map((path) => { | |
| return supportedLocales.map((locale) => ({ | |
| params: { | |
| slug: path.params.slug, | |
| locale, | |
| }, | |
| })); | |
| }).flat() ?? []; | |
| return { | |
| paths, | |
| fallback: 'blocking', | |
| }; | |
| }; |
|
|
||
| return ( | ||
| <UserLayout> | ||
| <Head>{t('donorContactManagement')}</Head> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Head component implementation.
The Head component should contain a title tag rather than direct text content for proper SEO and browser tab display.
- <Head>{t('donorContactManagement')}</Head>
+ <Head>
+ <title>{t('donorContactManagement')}</title>
+ </Head>📝 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.
| <Head>{t('donorContactManagement')}</Head> | |
| <Head> | |
| <title>{t('donorContactManagement')}</title> | |
| </Head> |
| unissuedReceipt; | ||
|
|
||
| // get the last donation reference number | ||
| const reference = donations[donations.length - 1].uid; |
There was a problem hiding this comment.
Add null check for donations array before accessing elements.
The code directly accesses the last element of the donations array without checking if the array is empty. This could cause a runtime error if donations is empty.
- const reference = donations[donations.length - 1].uid;
+ const reference = donations.length > 0 ? donations[donations.length - 1].uid : '';📝 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.
| const reference = donations[donations.length - 1].uid; | |
| const reference = donations.length > 0 ? donations[donations.length - 1].uid : ''; |
| useEffect(() => { | ||
| (async () => { | ||
| if (!user || !contextLoaded) return; | ||
| try { | ||
| const response = await getApiAuthenticated<DonationReceiptsStatus>( | ||
| '/app/donationReceiptsStatus' | ||
| ); | ||
| if (response) setDonationReceipts(response); | ||
| } catch (error) { | ||
| handleError(error as APIError); | ||
| redirect('/'); | ||
| } | ||
| })(); | ||
| }, []); |
There was a problem hiding this comment.
Add missing dependencies to useEffect dependency array
The useEffect hook has an empty dependency array but uses several variables from the outer scope which should be included in the dependency array.
Add the missing dependencies to ensure the hook re-runs when these values change:
useEffect(() => {
(async () => {
if (!user || !contextLoaded) return;
try {
const response = await getApiAuthenticated<DonationReceiptsStatus>(
'/app/donationReceiptsStatus'
);
if (response) setDonationReceipts(response);
} catch (error) {
handleError(error as APIError);
redirect('/');
}
})();
- }, []);
+ }, [user, contextLoaded, getApiAuthenticated, redirect]);📝 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.
| useEffect(() => { | |
| (async () => { | |
| if (!user || !contextLoaded) return; | |
| try { | |
| const response = await getApiAuthenticated<DonationReceiptsStatus>( | |
| '/app/donationReceiptsStatus' | |
| ); | |
| if (response) setDonationReceipts(response); | |
| } catch (error) { | |
| handleError(error as APIError); | |
| redirect('/'); | |
| } | |
| })(); | |
| }, []); | |
| useEffect(() => { | |
| (async () => { | |
| if (!user || !contextLoaded) return; | |
| try { | |
| const response = await getApiAuthenticated<DonationReceiptsStatus>( | |
| '/app/donationReceiptsStatus' | |
| ); | |
| if (response) setDonationReceipts(response); | |
| } catch (error) { | |
| handleError(error as APIError); | |
| redirect('/'); | |
| } | |
| })(); | |
| }, [user, contextLoaded, getApiAuthenticated, redirect]); |
| if (!donorView || !addressView || !addressGuid) { | ||
| console.error('❌ Missing user or primary address.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for missing user data
The code logs an error to the console when user data is missing but doesn't provide user feedback.
Consider showing an error message to the user when required data is missing:
if (!donorView || !addressView || !addressGuid) {
console.error('❌ Missing user or primary address.');
+ // Show an error message to the user
+ const errorMessage = 'Unable to proceed. Your profile information is incomplete. Please update your profile.';
+ // Display this message in the UI
return;
}📝 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.
| if (!donorView || !addressView || !addressGuid) { | |
| console.error('❌ Missing user or primary address.'); | |
| return; | |
| } | |
| if (!donorView || !addressView || !addressGuid) { | |
| console.error('❌ Missing user or primary address.'); | |
| // Show an error message to the user | |
| const errorMessage = 'Unable to proceed. Your profile information is incomplete. Please update your profile.'; | |
| // Display this message in the UI | |
| return; | |
| } |
| useEffect(() => { | ||
| if (!isOwner) clearSessionStorage(); | ||
| }, [isOwner]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing dependency to useEffect.
The clearSessionStorage function is missing from the dependency array of the useEffect. This could lead to the effect using a stale reference if the function gets recreated.
useEffect(() => {
if (!isOwner) clearSessionStorage();
- }, [isOwner]);
+ }, [isOwner, clearSessionStorage]);📝 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.
| useEffect(() => { | |
| if (!isOwner) clearSessionStorage(); | |
| }, [isOwner]); | |
| useEffect(() => { | |
| if (!isOwner) clearSessionStorage(); | |
| }, [isOwner, clearSessionStorage]); |
| if (response) { | ||
| initForVerification(response, null); | ||
| } else { | ||
| console.error('❌ Failed to process receipt.'); | ||
| } | ||
| } catch (error) { | ||
| console.error('❌ Error during receipt operation:', error); | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add structured error handling with a better user experience.
The current error handling only logs to console, which isn't helpful for users. Consider implementing a more structured approach that provides meaningful feedback.
if (response) {
initForVerification(response, null);
} else {
console.error('❌ Failed to process receipt.');
+ setErrors([{ message: 'Failed to process receipt. Please try again later.' }]);
}
} catch (error) {
console.error('❌ Error during receipt operation:', error);
+ if (error instanceof Error) {
+ setErrors([{ message: `Error processing receipt: ${error.message}` }]);
+ } else {
+ setErrors([{ message: 'An unexpected error occurred. Please try again later.' }]);
+ }
}📝 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.
| if (response) { | |
| initForVerification(response, null); | |
| } else { | |
| console.error('❌ Failed to process receipt.'); | |
| } | |
| } catch (error) { | |
| console.error('❌ Error during receipt operation:', error); | |
| } finally { | |
| if (response) { | |
| initForVerification(response, null); | |
| } else { | |
| console.error('❌ Failed to process receipt.'); | |
| setErrors([{ message: 'Failed to process receipt. Please try again later.' }]); | |
| } | |
| } catch (error) { | |
| console.error('❌ Error during receipt operation:', error); | |
| if (error instanceof Error) { | |
| setErrors([{ message: `Error processing receipt: ${error.message}` }]); | |
| } else { | |
| setErrors([{ message: 'An unexpected error occurred. Please try again later.' }]); | |
| } | |
| } finally { |
| } = issuedReceipt; | ||
|
|
||
| // get the last donation reference number | ||
| const reference = donations[donations.length - 1].reference; |
There was a problem hiding this comment.
Add null/empty check for donations array.
The code assumes that donations array always has at least one element. This could lead to runtime errors if the array is empty.
Consider adding a safeguard:
- const reference = donations[donations.length - 1].reference;
+ const reference = donations.length > 0 ? donations[donations.length - 1].reference : '';📝 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.
| const reference = donations[donations.length - 1].reference; | |
| const reference = donations.length > 0 ? donations[donations.length - 1].reference : ''; |
| <WebappButton | ||
| variant="primary" | ||
| elementType="link" | ||
| href={downloadUrl ?? ''} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure download URL is properly validated.
Using nullish coalescing to empty string for downloadUrl might lead to broken links if the URL is unexpectedly null.
🏁 Script executed:
#!/bin/bash
# Check if downloadUrl is ever null in the codebase when isReceiptVerified is true
rg -A 5 -B 5 "isReceiptVerified.*?downloadUrl.*?null" --glob "*.tsx" --glob "*.ts"Length of output: 84
I'll generate a follow-up shell script to inspect how downloadUrl is used and defined in IssuedReceiptCard.tsx, so we can verify if the fallback with an empty string might lead to broken links or if there's upstream validation in place.
Please run this script and share its output so we can proceed with a final assessment.
🏁 Script executed:
#!/bin/bash
# Inspect the file to see how downloadUrl is declared and referenced.
rg "downloadUrl" -n src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsxLength of output: 231
Action required: Validate and conditionally render the download link
It appears that the code currently computes the receipt verification status by checking that both the verificationDate is not null and that downloadUrl is truthy. However, even with that condition, rendering the link with
href={downloadUrl ?? ''}means that if somehow downloadUrl ends up being null or undefined, the anchor’s href will become an empty string—which can lead to broken or confusing links. Consider the following improvements:
- Conditional Rendering: Only render the download link when
downloadUrlis truthy (e.g., usingisReceiptVerifiedas a condition). - Fallback Behavior: Instead of falling back to an empty string, use a more intentional fallback (like not rendering the link at all or using a safe default value) to avoid accidental navigation issues.
- URL Validation: If a download URL is available, validate that it conforms to your expected URL format before using it.
Please update the logic in src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx to implement one or more of these improvements.
| export interface DonorAPI { | ||
| reference: string; | ||
| tin: string | null; | ||
| type: 'individual' | 'organization'; | ||
| name: string; | ||
| email: string; | ||
| address1: string; | ||
| address2: string | null; | ||
| city: string; | ||
| zipCode: string; | ||
| country: string; | ||
| guid: string | null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize naming conventions for API interfaces.
There's an inconsistency in naming between DonorAPI (line 38) and DonorApi (line 122). These interfaces also have overlapping properties with different requirements. Standardize the naming and consider consolidating or clearly differentiating these interfaces.
-export interface DonorAPI {
+export interface DonorApiRequest {
reference: string;
tin: string | null;
type: 'individual' | 'organization';
name: string;
email: string;
address1: string;
address2: string | null;
city: string;
zipCode: string;
country: string;
guid: string | null;
}
// Later in the file...
-export interface DonorApi {
+export interface DonorApiResponse {
address1: string | null;
address2: string | null;
city: string | null;
country: string | null;
email: string;
name: string;
reference: string;
tin: string | null;
type: string; // e.g., "individual" or "organization"
zipCode: string;
}Also applies to: 122-133
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx (3)
39-47: Consider disabling the button when processing instead of using an empty click handler.Currently, when
isProcessingis true, the button is still clickable but has an empty function handler. It would be better to disable the button entirely during processing to provide clearer feedback to users.<WebappButton variant="primary" elementType="button" text={!isProcessing ? tReceipt('verifyAndDownload') : undefined} - onClick={!isProcessing ? onReceiptClick : () => {}} + onClick={onReceiptClick} + disabled={isProcessing} icon={isProcessing ? <div className={styles.spinner} /> : undefined} buttonClasses={styles.receiptCardButton} />
34-34: Add error handling for formatDate function.The
formatDatefunction is called directly withpaymentDatewithout any error handling. IfpaymentDateis invalid or undefined, this might cause runtime errors.- date={formatDate(paymentDate)} + date={paymentDate ? formatDate(paymentDate) : ''}
42-45: Improve accessibility for processing state.When the button is in a processing state, it's not clear to screen readers what's happening. Consider adding an aria-label to improve accessibility.
<WebappButton variant="primary" elementType="button" text={!isProcessing ? tReceipt('verifyAndDownload') : undefined} onClick={!isProcessing ? onReceiptClick : () => {}} icon={isProcessing ? <div className={styles.spinner} /> : undefined} + aria-label={isProcessing ? tReceipt('processing') : tReceipt('verifyAndDownload')} buttonClasses={styles.receiptCardButton} />src/features/user/DonationReceipt/DonationReceiptWrapper.tsx (1)
106-115: Provide user-friendly error feedback.Currently, an error only logs to the console. Consider offering more structured error handling (e.g., showing an error message in the UI) so users know what's happening.
} catch (error) { console.error('❌ Error during receipt operation:', error); + // Optionally show a user-facing error message + // setErrors([{ message: 'An unexpected error occurred while processing your receipt.' }]); } finally { setIsLoading(false); }src/features/common/Layout/DonationReceiptContext.tsx (3)
77-86: Enhance error handling when loading from session storage.The current implementation catches errors and logs them to the console, but consider adding more robust error handling that could notify users or trigger a recovery mechanism.
const loadStateFromSession = (): DonationReceiptContextState => { try { const storedState = sessionStorage.getItem('donationReceiptContext'); return storedState ? JSON.parse(storedState) : defaultState; } catch (error) { console.error('Failed to parse session storage:', error); sessionStorage.removeItem('donationReceiptContext'); + // Consider adding analytics or user notification for persistent errors + // Optionally trigger recovery mechanism if appropriate return defaultState; } };
29-33: Ensure responsive design for all viewport sizes.The current layout adjusts for extra-small phone views, but consider adding media queries for other common breakpoints (tablet, larger phones) to ensure a smooth experience across all device sizes.
134-136: Consider adding a cleanup mechanism on component unmount.Currently, session storage is cleared manually via the
clearSessionStoragemethod, but consider adding a cleanup mechanism using a useEffect with an empty dependency array to clear storage when the provider unmounts in certain scenarios.export const DonationReceiptProvider: React.FC<{ children: React.ReactNode; }> = ({ children }) => { const [state, setState] = useState<DonationReceiptContextState>(loadStateFromSession); // Persist state to sessionStorage useEffect(() => { sessionStorage.setItem('donationReceiptContext', JSON.stringify(state)); }, [state]); + // Optional: Clean up on unmount if needed in specific scenarios + // useEffect(() => { + // return () => { + // // Only clear in specific scenarios, not on every unmount + // // Add logic to determine when to clear + // // if (shouldClearOnUnmount) { + // // sessionStorage.removeItem('donationReceiptContext'); + // // } + // }; + // }, []); //clean donation receipt data from the session storage const clearSessionStorage = (): void => { sessionStorage.removeItem('donationReceiptContext'); };src/features/user/DonationReceipt/DonationReceipt.module.scss (2)
401-402: Add a method to handle scrollbar visibility across browsers.While you've hidden scrollbars in Firefox and IE/Edge, Chrome requires a different approach. Consider adding the following to ensure consistent scrollbar hiding across all browsers.
.donationReceipts { max-width: 524px; max-height: 494px; overflow-y: auto; width: 100%; display: flex; flex-direction: column; gap: 21px; margin-top: 21px; scrollbar-width: none; /* Hides scrollbar in Firefox */ -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ + &::-webkit-scrollbar { + display: none; /* Hides scrollbar in Chrome/Safari */ + } }
489-515: Consider using CSS variables for animation properties.For better maintainability, consider defining the animation properties as CSS variables. This would make it easier to adjust timing and behavior across multiple elements.
+ :root { + --spinner-animation-duration: 1s; + --spinner-size: 16px; + --spinner-border-width: 2px; + } .spinner { border: 2px solid #f3f3f3; border-radius: 50%; border-top: 2px solid transparent; - width: 16px; - height: 16px; + width: var(--spinner-size); + height: var(--spinner-size); - -webkit-animation: spin 1s linear infinite; /* Safari */ - animation: spin 1s linear infinite; + -webkit-animation: spin var(--spinner-animation-duration) linear infinite; /* Safari */ + animation: spin var(--spinner-animation-duration) linear infinite; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/donationReceipt.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
public/assets/images/icons/projectV2/NoDataFound.tsx(2 hunks)src/features/common/Layout/DonationReceiptContext.tsx(1 hunks)src/features/projectsV2/ProjectList/index.tsx(2 hunks)src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceiptValidator.ts(1 hunks)src/features/user/DonationReceipt/DonationReceiptWrapper.tsx(1 hunks)src/features/user/DonationReceipt/DonationReceipts.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- public/assets/images/icons/projectV2/NoDataFound.tsx
- src/features/projectsV2/ProjectList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/user/DonationReceipt/DonationReceipts.tsx
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/user/DonationReceipt/DonationReceipt.module.scss
[warning] 363-363: src/features/user/DonationReceipt/DonationReceipt.module.scss#L363
Unexpected shorthand "margin" after "margin-top". (declaration-block-no-shorthand-property-overrides)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/features/user/DonationReceipt/microComponents/UnissuedReceiptCard.tsx (4)
25-26: Add null check for donations array before accessing elements.The code directly accesses the last element of the
donationsarray without checking if the array is empty. This could cause a runtime error ifdonationsis empty.- const reference = donations[donations.length - 1].uid; + const reference = donations.length > 0 ? donations[donations.length - 1].uid : '';
1-9: LGTM - Well-structured imports.The imports are well-organized, separating type imports from regular imports and properly importing required components and utilities.
10-14: LGTM - Clear prop type definition.The component's prop type is well-defined with appropriate types for each prop.
28-56: LGTM - Clean component structure with conditional rendering.The component is well-structured with appropriate conditional rendering based on the receipt type. The use of internationalization for text is a good practice for supporting multiple languages.
src/features/user/DonationReceipt/DonationReceiptWrapper.tsx (2)
38-40: Add missing dependency touseEffect.Your
clearSessionStoragefunction is not included in the dependency array, which can lead to stale references if the function is recreated.
94-105: Consider adding a fallback for unrecognized operation.The current logic handles
VERIFYandISSUEbut does nothing if the operation has a different status. For robustness, you might add anelsebranch with a warning or error to handle unexpected values.src/features/user/DonationReceipt/DonationReceiptValidator.ts (1)
24-31: Verify logic for returningtruewhen user is not logged in.
validateOwnershipgrants ownership if there is no logged-in user. Ensure this is the intended behavior, as it allows unauthenticated access ifissuedToEmailis not validated.src/features/user/DonationReceipt/microComponents/IssuedReceiptCard.tsx (2)
33-33: Add null/empty check fordonationsarray.Accessing
donations[donations.length - 1]can cause a runtime error ifdonationsis empty.
47-55: Better handle potential null or undefineddownloadUrl.Using an empty string fallback (
downloadUrl ?? '') can create broken links. Consider conditionally rendering the link only ifdownloadUrlis present or providing a safer fallback.src/features/user/DonationReceipt/DonationReceipt.module.scss (1)
436-436: LGTM: Good responsive design patterns.The stylesheet makes effective use of media queries with the
@include xsPhoneViewmixin to adapt layouts for smaller screens, which is crucial for the mobile responsiveness mentioned in the PR objectives.
| // TODO: review the following validation | ||
| return donor !== null && |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO comment about validation.
There's a comment indicating that the validation logic needs to be reviewed. Consider implementing more comprehensive validation that checks not only for null values but also validates the format and content of the data.
🏁 Script executed:
#!/bin/bash
# Find if there are other validation implementations in the codebase that could be reused
echo "Searching for validation patterns in the codebase..."
rg -A 3 -B 3 "validate.*Receipt|receipt.*validate" --type tsx --type tsLength of output: 216
Below is a new shell script that uses a glob pattern to include both TypeScript and TSX files, which should help us verify whether any reusable receipt validation logic exists:
Once you run this command, please check if any reusable validation implementations are found. If no such patterns exist, then the review comment still holds: the current check only verifies that donor is not null, without ensuring that the donor data’s format or content is correct.
Based on the current evidence—and pending further verification—the rewritten review comment is as follows:
Action Required: Enhance Donation Receipt Validation
In src/features/common/Layout/DonationReceiptContext.tsx (lines 280–281), the donor validation only checks for a non-null value:
// TODO: review the following validation
return donor !== null &&
This minimal check does not confirm that the donor data is properly formatted or contains valid content. Since no reusable validation utilities for receipt data appear to exist in the codebase, please consider one of the following:
- Extend this validation: Implement additional checks to ensure the data meets expected format and content criteria.
- Refactor if needed: If similar validation logic is required elsewhere, abstract it into a reusable utility function.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Re-run search for validation patterns in TS and TSX files using globbing
rg -g '*.{ts,tsx}' -A 3 -B 3 "validate.*Receipt|receipt.*validate"Length of output: 4936
Action Required: Replace Minimal Donor Check with Robust Validation
In src/features/common/Layout/DonationReceiptContext.tsx (lines 280–281), the current check:
// TODO: review the following validation
return donor !== null &&only ensures that donor is not null. However, similar parts of the codebase already leverage more comprehensive receipt validations via the functions validateIssuedReceipt and validateUnissuedReceipt (located in src/features/user/DonationReceipt/DonationReceiptValidator.ts). To avoid duplication and ensure consistent validation across the application, please consider one of the following approaches:
- Refactor to use existing validators: Replace the inline
donor !== nullcheck with a call to eithervalidateIssuedReceiptorvalidateUnissuedReceipt(depending on context) so that both the format and content of the donor data are properly validated. - Extend the current validation: If a more tailored validation is needed in this context, augment the check to include necessary format and data integrity verification beyond simply confirming a non-null value.
Please update this logic accordingly.
| justify-content: space-between; | ||
| gap: 12px; | ||
| margin-top: 68px; | ||
| margin: 42px 12px 0px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix conflicting margin declarations.
The margin-top property is being overridden by the subsequent margin shorthand property, making the first declaration ineffective.
- margin-top: 68px;
margin: 42px 12px 0px;📝 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.
| margin: 42px 12px 0px; | |
| margin: 42px 12px 0px; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 363-363: src/features/user/DonationReceipt/DonationReceipt.module.scss#L363
Unexpected shorthand "margin" after "margin-top". (declaration-block-no-shorthand-property-overrides)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx (3)
46-51: Consider simplifying the condition for invalid address.The current check for invalid address fields requires each field to be evaluated separately. This can be simplified for better readability and maintainability.
isAddressInvalid={ - !address.address1 || - !address.zipCode || - !address.city || - !address.country + !address.address1 || !address.zipCode || !address.city || !address.country }
53-59: Enhance the login alert with ActionButton component.The login alert simply informs the user that login is required but doesn't provide a direct way to login.
Consider adding an ActionButton component to allow users to login directly from this notification:
{!isAuthenticated && ( <p className={styles.loginAlert}> {tReceipt.rich('notifications.loginRequired', { b: (chunks) => <strong>{chunks}</strong>, })} + <ActionButton className={styles.loginButton} onClick={() => loginWithRedirect()}> + {tReceipt('actions.login')} + </ActionButton> </p> )}This would require importing the necessary components and hooks:
import { useAuth0 } from '@auth0/auth0-react'; import ActionButton from '../../../../common/InputTypes/ActionButton'; // In the component: const { isAuthenticated, loginWithRedirect } = useAuth0();
61-71: Improve loading state with more descriptive feedback.The current loading state shows only a spinner without any context about what's happening.
Consider adding a loading message to provide better user feedback:
) : ( <div className={styles.donationReceiptSpinner}> <CircularProgress color="success" /> + <p className={styles.loadingMessage}>{tReceipt('messages.processingReceipt')}</p> </div> )}You'll need to add the corresponding style in the SCSS file:
.loadingMessage { margin-top: 10px; font-size: $fontSmall; color: rgba(130, 130, 130, 1); }And add a translation key for the loading message in your locale files.
src/features/user/DonationReceipt/DonationReceipt.module.scss (2)
133-136: Use CSS variables for consistent colors.The background color is hardcoded as an rgba value, which makes it difficult to maintain consistent theming across the application.
.donorDetails { display: flex; flex-direction: column; gap: 20px; - background: rgba(242, 242, 242, 0.5); + background: var(--background-light); width: 100%; border-radius: 9px; padding: 10px 15px;Define this variable in your theme file if it doesn't exist yet.
488-496: Replace custom spinner with MUI's CircularProgress.You've defined a custom spinner when the application is already using MUI's CircularProgress component elsewhere in the codebase (as seen in ReceiptDataSection.tsx).
Consider replacing this custom spinner implementation with the consistent use of MUI's CircularProgress component to maintain visual consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
public/static/locales/cs/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/de/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/en/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/es/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/fr/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/it/donationReceipt.jsonis excluded by!**/*.jsonpublic/static/locales/pt-BR/donationReceipt.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
src/features/user/DonationReceipt/DonationReceipt.module.scss(1 hunks)src/features/user/DonationReceipt/DonationReceipts.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationsTable.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/ReceiptDataSection.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/user/DonationReceipt/DonationReceipts.tsx
- src/features/user/DonationReceipt/microComponents/DonationsTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
| <button | ||
| type="button" | ||
| onClick={openPopover} | ||
| className={styles.donationCount} | ||
| > | ||
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | ||
| reference, | ||
| count, | ||
| date, | ||
| u: (chunks) => <span>{chunks}</span>, | ||
| })} | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for the donation count button.
The button lacks proper accessibility attributes, which could make it difficult for screen reader users to understand its purpose.
<button
type="button"
onClick={openPopover}
className={styles.donationCount}
+ aria-label={tReceipt('aria.showDonationDetails')}
+ aria-haspopup="true"
+ aria-expanded={Boolean(popoverAnchor)}
>
{tReceipt.rich('itemsReferenceDateMultiDonation', {
reference,
count,
date,
u: (chunks) => <span>{chunks}</span>,
})}
</button>This requires adding a new translation key for the aria-label.
📝 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.
| <button | |
| type="button" | |
| onClick={openPopover} | |
| className={styles.donationCount} | |
| > | |
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | |
| reference, | |
| count, | |
| date, | |
| u: (chunks) => <span>{chunks}</span>, | |
| })} | |
| </button> | |
| )} | |
| <button | |
| type="button" | |
| onClick={openPopover} | |
| className={styles.donationCount} | |
| aria-label={tReceipt('aria.showDonationDetails')} | |
| aria-haspopup="true" | |
| aria-expanded={Boolean(popoverAnchor)} | |
| > | |
| {tReceipt.rich('itemsReferenceDateMultiDonation', { | |
| reference, | |
| count, | |
| date, | |
| u: (chunks) => <span>{chunks}</span>, | |
| })} | |
| </button> | |
| )} |
| {tReceipt('donationDetails.donationAmount', { | ||
| currency, | ||
| amount: amount.toFixed(2), | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for amount formatting.
The amount.toFixed(2) call assumes amount is always a valid number. This could potentially cause runtime errors if amount is null, undefined, or NaN.
{tReceipt('donationDetails.donationAmount', {
currency,
- amount: amount.toFixed(2),
+ amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00',
})}📝 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.
| {tReceipt('donationDetails.donationAmount', { | |
| currency, | |
| amount: amount.toFixed(2), | |
| })} | |
| {tReceipt('donationDetails.donationAmount', { | |
| currency, | |
| amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00', | |
| })} |
| max-width: 524px; | ||
| max-height: 494px; | ||
| overflow-y: auto; | ||
| width: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 21px; | ||
| margin-top: 21px; | ||
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | ||
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential scrollbar issues on different browsers.
While you're hiding scrollbars in Firefox and IE/Edge, you're missing the WebKit implementation for Chrome and Safari.
.donationReceipts {
max-width: 524px;
max-height: 494px;
overflow-y: auto;
width: 100%;
display: flex;
flex-direction: column;
gap: 21px;
margin-top: 21px;
scrollbar-width: none; /* Hides scrollbar in Firefox */
-ms-overflow-style: none; /* Hides scrollbar in IE/Edge */
+ &::-webkit-scrollbar {
+ display: none; /* Hides scrollbar in Chrome and Safari */
+ }
}📝 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.
| max-width: 524px; | |
| max-height: 494px; | |
| overflow-y: auto; | |
| width: 100%; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 21px; | |
| margin-top: 21px; | |
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | |
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | |
| } | |
| .donationReceipts { | |
| max-width: 524px; | |
| max-height: 494px; | |
| overflow-y: auto; | |
| width: 100%; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 21px; | |
| margin-top: 21px; | |
| scrollbar-width: none; /* Hides scrollbar in Firefox */ | |
| -ms-overflow-style: none; /* Hides scrollbar in IE/Edge */ | |
| &::-webkit-scrollbar { | |
| display: none; /* Hides scrollbar in Chrome and Safari */ | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx (1)
72-89: 🛠️ Refactor suggestionAdd error handling to getStaticProps
The function doesn't handle potential errors when fetching tenant config or messages.
Add try/catch blocks and provide fallback values:
export const getStaticProps: GetStaticProps<PageProps> = async ( context: GetStaticPropsContext ): Promise<GetStaticPropsResult<PageProps>> => { - const tenantConfig = - (await getTenantConfig(context.params?.slug as string)) ?? defaultTenant; - - const messages = await getMessagesForPage({ - locale: context.params?.locale as string, - filenames: ['common', 'me', 'country', 'donationReceipt'], - }); + try { + const tenantConfig = + (await getTenantConfig(context.params?.slug as string)) ?? defaultTenant; + + const messages = await getMessagesForPage({ + locale: context.params?.locale as string, + filenames: ['common', 'me', 'country', 'donationReceipt'], + }); + + return { + props: { + messages, + tenantConfig, + }, + }; + } catch (error) { + console.error('Error in getStaticProps:', error); + return { + notFound: true, + }; + } - return { - props: { - messages, - tenantConfig, - }, - }; };
🧹 Nitpick comments (4)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (1)
42-75: Consider adding loading and error states.The component currently doesn't handle loading or error states. For a better user experience, consider adding indicators for when data is loading or when there's an error fetching data.
Consider adding props for loading and error states, and conditionally render appropriate UI elements such as spinners or error messages:
type Props = { currency: string; amount: number; count: number; reference: string; date: string; donations: UnissuedDonationApi[] | IssuedDonationApi[]; + isLoading?: boolean; + error?: Error | null; }; // Inside the component render: return ( <div className={styles.donationInfo}> + {isLoading && <div className={styles.loading}>Loading...</div>} + {error && <div className={styles.error}>{error.message}</div>} + {!isLoading && !error && ( <> <span className={styles.amount}> {tReceipt('donationDetails.donationAmount', { currency, amount: Number(amount).toFixed(2), })} </span> {/* Rest of the component */} </> + )} </div> );pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx (3)
44-61: Consider supporting multiple locales in getStaticPathsThe current implementation hardcodes the locale to 'en', which limits the static generation to English only. For an internationalized application, consider generating paths for all supported locales.
export const getStaticPaths: GetStaticPaths = async () => { const subDomainPaths = await constructPathsForTenantSlug(); + const supportedLocales = ['en', 'de', 'es']; // Add all supported locales const paths = subDomainPaths?.map((path) => { - return { - params: { - slug: path.params.slug, - locale: 'en', - }, - }; + return supportedLocales.map((locale) => ({ + params: { + slug: path.params.slug, + locale, + }, + })); - }) ?? []; + }).flat() ?? []; return { paths, fallback: 'blocking', }; };
30-32: Add setTenantConfig to the useEffect dependencies arrayThe effect uses
setTenantConfigbut it's not included in the dependency array. While this might work as expected sincesetTenantConfigis likely stable across renders, it's best practice to include all dependencies.useEffect(() => { if (router.isReady) setTenantConfig(tenantConfig); -}, [router.isReady]); +}, [router.isReady, setTenantConfig, tenantConfig]);
23-42: Add null check before setting tenantConfigEnsure
tenantConfigis valid before setting it to prevent potential runtime errors.useEffect(() => { - if (router.isReady) setTenantConfig(tenantConfig); + if (router.isReady && tenantConfig) setTenantConfig(tenantConfig); }, [router.isReady]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pages/sites/[slug]/[locale]/profile/donation-receipt/verify.tsx(1 hunks)src/features/user/DonationReceipt/microComponents/DonationInfo.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/features/user/DonationReceipt/microComponents/DonationInfo.tsx (3)
56-68: Improve accessibility for the donation count button.The button lacks proper accessibility attributes, which could make it difficult for screen reader users to understand its purpose.
Apply this diff to improve accessibility:
<button type="button" onClick={openPopover} className={styles.donationCount} + aria-label={tReceipt('aria.showDonationDetails')} + aria-haspopup="true" + aria-expanded={Boolean(popoverAnchor)} > {tReceipt.rich('itemsReferenceDateMultiDonation', { reference, count, date, u: (chunks) => <span>{chunks}</span>, })} </button>This requires adding a new translation key for the aria-label.
45-48: Add error handling for amount formatting.The
Number(amount).toFixed(2)call assumes amount is always a valid number. This could potentially cause runtime errors if amount is null, undefined, or NaN.Apply this diff to add robust error handling:
{tReceipt('donationDetails.donationAmount', { currency, - amount: Number(amount).toFixed(2), + amount: (typeof amount === 'number' && !isNaN(amount)) ? amount.toFixed(2) : '0.00', })}
11-18: Consider adding validation for amount.The current implementation doesn't handle cases where amount might be zero or negative. Depending on your business requirements, you might want to add validation or special handling for these cases.
Consider whether zero or negative amounts are valid in your domain context and if they need special formatting or validation.
Introduce a new DonationReceipt page with components for displaying donation details, recipient information, and actions. Enhance user experience with improved validation, error handling, and mobile responsiveness. Refactor existing code for better structure and maintainability.