Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Giftable donations #470

Merged
merged 28 commits into from
Dec 3, 2024
Merged

Giftable donations #470

merged 28 commits into from
Dec 3, 2024

Conversation

mohitb35
Copy link
Contributor

@mohitb35 mohitb35 commented Nov 12, 2024

This PR enables invitation and direct gifts across a wide range of project and donation types. This includes updates to the planet-sdk to get the latest types, updates for gift details handling, and refactoring project details initialization.

Main Changes

  1. Enables direct gifts for all projects for all frequencies (once, monthly, yearly) except membership projects, bouquet projects and planet-cash
  2. Enables invitation gifts for membership projects for monthly and yearly frequencies. For membership projects, only recipient name is captured in the gift form
  3. Enables invitation gifts for one time donations for all projects except bouquet and planet-cash.

Dependency Updates:

  • package.json: Updated @planet-sdk/common package version from ^0.1.34 to ^0.1.42.

Gift Handling:

  • src/Common/Types/index.tsx
    • adds GiftDetails type to represent gift details stored in QueryParamsContext, keeping SentGift to represent the types for gift used in the API payloads, and removing unrequired properties from the direct gift details.
  • src/Common/Types/QueryParamContextInterface.ts
    • Updates usage of SentGift with GiftDetails
  • /pages/index.tsx
    • Updates usage of SentGift with GiftDetails
    • Updates logic in getServerSideProps to initialize direct gifts as per the updated GiftDetail types
    • Adds logic to remove s from query parameters when direct gifts should not be supported.
  • src/Donations/Components/DonationsForm.tsx
    • Adds logic to show the GiftForm component while handling invitation and direct gifts, handling membership and non giftable projects
    • Updates logic to populate gift in donationData in handlePlanetCashDonate
    • Removes usage of onBehalf while handling gift or donation data
  • src/Donations/Micros/GiftForm.tsx
    • updates logic to set defaultDetails to reflect updated types
    • adds logic to capture only recipient name for membership projects
    • updates text seen when either an invitation or direct gift is set - translation key change fromdirectGiftRecipient --> giftDedicatedTo
  • src/Donations/LeftPanel/LeftPanelInfo.tsx
  • Updates usage of SentGift with GiftDetails
  • src/Donations/LeftPanel/GiftInfo.tsx
    • Updates usage of SentGift with GiftDetails
  • src/Donations/Micros/PaymentStatus/FailedDonation.tsx
    • Updates usage of SentGift with GiftDetails
  • src/Donations/Micros/PaymentStatus/ThankyouMessage.tsx
    • Updates usage of SentGift with GiftDetails

Refactors

  • Added utility function createProjectDetails to streamline project details initialization from /paymentOptions response in src/Layout/QueryParamContext.tsx and pages/index.tsx
  • src/Common/Types/index.tsx --> Split FetchedProjectDetails into a union of discriminated types like FetchedTreeProjectDetails, FetchedFundsProjectDetails, and FetchedOtherProjectDetails, and adds classification to the project related types
  • Corrects multiple minor typos across app to use camelCase for variables e.g. setshowEmail --> setShowEmail, setisGift --> setIsGift
  • Resolve minor type warnings due to string type of donation.amount returned in various API responses

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced donation process with improved handling of gift details and project eligibility.
    • Introduced new types for better structuring of gift and project data.
    • Added constants to identify non-giftable project purposes and allowed project purposes for Planet Cash.
    • Introduced a new utility function to evaluate Planet Cash eligibility for donations.
  • Bug Fixes

    • Improved logic for displaying gift messages based on type and recipient information.
  • Documentation

    • Updated localization keys for clarity in donation messaging.
  • Chores

    • Updated dependency version for improved stability and performance.

Copy link

vercel bot commented Nov 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
donate-with-planet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 10:18am

Copy link

coderabbitai bot commented Nov 12, 2024

Walkthrough

The pull request introduces several modifications across multiple files, primarily focusing on updating the handling of gift details within the donation process. Key changes include updating types from SentGift to GiftDetails, enhancing the logic for determining gift eligibility, and refining the rendering of gift-related components. Additionally, a new utility function createProjectDetails is added to streamline project detail creation, and constants such as NON_GIFTABLE_PROJECT_PURPOSES are introduced to define project purposes that do not allow gifts.

Changes

File Change Summary
package.json Updated dependency version: "@planet-sdk/common": "^0.1.34""@planet-sdk/common": "^0.1.42"
pages/index.tsx Changed giftDetails type in Props interface and getServerSideProps from `SentGift
public/locales/en/common.json Updated key from "directGiftRecipient" to "giftDedicatedTo" with corresponding message change.
src/Common/Types/QueryParamContextInterface.ts Updated giftDetails type from `SentGift
src/Common/Types/index.tsx Introduced new types DirectGiftDetails, InvitationGiftDetails, and updated SentGift to GiftDetails. Modified several interfaces to reflect new types.
src/Donations/Components/DonationsForm.tsx Added NON_GIFTABLE_PROJECT_PURPOSES constant and refined logic for sending gifts based on project purposes.
src/Donations/LeftPanel/GiftInfo.tsx Updated giftDetails prop type from SentGift to GiftDetails and modified rendering logic based on new structure.
src/Donations/LeftPanel/LeftPanelInfo.tsx Updated giftDetails prop type from `SentGift
src/Donations/Micros/GiftForm.tsx Renamed setshowEmail to setShowEmail, updated logic for default values based on giftDetails.type.
src/Donations/Micros/PaymentStatus/FailedDonation.tsx Updated type from SentGift to GiftDetails, renamed setisGift to setIsGift, and adjusted _giftDetails construction.
src/Donations/Micros/PaymentStatus/ThankyouMessage.tsx Enhanced handling of donation.amount and refined logic for displaying gift messages.
src/Layout/QueryParamContext.tsx Renamed SentGift to GiftDetails, updated state management for gift-related data, and streamlined project detail loading.
src/Utils/createProjectDetails.ts Introduced createProjectDetails function to construct project details based on payment options.
src/Utils/projects/constants.ts Added constant NON_GIFTABLE_PROJECT_PURPOSES as an array of non-giftable project purposes.
src/Donations/Micros/DonationTypes/BouquetDonations.tsx Updated class names for bouquet donation styling without altering functionality.
src/Donations/Micros/DonationTypes/FundingDonations.tsx Modified class names for funding donation styling while preserving original logic.
styles/donations.scss Expanded styles to include bouquet selection alongside funding options, ensuring cohesive design.

Poem

🐇 In the garden of gifts, we now see,
A clearer path for you and me.
With projects bright and details neat,
Donations flow, oh what a treat!
From trees to funds, our hearts align,
Each gift a story, each purpose divine. 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@mohitb35 mohitb35 changed the title Enhance project details handling with classification and purpose variations Giftable donations Nov 12, 2024
- giftDetails stored in context is now of type `GiftDetails`
- `SentGift` is used purely for the API request data
- updates types and type guards through the app
@mohitb35

This comment was marked as resolved.

@sunilsabatp
Copy link
Contributor

Bug found: Currency selector seen in gift form for membership projects and other non-tree projects.

Screenshot 2024-11-19 at 4 51 30 PM

Are you planning to resolve this bug in this PR?

@mohitb35
Copy link
Contributor Author

Bug found: Currency selector seen in gift form for membership projects and other non-tree projects.
Screenshot 2024-11-19 at 4 51 30 PM

Are you planning to resolve this bug in this PR?

This is now resolved on bc4af28

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (9)
src/Donations/Micros/DonationTypes/FundingDonations.tsx (2)

Line range hint 196-211: Consider adding gift-specific amount validation

The custom amount input validation only handles numeric input and length restrictions. Consider adding gift-specific validations such as:

  • Minimum/maximum gift amounts
  • Restrictions based on gift type (direct vs invitation)
  • Project-specific gift amount rules

Example implementation:

 onInput={(e: ChangeEvent<HTMLInputElement>) => {
   // replaces any character other than number to blank
   e.target.value = e.target.value.replace(/[^0-9]/g, "");
   //  if length of input more than 12, display only 12 digits
   if (e.target.value.toString().length >= 12) {
     e.target.value = e.target.value.toString().slice(0, 12);
   }
+  // Add gift-specific validation
+  if (isGift) {
+    const amount = Number(e.target.value);
+    if (amount < MIN_GIFT_AMOUNT || amount > MAX_GIFT_AMOUNT) {
+      // Handle invalid gift amount
+    }
+  }
 }}
🧰 Tools
🪛 Biome

[error] 112-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


Currency selector should respect non-giftable project restrictions

The verification reveals that while NON_GIFTABLE_PROJECT_PURPOSES is properly defined as ["planet-cash", "bouquet"] and used in DonationsForm.tsx for gift-related logic, the currency selector behavior in FundingDonations.tsx only checks for planet-cash purpose. This creates an inconsistency where bouquet projects, which should have similar restrictions, are not properly handled in the currency selector logic.

  • In src/Donations/Micros/DonationTypes/FundingDonations.tsx, update the condition to include both non-giftable project types:
projectDetails?.purpose !== "planet-cash" &&
!isPlanetCashActive &&
setopenCurrencyModal(true);

should be:

!NON_GIFTABLE_PROJECT_PURPOSES.includes(projectDetails?.purpose) &&
!isPlanetCashActive &&
setopenCurrencyModal(true);
🔗 Analysis chain

Line range hint 249-258: Verify currency selector behavior with gift scenarios

The currency selector's visibility logic only considers planet-cash purpose and isPlanetCashActive. According to the PR objectives, there are additional restrictions for membership projects, bouquet projects, and various gift scenarios that should be reflected here.

Let's verify the currency selector behavior across different project types:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for project type constants and their usage
ast-grep --pattern 'NON_GIFTABLE_PROJECT_PURPOSES'

# Check for currency selector handling in other components
rg -A 5 "setopenCurrencyModal"

Length of output: 6555

🧰 Tools
🪛 Biome

[error] 112-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/Donations/Micros/DonationTypes/BouquetDonations.tsx (4)

134-138: Consider improving UX feedback for gift recipient requirement

Instead of hiding the entire bouquet selection when a gift recipient is not specified, consider showing a message prompting the user to enter the recipient's name first. This would provide clearer feedback to users.

 <div
   className={`bouquet-selection ${
-    isGift && giftDetails.recipientName === "" ? "display-none" : ""
+    isGift && giftDetails.recipientName === "" ? "disabled-selection" : ""
   }`}
 >
+  {isGift && giftDetails.recipientName === "" && (
+    <div className="recipient-prompt">
+      {t("pleaseEnterRecipientName")}
+    </div>
+  )}

152-156: LGTM with a note about future refactoring

The class name changes are consistent with the bouquet-specific styling approach. However, note the TODO comment about rounded costs that will require structural changes. This should be tracked separately.

Would you like me to create a GitHub issue to track the rounded costs refactoring mentioned in the TODO comment?

Also applies to: 159-159


178-179: Consider improving currency symbol handling for internationalization

While the styling changes look good, the currency symbol handling could be improved to better support different currency formats (pre/post-fixed symbols, spacing rules, etc.).

Consider using the Intl.NumberFormat API for more robust currency formatting:

- <p style={{ fontSize: "18px", marginTop: "3px" }}>
-   {paymentSetup.purpose === "bouquet" ? getFormatedCurrencySymbol(currency) : []}
- </p>
+ <span className="currency-symbol" style={{ fontSize: "18px" }}>
+   {new Intl.NumberFormat(i18n.language, {
+     style: 'currency',
+     currency: currency,
+     currencyDisplay: 'symbol'
+   }).formatToParts(0).find(part => part.type === 'currency')?.value}
+ </span>

Also applies to: 200-200


Line range hint 1-302: Clean up commented code and track TODOs

There are several areas that need cleanup:

  1. The commented-out roundedCostCalculator function should be removed if not needed
  2. The "redundant" conditions left as reminders should be documented in the codebase or removed

Would you like me to:

  1. Create a GitHub issue to track the rounded costs TODO?
  2. Help document the "redundant" conditions in a more maintainable way?
styles/donations.scss (3)

299-300: Consider using a shared base class for selection containers

The identical styles for .funding-selection-options-container and .bouquet-selection-options-container suggest an opportunity to reduce duplication.

Consider this refactor:

-.funding-selection-options-container,
-.bouquet-selection-options-container {
+.selection-options-container {
  display: flex;
  flex-direction: row;
  flex-wrap: wrap;
  justify-content: space-between;
  margin-top: 10px;
}

+.funding-selection-options-container,
+.bouquet-selection-options-container {
+  @extend .selection-options-container;
+}

306-307: Apply DRY principle to selection option styles

Similar to the containers, the option classes share identical styles and selected states.

Consider this refactor:

-.funding-selection-option,
-.bouquet-selection-option {
+.selection-option {
  width: 130px;
  margin: 10px;
  border: 1px solid $greyColor;
  border-radius: 4px;
  // ... (common styles)
  
-  &.funding-selection-option-selected,
-  &.bouquet-selection-option-selected,
+  &.selection-option-selected,
  &:hover,
  &:focus {
    // ... (common styles)
  }
}

+.funding-selection-option,
+.bouquet-selection-option {
+  @extend .selection-option;
+}

Also applies to: 330-330


487-488: Consolidate custom input styles

The custom input styles for both funding and bouquet are identical.

Consider this refactor:

-.funding-custom-tree-input,
-.bouquet-custom-tree-input {
+.custom-tree-input-base {
  border: 0px;
  border-bottom: 2px solid $activeGreenBgTextColor;
  // ... (common styles)
}

+.funding-custom-tree-input,
+.bouquet-custom-tree-input {
+  @extend .custom-tree-input-base;
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fb98cd9 and bc4af28.

📒 Files selected for processing (3)
  • src/Donations/Micros/DonationTypes/BouquetDonations.tsx (6 hunks)
  • src/Donations/Micros/DonationTypes/FundingDonations.tsx (2 hunks)
  • styles/donations.scss (5 hunks)
🔇 Additional comments (3)
src/Donations/Micros/DonationTypes/FundingDonations.tsx (1)

106-111: Review the gift recipient visibility logic

The condition isGift && giftDetails.recipientName === "" hides the funding selection when there's a gift without a recipient name. This might lead to a confusing user experience where the funding options disappear completely instead of showing a more informative state.

Consider one of these alternatives:

  1. Show a message prompting the user to enter recipient details first
  2. Disable the funding options instead of hiding them completely

Let's verify the CSS classes and related components:

src/Donations/Micros/DonationTypes/BouquetDonations.tsx (1)

139-139: LGTM! Improved class naming specificity

The change from generic to bouquet-specific class names improves maintainability and styling isolation.

styles/donations.scss (1)

760-764: LGTM: Responsive design implementation

The responsive styles for tablet view are well-structured and maintain consistency between funding and bouquet options while providing specific width adjustments for different scenarios.

Copy link
Contributor

@mariahosfeld mariahosfeld left a comment

Choose a reason for hiding this comment

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

UI for recipient data is broken, below partial example, affects whole UI
image
Addition:
Almost all UIs with input textfields is broken, contact details has the issue as well, payment details work though

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
src/Common/InputTypes/MaterialTextField.tsx (5)

17-31: Consider using theme spacing for consistency.

The hardcoded values for positioning could be replaced with theme spacing utilities for better maintainability and consistency across the application.

 "& .MuiInputLabel-root": {
-    transform: "translate(14px, 16px) scale(1)",
+    transform: `translate(${theme.spacing(1.75)}, ${theme.spacing(2)}) scale(1)`,
     // ... other properties
   },

33-43: Document critical positioning values.

The magic numbers (133%, -3px) used for positioning should be documented to explain their significance and prevent accidental modifications.

Add comments explaining:

  1. Why 133% is used for maxWidth calculation
  2. Why -3px top positioning is critical for the shrunk label

46-52: Consider dynamic height for accessibility.

Fixed height values might cause text clipping when users increase their browser's font size. Consider using min-height or padding-based height calculation for better accessibility.

 "& .MuiInputBase-input.MuiOutlinedInput-input": {
   color: "var(--primary-font-color)",
-  height: "20px",
-  lineHeight: "20px",
+  minHeight: "20px",
+  lineHeight: 1.5,
   padding: "14px",
 },

76-85: Enhance disabled state styling.

Consider the following improvements:

  1. Add vendor prefixes for cross-browser compatibility
  2. Use CSS custom property for disabled color consistency
 "& .Mui-disabled": {
   "&.MuiInputLabel-root": {
-    color: "var(--primary-font-color)",
+    color: "var(--disabled-font-color)",
   },
   "&.MuiOutlinedInput-input": {
     color: "var(--disabled-font-color)",
-    WebkitTextFillColor: "initial",
+    "-webkit-text-fill-color": "var(--disabled-font-color)",
+    "-moz-text-fill-color": "var(--disabled-font-color)",
   },
 },

87-103: Simplify nested selectors and padding.

The current implementation has deeply nested selectors and might cause inconsistent text alignment. Consider simplifying:

-  "& .MuiInputBase-multiline": {
-    padding: "0px",
-  },
+  "& .MuiInputBase-multiline": {
+    padding: "14px",
+    "& .MuiInputBase-input": {
+      padding: 0,
+    }
+  },

-  '& .MuiAutocomplete-inputRoot[class*="MuiOutlinedInput-root"]': {
-    padding: "14px",
-
-    "& .MuiAutocomplete-input": {
-      padding: "0px",
-
-      "&:first-of-type": {
-        paddingLeft: "0px",
-      },
-    },
-  },
+  "& .MuiAutocomplete-root .MuiOutlinedInput-root": {
+    "& .MuiAutocomplete-input": {
+      padding: "14px",
+    }
+  },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bc4af28 and 83e1544.

📒 Files selected for processing (1)
  • src/Common/InputTypes/MaterialTextField.tsx (1 hunks)
🔇 Additional comments (1)
src/Common/InputTypes/MaterialTextField.tsx (1)

Line range hint 1-106: Verify component usage in gift-related forms.

Since this component is part of the giftable donations PR, let's verify its usage in gift-related forms to ensure the styling changes don't affect the gift form functionality.

✅ Verification successful

MaterialTextField styling changes are safe for gift-related forms

The verification shows that MaterialTextField is used in GiftForm.tsx for standard form inputs (recipient name, email, and a multiline field) without any custom styling overrides. The component uses only the default styling defined in the MaterialTextField component, which means the current styling changes won't negatively impact the gift form functionality.

  • Used in GiftForm.tsx for:
    • Recipient name input (single line)
    • Recipient email input (single line)
    • Multiline input with min/max rows
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find usage of MaterialTextField in gift-related components

# Search for MaterialTextField imports in gift-related files
echo "Searching for MaterialTextField usage in gift-related files..."
rg -l "MaterialTextField" | rg -i "gift"

# Search for specific gift form components that might use this TextField
echo "Searching for gift form components..."
rg -l "GiftForm|GiftDetails" 

Length of output: 869


Script:

#!/bin/bash
# Let's examine the GiftForm component to understand how MaterialTextField is used
echo "Checking MaterialTextField usage in GiftForm.tsx..."
rg -A 5 "MaterialTextField" "src/Donations/Micros/GiftForm.tsx"

# Also check if there are any specific styling overrides for MaterialTextField in GiftForm
echo -e "\nChecking for style-related props passed to MaterialTextField..."
ast-grep --pattern 'MaterialTextField $$$={$$$}'  "src/Donations/Micros/GiftForm.tsx"

# Look for any existing style-related issues or comments about MaterialTextField
echo -e "\nChecking for any related issues in git history..."
rg -i "materialtext.*style|text.*field.*style" "src/Donations/Micros/GiftForm.tsx"

Length of output: 1758

src/Common/InputTypes/MaterialTextField.tsx Show resolved Hide resolved
@mariahosfeld mariahosfeld self-requested a review November 22, 2024 10:47
Copy link
Contributor

@mariahosfeld mariahosfeld left a comment

Choose a reason for hiding this comment

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

Functionality works, no longer issues with UI detected, but lets keep an eye out.

mohitb35 and others added 6 commits November 26, 2024 16:01
- refactors code to extract logic to show `PlanetCashSelector` to `isPlanetCashAllowed` utility function
- adds authentication to loadPaymentSetup call (as "planet-cash" gateway is only returned in authenticated call)
- adds debouncing while loading payment setup for non planet cash projects
…planetcash-usage

Allow PlanetCash payments for more types of donations
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/Utils/projects/constants.ts (1)

4-7: LGTM! Consider adding JSDoc comments for better documentation.

The constants are well-typed and correctly define non-giftable project purposes.

Consider adding JSDoc comments to explain the purpose and usage of these constants:

+/**
+ * Project purposes that do not support gifting functionality
+ */
 export const NON_GIFTABLE_PROJECT_PURPOSES: Array<ProjectPurpose> = [
   "planet-cash",
   "bouquet",
 ];
src/Utils/donationOptions.ts (1)

20-43: Consider refactoring for better readability using early returns.

The function logic is correct but could be more maintainable with early returns instead of a single large boolean expression.

Consider this refactor:

 export const isPlanetCashAllowed = ({
   profile,
   isSignedUp,
   projectDetails,
   isGift,
   giftDetails,
   hasPlanetCashGateway,
 }: PlanetCashAllowedParams): boolean => {
-  return (
-    profile !== null &&
-    isSignedUp &&
-    profile.planetCash !== null &&
-    hasPlanetCashGateway &&
-    projectDetails !== null &&
-    PLANETCASH_ALLOWED_PROJECT_PURPOSES.includes(projectDetails.purpose) &&
-    (projectDetails.classification === null ||
-      !PLANETCASH_DISALLOWED_PROJECT_CLASSIFICATIONS.includes(
-        projectDetails.classification
-      )) &&
-    projectDetails.taxDeductionCountries !== undefined &&
-    projectDetails.taxDeductionCountries.includes(profile.planetCash.country) &&
-    !(isGift && giftDetails.recipientName === "")
-  );
+  if (!profile || !isSignedUp || !profile.planetCash || !hasPlanetCashGateway) {
+    return false;
+  }
+
+  if (!projectDetails || !PLANETCASH_ALLOWED_PROJECT_PURPOSES.includes(projectDetails.purpose)) {
+    return false;
+  }
+
+  if (projectDetails.classification !== null && 
+      PLANETCASH_DISALLOWED_PROJECT_CLASSIFICATIONS.includes(projectDetails.classification)) {
+    return false;
+  }
+
+  if (!projectDetails.taxDeductionCountries?.includes(profile.planetCash.country)) {
+    return false;
+  }
+
+  if (isGift && giftDetails.recipientName === "") {
+    return false;
+  }
+
+  return true;
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 83e1544 and b7cb75c.

📒 Files selected for processing (5)
  • src/Common/Types/index.tsx (8 hunks)
  • src/Donations/Components/DonationsForm.tsx (4 hunks)
  • src/Layout/QueryParamContext.tsx (7 hunks)
  • src/Utils/donationOptions.ts (1 hunks)
  • src/Utils/projects/constants.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Donations/Components/DonationsForm.tsx
  • src/Layout/QueryParamContext.tsx
🧰 Additional context used
📓 Learnings (1)
src/Common/Types/index.tsx (1)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-donations#470
File: src/Utils/createProjectDetails.ts:18-37
Timestamp: 2024-11-18T09:45:48.416Z
Learning: In `src/Utils/createProjectDetails.ts`, the `createProjectDetails` function is structured to handle the discriminated union type `FetchedProjects` (extended by `PaymentOptions`). Refactoring the switch statement may introduce TypeScript warnings because the type of `classification` differs for "trees" and "funds" projects.
🔇 Additional comments (4)
src/Utils/projects/constants.ts (1)

9-17: LGTM! Planet Cash constants are well-defined.

The constants correctly define the allowed project purposes and disallowed classifications for Planet Cash functionality.

src/Common/Types/index.tsx (3)

17-25: LGTM! Gift types are well-structured.

The separation between DirectGiftDetails and InvitationGiftDetails is clear and type-safe.


172-200: LGTM! Comprehensive project classifications.

The classification types are well-defined and exhaustive. The separation between tree and funds classifications provides good type safety.


258-263: Consider adding validation for PlanetCashGateway balance.

The interface correctly defines the structure, but consider adding runtime validation to ensure available is not greater than balance + creditLimit.

Let's check for potential balance validation in the codebase:

Comment on lines +130 to +162
export interface FetchedBaseProjectDetails {
id: string;
name: string;
description?: string | null;
ownerAvatar: string | null;
ownerName: string | null;
image?: string | null;
purpose: ProjectPurpose;
classification: Nullable<ProjectClassification>;
taxDeductionCountries?: Array<string>;
isApproved: boolean;
isTopProject: boolean;
}

export interface FetchedTreeProjectDetails extends FetchedBaseProjectDetails {
purpose: "trees";
classification: TreeProjectClassification;
}

export interface FetchedFundsProjectDetails extends FetchedBaseProjectDetails {
purpose: "funds";
classification: FundsProjectClassification;
}

export interface FetchedOtherProjectDetails extends FetchedBaseProjectDetails {
purpose: "conservation" | "reforestation" | "bouquet" | "planet-cash";
classification: null;
}

export type FetchedProjectDetails =
| FetchedTreeProjectDetails
| FetchedFundsProjectDetails
| FetchedOtherProjectDetails;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Runtime validation for project classifications is recommended

Based on the codebase analysis, while the types are well-structured using discriminated unions, there's no explicit runtime validation to ensure classification values match their respective project purposes. The createProjectDetails function shows the mapping logic but doesn't validate the classification values.

  • Consider adding runtime validation in src/Utils/createProjectDetails.ts to ensure:
    • Tree projects only receive TreeProjectClassification values
    • Fund projects only receive FundsProjectClassification values
    • Other project types have null classification

This can prevent potential type mismatches that TypeScript can't catch at runtime.

🔗 Analysis chain

Consider adding validation for project classifications.

The project details types are well-structured using discriminated unions. However, there's no runtime validation to ensure the classification matches the project purpose.

Let's verify if there are any mismatches in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potential classification mismatches
ast-grep --pattern 'classification: $classification' | rg -A 2 'purpose: "(trees|funds)"'

Length of output: 91


Script:

#!/bin/bash
# Let's search for usage of these types and validation patterns
rg -l "FetchedProjectDetails|TreeProjectClassification|FundsProjectClassification"

# Then look for any validation logic around project classification
rg -A 5 "classification.*purpose" 

Length of output: 739


Script:

#!/bin/bash
# Let's check createProjectDetails.ts as it might contain validation logic
cat src/Utils/createProjectDetails.ts

# Also check for any validation functions or type guards
rg -A 5 "function.*Project.*Classification|isTree.*Project|isFunds.*Project"

Length of output: 1174


Script:

#!/bin/bash
# Let's check the PaymentOptions type definition and any validation
rg -A 10 "interface PaymentOptions" src/Common/Types/

# Also check for any zod schemas or validation utilities
rg -l "z\.object|validator|schema|validate"

Length of output: 475

…t-gift-recipient-name-length

Add validation for recipient name length in GiftForm
@mariahosfeld mariahosfeld merged commit 2a71cb1 into develop Dec 3, 2024
3 of 5 checks passed
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
src/Donations/Micros/GiftForm.tsx (3)

34-36: Consider simplifying default value assignments

The conditional assignments can be simplified using optional chaining and nullish coalescing.

    recipientEmail:
-      (giftDetails.type === "invitation" && giftDetails.recipientEmail) || "",
+      giftDetails.type === "invitation" ? giftDetails.recipientEmail ?? "" : "",
    message:
-      (giftDetails.type === "invitation" && giftDetails.message) || "",
+      giftDetails.type === "invitation" ? giftDetails.message ?? "" : "",

121-124: Consider adding defensive error message handling

The current error message display could be more robust by handling undefined message cases.

  {errors.recipientName !== undefined && (
    <div className={"form-errors"}>
-      {errors.recipientName.message}
+      {errors.recipientName.message || t("genericError")}
    </div>
  )}

128-129: Remove unnecessary Fragment

The Fragment (<>) wrapper is redundant here as it contains only conditional content under a parent div.

  {projectDetails?.classification !== "membership" && (
-    <>
      {showEmail ? (
        // ... content
      ) : (
        // ... content
      )}
-    </>
  )}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b7cb75c and 8546ebb.

📒 Files selected for processing (2)
  • public/locales/en/common.json (3 hunks)
  • src/Donations/Micros/GiftForm.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/locales/en/common.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/Donations/Micros/GiftForm.tsx

[error] 129-203: Avoid using unnecessary Fragment.

A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment

(lint/complexity/noUselessFragments)

🔇 Additional comments (3)
src/Donations/Micros/GiftForm.tsx (3)

94-103: LGTM! Well-structured validation rules

The validation rules for the recipient name are comprehensive, including both required validation and a reasonable maximum length limit of 35 characters. Good use of translation keys for error messages.


145-154: LGTM! Robust email validation implementation

The email validation implementation is well-structured with:

  • Required field validation
  • Custom validation using isEmailValid utility
  • Proper handling of empty values

217-219: LGTM! Proper translation implementation

The gift dedication message correctly uses the translation system with parameter interpolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants