Remove KFS integration and update Aggie Enterprise API#658
Open
jSylvestre wants to merge 3 commits into
Open
Conversation
The legacy KFS integration components (Purchasing.WS project, IFinancialSystemService) are no longer needed and have been removed. This includes the client-side KFS to AE account conversion logic, which is now expected to be handled by upstream validation or the updated Aggie Enterprise API directly.
The legacy KFS integration components (Purchasing.WS project, IFinancialSystemService) are no longer needed and have been removed. This includes the client-side KFS to AE account conversion announcement, which is now obsolete as the system fully transitions to Aggie Enterprise.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes the Purchasing.WS project containing SOAP-based KFS (Campus Financial System) integration and updates all dependent code to use Aggie Enterprise API exclusively. The solution, project files, service contracts, and all implementations are deleted; OrderController and OrderService remove their KFS integration dependencies and logic; tests are updated to remove mocks and fixtures; and the AggieEnterpriseService adds stricter validation with exception throwing instead of fallback logic. ChangesKFS Integration Removal and Migration to Aggie Enterprise API
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Purchasing.Core/Services/AggieEnterpriseService.cs`:
- Around line 510-513: The code currently throws a generic Exception when
split.FinancialSegmentString is null and ConvertKfsAccount silently returns
empty for non-empty input, causing misleading failures; update ConvertKfsAccount
to return a clear failure (null or a Result/Error object) or throw a specific
MigrationException when it cannot convert, and change LookupAccount to handle
missing FinancialSegmentString by returning a controlled error result instead of
throwing; then update callers such as UploadOrder and validation paths to catch
that specific failure and translate it into a user-safe SubmitResult/validation
error (do not leave a raw Exception), referencing ConvertKfsAccount,
LookupAccount, UploadOrder, validation callers and SubmitResult to locate places
to change.
In `@Purchasing.Mvc/Services/OrderService.cs`:
- Around line 586-593: The WillOrderBeSentToKfs method currently always returns
false which lets completion silently succeed for deprecated KFS order types;
restore the original guard logic in WillOrderBeSentToKfs to return true when
newOrderType.Id == OrderType.Types.KfsDocument AND
_financialSystemService.AllowedType(kfsDocType), and then update the completion
path (the Complete(...) method) to call WillOrderBeSentToKfs and, when it
indicates a KFS document that cannot/should not be submitted, prevent silent
success by throwing a clear exception (e.g., InvalidOperationException) or
returning a failure result so non-AE/KFS orders are not marked completed without
explicit external submission handling; ensure you update any callers of
Complete(...) to handle the new error/return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c806d73-2b57-47a4-86ab-324893b6ef11
📒 Files selected for processing (53)
Purchasing.Core/Purchasing.Core.csprojPurchasing.Core/Services/AggieEnterpriseService.csPurchasing.Mvc/ComponentInstaller.csPurchasing.Mvc/Controllers/OrderController.csPurchasing.Mvc/Purchasing.Mvc.csprojPurchasing.Mvc/Services/OrderService.csPurchasing.Mvc/Views/Home/Index.cshtmlPurchasing.Tests/ControllerTests/OrderControllerTests/OrderControllerTestsInit.csPurchasing.Tests/ControllerTests/OrderControllerTests/OrderControllerTestsMapping.csPurchasing.Tests/ControllerTests/OrderControllerTests/OrderControllerTestsMethodsPart03.csPurchasing.Tests/ControllerTests/OrderControllerTests/OrderControllerTestsReflection.csPurchasing.Tests/Purchasing.Tests.csprojPurchasing.Tests/ServiceTests/OrderServiceTests/OrderServiceTestsInit.csPurchasing.Tests/ServiceTests/OrderServiceTests/OrderServiceTestsReRouteApprovalsForExistingOrder01.csPurchasing.WS/AccountInfo.csPurchasing.WS/App_Readme/Elmah.txtPurchasing.WS/AzureStorageService.csPurchasing.WS/FinancialDocumentStatus.csPurchasing.WS/FinancialRoleSystemService.csPurchasing.WS/FinancialSystemService.csPurchasing.WS/IFinancialRoleSystemService.csPurchasing.WS/IFinancialSystemService.csPurchasing.WS/Purchasing.WS.csprojPurchasing.WS/Service References/AzureStorage/Microsoft.SqlServer.Management.Dac.ServiceTypes.xsdPurchasing.WS/Service References/AzureStorage/Microsoft.SqlServer.Management.Dac.Services.wsdlPurchasing.WS/Service References/AzureStorage/Purchasing.WS.AzureStorage.ExportResponse.datasourcePurchasing.WS/Service References/AzureStorage/Reference.csPurchasing.WS/Service References/AzureStorage/Reference.svcmapPurchasing.WS/Service References/AzureStorage/configuration.svcinfoPurchasing.WS/Service References/AzureStorage/configuration91.svcinfoPurchasing.WS/Service References/AzureStorage/dotnet-svcutil.params.jsonPurchasing.WS/Service References/AzureStorage/service.wsdlPurchasing.WS/Service References/AzureStorage/service.xsdPurchasing.WS/Service References/AzureStorage/service1.xsdPurchasing.WS/Service References/FinancialRoleService/Purchasing.WS.FinancialRoleService.simpleAccountInfo.datasourcePurchasing.WS/Service References/FinancialRoleService/Reference.csPurchasing.WS/Service References/FinancialRoleService/Reference.svcmapPurchasing.WS/Service References/FinancialRoleService/configuration.svcinfoPurchasing.WS/Service References/FinancialRoleService/configuration91.svcinfoPurchasing.WS/Service References/FinancialRoleService/dotnet-svcutil.params.jsonPurchasing.WS/Service References/FinancialRoleService/financialSystemRoleServiceSOAP2.wsdlPurchasing.WS/Service References/PurchaseDocumentService/Purchasing.WS.PurchaseDocumentService.documentCreationResult.datasourcePurchasing.WS/Service References/PurchaseDocumentService/Purchasing.WS.PurchaseDocumentService.purchaseRequisitionStatusInfo.datasourcePurchasing.WS/Service References/PurchaseDocumentService/Reference.csPurchasing.WS/Service References/PurchaseDocumentService/Reference.svcmapPurchasing.WS/Service References/PurchaseDocumentService/configuration.svcinfoPurchasing.WS/Service References/PurchaseDocumentService/configuration91.svcinfoPurchasing.WS/Service References/PurchaseDocumentService/dotnet-svcutil.params.jsonPurchasing.WS/Service References/PurchaseDocumentService/purchaseDocumentsInterfaceServiceSOAP.wsdlPurchasing.WS/Service References/PurchaseDocumentService/purchasingDocumentsInterfaceServiceSOAP1.wsdlPurchasing.WS/SubmitResult.csPurchasing.WS/appsettings.jsonPurchasing.sln
💤 Files with no reviewable changes (39)
- Purchasing.WS/Service References/AzureStorage/configuration91.svcinfo
- Purchasing.WS/Service References/FinancialRoleService/Purchasing.WS.FinancialRoleService.simpleAccountInfo.datasource
- Purchasing.WS/Service References/FinancialRoleService/dotnet-svcutil.params.json
- Purchasing.WS/Service References/AzureStorage/service.xsd
- Purchasing.WS/Purchasing.WS.csproj
- Purchasing.WS/Service References/PurchaseDocumentService/configuration.svcinfo
- Purchasing.WS/Service References/AzureStorage/configuration.svcinfo
- Purchasing.WS/Service References/AzureStorage/service1.xsd
- Purchasing.Mvc/Purchasing.Mvc.csproj
- Purchasing.WS/Service References/PurchaseDocumentService/purchaseDocumentsInterfaceServiceSOAP.wsdl
- Purchasing.WS/Service References/PurchaseDocumentService/Reference.svcmap
- Purchasing.WS/Service References/PurchaseDocumentService/Purchasing.WS.PurchaseDocumentService.documentCreationResult.datasource
- Purchasing.WS/App_Readme/Elmah.txt
- Purchasing.WS/Service References/AzureStorage/Microsoft.SqlServer.Management.Dac.ServiceTypes.xsd
- Purchasing.WS/appsettings.json
- Purchasing.WS/Service References/AzureStorage/Microsoft.SqlServer.Management.Dac.Services.wsdl
- Purchasing.WS/Service References/FinancialRoleService/configuration.svcinfo
- Purchasing.WS/IFinancialRoleSystemService.cs
- Purchasing.WS/Service References/FinancialRoleService/Reference.svcmap
- Purchasing.WS/Service References/AzureStorage/Reference.svcmap
- Purchasing.WS/FinancialDocumentStatus.cs
- Purchasing.WS/SubmitResult.cs
- Purchasing.WS/Service References/AzureStorage/service.wsdl
- Purchasing.WS/Service References/PurchaseDocumentService/configuration91.svcinfo
- Purchasing.WS/Service References/PurchaseDocumentService/dotnet-svcutil.params.json
- Purchasing.WS/Service References/PurchaseDocumentService/purchasingDocumentsInterfaceServiceSOAP1.wsdl
- Purchasing.WS/FinancialSystemService.cs
- Purchasing.WS/Service References/PurchaseDocumentService/Purchasing.WS.PurchaseDocumentService.purchaseRequisitionStatusInfo.datasource
- Purchasing.WS/IFinancialSystemService.cs
- Purchasing.WS/Service References/AzureStorage/Reference.cs
- Purchasing.WS/FinancialRoleSystemService.cs
- Purchasing.WS/Service References/FinancialRoleService/configuration91.svcinfo
- Purchasing.WS/Service References/FinancialRoleService/financialSystemRoleServiceSOAP2.wsdl
- Purchasing.WS/Service References/FinancialRoleService/Reference.cs
- Purchasing.WS/AccountInfo.cs
- Purchasing.WS/AzureStorageService.cs
- Purchasing.WS/Service References/PurchaseDocumentService/Reference.cs
- Purchasing.Mvc/Views/Home/Index.cshtml
- Purchasing.WS/Service References/AzureStorage/dotnet-svcutil.params.json
Comment on lines
510
to
+513
| if (string.IsNullOrWhiteSpace(split.FinancialSegmentString)) | ||
| { | ||
| var chart = split.Account.Split('-')[0]; | ||
| var account = split.Account.Split('-')[1]; | ||
|
|
||
|
|
||
| var distributionResult = await _aggieClient.KfsConvertAccount.ExecuteAsync(chart, account, split.SubAccount); | ||
| var distributionData = distributionResult.ReadData(); | ||
| if (distributionData.KfsConvertAccount.GlSegments != null) | ||
| { | ||
| var tempGlSegments = new GlSegments(distributionData.KfsConvertAccount.GlSegments); | ||
| if (string.IsNullOrWhiteSpace(tempGlSegments.Account) || tempGlSegments.Account == "000000") | ||
| { | ||
| //770000 | ||
| Log.Warning($"Natural Account of 000000 detected. Substituting {_options.DefaultNaturalAccount}"); | ||
| tempGlSegments.Account = _options.DefaultNaturalAccount; | ||
| } | ||
| rtValue.FinincialSegmentString = tempGlSegments.ToSegmentString(); | ||
| } | ||
| else | ||
| { | ||
| if (distributionData.KfsConvertAccount.PpmSegments != null) | ||
| { | ||
| rtValue.IsPPm = true; | ||
| var tempPpmSegments = new PpmSegments(distributionData.KfsConvertAccount.PpmSegments); | ||
| if (string.IsNullOrWhiteSpace(tempPpmSegments.ExpenditureType) || tempPpmSegments.ExpenditureType == "000000") | ||
| { | ||
| //770000 | ||
| Log.Warning($"Natural Account (ExpenditureType) of 000000 detected. Substituting {_options.DefaultNaturalAccount}"); | ||
| tempPpmSegments.ExpenditureType = _options.DefaultNaturalAccount; | ||
| } | ||
| rtValue.FinincialSegmentString = tempPpmSegments.ToSegmentString(); | ||
| } | ||
| else | ||
| { | ||
| //TODO: REMOVE THIS!!!! | ||
| Log.Error("No GL Segments found for {chart}-{account}-{subAccount} FAKING IT!!!!", chart, account, split.SubAccount); | ||
| rtValue.FinincialSegmentString = $"3110-13U02-ADNO006-{_options.DefaultNaturalAccount}-43-000-0000000000-000000-0000-000000-000000"; | ||
| } | ||
| } | ||
| throw new Exception("FinancialSegmentString is null or whitespace. This should have been caught in validation. If you are seeing this error, please contact support."); | ||
|
|
There was a problem hiding this comment.
Avoid silent conversion failure and generic hard-throw in account migration paths.
ConvertKfsAccount(...) now always returns empty for non-empty input, while LookupAccount(...) throws on missing FinancialSegmentString. This combination can cause misleading validation failures and hard runtime breaks instead of a clear migration error path.
Suggested direction for explicit deprecation behavior
public async Task<string> ConvertKfsAccount(string account, bool updateNaturalAccount = true)
{
if(string.IsNullOrWhiteSpace(account))
{
return String.Empty;
}
- return String.Empty;
+ throw new NotSupportedException(
+ "KFS-to-Aggie Enterprise conversion is no longer supported. Use a valid FinancialSegmentString.");
}// Also ensure UploadOrder/validation callers catch this and return a user-safe SubmitResult
// instead of surfacing an unhandled exception.Also applies to: 904-912
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Purchasing.Core/Services/AggieEnterpriseService.cs` around lines 510 - 513,
The code currently throws a generic Exception when split.FinancialSegmentString
is null and ConvertKfsAccount silently returns empty for non-empty input,
causing misleading failures; update ConvertKfsAccount to return a clear failure
(null or a Result/Error object) or throw a specific MigrationException when it
cannot convert, and change LookupAccount to handle missing
FinancialSegmentString by returning a controlled error result instead of
throwing; then update callers such as UploadOrder and validation paths to catch
that specific failure and translate it into a user-safe SubmitResult/validation
error (do not leave a raw Exception), referencing ConvertKfsAccount,
LookupAccount, UploadOrder, validation callers and SubmitResult to locate places
to change.
Comment on lines
586
to
593
| public bool WillOrderBeSentToKfs(OrderType newOrderType, string kfsDocType = null) | ||
| { | ||
| if (newOrderType.Id == OrderType.Types.KfsDocument && _financialSystemService.AllowedType(kfsDocType)) | ||
| { | ||
| return true; | ||
| } | ||
| //if (newOrderType.Id == OrderType.Types.KfsDocument && _financialSystemService.AllowedType(kfsDocType)) | ||
| //{ | ||
| // return true; | ||
| //} | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Block completion for deprecated KFS order types instead of silently succeeding.
With KFS submission removed, Complete(...) can still mark non-AE/KFS orders as completed without any external submission or explicit failure. That creates false-success completion states.
Proposed guard in completion path
public async Task<string[]> Complete(Order order, OrderType newOrderType, string kfsDocType = null)
{
+ if (newOrderType?.Id == OrderType.Types.KfsDocument)
+ {
+ return new[] { "KFS integration has been removed. Please complete this order as Aggie Enterprise." };
+ }
+
order.StatusCode = _repositoryFactory.OrderStatusCodeRepository.GetById(OrderStatusCode.Codes.Complete);
order.OrderType = newOrderType;
order.KfsDocType = kfsDocType;Also applies to: 606-619, 621-635
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Purchasing.Mvc/Services/OrderService.cs` around lines 586 - 593, The
WillOrderBeSentToKfs method currently always returns false which lets completion
silently succeed for deprecated KFS order types; restore the original guard
logic in WillOrderBeSentToKfs to return true when newOrderType.Id ==
OrderType.Types.KfsDocument AND _financialSystemService.AllowedType(kfsDocType),
and then update the completion path (the Complete(...) method) to call
WillOrderBeSentToKfs and, when it indicates a KFS document that cannot/should
not be submitted, prevent silent success by throwing a clear exception (e.g.,
InvalidOperationException) or returning a failure result so non-AE/KFS orders
are not marked completed without explicit external submission handling; ensure
you update any callers of Complete(...) to handle the new error/return behavior.
Continues the deprecation of KFS-specific UI components as part of the transition to Aggie Enterprise. The KFS review section is no longer needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The legacy KFS integration components (Purchasing.WS project, IFinancialSystemService) are no longer needed and have been removed. This includes the client-side KFS to AE account conversion logic, which is now expected to be handled by upstream validation or the updated Aggie Enterprise API directly.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation