Develop #723
Develop #723
Conversation
Tenant dynamic lat long
…ather than UsageCategory
PFM-5655: Chnaged the logic to check buildingType with propertyType r…
WalkthroughThe recent updates encompass a series of changes across various components of a municipal services application. These modifications include adjustments to data types, logic enhancements, and service improvements. From refining data handling in consumer details to updating billing and expenses logic, alongside adjustments in property and water service calculations, the changes aim to enhance system functionality and performance. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
frontend/mgramseva/package.jsonis excluded by:!**/*.jsonfrontend/mgramseva/pubspec.yamlis excluded by:!**/*.yamlmunicipal-services/vendor/src/main/java/org/egov/vendor/service/BoundaryService.javais excluded by:!**/vendor/**municipal-services/vendor/src/main/java/org/egov/vendor/service/EnrichmentService.javais excluded by:!**/vendor/**municipal-services/vendor/src/main/java/org/egov/vendor/validator/VendorValidator.javais excluded by:!**/vendor/**
Files selected for processing (12)
- frontend/mgramseva/lib/model/mdms/tenants.dart (1 hunks)
- frontend/mgramseva/lib/model/mdms/tenants.g.dart (1 hunks)
- frontend/mgramseva/lib/providers/consumer_details_provider.dart (2 hunks)
- frontend/mgramseva/lib/providers/expenses_details_provider.dart (1 hunks)
- frontend/mgramseva/lib/screeens/consumer_details/consumer_details.dart (2 hunks)
- frontend/mgramseva/lib/services/urls.dart (1 hunks)
- frontend/mgramseva/lib/utils/common_methods.dart (2 hunks)
- municipal-services/property-services/src/main/java/org/egov/pt/service/BoundaryService.java (1 hunks)
- municipal-services/property-services/src/main/java/org/egov/pt/service/EnrichmentService.java (1 hunks)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/DemandService.java (3 hunks)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/EstimationService.java (1 hunks)
- municipal-services/ws-calculator/src/main/resources/application.properties (1 hunks)
Additional comments: 10
frontend/mgramseva/lib/model/mdms/tenants.g.dart (1)
- 88-89: The removal of explicit type conversion for
longitudeandlatitudeduring JSON deserialization aligns with the updateddynamicdata types in theCityclass. This change is necessary for consistency and to prevent deserialization errors.municipal-services/property-services/src/main/java/org/egov/pt/service/BoundaryService.java (1)
- 53-53: The added comment provides useful context about the previous usage of the
getAreaTypemethod. It's important to verify the impact of this change on the application's logic and ensure that the intended functionality is maintained.frontend/mgramseva/lib/services/urls.dart (1)
- 6-6: The update to the MDMS URL constant reflects a change to the service endpoint. Ensure that all references to the MDMS service within the application are updated to use this new endpoint.
municipal-services/ws-calculator/src/main/resources/application.properties (1)
- 25-25: The addition of the
max.poll.interval.msconfiguration property with a value of600000is a good practice to improve Kafka consumer reliability, especially for long-running processes. Ensure that this value aligns with the expected processing times in your application.municipal-services/property-services/src/main/java/org/egov/pt/service/EnrichmentService.java (1)
- 202-202: The line calling
boundaryService.getAreaTypehas been commented out in theenrichBoundarymethod. It's crucial to verify the impact of this change on property boundary enrichment and ensure that the intended functionality is preserved.Verification successful
The search results indicate that the
enrichBoundarymethod is still actively used in the application, suggesting that the functionality for property boundary enrichment is maintained, albeit potentially in a modified form. Given this context, it appears that the impact of commenting out theboundaryService.getAreaTypecall has been mitigated or addressed through other means within the application. Therefore, the original concern about preserving intended functionality seems to be addressed by the continued use and potential modification of theenrichBoundarymethod.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for alternative implementations or references to boundary enrichment to ensure functionality is maintained. rg --type java 'enrichBoundary'Length of output: 822
frontend/mgramseva/lib/utils/common_methods.dart (2)
- 88-101: The
isLeapYearmethod correctly implements the leap year calculation following standard rules. Good job on structuring the logic clearly.- 103-116: The
daysToSubtractmethod correctly calculates the total days to subtract based on the given parameters. However, please verify the index calculation when accessingdaysInMonthLeapanddaysInMontharrays to ensure there are no off-by-one errors.municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/EstimationService.java (1)
- 277-279: The logic for determining the
buildingTypehas been updated to usePropertyTypeinstead ofUsageCategory. This change aims to improve the accuracy of categorization based on property information. However, ensure that all downstream logic that relies onbuildingTypeis compatible with this change and thatPropertyTypevalues are consistently used across the application to prevent any inconsistencies.frontend/mgramseva/lib/screeens/consumer_details/consumer_details.dart (1)
- 512-519: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [515-536]
The modifications to the
itemAsStringandcontrollerparameters in theSelectFieldBuildercalls are noteworthy. Ensure that these changes accurately reflect the desired display and management of items within the select fields. Specifically, the use ofApplicationLocalizations.of(context).translateinitemAsStringis a good practice for supporting localization. However, verify that all possible values passed totranslateare accounted for in your localization files to prevent runtime errors or missing translations.municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/DemandService.java (1)
- 305-308: The addition of the
totalAmountcheck before sending SMS notifications is a good practice to ensure that notifications are only sent when there is a non-zero amount to be notified about. However, it's important to ensure thattotalAmountis accurately calculated and includes all relevant charges to avoid scenarios where a notification might be mistakenly not sent due to an incorrect calculation oftotalAmount.
| } | ||
| String messageString = localizationMessage.get(WSCalculationConstant.MSG_KEY); | ||
|
|
||
| System.out.println("Localization message get bill::" + messageString); | ||
| System.out.println("isForConnectionNO:" + isForConnectionNO); | ||
| //System.out.println("Localization message get bill::" + messageString); | ||
| //System.out.println("isForConnectionNO:" + isForConnectionNO); | ||
| if (!StringUtils.isEmpty(messageString) && isForConnectionNO) { | ||
| log.info("Demand Object get bill" + demands.toString()); | ||
| log.info("requestInfo get Bill" + requestInfo); | ||
| log.info("bill number get bill size :" + billNumbers.size()); | ||
| if (billNumbers.size() > 0) { | ||
| if (totalamount!=null && billNumbers.size() > 0 && totalamount.signum()>0) { | ||
| actionLink = actionLink.replace("$billNumber", billNumbers.get(0)); | ||
| messageString = messageString.replace("{ownername}", owner.getName()); | ||
| messageString = messageString.replace("{Period}", billCycle); | ||
| messageString = messageString.replace("{consumerno}", consumerCode); | ||
| messageString = messageString.replace("{billamount}", demandDetails.stream() | ||
| .map(DemandDetail::getTaxAmount).reduce(BigDecimal.ZERO, BigDecimal::add).toString()); | ||
| messageString = messageString.replace("{billamount}", totalamount.toString()); | ||
| messageString = messageString.replace("{BILL_LINK}", getShortenedUrl(actionLink)); | ||
|
|
||
| System.out.println("Demand genaration Message get bill::" + messageString); | ||
| // System.out.println("Demand genaration Message get bill::" + messageString); | ||
|
|
||
| SMSRequest sms = SMSRequest.builder().mobileNumber(owner.getMobileNumber()).message(messageString).tenantid(tenantId) | ||
| .category(Category.TRANSACTION).build(); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [354-384]
The method sendDownloadBillSMSNotification has been updated to include a new parameter totalamount, which is used to conditionally send SMS notifications based on the bill amount. This is a logical improvement for user experience, ensuring that notifications are meaningful. However, ensure that:
- The
totalamountparameter is accurately calculated and represents the total bill amount correctly. - The method's documentation is updated to reflect the new parameter and its usage.
- Any upstream method calls to
sendDownloadBillSMSNotificationare updated to pass thetotalamountparameter.
Additionally, consider handling potential exceptions or errors that might occur during the SMS sending process to ensure the application's robustness.
// Suggestion to handle potential exceptions in SMS sending logic
try {
// Existing logic to send SMS
} catch (Exception e) {
log.error("Error sending SMS notification", e);
// Handle exception appropriately
}| //Displaying Billing Cycle Vaule (EX- JAN-2021,,) | ||
| List<Map<String,dynamic>> getBillingCycleMonthCountCurrent(TaxPeriod? billYear) { | ||
| var dates = <Map<String,dynamic>>[]; | ||
| if (billYear!=null) { | ||
| DatePeriod ytd; | ||
| var fromDate = DateFormats.getFormattedDateToDateTime( | ||
| DateFormats.timeStampToDate(billYear?.fromDate)) as DateTime; | ||
|
|
||
| var toDate = DateFormats.getFormattedDateToDateTime( | ||
| DateFormats.timeStampToDate(billYear?.toDate)) as DateTime; | ||
|
|
||
| ytd = DatePeriod(fromDate,toDate,DateType.YTD); | ||
|
|
||
| /// Get months based on selected billing year | ||
| var months = CommonMethods.getPastMonthUntilFinancialYTDMonthCount(ytd); | ||
|
|
||
| /// if selected year is future year means all the months will be removed | ||
| if(fromDate.year > ytd.endDate.year) months.clear(); | ||
|
|
||
| for (var i = 0; i < months.length; i++) { | ||
| var prevMonth = months[i].startDate; | ||
| var r = {"code": prevMonth, "name": '${ApplicationLocalizations.of(navigatorKey.currentContext!) | ||
| .translate((Constants.MONTHS[prevMonth.month - 1])) + | ||
| " - " + | ||
| prevMonth.year.toString()}'}; | ||
| dates.add(r); | ||
| } | ||
| } | ||
| if (dates.length > 0 && waterconnection.connectionType == 'Non_Metered') { | ||
| return dates; | ||
| } | ||
| return <Map<String,dynamic>>[]; | ||
| } |
There was a problem hiding this comment.
The addition of getBillingCycleMonthCountCurrent method is a good approach to encapsulate the logic for calculating billing cycle months. However, consider the following points for improvement:
- Code Clarity: The method involves complex date manipulations and conditions. Adding comments to explain the logic, especially around the calculation of
monthsand the condition for clearingmonths, would improve readability and maintainability. - Error Handling: There's no explicit error handling for potential issues with date parsing or invalid
billYearvalues. Consider adding checks or try-catch blocks to handle unexpected inputs gracefully. - Consistency: Ensure that the method's approach to handling dates and billing cycles is consistent with other parts of the application. Inconsistencies in date handling can lead to subtle bugs that are difficult to track down.
Enhance code clarity with comments explaining complex logic and consider adding error handling for more robustness. Ensure consistency in date handling across the application.
| '', | ||
| consumerProvider | ||
| .onChangeOfBillYear, | ||
| consumerProvider | ||
| .getFinancialYearList(), | ||
| [consumerProvider | ||
| .getFinancialYearList().first], | ||
| true, | ||
| itemAsString: (i) =>'${ApplicationLocalizations.of(context).translate(i.financialYear)}', | ||
| controller: consumerProvider |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [92-94]
There's an issue in the dispose method where it adds the _numberFocus listener instead of removing it. This can lead to memory leaks and unintended behavior. The correct approach is to remove the listener when the widget is disposed of.
- _numberFocus.addListener(_onFocusChange);
+ _numberFocus.removeListener(_onFocusChange);| static List<DatePeriod> getPastMonthUntilFinancialYTDMonthCount(DatePeriod ytd, | ||
| {int monthCount=2,bool showCurrentMonth = false}) { | ||
| var monthList = <DateTime>[]; | ||
| final currentTime = DateTime.now(); | ||
| DatePeriod newDT = DatePeriod(ytd.endDate.subtract(Duration(days: daysToSubtract(monthCount,currentTime.year,currentTime.month))), ytd.endDate, DateType.MONTH); | ||
| if (currentTime.year < newDT.startDate.year) { | ||
| return <DatePeriod>[]; | ||
| } | ||
| if (currentTime.year == newDT.startDate.year) { | ||
| //when current year is same as start year of financial year | ||
| for (int i = newDT.startDate.month; | ||
| i <= (showCurrentMonth ? currentTime.month : currentTime.month - 1); | ||
| i++) { | ||
| monthList.add(DateTime(currentTime.year, i)); | ||
| } | ||
| } else if (currentTime.year == newDT.endDate.year) { | ||
| //when current year is same as end year of financial year | ||
| for (int i = newDT.startDate.month; i <= 12; i++) { | ||
| monthList.add(DateTime(newDT.startDate.year, i)); | ||
| } | ||
| for (int i = 1; | ||
| i <= | ||
| (currentTime.month <= newDT.endDate.month | ||
| ? showCurrentMonth | ||
| ? currentTime.month | ||
| : currentTime.month - 1 | ||
| : newDT.endDate.month); | ||
| /* | ||
| * if current month is less than or equal to end month of financial year | ||
| * we are using months less than current month and if it is more than | ||
| * end month of financial year we are using till end month of financial | ||
| * year | ||
| */ | ||
| i++) { | ||
| monthList.add(DateTime(newDT.endDate.year, i)); | ||
| } | ||
| } else { | ||
| for (int i = newDT.startDate.month; i <= 12; i++) { | ||
| monthList.add(DateTime(newDT.startDate.year, i)); | ||
| } | ||
| for (int i = 1; i <= newDT.endDate.month; i++) { | ||
| monthList.add(DateTime(newDT.endDate.year, i)); | ||
| } | ||
| } | ||
| var list = monthList | ||
| .map((e) => DatePeriod(DateTime(e.year, e.month, 1), | ||
| DateTime(e.year, e.month + 1, 0, 23, 59, 59, 999), DateType.MONTH)) | ||
| .toList() | ||
| .reversed | ||
| .toList(); | ||
| return list; |
There was a problem hiding this comment.
The getPastMonthUntilFinancialYTDMonthCount method correctly calculates the list of DatePeriod objects for months considering a specific month count. Consider adding comments to explain the logic, especially for complex parts, to improve maintainability.
…atch-3 Update build-config.yml
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
build/build-config.ymlis excluded by:!**/*.yml
Files selected for processing (3)
- frontend/mgramseva/lib/providers/consumer_details_provider.dart (3 hunks)
- frontend/mgramseva/lib/screeens/consumer_details/consumer_details.dart (2 hunks)
- frontend/mgramseva/lib/utils/common_methods.dart (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- frontend/mgramseva/lib/providers/consumer_details_provider.dart
- frontend/mgramseva/lib/screeens/consumer_details/consumer_details.dart
- frontend/mgramseva/lib/utils/common_methods.dart
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/DemandService.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- municipal-services/ws-calculator/src/main/java/org/egov/wscalculation/service/DemandService.java
There was a problem hiding this comment.
@debasishchakraborty-egovt Please take the action on the comments
| @@ -199,7 +199,7 @@ public PropertyCriteria getPropertyCriteriaFromPropertyIds(List<Property> proper | |||
| */ | |||
| public void enrichBoundary(Property property, RequestInfo requestInfo){ | |||
|
|
|||
| boundaryService.getAreaType(property, requestInfo, PTConstants.BOUNDARY_HEIRARCHY_CODE); | |||
| // boundaryService.getAreaType(property, requestInfo, PTConstants.BOUNDARY_HEIRARCHY_CODE); | |||
There was a problem hiding this comment.
As in MDMS V2 we are not using location-service to get location
...l-services/ws-calculator/src/main/java/org/egov/wscalculation/service/EstimationService.java
Show resolved
Hide resolved
PFM-5692 changes url shorting endpoint
Summary by CodeRabbit
longitudeandlatitudedata types to be dynamic.