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

Con 3755 import payments pefrormance #2213

Merged
merged 39 commits into from
Nov 1, 2021
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
3c4dba5
[CON-3020] Add a correlationId to enable following of message instanc…
cofaulco Jan 21, 2021
53398d4
further logging
cofaulco Jan 21, 2021
ce2f1d2
fix unit tests
cofaulco Jan 21, 2021
6d48cf6
parallelise a couple of calls done per payment and log out iterations.
cofaulco Jan 21, 2021
7644bd1
more logging
cofaulco Jan 22, 2021
4ace1b9
Add webjobs app insight package
cofaulco Jan 22, 2021
f0630d9
Add webjobs app insights package
cofaulco Jan 22, 2021
1573622
enable app insights in employer finance message handlers webjob
cofaulco Jan 22, 2021
24554a6
Configure app insights for employer finance jobs webjob
cofaulco Jan 22, 2021
931f2f4
Add application insights config files
cofaulco Jan 22, 2021
2f7055d
Setup App Insights for finance worker in ARM
nbowes24 Jan 22, 2021
b6def5e
AppInsights App Setting Rename
nbowes24 Jan 22, 2021
68e3cda
add missing nuget package for application insight
cofaulco Jan 22, 2021
525002b
Merge branch 'CON-3020_Extra-logging-info' of https://github.com/Skil…
cofaulco Jan 22, 2021
55644d4
try parallel calling commitments
cofaulco Jan 22, 2021
33c5e20
fix broken unit test
cofaulco Jan 22, 2021
145a64f
additional logging on null transfer payment error
cofaulco Jan 22, 2021
777c6b3
check for null payment details to allow handler to continue
cofaulco Jan 25, 2021
3c89dca
miss one null payment detail handling
cofaulco Jan 25, 2021
f4a52b9
add extra logging around payments
cofaulco Jan 25, 2021
d820826
Merged latest changes from master
ChrisJWoodcock Apr 16, 2021
0b1d103
CON-3124
hkf-tech Jun 25, 2021
887c04d
Merge branch 'CON-3020_Extra-logging-info' into CON-3124-Merge
hkf-tech Jun 25, 2021
122ec8e
bug fix
hkf-tech Jul 6, 2021
5c8b341
code improement
hkf-tech Jul 6, 2021
8498242
temp test fix
hkf-tech Jul 6, 2021
3132131
fixed issue with insert.
hkf-tech Jul 7, 2021
c64db0a
making further adjustments for performance improvement.
hkf-tech Jul 8, 2021
c988e35
adding comments
hkf-tech Jul 9, 2021
2bc28f4
Merge branch 'master' into CON-3124-Merge
hkf-tech Oct 5, 2021
7c2bdfd
Merge branch 'master' into CON-3755-ImportPaymentsPefrormance
hkf-tech Oct 6, 2021
44a430d
changes for the import payments
hkf-tech Oct 8, 2021
2c2d8b6
removing commented code, enabling tests
hkf-tech Oct 11, 2021
24d7e1d
code refactoring
hkf-tech Oct 11, 2021
660a2ff
CON-3755 changes for parallel async loop
hkf-tech Oct 13, 2021
5589921
Merge branch 'master' into CON-3755-ImportPaymentsPefrormance
hkf-tech Oct 14, 2021
71b9370
Merge branch 'master' of https://github.com/SkillsFundingAgency/das-e…
reachash Oct 20, 2021
8792966
bug fix
hkf-tech Oct 21, 2021
4e51f5c
Merge branch 'master' into CON-3755-ImportPaymentsPefrormance
hkf-tech Oct 21, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azure/finance.template.json
Original file line number Diff line number Diff line change
Expand Up @@ -586,4 +586,4 @@
"value": "[variables('workerAppServiceName')]"
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@
<Build Include="Tables\TransactionLine_EOF.sql" />
<Build Include="StoredProcedures\CreateDraftExpiredFunds.sql" />
<Build Include="StoredProcedures\GetDraftExpiredFunds.sql" />
<Build Include="StoredProcedures\CreateAccountTransfersV1.sql" />
</ItemGroup>
<ItemGroup>
<None Include="Database.publish.xml" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
CREATE PROCEDURE [employer_financial].[CreateAccountTransfersV1]
@transfers [employer_financial].[AccountTransferTable] READONLY
AS
INSERT INTO [employer_financial].[AccountTransfers]
(
SenderAccountId,
SenderAccountName,
ReceiverAccountId,
ReceiverAccountName,
ApprenticeshipId,
CourseName,
CourseLevel,
PeriodEnd,
Amount,
Type,
CreatedDate,
RequiredPaymentId
)
Select
tr.SenderAccountId,
actSender.Name,
tr.ReceiverAccountId,
actReceiver.Name,
tr.ApprenticeshipId,
IsNull(Ir.CourseName, 'Unknown Course'),
Ir.CourseLevel,
tr.PeriodEnd,
tr.Amount,
tr.Type, GetDate(),
tr.RequiredPaymentId
FROM
(SELECT
p.AccountId,
p.PeriodEnd,
p.ApprenticeshipId,
meta.ApprenticeshipCourseName as CourseName
,meta.ApprenticeshipCourseLevel as CourseLevel
,COUNT(DISTINCT Uln) as ApprenticeCount
,SUM(p.Amount) as PaymentTotal
FROM [employer_financial].[Payment] p
INNER JOIN [employer_financial].[PaymentMetaData] meta ON p.PaymentMetaDataId = meta.Id
Inner join @transfers tr on p.AccountId = tr.ReceiverAccountId and p.PeriodEnd = tr.PeriodEnd and p.ApprenticeshipId = tr.ApprenticeshipId
GROUP BY meta.ApprenticeshipCourseName, meta.ApprenticeshipCourseLevel, p.AccountId, p.PeriodEnd, p.ApprenticeshipId) Ir
Inner join @transfers tr on Ir.AccountId = tr.ReceiverAccountId and Ir.PeriodEnd = tr.PeriodEnd and Ir.ApprenticeshipId = tr.ApprenticeshipId
Inner join [employer_financial].[Account] actSender on actSender.Id = tr.SenderAccountId
Inner join [employer_financial].[Account] actReceiver on actReceiver.Id = tr.ReceiverAccountId
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,10 @@ CREATE NONCLUSTERED INDEX [IX_Payment_AccountIdUkprnPeriodEndUln] ON [employer_f
GO

CREATE NONCLUSTERED INDEX [IX_Payment_AccountIdCollectionPeriodMonthCollectionPeriodYear] ON [employer_financial].[Payment] ([AccountId], [CollectionPeriodMonth], [CollectionPeriodYear]) INCLUDE ([Amount], [ApprenticeshipId], [CollectionPeriodId], [DeliveryPeriodMonth], [DeliveryPeriodYear], [FundingSource], [PaymentMetaDataId], [PeriodEnd], [Ukprn], [Uln]) WITH (ONLINE = ON)
GO

CREATE NONCLUSTERED INDEX [IX_Payment_Ukprn] ON [employer_financial].[Payment] ([Ukprn]) WITH (ONLINE = ON)
GO

CREATE NONCLUSTERED INDEX [IX_Payment_UkprnAccountId] ON [employer_financial].[Payment] ([AccountId], [Ukprn]) WITH (ONLINE = ON)
GO
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,6 @@ GO

CREATE UNIQUE NONCLUSTERED INDEX [UK_TransactionLine_AccountId_TransactionType_TransactionDate] ON [employer_financial].[TransactionLine] ([AccountId] ASC, [TransactionType] ASC, [TransactionDate] ASC) WHERE [TransactionType] = /*ExpiredFund*/ 5
GO

CREATE INDEX [IX_TransactionLine_Account_DateCreated] ON [employer_financial].[TransactionLine] (AccountId, DateCreated) WITH (ONLINE = ON)
GO
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Threading.Tasks;
using System;
using System.Threading.Tasks;
using MediatR;
using NServiceBus;
using SFA.DAS.EmployerFinance.Commands.CreateTransferTransactions;
Expand All @@ -22,28 +23,32 @@ public ImportAccountPaymentsCommandHandler(IMediator mediator, ILog logger)

public async Task Handle(ImportAccountPaymentsCommand message, IMessageHandlerContext context)
{
_logger.Info($"Processing refresh payment command for Account ID: {message.AccountId} PeriodEnd: {message.PeriodEndRef}");
var correlationId = Guid.NewGuid();
_logger.Info($"Processing refresh payment command for Account ID: {message.AccountId} PeriodEnd: {message.PeriodEndRef} CorrelationId: {correlationId}");

await _mediator.SendAsync(new RefreshPaymentDataCommand
{
AccountId = message.AccountId,
PeriodEnd = message.PeriodEndRef
});
PeriodEnd = message.PeriodEndRef,
CorrelationId = correlationId
});

_logger.Info($"Processing refresh account transfers command for AccountId:{message.AccountId} PeriodEnd:{message.PeriodEndRef}");
_logger.Info($"Processing refresh account transfers command for AccountId:{message.AccountId} PeriodEnd:{message.PeriodEndRef}, CorrelationId: {correlationId}");

await _mediator.SendAsync(new RefreshAccountTransfersCommand
{
ReceiverAccountId = message.AccountId,
PeriodEnd = message.PeriodEndRef
PeriodEnd = message.PeriodEndRef,
CorrelationId = correlationId
});

_logger.Info($"Processing create account transfer transactions command for AccountId:{message.AccountId} PeriodEnd:{message.PeriodEndRef}");

await _mediator.SendAsync(new CreateTransferTransactionsCommand
{
ReceiverAccountId = message.AccountId,
PeriodEnd = message.PeriodEndRef
PeriodEnd = message.PeriodEndRef,
CorrelationId = correlationId
});
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/SFA.DAS.EmployerFinance.MessageHandlers/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
using SFA.DAS.AutoConfiguration;
using SFA.DAS.EmployerFinance.MessageHandlers.DependencyResolution;
using SFA.DAS.EmployerFinance.Startup;
using System.Configuration;
using Microsoft.ApplicationInsights.Extensibility;
using System.Configuration;

namespace SFA.DAS.EmployerFinance.MessageHandlers
{
Expand All @@ -14,6 +14,7 @@ public class Program
public static void Main()
{
TelemetryConfiguration.Active.InstrumentationKey = ConfigurationManager.AppSettings["APPINSIGHTS_INSTRUMENTATIONKEY"];

MainAsync().GetAwaiter().GetResult();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ public void Arrange()
ReceiverAccountId = ReceiverAccountId,
ReceiverAccountName = ReceiverAccountName,
ApprenticeshipId = 1245,
Amount = 1200
Amount = 1200,
PeriodEnd = "1718-R01"
};

_transfers = new List<AccountTransfer>
Expand All @@ -85,7 +86,7 @@ public void Arrange()
_validator.Setup(x => x.Validate(It.IsAny<RefreshAccountTransfersCommand>()))
.Returns(new ValidationResult());

_paymentService.Setup(x => x.GetAccountTransfers(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountTransfers(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(_transfers);

_transferRepository.Setup(x => x.GetTransferPaymentDetails(It.IsAny<AccountTransfer>()))
Expand All @@ -108,7 +109,7 @@ public async Task ThenTheTransfersShouldBeRetrieved()
await _handler.Handle(_command);

//Assert
_paymentService.Verify(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId), Times.Once);
_paymentService.Verify(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId, It.IsAny<Guid>()), Times.Once);
}

[Test]
Expand All @@ -121,9 +122,7 @@ public async Task ThenTheRetrievedTransfersShouldBeSaved()
_transferRepository.Verify(x => x.CreateAccountTransfers(It.Is<IEnumerable<AccountTransfer>>(t =>
t.All(at => at.ApprenticeshipId.Equals(_accountTransfer.ApprenticeshipId) &&
at.SenderAccountId.Equals(_accountTransfer.SenderAccountId) &&
at.SenderAccountName.Equals(_accountTransfer.SenderAccountName) &&
at.ReceiverAccountId.Equals(_accountTransfer.ReceiverAccountId) &&
at.ReceiverAccountName.Equals(_accountTransfer.ReceiverAccountName) &&
at.Amount.Equals(_accountTransfer.Amount)))), Times.Once);
}

Expand Down Expand Up @@ -180,7 +179,7 @@ public async Task ThenThePaymentShouldNotBeRetrievedIfTheCommandIsInvalid()
}

//Assert
_paymentService.Verify(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId), Times.Never);
_paymentService.Verify(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId, Guid.NewGuid()), Times.Never);
}

[Test]
Expand Down Expand Up @@ -214,7 +213,7 @@ public async Task ThenTheTransfersShouldNotBeSavedIfTheCommandIsInvalid()
public void ThenIfWeCannotGetTransfersWeShouldNotTryToProcessThem()
{
//Assert
_paymentService.Setup(x => x.GetAccountTransfers(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountTransfers(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.Throws<WebException>();

//Act + Assert
Expand All @@ -223,91 +222,16 @@ public void ThenIfWeCannotGetTransfersWeShouldNotTryToProcessThem()
_transferRepository.Verify(x => x.CreateAccountTransfers(_transfers), Times.Never);
}

[Test]
public void ThenIfWeCannotGetTransfersWeShouldLogAnError()
{
//Assert
var exception = new Exception();
_paymentService.Setup(x => x.GetAccountTransfers(It.IsAny<string>(), It.IsAny<long>()))
.Throws(exception);

//Act + Assert
Assert.ThrowsAsync<Exception>(() => _handler.Handle(_command));

_logger.Verify(x => x.Error(exception, It.IsAny<string>()), Times.Once);
}

[Test]

public async Task ThenPaymentDetailsShouldBeCalledForEachTransfer()
{
//Act
await _handler.Handle(_command);

//Assert
foreach (var transfer in _transfers)
{
_transferRepository.Verify(x => x.GetTransferPaymentDetails(It.Is<AccountTransfer>(t =>
t.ApprenticeshipId.Equals(_accountTransfer.ApprenticeshipId) &&
t.SenderAccountId.Equals(_accountTransfer.SenderAccountId) &&
t.SenderAccountName.Equals(_accountTransfer.SenderAccountName) &&
t.ReceiverAccountId.Equals(_accountTransfer.ReceiverAccountId) &&
t.ReceiverAccountName.Equals(_accountTransfer.ReceiverAccountName) &&
t.Amount.Equals(_accountTransfer.Amount))), Times.Once);
}
}

[Test]

public async Task ThenATransferShouldBeCreatedWithTheCorrectPaymentDetails()
{
//Act
await _handler.Handle(_command);

//Assert
_transferRepository.Verify(x => x.CreateAccountTransfers(It.Is<IEnumerable<AccountTransfer>>(
transfers =>
transfers.All(t => t.CourseName.Equals(_details.CourseName) &&
t.CourseLevel.Equals(_details.CourseLevel) &&
t.ApprenticeCount.Equals(_details.ApprenticeCount)))), Times.Once);
}

[Test]

public async Task ThenATransferShouldBeCreatedWithTheCorrectReceiverAccountName()
{
//Act
await _handler.Handle(_command);

//Assert
_transferRepository.Verify(x => x.CreateAccountTransfers(It.Is<IEnumerable<AccountTransfer>>(
transfers =>
transfers.All(t => t.ReceiverAccountName.Equals(ReceiverAccountName)))), Times.Once);
}

[Test]

public async Task ThenIfTransferAmountAndPaymentTotalsDoNotMatchAWarningIsLogged()
{
//Assign
_details.PaymentTotal = 10; //Should be 1200

//Act
await _handler.Handle(_command);

//Assert
_logger.Verify(x => x.Warn("Transfer total does not match transfer payments total"));
}

[Test]
public async Task ThenATransferPamentsForTheSameApprenticeAndCourseShouldBeAggregated()
{
//Assert
//Duplicate the transfer to simulate two transfers from different delivery periods
//(We will not be catching duplicate transfers that exactly match as there is no ID or value in the transfer that remains unique to help us)
_transfers.Add(_accountTransfer);
//TODO: will need to updated if approach agreed.
//Assert
//Duplicate the transfer to simulate two transfers from different delivery periods
//(We will not be catching duplicate transfers that exactly match as there is no ID or value in the transfer that remains unique to help us)
_transfers.Add(_accountTransfer);

_paymentService.Setup(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId))
_paymentService.Setup(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId, Guid.NewGuid()))
.ReturnsAsync(_transfers);

//Act
Expand Down Expand Up @@ -335,7 +259,7 @@ public async Task ThenATransferPamentsForTheSameApprenticeButDifferentCourseShou
});


_paymentService.Setup(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId))
_paymentService.Setup(x => x.GetAccountTransfers(_command.PeriodEnd, _command.ReceiverAccountId, Guid.NewGuid()))
.ReturnsAsync(_transfers);


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ public RefreshPaymentDataCommandHandlerTestsFixture SetExistingPayments(List<Gui

public RefreshPaymentDataCommandHandlerTestsFixture SetIncomingPayments(List<PaymentDetails> paymentDetails)
{
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(paymentDetails);

return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ public void Arrange()
_command = new RefreshPaymentDataCommand
{
AccountId = AccountId,
PeriodEnd = PeriodEnd
PeriodEnd = PeriodEnd,
CorrelationId = Guid.NewGuid()
};

_existingPaymentIds = new List<Guid>
Expand All @@ -74,7 +75,7 @@ public void Arrange()
}};

_paymentService = new Mock<IPaymentService>();
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(_paymentDetails);

_mediator = new Mock<IMediator>();
Expand Down Expand Up @@ -118,14 +119,14 @@ public async Task ThenThePaymentsApiIsCalledToGetPaymentData()
await _handler.Handle(_command);

//Assert
_paymentService.Verify(x => x.GetAccountPayments(_command.PeriodEnd, _command.AccountId));
_paymentService.Verify(x => x.GetAccountPayments(_command.PeriodEnd, _command.AccountId, It.IsAny<Guid>()));
}

[Test]
public async Task ThenTheRepositoryIsNotCalledIfTheCommandIsValidAndThereAreNotPayments()
{
//Arrange
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(new List<PaymentDetails>());
//Act
await _handler.Handle(_command);
Expand Down Expand Up @@ -164,7 +165,7 @@ public async Task ThenTheRepositoryIsCalledToSeeIfTheDataHasAlreadyBeenSavedAndI
new PaymentDetails { Id = _existingPaymentIds[1]}
};

_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(_paymentDetails);

//Act
Expand All @@ -180,7 +181,7 @@ public async Task ThenTheRepositoryIsCalledToSeeIfTheDataHasAlreadyBeenSavedAndI
public async Task ThenWhenAnExceptionIsThrownFromTheApiClientNothingIsProcessedAndAnErrorIsLogged()
{
//Assert
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ThrowsAsync(new WebException());

//Act
Expand All @@ -191,7 +192,7 @@ public async Task ThenWhenAnExceptionIsThrownFromTheApiClientNothingIsProcessedA
_mediator.Verify(x => x.PublishAsync(It.IsAny<ProcessPaymentEvent>()), Times.Never);

_logger.Verify(x => x.Error(It.IsAny<WebException>(),
$"Unable to get payment information for AccountId = '{AccountId}' and PeriodEnd = '{_command.PeriodEnd}'"));
$"Unable to get payment information for AccountId = '{AccountId}' and PeriodEnd = '{_command.PeriodEnd}' CorrelationId: {_command.CorrelationId}"));
}

[Test]
Expand All @@ -216,7 +217,7 @@ public async Task ShouldOnlyAddPaymentsWhichAreNotAlreadyAdded()
new PaymentDetails { Id = newPaymentGuid}
};

_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(_paymentDetails);

//Act
Expand All @@ -242,7 +243,7 @@ public async Task ShouldOnlyAddNonFullyFundedPayments()
new PaymentDetails { Id = fullyFundedPaymentGuid, FundingSource = FundingSource.FullyFundedSfa}
};

_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>()))
_paymentService.Setup(x => x.GetAccountPayments(It.IsAny<string>(), It.IsAny<long>(), It.IsAny<Guid>()))
.ReturnsAsync(_paymentDetails);

//Act
Expand Down
Loading