[Feat]: Prospect Updates #3748
[Feat]: Prospect Updates #3748
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Modules/Prospect/Entities/Prospect.php (1)
Line range hint
41-52: Consider extracting the formatting logic into a service class.The current implementation mixes currency formatting logic with the model class, which violates the Single Responsibility Principle. Consider:
- Moving this to a dedicated
CurrencyFormatterservice- Making the format configurable (Indian, International, etc.)
- Adding unit tests for various edge cases
Would you like me to help create a separate service class for currency formatting?
Modules/Prospect/Resources/views/index.blade.php (1)
71-73: Consider improving the currency display implementation.The current implementation separates the currency symbol and the formatted amount into different spans, which could lead to layout issues (unwanted spaces or line breaks). Consider combining them into a single formatted string.
- {{ isset($prospect->currency) && isset($currencySymbols[$prospect->currency]) ? $currencySymbols[$prospect->currency] : '' }} - {{ $prospect->budget ? $prospect->formattedIndianAmount($prospect->budget) : '-' }} + @if($prospect->budget) + {{ ($prospect->currency && isset($currencySymbols[$prospect->currency]) ? $currencySymbols[$prospect->currency] : '') . $prospect->formattedIndianAmount($prospect->budget) }} + @else + - + @endifModules/Prospect/Resources/views/subviews/prospect-details.blade.php (1)
59-62: Consider extracting currency formatting logicThe currency display logic is split across multiple lines, mixing currency symbol lookup with amount formatting. Consider extracting this into a dedicated helper method for better maintainability.
- {{ isset($prospect->currency) && isset($currencySymbols[$prospect->currency]) ? $currencySymbols[$prospect->currency] : '' }} - {{ $prospect->budget ? $prospect->formattedIndianAmount($prospect->budget) : 'N/A' }} + {{ $prospect->getFormattedBudgetWithCurrency() ?? 'N/A' }}This would encapsulate both the currency symbol and amount formatting in a single method, making the template cleaner and the logic more reusable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Modules/Prospect/Entities/Prospect.php(1 hunks)Modules/Prospect/Resources/views/index.blade.php(1 hunks)Modules/Prospect/Resources/views/subviews/prospect-details.blade.php(1 hunks)
🔇 Additional comments (2)
Modules/Prospect/Entities/Prospect.php (1)
41-42: Verify the impact of method renaming across the codebase.
The method has been renamed from formattedAmount to formattedIndianAmount, which better reflects its specific purpose. However, we should ensure all calling code has been updated.
✅ Verification successful
All method references have been properly updated
The codebase scan shows that:
- The old method name
formattedAmountonly appears within the implementation offormattedIndianAmountas a variable name - The new method name
formattedIndianAmountis correctly used in two view files:Modules/Prospect/Resources/views/index.blade.phpModules/Prospect/Resources/views/subviews/prospect-details.blade.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to the old method name
rg "formattedAmount" --type php
# Search for calls to the new method name to verify updates
rg "formattedIndianAmount" --type php
Length of output: 715
Modules/Prospect/Resources/views/index.blade.php (1)
72-72: Verify consistent usage of the new formatting method across all views.
The change from formattedAmount to formattedIndianAmount needs to be consistently applied across all views to maintain uniform formatting.
✅ Verification successful
The change to formattedIndianAmount appears to be consistently applied
The verification shows that:
- No instances of the old
formattedAmountmethod were found in the relevant modules - The new
formattedIndianAmountmethod is consistently used in both places where prospect budget is displayed:Modules/Prospect/Resources/views/index.blade.phpModules/Prospect/Resources/views/subviews/prospect-details.blade.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old formatting method
rg "formattedAmount" --type blade
# Search for the new formatting method to ensure consistent adoption
rg "formattedIndianAmount" --type blade
Length of output: 133
Script:
#!/bin/bash
# Search for any remaining instances of the old formatting method in blade files
rg "formattedAmount" "*.blade.php"
# Search for the new formatting method to ensure consistent adoption
rg "formattedIndianAmount" "*.blade.php"
# Find all blade files in the repository to ensure we're not missing any
fd ".blade.php$"
Length of output: 23287
Script:
#!/bin/bash
# Search for any remaining instances of the old formatting method in the specific directories
rg "formattedAmount" Modules/Prospect/Resources/views/
rg "formattedAmount" Modules/Invoice/Resources/views/
rg "formattedAmount" Modules/Client/Resources/views/
# Search for the new formatting method to ensure consistent adoption
rg "formattedIndianAmount" Modules/Prospect/Resources/views/
rg "formattedIndianAmount" Modules/Invoice/Resources/views/
rg "formattedIndianAmount" Modules/Client/Resources/views/
Length of output: 689
| $amount = (string) $amount; | ||
|
|
||
| if (strlen($amount) <= 3) { | ||
| return $amount; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation and documentation.
The new length check logic needs input validation and documentation:
- The type cast to string assumes the input is numeric
- The purpose of the length check (≤3) should be documented
- Consider handling negative numbers and decimals
public function formattedIndianAmount($amount)
{
+ // Return early if amount is null or empty
+ if ($amount === null || $amount === '') {
+ return '0';
+ }
+
+ // Handle negative numbers
+ $isNegative = $amount < 0;
+ $amount = abs($amount);
+
$amount = (string) $amount;
if (strlen($amount) <= 3) {
- return $amount;
+ return $isNegative ? "-{$amount}" : $amount;
}📝 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.
| $amount = (string) $amount; | |
| if (strlen($amount) <= 3) { | |
| return $amount; | |
| } | |
| // Return early if amount is null or empty | |
| if ($amount === null || $amount === '') { | |
| return '0'; | |
| } | |
| // Handle negative numbers | |
| $isNegative = $amount < 0; | |
| $amount = abs($amount); | |
| $amount = (string) $amount; | |
| if (strlen($amount) <= 3) { | |
| return $isNegative ? "-{$amount}" : $amount; | |
| } |
| @@ -69,7 +69,7 @@ | |||
| <td class="w-30p"> | |||
| <span> | |||
| {{ isset($prospect->currency) && isset($currencySymbols[$prospect->currency]) ? $currencySymbols[$prospect->currency] : '' }} | |||
| {{ $prospect->budget ? $prospect->formattedAmount($prospect->budget) : '-' }} | |||
| {{ $prospect->budget ? $prospect->formattedIndianAmount($prospect->budget) : '-' }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback formatting for error handling.
The code directly calls formattedIndianAmount without any error handling. Consider adding a try-catch block or helper method to handle potential formatting errors gracefully.
- {{ $prospect->budget ? $prospect->formattedIndianAmount($prospect->budget) : '-' }}
+ {{ $prospect->budget ? ($prospect->tryFormatAmount($prospect->budget) ?? '-') : '-' }}Consider adding this helper method to the Prospect model:
public function tryFormatAmount($amount) {
try {
return $this->formattedIndianAmount($amount);
} catch (\Exception $e) {
\Log::error('Amount formatting failed: ' . $e->getMessage());
return null;
}
}| @@ -58,7 +58,7 @@ class="ml-2">{{ config('prospect.customer-types')[$prospect->customer_type] ?? ' | |||
| <label for="budget" class="font-weight-bold">Budget:</label> | |||
| <span class="ml-2"> | |||
| {{ isset($prospect->currency) && isset($currencySymbols[$prospect->currency]) ? $currencySymbols[$prospect->currency] : '' }} | |||
| {{ $prospect->budget ? $prospect->formattedAmount($prospect->budget) : 'N/A' }} | |||
| {{ $prospect->budget ? $prospect->formattedIndianAmount($prospect->budget) : 'N/A' }} | |||
There was a problem hiding this comment.
💡 Codebase verification
Method exists but needs attention to potential issues
The formattedIndianAmount method exists in the Prospect model, but there are a few concerns:
- The method is being used in the view but appears to be missing proper documentation
- The implementation shows it's a public method that could be reused across the application, suggesting it might be better placed in a dedicated formatting trait or helper class
- The method name implies Indian-specific formatting, but the implementation is tightly coupled to the Prospect model
🔗 Analysis chain
Verify the existence of formattedIndianAmount method
The change from formattedAmount to formattedIndianAmount looks good, but let's verify that this method exists in the Prospect model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of formattedIndianAmount method in Prospect model
# and check for consistent usage across the codebase
# Check if the method exists in the Prospect model
ast-grep --pattern 'function formattedIndianAmount' Modules/Prospect/Entities/Prospect.php
# Check for any remaining uses of the old method
rg "formattedAmount" --type php
Length of output: 332
Script:
#!/bin/bash
# Let's check the actual implementation in the Prospect model
rg -A 10 "function.*Amount" Modules/Prospect/Entities/Prospect.php
Length of output: 382
Employee portal
|
||||||||||||||||||||||||||||
| Project |
Employee portal
|
| Branch Review |
refs/pull/3748/merge
|
| Run status |
|
| Run duration | 00m 26s |
| Commit |
|
| Committer | Ayush |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
|
Coverage report for commit: 2eea75a Summary - Lines: 3.16% | Methods: 5.43%
🤖 comment via lucassabreu/comment-coverage-clover |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ayush8923
added
the
status : ready for review
Targets: #3745
Changes
Summary by CodeRabbit
New Features
Bug Fixes