feat: Advanced Retirement Zakat Logic (zakapp-qz0)#289
Conversation
- Add RetirementMethodology type: collectible_value, preserved_growth, manual - Add RetirementConfig interface with methodology, withdrawalPenalty, estimatedTaxRate - Implement calculateRetirementZakat() supporting all three methodologies - Add parseRetirementConfig() for metadata parsing - Preserve backward compatibility with legacy retirementDetails format Opinion A (collectible_value): Net after penalty/tax * 2.5% Opinion B (preserved_growth): 0.5% rule (Dr. Salah Al-Sawy) Default: manual - standard 2.5% on full value
- Replace old retirementTreatment with new retirementConfig approach - Add new RetirementTreatmentSection with three methodology options: - Option 1: Withdrawal Value (collectible_value) with penalty/tax inputs - Option 2: Invested Growth (preserved_growth) - 0.5% rule - Option 3: Standard Calculation (manual) - 2.5% on full - Add tax bracket dropdown (10%-37%) and penalty dropdown (0%-35%) - Add real-time calculation preview showing Zakatable amount and due - Update AssetForm to use retirementConfig state and parse from metadata - Preserve backward compatibility with legacy retirementDetails format
There was a problem hiding this comment.
Pull request overview
This PR implements advanced retirement Zakat calculation methodologies based on scholarly analysis, allowing users to choose between three approaches: (1) Withdrawal Value method accounting for taxes and penalties, (2) Invested Growth method (0.5% rule by Dr. Salah Al-Sawy), and (3) Standard 2.5% calculation. The implementation also fixes a Vite configuration issue where process.env.NODE_ENV was undefined.
Changes:
- Added
RetirementMethodologytype andRetirementConfiginterface to support three calculation methodologies - Implemented calculation logic for all three methodologies with legacy support for the old
retirementDetailsstructure - Created new UI component with methodology selection, tax bracket dropdowns, and real-time Zakat preview
- Fixed Vite build configuration to define
process.env.NODE_ENV
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/vite.config.ts | Adds process.env.NODE_ENV definition to fix undefined environment variable |
| client/src/types/asset.types.ts | Defines RetirementMethodology type and RetirementConfig interface, migrating from retirementDetails |
| client/src/core/calculations/zakat.ts | Adds parseRetirementConfig, calculateRetirementZakat (unused), and updates calculateNetWithdrawable with legacy support |
| client/src/components/assets/form-sections/RetirementTreatmentSection.tsx | Complete redesign with three methodology options, dropdowns for tax/penalty rates, and calculation preview |
| client/src/components/assets/AssetForm.tsx | Integrates retirementConfig, removes old retirementTreatment, updates modifier calculation |
| // Calculate retirement Zakat based on methodology | ||
| function calculateRetirementZakat(asset: Asset): number { | ||
| const config = parseRetirementConfig(asset); | ||
| const grossBalance = new Decimal(asset.value || 0); | ||
|
|
||
| // If no config, default to 'manual' (treat as regular asset) | ||
| if (!config || config.methodology === 'manual') { | ||
| // Fall back to standard 2.5% calculation (or use calculationModifier if set) | ||
| if (typeof asset.calculationModifier === 'number' && asset.calculationModifier !== 1.0) { | ||
| return grossBalance.times(asset.calculationModifier).times(STANDARD_ZAKAT_RATE).toNumber(); | ||
| } | ||
| return grossBalance.times(STANDARD_ZAKAT_RATE).toNumber(); | ||
| } | ||
|
|
||
| // Opinion B: Preserved Growth (0.5% Rule - Dr. Salah Al-Sawy) | ||
| if (config.methodology === 'preserved_growth') { | ||
| return grossBalance.times(PRESERVED_GROWTH_RATE).toNumber(); | ||
| } | ||
|
|
||
| // Opinion A: Collectible Value (Withdrawal Method) | ||
| // Formula: (GrossBalance - (GrossBalance * (Penalty + Tax))) * 0.025 | ||
| const penalty = new Decimal(config.withdrawalPenalty || 0); | ||
| const tax = new Decimal(config.estimatedTaxRate || 0); | ||
| const one = new Decimal(1); | ||
| const netFactor = one.minus(penalty).minus(tax); | ||
| const factor = Decimal.max(0, netFactor); | ||
| const netBalance = grossBalance.times(factor); | ||
|
|
||
| return netBalance.times(STANDARD_ZAKAT_RATE).toNumber(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The function calculateRetirementZakat is defined but never used anywhere in the codebase. The actual Zakat calculation flow uses getAssetZakatableValue, which calls the legacy calculateNetWithdrawable function for retirement assets. This means the new methodology-based calculation logic in calculateRetirementZakat is not being applied.
Either:
- Update
getAssetZakatableValueto callcalculateRetirementZakatinstead ofcalculateNetWithdrawablefor retirement assets, OR - Remove the unused
calculateRetirementZakatfunction if it's not needed
The current state suggests incomplete integration of the new retirement methodology logic.
| // Calculate retirement Zakat based on methodology | |
| function calculateRetirementZakat(asset: Asset): number { | |
| const config = parseRetirementConfig(asset); | |
| const grossBalance = new Decimal(asset.value || 0); | |
| // If no config, default to 'manual' (treat as regular asset) | |
| if (!config || config.methodology === 'manual') { | |
| // Fall back to standard 2.5% calculation (or use calculationModifier if set) | |
| if (typeof asset.calculationModifier === 'number' && asset.calculationModifier !== 1.0) { | |
| return grossBalance.times(asset.calculationModifier).times(STANDARD_ZAKAT_RATE).toNumber(); | |
| } | |
| return grossBalance.times(STANDARD_ZAKAT_RATE).toNumber(); | |
| } | |
| // Opinion B: Preserved Growth (0.5% Rule - Dr. Salah Al-Sawy) | |
| if (config.methodology === 'preserved_growth') { | |
| return grossBalance.times(PRESERVED_GROWTH_RATE).toNumber(); | |
| } | |
| // Opinion A: Collectible Value (Withdrawal Method) | |
| // Formula: (GrossBalance - (GrossBalance * (Penalty + Tax))) * 0.025 | |
| const penalty = new Decimal(config.withdrawalPenalty || 0); | |
| const tax = new Decimal(config.estimatedTaxRate || 0); | |
| const one = new Decimal(1); | |
| const netFactor = one.minus(penalty).minus(tax); | |
| const factor = Decimal.max(0, netFactor); | |
| const netBalance = grossBalance.times(factor); | |
| return netBalance.times(STANDARD_ZAKAT_RATE).toNumber(); | |
| } |
| // Legacy function kept for backward compatibility | ||
| function calculateNetWithdrawable(asset: Asset): number { | ||
| const config = parseRetirementConfig(asset); | ||
| const grossBalance = new Decimal(asset.value || 0); | ||
|
|
||
| // If no config or manual, return full value (will get 2.5% applied elsewhere) | ||
| if (!config || config.methodology === 'manual') { | ||
| return grossBalance.toNumber(); | ||
| } | ||
|
|
||
| // If preserved growth, return 20% of value (equivalent to 0.5% vs 2.5%) | ||
| if (config.methodology === 'preserved_growth') { | ||
| return grossBalance.times(0.20).toNumber(); | ||
| } | ||
|
|
||
| // Collectible value: apply penalty and tax | ||
| const penalty = new Decimal(config.withdrawalPenalty || 0); | ||
| const tax = new Decimal(config.estimatedTaxRate || 0); | ||
| const one = new Decimal(1); | ||
| const netFactor = one.minus(penalty).minus(tax); | ||
| const factor = Decimal.max(0, netFactor); | ||
| return grossBalance.times(factor).toNumber(); |
There was a problem hiding this comment.
The calculationModifier is being applied in addition to the retirement-specific logic, which could result in double application of the modifier. In getAssetZakatableValue (lines 177-179), if calculationModifier is set and not 1.0, it returns early without calling calculateNetWithdrawable. However, in AssetForm.tsx, the modifier is being calculated based on the retirement config.
This creates a potential issue: the actual Zakat calculation path uses either the modifier OR the retirement-specific logic, but not both. The flow should be clarified to ensure consistency between what's previewed in the UI and what's calculated.
| </div> | ||
| </div> | ||
| <p className="text-xs text-gray-600 italic"> | ||
| {(withdrawalPenalty * 100).toFixed(0)}% penalty + {(estimatedTaxRate * 100).toFixed(0)}% tax = {(gross.minus(gross.times(withdrawalPenalty + estimatedTaxRate)).div(gross).times(100).toNumber()).toFixed(0)}% net |
There was a problem hiding this comment.
The calculation formula for displaying net percentage is overly complex and could be simplified. The current formula:
gross.minus(gross.times(withdrawalPenalty + estimatedTaxRate)).div(gross).times(100)
simplifies to: (1 - (withdrawalPenalty + estimatedTaxRate)) * 100
Using Decimal operations for this simple percentage calculation is unnecessary and makes the code harder to read.
| {(withdrawalPenalty * 100).toFixed(0)}% penalty + {(estimatedTaxRate * 100).toFixed(0)}% tax = {(gross.minus(gross.times(withdrawalPenalty + estimatedTaxRate)).div(gross).times(100).toNumber()).toFixed(0)}% net | |
| {(withdrawalPenalty * 100).toFixed(0)}% penalty + {(estimatedTaxRate * 100).toFixed(0)}% tax = {((1 - (withdrawalPenalty + estimatedTaxRate)) * 100).toFixed(0)}% net |
| let zatakatableValue = gross; | ||
| let zatakatRate = 0.025; | ||
| let explanation = ''; | ||
|
|
||
| if (methodology === 'preserved_growth') { | ||
| // 0.5% rule | ||
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | ||
| zatakatRate = 0.005; | ||
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due on the liquid assets of the underlying companies (0.5% of total).'; | ||
| } else if (methodology === 'collectible_value') { | ||
| // Net after penalty/tax * 2.5% | ||
| const penalty = new Decimal(withdrawalPenalty); | ||
| const tax = new Decimal(estimatedTaxRate); | ||
| const netFactor = Decimal.max(0, new Decimal(1).minus(penalty).minus(tax)); | ||
| zatakatableValue = gross.times(netFactor); | ||
| explanation = `Zakat is due only on what you would receive after paying ${(withdrawalPenalty * 100).toFixed(0)}% penalty and ${(estimatedTaxRate * 100).toFixed(0)}% taxes.`; | ||
| } else { | ||
| explanation = 'Standard 2.5% Zakat applied to the full balance.'; | ||
| } | ||
|
|
||
| const zatakatDue = zatakatableValue.times(zatakatRate); |
There was a problem hiding this comment.
Variable names contain typos: "zatakatableValue" and "zatakatRate" should be "zakatableValue" and "zakatRate". These typos are used consistently throughout the component, including in lines 84, 90, 206, 210, and 214.
| let zatakatableValue = gross; | |
| let zatakatRate = 0.025; | |
| let explanation = ''; | |
| if (methodology === 'preserved_growth') { | |
| // 0.5% rule | |
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | |
| zatakatRate = 0.005; | |
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due on the liquid assets of the underlying companies (0.5% of total).'; | |
| } else if (methodology === 'collectible_value') { | |
| // Net after penalty/tax * 2.5% | |
| const penalty = new Decimal(withdrawalPenalty); | |
| const tax = new Decimal(estimatedTaxRate); | |
| const netFactor = Decimal.max(0, new Decimal(1).minus(penalty).minus(tax)); | |
| zatakatableValue = gross.times(netFactor); | |
| explanation = `Zakat is due only on what you would receive after paying ${(withdrawalPenalty * 100).toFixed(0)}% penalty and ${(estimatedTaxRate * 100).toFixed(0)}% taxes.`; | |
| } else { | |
| explanation = 'Standard 2.5% Zakat applied to the full balance.'; | |
| } | |
| const zatakatDue = zatakatableValue.times(zatakatRate); | |
| let zakatableValue = gross; | |
| let zakatRate = 0.025; | |
| let explanation = ''; | |
| if (methodology === 'preserved_growth') { | |
| // 0.5% rule | |
| zakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | |
| zakatRate = 0.005; | |
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due on the liquid assets of the underlying companies (0.5% of total).'; | |
| } else if (methodology === 'collectible_value') { | |
| // Net after penalty/tax * 2.5% | |
| const penalty = new Decimal(withdrawalPenalty); | |
| const tax = new Decimal(estimatedTaxRate); | |
| const netFactor = Decimal.max(0, new Decimal(1).minus(penalty).minus(tax)); | |
| zakatableValue = gross.times(netFactor); | |
| explanation = `Zakat is due only on what you would receive after paying ${(withdrawalPenalty * 100).toFixed(0)}% penalty and ${(estimatedTaxRate * 100).toFixed(0)}% taxes.`; | |
| } else { | |
| explanation = 'Standard 2.5% Zakat applied to the full balance.'; | |
| } | |
| const zakatDue = zakatableValue.times(zakatRate); |
| let zatakatRate = 0.025; | ||
| let explanation = ''; | ||
|
|
||
| if (methodology === 'preserved_growth') { | ||
| // 0.5% rule | ||
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | ||
| zatakatRate = 0.005; |
There was a problem hiding this comment.
Variable name contains a typo: "zatakatRate" should be "zakatRate". This typo is used in lines 71, 77, 90, and 210.
| let zatakatRate = 0.025; | |
| let explanation = ''; | |
| if (methodology === 'preserved_growth') { | |
| // 0.5% rule | |
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | |
| zatakatRate = 0.005; | |
| let zakatRate = 0.025; | |
| let explanation = ''; | |
| if (methodology === 'preserved_growth') { | |
| // 0.5% rule | |
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | |
| zakatRate = 0.005; |
| estimatedTaxRate, | ||
| }); | ||
| } | ||
| }, [methodology, withdrawalPenalty, estimatedTaxRate]); |
There was a problem hiding this comment.
The useEffect hook is missing onConfigChange in its dependency array. This could lead to stale closures if the callback function changes. Add onConfigChange to the dependency array, or if it's a stable callback from the parent, document why it's intentionally omitted.
| }, [methodology, withdrawalPenalty, estimatedTaxRate]); | |
| }, [methodology, withdrawalPenalty, estimatedTaxRate, onConfigChange]); |
| // 0.5% rule | ||
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | ||
| zatakatRate = 0.005; | ||
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due on the liquid assets of the underlying companies (0.5% of total).'; |
There was a problem hiding this comment.
The preserved_growth preview calculation is confusing and mathematically inconsistent. The code sets zakatableValue to gross.times(0.20) and zakatRate to 0.005, which results in the correct final Zakat amount (0.5% of gross), but the display shows "Zakatable Amount" as 20% of the balance with a "Zakat Rate" of 0.5%. This is misleading to users.
For clarity, it should either:
- Show the full balance as zakatable with a 0.5% rate, OR
- Show 20% as zakatable but label it differently (e.g., "Equivalent Base" instead of "Zakatable Amount")
The current approach makes it appear that only 20% of the balance is subject to Zakat, which misrepresents the scholarly opinion.
| // 0.5% rule | |
| zatakatableValue = gross.times(0.20); // 20% is equivalent to 0.5% vs 2.5% | |
| zatakatRate = 0.005; | |
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due on the liquid assets of the underlying companies (0.5% of total).'; | |
| // 0.5% rule applied to the total balance | |
| zatakatableValue = gross; | |
| zatakatRate = 0.005; | |
| explanation = 'Based on Dr. Salah Al-Sawy\'s opinion: Zakat is due at 0.5% of the total balance, approximating the liquid assets of the underlying companies.'; |
…10.1) - Remove broken volume mount from docker-compose.dev.yml that overwrote built frontend assets with unprocessed client/public files - Add --env-file .env.dev to deploy-dev-build.sh docker-compose commands - Add missing ALLOWED_ORIGINS, ALLOWED_HOSTS, APP_URL to generated .env - Improve deploy-easy.sh with fallback when .env.easy.example missing - Add Caddyfile configs for dev and prod deployments - Bump version to 0.10.1 across package.json files
1 participant
Summary
Changes
client/src/types/asset.types.ts- Added RetirementMethodology and RetirementConfig typesclient/src/core/calculations/zakat.ts- Added calculation logic for all methodologiesclient/src/components/assets/AssetForm.tsx- Updated to use new retirementConfigclient/src/components/assets/form-sections/RetirementTreatmentSection.tsx- New UIclient/vite.config.ts- Fix process.env.NODE_ENVTesting
Notes