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

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion source/backend/api/Services/AcquisitionFileService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,15 @@ public PimsAcquisitionFile Add(PimsAcquisitionFile acquisitionFile, IEnumerable<
ValidateStaff(acquisitionFile);
ValidateOrganizationStaff(acquisitionFile);

acquisitionFile.AcquisitionFileStatusTypeCode = "ACTIVE";
MatchProperties(acquisitionFile, userOverrides);
ValidatePropertyRegions(acquisitionFile);

PopulateAcquisitionChecklist(acquisitionFile);

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

return newAcqFile;
}

Expand Down
1 change: 0 additions & 1 deletion source/backend/api/Services/DispositionFileService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ public PimsDispositionFile Add(PimsDispositionFile dispositionFile, IEnumerable<
ValidateStaff(dispositionFile);

MatchProperties(dispositionFile, userOverrides);

ValidatePropertyRegions(dispositionFile);

var newDispositionFile = _dispositionFileRepository.Add(dispositionFile);
Expand Down
12 changes: 11 additions & 1 deletion source/backend/api/Services/LeaseService.cs
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 @@ -185,6 +186,7 @@ public PimsLease Add(PimsLease lease, IEnumerable<UserOverrideCode> userOverride
pimsUser.ThrowInvalidAccessToLeaseFile(lease.RegionCode);

var leasesWithProperties = AssociatePropertyLeases(lease, userOverrides);

return _leaseRepository.Add(leasesWithProperties);
}

Expand All @@ -203,12 +205,20 @@ public PimsLease Update(PimsLease lease, IEnumerable<UserOverrideCode> userOverr
{
_logger.LogInformation("Updating lease {leaseId}", lease.LeaseId);
_user.ThrowIfNotAuthorized(Permissions.LeaseEdit);

var pimsUser = _userRepository.GetByKeycloakUserId(_user.GetUserKey());
var currentLease = _leaseRepository.GetNoTracking(lease.LeaseId);

pimsUser.ThrowInvalidAccessToLeaseFile(currentLease.RegionCode); // need to check that the user is able to access the current lease as well as has the region for the updated lease.
pimsUser.ThrowInvalidAccessToLeaseFile(lease.RegionCode);

var currentProperties = _propertyLeaseRepository.GetAllByLeaseId(lease.LeaseId);
var newPropertiesAdded = lease.PimsPropertyLeases.Where(x => !currentProperties.Any(y => y.Internal_Id == x.Internal_Id)).ToList();

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

if (currentLease.LeaseStatusTypeCode != lease.LeaseStatusTypeCode)
{
Expand Down Expand Up @@ -237,7 +247,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
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/AcquisitionFileRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using LinqKit;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;
using Pims.Dal.Entities.Models;
Expand Down Expand Up @@ -656,6 +657,11 @@ public PimsAcquisitionFile Add(PimsAcquisitionFile acquisitionFile)
using var scope = Logger.QueryScope();
acquisitionFile.ThrowIfNull(nameof(acquisitionFile));

if (acquisitionFile.PimsPropertyAcquisitionFiles.Any(x => x.Property.IsRetired.HasValue && x.Property.IsRetired.Value))
{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}

// Existing properties should not be added.
foreach (var acquisitionProperty in acquisitionFile.PimsPropertyAcquisitionFiles)
{
Expand Down
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
6 changes: 6 additions & 0 deletions source/backend/dal/Repositories/DispositionFileRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using LinqKit;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;
using Pims.Dal.Entities.Models;
Expand Down Expand Up @@ -86,6 +87,11 @@ public PimsDispositionFile Add(PimsDispositionFile disposition)
using var scope = Logger.QueryScope();
disposition.ThrowIfNull(nameof(disposition));

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

disposition.FileNumber = _sequenceRepository.GetNextSequenceValue(FILENUMBERSEQUENCETABLE).ToString();

Context.PimsDispositionFiles.Add(disposition);
Expand Down
6 changes: 6 additions & 0 deletions source/backend/dal/Repositories/LeaseRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Security.Claims;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Core.Exceptions;
using Pims.Core.Extensions;
using Pims.Dal.Entities;
using Pims.Dal.Entities.Models;
Expand Down Expand Up @@ -761,6 +762,11 @@ public PimsLease Add(PimsLease lease)

User.ThrowIfNotAuthorized(Permissions.LeaseAdd);

if (lease.PimsPropertyLeases.Any(x => x.Property.IsRetired.HasValue && x.Property.IsRetired.Value))
{
throw new BusinessRuleViolationException("Retired property can not be selected.");
}

lease = GenerateLFileNo(lease);

Context.PimsLeases.Add(lease);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ public void Add_DuplicateTeam()
act.Should().Throw<BadRequestException>();
repository.Verify(x => x.Add(It.IsAny<PimsAcquisitionFile>()), Times.Never);
}

#endregion

#region GetById
Expand Down
39 changes: 38 additions & 1 deletion source/backend/tests/unit/api/Services/LeaseServiceTest.cs
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 @@ -193,19 +195,54 @@ public void Update_Properties_Success()
var propertyLeaseRepository = this._helper.GetService<Mock<IPropertyLeaseRepository>>();
var propertyRepository = this._helper.GetService<Mock<IPropertyRepository>>();
var userRepository = this._helper.GetService<Mock<IUserRepository>>();

propertyRepository.Setup(x => x.GetByPid(It.IsAny<int>())).Returns(lease.PimsPropertyLeases.FirstOrDefault().Property);
leaseRepository.Setup(x => x.GetNoTracking(It.IsAny<long>())).Returns(lease);
leaseRepository.Setup(x => x.Get(It.IsAny<long>())).Returns(EntityHelper.CreateLease(1));
userRepository.Setup(x => x.GetByKeycloakUserId(It.IsAny<Guid>())).Returns(new PimsUser());

// Act

var updatedLease = service.Update(lease, new List<UserOverrideCode>() { UserOverrideCode.AddLocationToProperty });

// Assert
leaseRepository.Verify(x => x.Update(lease, false), Times.Once);
}

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

var service = this.CreateLeaseService(Permissions.LeaseEdit, Permissions.LeaseView);
var leaseRepository = this._helper.GetService<Mock<ILeaseRepository>>();
var propertyLeaseRepository = this._helper.GetService<Mock<IPropertyLeaseRepository>>();
var propertyRepository = this._helper.GetService<Mock<IPropertyRepository>>();
var userRepository = this._helper.GetService<Mock<IUserRepository>>();

propertyRepository.Setup(x => x.GetByPid(It.IsAny<int>())).Returns(lease.PimsPropertyLeases.FirstOrDefault().Property);
leaseRepository.Setup(x => x.GetNoTracking(It.IsAny<long>())).Returns(lease);
leaseRepository.Setup(x => x.Get(It.IsAny<long>())).Returns(EntityHelper.CreateLease(1));
userRepository.Setup(x => x.GetByKeycloakUserId(It.IsAny<Guid>())).Returns(new PimsUser());

lease.PimsPropertyLeases.Add(new PimsPropertyLease()
{
PropertyId = 100,
Property = new PimsProperty()
{
Pid = 1,
IsRetired = true,
}
});

// Act
Action act = () => service.Update(lease, new List<UserOverrideCode>() { UserOverrideCode.AddLocationToProperty });

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

[Fact]
public void Update_Properties_Delete_Success()
{
Expand Down
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
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,39 @@ public void Add_ThrowIfNull()
// Assert
act.Should().Throw<ArgumentNullException>();
}


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

var repository = helper.CreateRepository<AcquisitionFileRepository>(user);

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

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

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


#endregion

#region GetById
Expand Down
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
Loading