-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add receipt export and viewing on request scan #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| export { debounce } from "./debounce"; | ||
| export { formatAddress } from "./formatAddress"; | ||
| export { exportToPDF } from "./generateInvoice"; | ||
| export { publicClientToProvider, walletClientToSigner } from "./wallet-utils"; | ||
| export { capitalize } from "./capitalize"; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,31 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <script lang="ts"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { fade } from "svelte/transition"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { exportToPDF } from "@requestnetwork/shared-utils/generateInvoice"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getCurrencyFromManager } from "@requestnetwork/shared-utils/getCurrency"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { initializeCurrencyManager } from "@requestnetwork/shared-utils/initCurrencyManager"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let createdRequest: any; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use specific TypeScript types instead of 'any' for 'createdRequest' Using Consider defining an interface for interface CreatedRequest {
inMemoryInfo?: {
requestData?: RequestDataType; // Define RequestDataType appropriately
};
requestId: string;
// ... other properties
}
export let createdRequest: CreatedRequest; |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let enablePdfReceipt: boolean = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let enableRequestScanLink: boolean = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let sellerLogo: string | undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function handleDownloadReceipt() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (createdRequest) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const currencyManager = initializeCurrencyManager([]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const currencyData = createdRequest?.inMemoryInfo?.requestData; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await exportToPDF( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| currencyData, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getCurrencyFromManager(currencyData.currencyInfo, currencyManager), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sellerLogo! | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const currencyData = createdRequest?.inMemoryInfo?.requestData; | |
| await exportToPDF( | |
| currencyData, | |
| getCurrencyFromManager(currencyData.currencyInfo, currencyManager), | |
| sellerLogo! | |
| const currencyData = createdRequest?.inMemoryInfo?.requestData; | |
| if (!currencyData) { | |
| throw new Error("Currency data is unavailable."); | |
| } | |
| await exportToPDF( | |
| currencyData, | |
| getCurrencyFromManager(currencyData.currencyInfo, currencyManager), | |
| sellerLogo! |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a sonner or alert when this error occurs so the user is informed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Provide user feedback when receipt download fails
Currently, if an error occurs during the receipt download, it is only logged to the console. This might leave users unaware of the failure.
Consider displaying an error message to the user:
} catch (error) {
console.error("Error downloading receipt:", error);
+ alert("Failed to download receipt. 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.
| } catch (error) { | |
| console.error("Error downloading receipt:", error); | |
| } | |
| } | |
| } catch (error) { | |
| console.error("Error downloading receipt:", error); | |
| alert("Failed to download receipt. Please try again later."); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle possible undefined values to prevent runtime errors
In handleDownloadReceipt(), the sellerLogo variable is asserted as non-null with sellerLogo!, but it can be undefined based on its type. This could lead to runtime errors if sellerLogo is not provided.
Consider providing a default value or checking if sellerLogo is defined before using it:
+ const logo = sellerLogo ?? '';
+ await exportToPDF(
currencyData,
getCurrencyFromManager(currencyData.currencyInfo, currencyManager),
- sellerLogo!
+ logo
);📝 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.
| async function handleDownloadReceipt() { | |
| if (createdRequest) { | |
| try { | |
| const currencyManager = initializeCurrencyManager([]); | |
| const currencyData = createdRequest?.inMemoryInfo?.requestData; | |
| await exportToPDF( | |
| currencyData, | |
| getCurrencyFromManager(currencyData.currencyInfo, currencyManager), | |
| sellerLogo! | |
| ); | |
| } catch (error) { | |
| console.error("Error downloading receipt:", error); | |
| } | |
| } | |
| } | |
| async function handleDownloadReceipt() { | |
| if (createdRequest) { | |
| try { | |
| const currencyManager = initializeCurrencyManager([]); | |
| const currencyData = createdRequest?.inMemoryInfo?.requestData; | |
| const logo = sellerLogo ?? ''; | |
| await exportToPDF( | |
| currencyData, | |
| getCurrencyFromManager(currencyData.currencyInfo, currencyManager), | |
| logo | |
| ); | |
| } catch (error) { | |
| console.error("Error downloading receipt:", error); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure createdRequest.requestId is defined before using it in URL
In the link to Request Scan, you're using createdRequest.requestId without checking if requestId exists. If requestId is undefined, the URL will be incorrect.
Consider adding a check or fallback:
{#if enableRequestScanLink && createdRequest}
<a
- href={`https://scan.request.network/request/${createdRequest.requestId}`}
+ href={`https://scan.request.network/request/${createdRequest.requestId ?? ''}`}
>
View on Request Scan
</a>
{/if}📝 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 enablePdfReceipt || (enableRequestScanLink && createdRequest)} | |
| <div class="buttons-container"> | |
| {#if enablePdfReceipt} | |
| <button on:click={handleDownloadReceipt}>Download Receipt</button> | |
| {/if} | |
| {#if enableRequestScanLink && createdRequest} | |
| <a | |
| href={`https://scan.request.network/request/${createdRequest.requestId}`} | |
| > | |
| View on Request Scan | |
| </a> | |
| {/if} | |
| </div> | |
| {/if} | |
| {#if enablePdfReceipt || (enableRequestScanLink && createdRequest)} | |
| <div class="buttons-container"> | |
| {#if enablePdfReceipt} | |
| <button on:click={handleDownloadReceipt}>Download Receipt</button> | |
| {/if} | |
| {#if enableRequestScanLink && createdRequest} | |
| <a | |
| href={`https://scan.request.network/request/${createdRequest.requestId ?? ''}`} | |
| > | |
| View on Request Scan | |
| </a> | |
| {/if} | |
| </div> | |
| {/if} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize CSS by reducing redundancy in button styles
The styles for button and a elements share common properties. To improve maintainability, consider combining shared styles.
Apply this diff to merge common styles:
.buttons-container {
display: flex;
gap: 16px;
margin-top: 24px;
- button,
- a {
- padding: 10px 20px;
- border-radius: 6px;
- font-size: 14px;
- font-weight: 500;
- text-decoration: none;
- transition: background-color 0.3s ease;
- }
+ .button-style {
+ padding: 10px 20px;
+ border-radius: 6px;
+ font-size: 14px;
+ font-weight: 500;
+ text-decoration: none;
+ transition: background-color 0.3s ease;
+ }
+
+ button,
+ a {
+ @extend .button-style;
+ }
button {
background-color: #0bb489;
color: white;
border: none;
cursor: pointer;
&:hover {
background-color: darken(#0bb489, 10%);
}
}
a {
background-color: #f5f5f5;
color: #333;
&:hover {
background-color: darken(#f5f5f5, 10%);
}
}
}📝 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.
| .buttons-container { | |
| display: flex; | |
| gap: 16px; | |
| margin-top: 24px; | |
| button, | |
| a { | |
| padding: 10px 20px; | |
| border-radius: 6px; | |
| font-size: 14px; | |
| font-weight: 500; | |
| text-decoration: none; | |
| transition: background-color 0.3s ease; | |
| } | |
| button { | |
| background-color: #0bb489; | |
| color: white; | |
| border: none; | |
| cursor: pointer; | |
| &:hover { | |
| background-color: darken(#0bb489, 10%); | |
| } | |
| } | |
| a { | |
| background-color: #f5f5f5; | |
| color: #333; | |
| &:hover { | |
| background-color: darken(#f5f5f5, 10%); | |
| } | |
| } | |
| } | |
| .buttons-container { | |
| display: flex; | |
| gap: 16px; | |
| margin-top: 24px; | |
| .button-style { | |
| padding: 10px 20px; | |
| border-radius: 6px; | |
| font-size: 14px; | |
| font-weight: 500; | |
| text-decoration: none; | |
| transition: background-color 0.3s ease; | |
| } | |
| button, | |
| a { | |
| @extend .button-style; | |
| } | |
| button { | |
| background-color: #0bb489; | |
| color: white; | |
| border: none; | |
| cursor: pointer; | |
| &:hover { | |
| background-color: darken(#0bb489, 10%); | |
| } | |
| } | |
| a { | |
| background-color: #f5f5f5; | |
| color: #333; | |
| &:hover { | |
| background-color: darken(#f5f5f5, 10%); | |
| } | |
| } | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ | |
| export let invoiceNumber: string | undefined; | ||
| export let feeAddress: string; | ||
| export let feeAmountInUSD: number; | ||
| export let createdRequest: any; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using a more specific type for The use of Here's a suggested improvement: // Assuming there's a Request type defined elsewhere
import type { Request } from '../types';
// The created request object after successful payment processing
export let createdRequest: Request | null = null;This change provides type safety and sets a default value, making the code more robust and self-documenting. |
||
| const COUNTDOWN_INTERVAL = 30; | ||
|
|
@@ -275,6 +276,8 @@ | |
| persistRequest, | ||
| }); | ||
|
|
||
| createdRequest = request; | ||
|
|
||
|
Comment on lines
+279
to
+280
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM! Consider adding type assertion for The assignment of the To further improve type safety and error handling, consider adding a type assertion and a null check: if (request) {
createdRequest = request as Request; // Assuming Request is the correct type
} else {
console.warn('Request object is null or undefined after payment processing');
}This change ensures that |
||
| if (onPaymentSuccess) { | ||
| onPaymentSuccess(request); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,8 @@ | |
| export let invoiceNumber: string | undefined = undefined; | ||
| export let feeAddress: string = ethers.constants.AddressZero; | ||
| export let feeAmountInUSD: number = 0; | ||
| export let enablePdfReceipt: boolean = true; | ||
| export let enableRequestScanLink: boolean = true; | ||
|
Comment on lines
+42
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM! Consider adding JSDoc comments. The new exported properties Consider adding JSDoc comments to describe these new properties and their purpose. For example: /** Enable the option to generate and download a PDF receipt. Default is true. */
export let enablePdfReceipt: boolean = true;
/** Enable the option to view the request on RequestScan. Default is true. */
export let enableRequestScanLink: boolean = true; |
||
| // State | ||
| let web3Modal: Web3Modal | null = null; | ||
|
|
@@ -47,7 +49,7 @@ | |
| let selectedCurrency: Currency | null = null; | ||
| let connectionCheckInterval: ReturnType<typeof setInterval> | null = null; | ||
| let currentPaymentStep: PaymentStep = "currency"; | ||
| let createdRequest: any; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM! Consider adding a type annotation. The new Consider adding a type annotation to let createdRequest: PaymentRequest | null = null;Replace |
||
| let scrollPosition = 0; | ||
| // Effects | ||
|
|
@@ -246,12 +248,18 @@ | |
| onPaymentError={onError} | ||
| bind:currentPaymentStep | ||
| bind:isConnected | ||
| bind:createdRequest | ||
| {sellerInfo} | ||
| buyerInfo={currentBuyerInfo} | ||
| {invoiceNumber} | ||
| /> | ||
| {:else} | ||
| <PaymentComplete /> | ||
| <PaymentComplete | ||
| {createdRequest} | ||
| {enablePdfReceipt} | ||
| {enableRequestScanLink} | ||
| sellerLogo={sellerInfo.logo} | ||
| /> | ||
| {/if} | ||
| </Modal> | ||
| </section> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ export interface PaymentWidgetProps { | |
| invoiceNumber?: string; | ||
| feeAddress?: string; | ||
| feeAmountInUSD?: number; | ||
| enablePdfReceipt?: boolean; | ||
| enableRequestScanLink?: boolean; | ||
|
Comment on lines
+26
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM! Consider updating documentation. The new properties Consider updating the JSDoc comment for the * @example
* <PaymentWidget
* // ... other props ...
* enablePdfReceipt={true}
* enableRequestScanLink={true}
* /> |
||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
LGTM! Consider consolidating utility imports.
The refactoring to use shared utilities for
exportToPDFandgetCurrencyFromManageris a good practice for improving code reusability.Consider moving
walletClientToSignerto the shared utils as well if it's used in multiple places across the project. This would further improve consistency and maintainability.Also applies to: 30-30