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

Jump to bottom

PSP-7929 : Backend error - For all files do not include a retired property on selection #3884

Merged
merged 5 commits into from
Mar 21, 2024
Merged

PSP-7929 : Backend error - For all files do not include a retired property on selection #3884

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cf82fa
- updates
Mar 19, 2024
47aa0f5
Merge branch 'dev' into psp-7929
Mar 19, 2024
18d451e
- test update
Mar 19, 2024
932db62
Merge branch 'dev' into psp-7929
eddherrera Mar 20, 2024
2b817f9
- updates
Mar 20, 2024
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
7 changes: 6 additions & 1 deletion source/backend/api/Services/AcquisitionFileService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,19 @@ public PimsAcquisitionFile Add(PimsAcquisitionFile acquisitionFile, IEnumerable<
ValidateStaff(acquisitionFile);
ValidateOrganizationStaff(acquisitionFile);

acquisitionFile.AcquisitionFileStatusTypeCode = "ACTIVE";
if (acquisitionFile.PimsPropertyAcquisitionFiles.Any(x => x.Property.IsRetired.HasValue && x.Property.IsRetired.Value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, yes. This appears to only affect add but I imagine it should affect update as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you were expecting the repository to handle file update adding new properties? in that case the lease property probably needs something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The adding does not call the AcquisitionFilePropertyRepository methods, so the check had to be added in the Service.
For the update I am taking into consideration the developer note that states: "Although if a property is already on a file, it can be ignored during this process." So it's only when "UpdatingProperties" and the flow is to "Add".

{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}
MatchProperties(acquisitionFile, userOverrides);
ValidatePropertyRegions(acquisitionFile);

PopulateAcquisitionChecklist(acquisitionFile);

acquisitionFile.AcquisitionFileStatusTypeCode = AcquisitionStatusTypes.ACTIVE.ToString();
var newAcqFile = _acqFileRepository.Add(acquisitionFile);
_acqFileRepository.CommitTransaction();

return newAcqFile;
}

Expand Down
5 changes: 4 additions & 1 deletion source/backend/api/Services/DispositionFileService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,11 @@ public PimsDispositionFile Add(PimsDispositionFile dispositionFile, IEnumerable<

ValidateStaff(dispositionFile);

if (dispositionFile.PimsDispositionFileProperties.Any(x => x.Property.IsRetired.HasValue && x.Property.IsRetired.Value))
{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}
MatchProperties(dispositionFile, userOverrides);

ValidatePropertyRegions(dispositionFile);

var newDispositionFile = _dispositionFileRepository.Add(dispositionFile);
Expand Down
8 changes: 7 additions & 1 deletion source/backend/api/Services/LeaseService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Security.Claims;
using Microsoft.Extensions.Logging;
using Pims.Api.Models.CodeTypes;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;
using Pims.Dal.Entities.Models;
Expand Down Expand Up @@ -184,6 +185,11 @@ public PimsLease Add(PimsLease lease, IEnumerable<UserOverrideCode> userOverride
var pimsUser = _userRepository.GetByKeycloakUserId(_user.GetUserKey());
pimsUser.ThrowInvalidAccessToLeaseFile(lease.RegionCode);

if (lease.PimsPropertyLeases.Any(x => x.Property.IsRetired.HasValue && x.Property.IsRetired.Value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like your pattern here is to add this logic to both the service and also to the repository, but not in the case of leases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}

var leasesWithProperties = AssociatePropertyLeases(lease, userOverrides);
return _leaseRepository.Add(leasesWithProperties);
}
Expand Down Expand Up @@ -237,7 +243,7 @@ public PimsLease Update(PimsLease lease, IEnumerable<UserOverrideCode> userOverr
List<PimsPropertyLease> differenceSet = currentProperties.Where(x => !lease.PimsPropertyLeases.Any(y => y.Internal_Id == x.Internal_Id)).ToList();
foreach (var deletedProperty in differenceSet)
{
if (deletedProperty.Property.IsPropertyOfInterest == true)
if (deletedProperty.Property.IsPropertyOfInterest)
{
PimsProperty propertyWithAssociations = _propertyRepository.GetAllAssociationsById(deletedProperty.PropertyId);
var leaseAssociationCount = propertyWithAssociations.PimsPropertyLeases.Count;
Expand Down
6 changes: 6 additions & 0 deletions source/backend/dal/Repositories/AcquisitionFilePropertyRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Security.Claims;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;

Expand Down Expand Up @@ -62,6 +63,11 @@ public PimsPropertyAcquisitionFile Add(PimsPropertyAcquisitionFile propertyAcqui
{
propertyAcquisitionFile.ThrowIfNull(nameof(propertyAcquisitionFile));

if (propertyAcquisitionFile.Property.IsRetired.HasValue && propertyAcquisitionFile.Property.IsRetired.Value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do sort of like having this logic at the repository level, because that means regardless of what service calls this, the user will be unable to add an acq file with a retired property. Is there a good argument for also having this at the service level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not particularly besides short-circuiting the method and the fact that the repository Add is only called in this Service so it's more contained. But yeah, I guess it could be done in the repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}

// Mark the property not to be changed if it did not exist already.
if (propertyAcquisitionFile.PropertyId != 0)
{
Expand Down
6 changes: 6 additions & 0 deletions source/backend/dal/Repositories/DispositionFilePropertyRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Security.Claims;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;

Expand Down Expand Up @@ -60,6 +61,11 @@ public PimsDispositionFileProperty Add(PimsDispositionFileProperty propertyDispo
{
propertyDispositionFile.ThrowIfNull(nameof(propertyDispositionFile));

if (propertyDispositionFile.Property.IsRetired.HasValue && propertyDispositionFile.Property.IsRetired.Value)
{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}

// Mark the property not to be changed if it did not exist already.
if (propertyDispositionFile.PropertyId != 0)
{
Expand Down
38 changes: 38 additions & 0 deletions 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 @@ -198,6 +198,44 @@ public void Add_DuplicateTeam()
act.Should().Throw<BadRequestException>();
repository.Verify(x => x.Add(It.IsAny<PimsAcquisitionFile>()), Times.Never);
}

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

var acqFile = EntityHelper.CreateAcquisitionFile();

acqFile.PimsPropertyAcquisitionFiles = new List<PimsPropertyAcquisitionFile>()
{
new PimsPropertyAcquisitionFile()
{
PropertyId = 100,
Property = new PimsProperty()
{
IsRetired = true,
}
},
};

var repository = this._helper.GetService<Mock<IAcquisitionFileRepository>>();
repository.Setup(x => x.Add(It.IsAny<PimsAcquisitionFile>())).Returns(acqFile);

var lookupRepository = this._helper.GetService<Mock<ILookupRepository>>();
lookupRepository.Setup(x => x.GetAllRegions()).Returns(new List<PimsRegion>() { new PimsRegion() { Code = 4, RegionName = "Cannot determine" } });

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

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

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
ex.WithMessage("Retired property can not be selected.");
}

#endregion

#region GetById
Expand Down
27 changes: 27 additions & 0 deletions source/backend/tests/unit/api/Services/DispositionFileServiceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,33 @@ public void Add_Success_IsContractor_AssignedToTeam()
repository.Verify(x => x.Add(It.IsAny<PimsDispositionFile>()), Times.Once);
}

[Fact]
public void Add_WithRetiredProperty_Should_Fail()
{
// Arrange
var service = this.CreateDispositionServiceWithPermissions(Permissions.DispositionAdd);

var dispositionFile = EntityHelper.CreateDispositionFile();
dispositionFile.PimsDispositionFileProperties = new List<PimsDispositionFileProperty>()
{
new PimsDispositionFileProperty()
{
PropertyId = 100,
Property = new PimsProperty()
{
IsRetired = true,
}
},
};

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

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
ex.WithMessage("Retired property can not be selected.");
}

#endregion

#region Update
Expand Down
39 changes: 39 additions & 0 deletions source/backend/tests/unit/api/Services/LeaseServiceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using FluentAssertions;
using Humanizer;
using MapsterMapper;
using Moq;
using Pims.Api.Constants;
using Pims.Api.Models.Concepts;
using Pims.Api.Services;
using Pims.Core.Exceptions;
using Pims.Core.Test;
using Pims.Dal;
using Pims.Dal.Entities;
Expand Down Expand Up @@ -113,6 +115,43 @@ public void Add_InvalidAccessToLeaseFile()
leaseRepository.Verify(x => x.Add(It.IsAny<PimsLease>()), Times.Never);
}

[Fact]
public void Add_WithRetiredProperty_Should_Fail()
{
// Arrange
var lease = EntityHelper.CreateLease(1);
lease.RegionCode = 1;

lease.PimsPropertyLeases = new List<PimsPropertyLease>() {
new PimsPropertyLease()
{
PropertyId = 100,
Property = new PimsProperty()
{
IsRetired = true,
}
}
};

var user = new PimsUser();
user.PimsRegionUsers.Add(new PimsRegionUser() { RegionCode = lease.RegionCode.Value });

var service = this.CreateLeaseService(Permissions.LeaseAdd);

var leaseRepository = this._helper.GetService<Mock<ILeaseRepository>>();
leaseRepository.Setup(x => x.Add(It.IsAny<PimsLease>())).Returns(lease);

var userRepository = this._helper.GetService<Mock<IUserRepository>>();
userRepository.Setup(x => x.GetByKeycloakUserId(It.IsAny<Guid>())).Returns(user);

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

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
ex.WithMessage("Retired property can not be selected.");
}

#endregion

#region Update
Expand Down
28 changes: 28 additions & 0 deletions source/backend/tests/unit/dal/Repositories/AcquisitionFilePropertyRepositoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using FluentAssertions;
using Pims.Core.Exceptions;
using Pims.Core.Test;
using Pims.Dal.Entities;
using Pims.Dal.Exceptions;
using Pims.Dal.Repositories;
using Pims.Dal.Security;
using Xunit;
Expand Down Expand Up @@ -140,6 +142,32 @@ public void Add_ThrowIfNull()
// Assert
act.Should().Throw<ArgumentNullException>();
}

[Fact]
public void Add_WithRetiredProperty_Should_Fail()
{
// Arrange
var helper = new TestHelper();
var user = PrincipalHelper.CreateForPermission(Permissions.AcquisitionFileAdd);

var context = helper.CreatePimsContext(user, true);
var repository = helper.CreateRepository<AcquisitionFilePropertyRepository>(user);

var pimsPropertyAcquisitionFile = new PimsPropertyAcquisitionFile()
{
AcquisitionFileId = 1,
PropertyId = 1,
Property = new PimsProperty() { RegionCode = 1, PropertyId = 1, IsRetired = true }
};

// Act
Action act = () => repository.Add(pimsPropertyAcquisitionFile);

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
ex.WithMessage("Retired property can not be selected.");
}

#endregion

#region Update
Expand Down
33 changes: 33 additions & 0 deletions source/backend/tests/unit/dal/Repositories/DispositionFilePropertyRepositoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using FluentAssertions;
using Pims.Core.Exceptions;
using Pims.Core.Test;
using Pims.Dal.Entities;
using Pims.Dal.Repositories;
Expand Down Expand Up @@ -140,6 +141,38 @@ public void Add_ThrowIfNull()
// Assert
act.Should().Throw<ArgumentNullException>();
}

[Fact]
public void Add_WithRetiredProperty_Should_Fail()
{
// Arrange
var helper = new TestHelper();
var user = PrincipalHelper.CreateForPermission(Permissions.DispositionAdd);

var context = helper.CreatePimsContext(user, true);
var repository = helper.CreateRepository<DispositionFilePropertyRepository>(user);

var pimsDispositionFileProperty = new PimsDispositionFileProperty()
{
DispositionFileId = 1,
PropertyId = 1,
Property = new PimsProperty()
{
RegionCode = 1,
PropertyId = 1,
IsRetired = true
}
};


// Act
Action act = () => repository.Add(pimsDispositionFileProperty);

// Assert
var ex = act.Should().Throw<BusinessRuleViolationException>();
ex.WithMessage("Retired property can not be selected.");
}

#endregion

#region Update
Expand Down
Loading