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

Added check status logic when updating acq and related entities | psp-7006 #3602

Merged
merged 12 commits into from
Nov 24, 2023
Merged

Added check status logic when updating acq and related entities | psp-7006 #3602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5b8aec9
Added status checking to details, take, compensation and other acq fi…
FuriousLlama Nov 17, 2023
ab1b02c
Merge branch 'dev' into features/PSP-7006
FuriousLlama Nov 18, 2023
8fe12ba
Updated backend and updated tests
FuriousLlama Nov 21, 2023
7c73745
Fixed lint
FuriousLlama Nov 21, 2023
ef23d36
Manual merge
FuriousLlama Nov 21, 2023
023085f
Refactored solver to be simpler
FuriousLlama Nov 21, 2023
96ee3af
Added tests
FuriousLlama Nov 21, 2023
c4eb179
Pr comments
FuriousLlama Nov 22, 2023
3a9a4eb
Merge branch 'dev' into features/PSP-7006
FuriousLlama Nov 22, 2023
a117b77
Merge branch 'dev' into features/PSP-7006
FuriousLlama Nov 22, 2023
294c885
Merge branch 'dev' into features/PSP-7006
FuriousLlama Nov 23, 2023
f16a01c
Added the sys-admin to edit acquisition fields
FuriousLlama Nov 23, 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
4 changes: 2 additions & 2 deletions source/backend/api/Areas/Documents/DocumentRelationshipController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@
return new JsonResult(mappedResearchFileDocuments);
case DocumentRelationType.AcquisitionFiles:
var acquistionFileDocuments = _documentFileService.GetFileDocuments<PimsAcquisitionFileDocument>(FileType.Acquisition, long.Parse(parentId));
var mappedAquisitionFileDocuments = _mapper.Map<List<DocumentRelationshipModel>>(acquistionFileDocuments);
return new JsonResult(mappedAquisitionFileDocuments);
var mappedAcquisitionFileDocuments = _mapper.Map<List<DocumentRelationshipModel>>(acquistionFileDocuments);
return new JsonResult(mappedAcquisitionFileDocuments);

Check warning on line 96 in source/backend/api/Areas/Documents/DocumentRelationshipController.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Areas/Documents/DocumentRelationshipController.cs#L95-L96 

Added lines #L95 - L96 were not covered by tests
case DocumentRelationType.Templates:
var templateDocuments = _formDocumentService.GetFormDocumentTypes(parentId);
var mappedTemplateDocuments = _mapper.Map<List<DocumentRelationshipModel>>(templateDocuments);
Expand Down
31 changes: 31 additions & 0 deletions source/backend/api/Constants/AcquisitionStatusTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using System.Runtime.Serialization;
using System.Text.Json.Serialization;

namespace Pims.Api.Constants
{
[JsonConverter(typeof(JsonStringEnumMemberConverter))]
public enum AcquisitionStatusTypes
{

[EnumMember(Value = "ACTIVE")]
ACTIVE,

[EnumMember(Value = "ARCHIV")]
ARCHIV,

[EnumMember(Value = "CANCEL")]
CANCEL,

[EnumMember(Value = "CLOSED")]
CLOSED,

[EnumMember(Value = "COMPLT")]
COMPLT,

[EnumMember(Value = "DRAFT")]
DRAFT,

[EnumMember(Value = "HOLD")]
HOLD,
}
}
19 changes: 19 additions & 0 deletions source/backend/api/Constants/AgreementStatusTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using System.Runtime.Serialization;
using System.Text.Json.Serialization;

namespace Pims.Api.Constants
{
[JsonConverter(typeof(JsonStringEnumMemberConverter))]
public enum AgreementStatusTypes
{
[EnumMember(Value = "CANCELLED")]
CANCELLED,

[EnumMember(Value = "DRAFT")]
DRAFT,

[EnumMember(Value = "FINAL")]
FINAL,

}
}
78 changes: 69 additions & 9 deletions source/backend/api/Services/AcquisitionFileService.cs
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you added an explicit check/throw for stakeholders and checklists even though those errors will not get thrown in current state. I'm fine with that as it provides a consistent pattern for update logic, but doesn't that mean you should add a check for updateProperties as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I gather on the confluence page the properties are allowed to be updated at all times (all statues)

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, but the same applies to stakeholders and checklists, why have explicit logic for those and not for properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I understand what you are saying now. Initially I had it like that, but it felt like adding those checks was adding unnecessary complexity. If you feel strongly about it I can add them back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm neutral about it.

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Security.Claims;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Api.Constants;
using Pims.Api.Helpers.Exceptions;
using Pims.Api.Helpers.Extensions;
using Pims.Core.Exceptions;
Expand Down Expand Up @@ -40,6 +41,7 @@
private readonly ICompReqFinancialService _compReqFinancialService;
private readonly IExpropriationPaymentRepository _expropriationPaymentRepository;
private readonly ITakeRepository _takeRepository;
private readonly IAcquisitionStatusSolver _statusSolver;

public AcquisitionFileService(
ClaimsPrincipal user,
Expand All @@ -57,7 +59,8 @@
IInterestHolderRepository interestHolderRepository,
ICompReqFinancialService compReqFinancialService,
IExpropriationPaymentRepository expropriationPaymentRepository,
ITakeRepository takeRepository)
ITakeRepository takeRepository,
IAcquisitionStatusSolver statusSolver)
{
_user = user;
_logger = logger;
Expand All @@ -75,6 +78,7 @@
_compReqFinancialService = compReqFinancialService;
_expropriationPaymentRepository = expropriationPaymentRepository;
_takeRepository = takeRepository;
_statusSolver = statusSolver;
}

public Paged<PimsAcquisitionFile> GetPage(AcquisitionFilter filter)
Expand Down Expand Up @@ -240,6 +244,12 @@
ValidateVersion(acquisitionFile.Internal_Id, acquisitionFile.ConcurrencyControlNumber);
ValidateDrafts(acquisitionFile.Internal_Id);

AcquisitionStatusTypes? currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFile.Internal_Id);
if (!_statusSolver.CanEditDetails(currentAcquisitionStatus) && !_user.HasPermission(Permissions.SystemAdmin))
{
throw new BusinessRuleViolationException("The file you are editing is not active or draft, so you cannot save changes. Refresh your browser to see file state.");
}

if (!userOverrides.Contains(UserOverrideCode.UpdateRegion))
{
ValidateMinistryRegion(acquisitionFile.Internal_Id, acquisitionFile.RegionCode);
Expand Down Expand Up @@ -341,6 +351,12 @@
_user.ThrowIfNotAuthorized(Permissions.AcquisitionFileEdit);
_user.ThrowInvalidAccessToAcquisitionFile(_userRepository, _acqFileRepository, acquisitionFile.Internal_Id);

var currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFile.Internal_Id);
if (!_statusSolver.CanEditChecklists(currentAcquisitionStatus) && !_user.HasPermission(Permissions.SystemAdmin))
{
throw new BusinessRuleViolationException("The file you are editing is not active or draft, so you cannot save changes. Refresh your browser to see file state.");
}

// Get the current checklist items for this acquisition file.
var currentItems = _checklistRepository.GetAllChecklistItemsByAcquisitionFileId(acquisitionFile.Internal_Id).ToDictionary(ci => ci.Internal_Id);

Expand Down Expand Up @@ -372,7 +388,7 @@
_user.ThrowIfNotAuthorized(Permissions.AgreementView);
_user.ThrowInvalidAccessToAcquisitionFile(_userRepository, _acqFileRepository, id);

return _agreementRepository.GetAgreementsByAquisitionFile(id);
return _agreementRepository.GetAgreementsByAcquisitionFile(id);

Check warning on line 391 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L391 

Added line #L391 was not covered by tests
}

public IEnumerable<PimsAgreement> SearchAgreements(AcquisitionReportFilterModel filter)
Expand All @@ -393,6 +409,31 @@
{
_user.ThrowInvalidAccessToAcquisitionFile(_userRepository, _acqFileRepository, acquisitionFileId);

var currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFileId);

Check warning on line 412 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L412 

Added line #L412 was not covered by tests

var currentAgreements = _agreementRepository.GetAgreementsByAcquisitionFile(acquisitionFileId);

Check warning on line 414 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L414 

Added line #L414 was not covered by tests

var toBeUpdated = currentAgreements.Where(ca => agreements.Any(na => ca.AgreementId == na.AgreementId && !ca.IsEqual(na)));
var toBeDeleted = currentAgreements.Where(ca => !agreements.Any(na => ca.AgreementId == na.AgreementId));

Check warning on line 417 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L416-L417 

Added lines #L416 - L417 were not covered by tests

foreach (var agreement in toBeUpdated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason that agreements being edited and agreements being deleted are being split out into their own loops? Seems like the business logic that is being executed is identical, sort of adds an implication that updates should not be handles the same as deletes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although true, doing the loop on each would be the same as doing the loop on an aggregated collection.

{
var agreementStatus = Enum.Parse<AgreementStatusTypes>(agreement.AgreementStatusTypeCode);

Check warning on line 421 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L420-L421 

Added lines #L420 - L421 were not covered by tests
if (!_statusSolver.CanEditOrDeleteAgreement(currentAcquisitionStatus, agreementStatus) && !_user.HasPermission(Permissions.SystemAdmin))
{
throw new BusinessRuleViolationException("The file you are editing is not active or draft, so you cannot save changes. Refresh your browser to see file state.");

Check warning on line 424 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L423-L424 

Added lines #L423 - L424 were not covered by tests
}
}

Check warning on line 426 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L426 

Added line #L426 was not covered by tests

foreach (var agreement in toBeDeleted)
{
var agreementStatus = Enum.Parse<AgreementStatusTypes>(agreement.AgreementStatusTypeCode);

Check warning on line 430 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L429-L430 

Added lines #L429 - L430 were not covered by tests
if (!_statusSolver.CanEditOrDeleteAgreement(currentAcquisitionStatus, agreementStatus) && !_user.HasPermission(Permissions.SystemAdmin))
{
throw new BusinessRuleViolationException("The file you are editing is not active or draft, so you cannot save changes. Refresh your browser to see file state.");

Check warning on line 433 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L432-L433 

Added lines #L432 - L433 were not covered by tests
}
}

Check warning on line 435 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L435 

Added line #L435 was not covered by tests

var updatedAgreements = _agreementRepository.UpdateAllForAcquisition(acquisitionFileId, agreements);
_agreementRepository.CommitTransaction();

Expand All @@ -414,6 +455,12 @@
_user.ThrowIfNotAuthorized(Permissions.AcquisitionFileEdit);
_user.ThrowInvalidAccessToAcquisitionFile(_userRepository, _acqFileRepository, acquisitionFileId);

var currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFileId);

Check warning on line 458 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L458 

Added line #L458 was not covered by tests
if (!_statusSolver.CanEditStakeholders(currentAcquisitionStatus) && !_user.HasPermission(Permissions.SystemAdmin))
{
throw new BusinessRuleViolationException("The file you are editing is not active or draft, so you cannot save changes. Refresh your browser to see file state.");

Check warning on line 461 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L460-L461 

Added lines #L460 - L461 were not covered by tests
}

var currentInterestHolders = _interestHolderRepository.GetInterestHoldersByAcquisitionFile(acquisitionFileId);

// Verify that the interest holder is still the same (person or org)
Expand Down Expand Up @@ -690,7 +737,7 @@

private void ValidateDrafts(long acqFileId)
{
var agreements = _agreementRepository.GetAgreementsByAquisitionFile(acqFileId);
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)))
{
Expand Down Expand Up @@ -838,7 +885,7 @@

private void ValidatePayeeDependency(PimsAcquisitionFile acquisitionFile)
{
var currentAquisitionFile = _acqFileRepository.GetById(acquisitionFile.Internal_Id);
var currentAcquisitionFile = _acqFileRepository.GetById(acquisitionFile.Internal_Id);
var compensationRequisitions = _compensationRequisitionRepository.GetAllByAcquisitionFileId(acquisitionFile.Internal_Id);

if (compensationRequisitions.Count == 0)
Expand All @@ -851,23 +898,23 @@
// Check for Acquisition File Owner removed
if (compReq.AcquisitionOwnerId is not null
&& !acquisitionFile.PimsAcquisitionOwners.Any(x => x.Internal_Id.Equals(compReq.AcquisitionOwnerId))
&& currentAquisitionFile.PimsAcquisitionOwners.Any(x => x.Internal_Id.Equals(compReq.AcquisitionOwnerId)))
&& currentAcquisitionFile.PimsAcquisitionOwners.Any(x => x.Internal_Id.Equals(compReq.AcquisitionOwnerId)))
{
throw new ForeignKeyDependencyException("Acquisition File Owner can not be removed since it's assigned as a payee for a compensation requisition");
}

// Check for Acquisition InterestHolders
if (compReq.InterestHolderId is not null
&& !acquisitionFile.PimsInterestHolders.Any(x => x.Internal_Id.Equals(compReq.InterestHolderId))
&& currentAquisitionFile.PimsInterestHolders.Any(x => x.Internal_Id.Equals(compReq.InterestHolderId)))
&& currentAcquisitionFile.PimsInterestHolders.Any(x => x.Internal_Id.Equals(compReq.InterestHolderId)))
{
throw new ForeignKeyDependencyException("Acquisition File Interest Holders can not be removed since it's assigned as a payee for a compensation requisition");
}

// Check for File Person
if (compReq.AcquisitionFileTeamId is not null
&& !acquisitionFile.PimsAcquisitionFileTeams.Any(x => x.Internal_Id.Equals(compReq.AcquisitionFileTeamId))
&& currentAquisitionFile.PimsAcquisitionFileTeams.Any(x => x.Internal_Id.Equals(compReq.AcquisitionFileTeamId)))
&& currentAcquisitionFile.PimsAcquisitionFileTeams.Any(x => x.Internal_Id.Equals(compReq.AcquisitionFileTeamId)))
{
throw new ForeignKeyDependencyException("Acquisition File team member can not be removed since it's assigned as a payee for a compensation requisition");
}
Expand All @@ -876,7 +923,7 @@

private void ValidateInterestHoldersDependency(long acquisitionFileId, List<PimsInterestHolder> interestHolders)
{
var currentAquisitionFile = _acqFileRepository.GetById(acquisitionFileId);
var currentAcquisitionFile = _acqFileRepository.GetById(acquisitionFileId);

Check warning on line 926 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L926 

Added line #L926 was not covered by tests
var compensationRequisitions = _compensationRequisitionRepository.GetAllByAcquisitionFileId(acquisitionFileId);

if (compensationRequisitions.Count == 0)
Expand All @@ -889,11 +936,24 @@
// Check for Interest Holder
if (compReq.InterestHolderId is not null
&& !interestHolders.Any(x => x.InterestHolderId.Equals(compReq.InterestHolderId))
&& currentAquisitionFile.PimsInterestHolders.Any(x => x.Internal_Id.Equals(compReq.InterestHolderId)))
&& currentAcquisitionFile.PimsInterestHolders.Any(x => x.Internal_Id.Equals(compReq.InterestHolderId)))

Check warning on line 939 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L939 

Added line #L939 was not covered by tests
{
throw new ForeignKeyDependencyException("Acquisition File Interest Holder can not be removed since it's assigned as a payee for a compensation requisition");
}
}
}

private AcquisitionStatusTypes? GetCurrentAcquisitionStatus(long acquisitionFileId)
{
var currentAcquisitionFile = _acqFileRepository.GetById(acquisitionFileId);
AcquisitionStatusTypes currentAcquisitionStatus;

if (Enum.TryParse(currentAcquisitionFile.AcquisitionFileStatusTypeCode, out currentAcquisitionStatus))
{
return currentAcquisitionStatus;
}

return currentAcquisitionStatus;

Check warning on line 956 in source/backend/api/Services/AcquisitionFileService.cs

View check run for this annotation

Codecov / codecov/patch

source/backend/api/Services/AcquisitionFileService.cs#L956 

Added line #L956 was not covered by tests
}
}
}
Loading
Loading