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.

Sign up for GitHub

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

psp-7346 correct bracket usage, ensure logic limited to file close only. #3622

Merged
merged 3 commits into from
Dec 2, 2023
Merged

psp-7346 correct bracket usage, ensure logic limited to file close only. #3622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c86827
psp-7346 correct bracket usage, ensure logic limited to file close only.
Nov 28, 2023
8792546
comment from code review.
Dec 1, 2023
49b1d24
test correction.
Dec 1, 2023
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
11 changes: 6 additions & 5 deletions source/backend/api/Services/AcquisitionFileService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public PimsAcquisitionFile Update(PimsAcquisitionFile acquisitionFile, IEnumerab

_user.ThrowInvalidAccessToAcquisitionFile(_userRepository, _acqFileRepository, acquisitionFile.Internal_Id);
ValidateVersion(acquisitionFile.Internal_Id, acquisitionFile.ConcurrencyControlNumber);
ValidateDrafts(acquisitionFile.Internal_Id);
ValidateDrafts(acquisitionFile);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda of a nit: The current acq file is being retrieved multiple times for each validation function, ideally we would retrieve it once and pass it to the validation logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I only see one request for the acquisition file in any validate functions within Update:
ValidatePayeeDependency
the only other call is at the end of the update function, but that is after the transaction is committed so I think that makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example teh GetCurrentAcquisitionStatus will retrieve the latest acq file for the given id


AcquisitionStatusTypes? currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFile.Internal_Id);
if (!_statusSolver.CanEditDetails(currentAcquisitionStatus) && !_user.HasPermission(Permissions.SystemAdmin))
Expand Down Expand Up @@ -735,11 +735,12 @@ private void ValidateMinistryRegion(long acqFileId, short updatedRegion)
}
}

private void ValidateDrafts(long acqFileId)
private void ValidateDrafts(PimsAcquisitionFile incomingFile)
{
var agreements = _agreementRepository.GetAgreementsByAcquisitionFile(acqFileId);
var compensations = _compensationRequisitionRepository.GetAllByAcquisitionFileId(acqFileId);
if (agreements.Any(a => a?.AgreementStatusTypeCode == "DRAFT" || compensations.Any(c => c.IsDraft.HasValue && c.IsDraft.Value)))
var agreements = _agreementRepository.GetAgreementsByAcquisitionFile(incomingFile.AcquisitionFileId);
var compensations = _compensationRequisitionRepository.GetAllByAcquisitionFileId(incomingFile.AcquisitionFileId);
if (incomingFile.AcquisitionFileStatusTypeCode == nameof(AcquisitionStatusTypes.COMPLT) &&
(agreements.Any(a => a?.AgreementStatusTypeCode == "DRAFT") || compensations.Any(c => c.IsDraft.HasValue && c.IsDraft.Value)))
{
throw new BusinessRuleViolationException("You cannot complete a file when there are one or more draft agreements, or one or more draft compensations requisitions." +
"\n\nRemove any draft compensations requisitions. Agreements should be set to final, cancelled, or removed.");
Expand Down
34 changes: 33 additions & 1 deletion source/backend/tests/unit/api/Services/AcquisitionFileServiceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ public void Update_Drafts_Violation()

var repository = this._helper.GetService<Mock<IAcquisitionFileRepository>>();
repository.Setup(x => x.GetRowVersion(It.IsAny<long>())).Returns(1);
repository.Setup(x => x.GetById(It.IsAny<long>())).Returns(acqFile);

var agreementRepository = this._helper.GetService<Mock<IAgreementRepository>>();
agreementRepository.Setup(x => x.GetAgreementsByAcquisitionFile(It.IsAny<long>())).Returns(new List<PimsAgreement>() { new PimsAgreement() { AgreementStatusTypeCode = "DRAFT" } });
Expand All @@ -470,7 +471,35 @@ public void Update_Drafts_Violation()
Action act = () => service.Update(acqFile, new List<UserOverrideCode>());

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
act.Should().Throw<BusinessRuleViolationException>();
}

[Fact]
public void Update_DraftsNotComplete_NoViolation()
{
// Arrange
var service = this.CreateAcquisitionServiceWithPermissions(Permissions.AcquisitionFileEdit);

var acqFile = EntityHelper.CreateAcquisitionFile();

var repository = this._helper.GetService<Mock<IAcquisitionFileRepository>>();
repository.Setup(x => x.GetRowVersion(It.IsAny<long>())).Returns(1);
repository.Setup(x => x.GetById(It.IsAny<long>())).Returns(acqFile);

var agreementRepository = this._helper.GetService<Mock<IAgreementRepository>>();
agreementRepository.Setup(x => x.GetAgreementsByAcquisitionFile(It.IsAny<long>())).Returns(new List<PimsAgreement>() { new PimsAgreement() { AgreementStatusTypeCode = "DRAFT" } });

var userRepository = this._helper.GetService<Mock<IUserRepository>>();
userRepository.Setup(x => x.GetUserInfoByKeycloakUserId(It.IsAny<Guid>())).Returns(EntityHelper.CreateUser("Test"));

var statusSolver = this._helper.GetService<Mock<IAcquisitionStatusSolver>>();
statusSolver.Setup(x => x.CanEditDetails(It.IsAny<AcquisitionStatusTypes>())).Returns(true);

// Act
Action act = () => service.Update(acqFile, new List<UserOverrideCode>());

// Assert
act.Should().NotThrow<BusinessRuleViolationException>();
}

[Fact]
Expand Down Expand Up @@ -539,6 +568,9 @@ public void Update_PropertyOfInterest_Violation_Owned()
var agreementRepository = this._helper.GetService<Mock<IAgreementRepository>>();
agreementRepository.Setup(x => x.GetAgreementsByAcquisitionFile(It.IsAny<long>())).Returns(new List<PimsAgreement>());

var compensationRequisitionRepository = this._helper.GetService<Mock<ICompensationRequisitionRepository>>();
compensationRequisitionRepository.Setup(x => x.GetAllByAcquisitionFileId(It.IsAny<long>())).Returns(new List<PimsCompensationRequisition>());

var solver = this._helper.GetService<Mock<IAcquisitionStatusSolver>>();
solver.Setup(x => x.CanEditDetails(It.IsAny<AcquisitionStatusTypes?>())).Returns(true);

Expand Down
Loading