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

FuriousLlama · 2023-11-21T01:31:02Z

No description provided.

…le pages

github-actions · 2023-11-21T01:45:52Z

✅ No secrets were detected in the code.

codecov · 2023-11-21T01:50:03Z

Codecov Report

Merging #3602 (f16a01c) into dev (a43d33c) will decrease coverage by 0.18%.
The diff coverage is 59.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3602      +/-   ##
==========================================
- Coverage   69.68%   69.51%   -0.18%     
==========================================
  Files        1372     1375       +3     
  Lines       34020    34362     +342     
  Branches     6366     6486     +120     
==========================================
+ Hits        23707    23886     +179     
- Misses      10059    10216     +157     
- Partials      254      260       +6

Flag	Coverage Δ
unittests	`69.51% <59.02%> (-0.18%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
source/backend/api/Services/TakeService.cs	`100.00% <100.00%> (ø)`
...ce/backend/dal/Repositories/AgreementRepository.cs	`73.46% <ø> (ø)`
...ures/acquisition/hooks/useAcquisitionFileExport.ts	`25.00% <ø> (ø)`
...ition/list/AcquisitionFilter/AcquisitionFilter.tsx	`86.20% <100.00%> (ø)`
.../features/acquisition/list/AcquisitionListView.tsx	`60.71% <100.00%> (ø)`
...isition/tabs/agreement/update/AgreementSubForm.tsx	`75.00% <ø> (ø)`
...abs/agreement/update/UpdateAgreementsContainer.tsx	`100.00% <100.00%> (ø)`
...tabs/checklist/detail/AcquisitionChecklistView.tsx	`95.55% <100.00%> (+0.10%)`	⬆️
.../detail/CompensationRequisitionDetailContainer.tsx	`81.08% <ø> (ø)`
...ation/detail/CompensationRequisitionDetailView.tsx	`73.68% <100.00%> (+2.25%)`	⬆️
... and 18 more

github-actions · 2023-11-21T19:06:27Z

✅ No secrets were detected in the code.

github-actions · 2023-11-21T19:56:23Z

✅ No secrets were detected in the code.

devinleighsmith · 2023-11-21T23:16:35Z

@FuriousLlama I checked this out, but I don't see this anywhere:

Edit Button Is Hidden

GIVEN
User does not have permissions to edit a tab
WHEN
User views restricted tab
THEN
System will show a tooltip that indicates why editing is not available.
"The file you are viewing is in a non-editable state. Change the file status to active or draft to allow editing."

devinleighsmith · 2023-11-21T23:27:44Z

Hmm, the story does not go into how to control the behaviour of making an agreement editable/non-editable when it is swapped from draft to final in a file that is already in the final status. I do think it is a bit jarring to disable the fields the instant the status change happens (before the agreement is saved). Could that be changed to only make non-editable when the agreement status is final AND there is no id (indicating an unsaved agreement)?

devinleighsmith · 2023-11-21T23:30:53Z

source/backend/api/Services/AcquisitionFileService.cs

@@ -240,6 +244,12 @@ public PimsAcquisitionFile Update(PimsAcquisitionFile acquisitionFile, IEnumerab
            ValidateVersion(acquisitionFile.Internal_Id, acquisitionFile.ConcurrencyControlNumber);
            ValidateDrafts(acquisitionFile.Internal_Id);

+            AcqusitionStatusTypes? currentAcquisitionStatus = GetCurrentAcquisitionStatus(acquisitionFile.Internal_Id);


nit: acqusition -> acquisition

devinleighsmith · 2023-11-21T23:31:19Z

source/backend/api/Constants/AcqusitionStatusTypes.cs

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


nit: acqusition

devinleighsmith · 2023-11-21T23:39:43Z

source/backend/api/Services/AcquisitionFileService.cs

+            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));
+
+            foreach (var agreement in toBeUpdated)


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.

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

devinleighsmith · 2023-11-21T23:46:26Z

source/backend/api/Services/AcquisitionFileService.cs

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?

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

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

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.

I'm neutral about it.

devinleighsmith · 2023-11-21T23:48:28Z

source/backend/api/Services/CompensationRequisitionService.cs

+            var currentAcquisitionFile = _acqFileRepository.GetById(currentCompensation.AcquisitionFileId);
+            var currentAcquisitionStatus = Enum.Parse<AcqusitionStatusTypes>(currentAcquisitionFile.AcquisitionFileStatusTypeCode);
+
+            if (!_statusSolver.CanEditOrDeleteCompensation(currentAcquisitionStatus, currentCompensation.IsDraft) && !_user.HasPermission(Permissions.SystemAdmin))


I actually don't see a requirement for an admin to be able to override this in the story, can you point that out to me?

It's from a different story. I figured those where additive instead of replaced.

devinleighsmith · 2023-11-21T23:48:54Z

source/backend/api/Services/CompensationRequisitionService.cs

@@ -90,6 +100,15 @@ public bool DeleteCompensation(long compensationId)
            _logger.LogInformation("Deleting compensation with id ...", compensationId);
            _user.ThrowIfNotAuthorized(Permissions.CompensationRequisitionDelete, Permissions.AcquisitionFileEdit);

+            var currentCompensation = _compensationRequisitionRepository.GetById(compensationId);
+
+            var currentAcqusitionStatus = GetCurrentAcqusitionStatus(currentCompensation.AcquisitionFileId);


nit: getcurrent acqusition

devinleighsmith · 2023-11-22T00:26:12Z

source/backend/entities/Partials/Agreement.cs

@@ -11,5 +11,28 @@ public partial class PimsAgreement : StandardIdentityBaseAppEntity<long>, IBaseA
        [NotMapped]
        public override long Internal_Id { get => this.AgreementId; set => this.AgreementId = value; }
        #endregion
+
+        public bool IsEqual(PimsAgreement other)


In my experience isEqual methods generally handle being passed a null other value.

That's a good point, however, this one is not a Comparable but rather a helper method. I can make it more standard too but I am not sure we gain much from it.

devinleighsmith · 2023-11-22T00:26:45Z

source/backend/tests/core/TestHelper.cs

 using Pims.Dal;
 using Pims.Dal.Configuration.Generators;
+using Pims.Dal.Entities;


this seems odd, looks like you are adding imports but then not using them anywhere?

devinleighsmith · 2023-11-22T00:29:05Z

...ntend/src/features/mapSideBar/acquisition/tabs/checklist/detail/AcquisitionChecklistView.tsx

  return (
    <StyledSummarySection>
      <StyledEditWrapper className="mr-3 my-1">
-        {keycloak.hasClaim(Claims.ACQUISITION_EDIT) && acquisitionFile !== undefined ? (


are we ok removing that undefined acquisitionFile check?

the status solver does take into account the acq file being null/undefined

devinleighsmith · 2023-11-22T00:30:14Z

...s/mapSideBar/acquisition/tabs/compensation/detail/CompensationRequisitionDetailContainer.tsx

      compensation={compensation}
      compensationContactPerson={payeePerson}
      compensationContactOrganization={payeeOrganization}
-      acqFileProject={acquisitionFile?.project}


huh, I wonder why these were split out like this?

note sure, but now is more cohesive

devinleighsmith · 2023-11-22T00:32:42Z

...d/src/features/mapSideBar/acquisition/tabs/stakeholders/detail/StakeHolderContainer.test.tsx

@@ -83,101 +78,4 @@ describe('StakeHolderContainer component', () => {
    const { asFragment } = setup({});
    expect(asFragment()).toMatchSnapshot();
  });
-
-  it('groups multiple interest holder properties by acquisition file id', async () => {


All the tests for the grouping logic was movied into its own test file that only deals with that.

devinleighsmith · 2023-11-22T00:33:41Z

...ontend/src/features/mapSideBar/acquisition/tabs/stakeholders/detail/StakeHolderView.test.tsx

    model.identifier = 'PID: 025-196-375';
    const { getByText } = setup({
      props: {
-        groupedInterestProperties: [],
-        groupedNonInterestProperties: [model],
+        //groupedInterestProperties: [],


? if this is a wip, please fix. if not, can you remove these comments?

ups. forgot to cleanup this tests

devinleighsmith · 2023-11-22T00:34:10Z

...ontend/src/features/mapSideBar/acquisition/tabs/stakeholders/detail/StakeHolderContainer.tsx

      onEdit={onEdit}
    />
  );
 };

-const getGroupedInterestProperties = (


Is this related to this PR? If not, I'd really prefer we separate these kind of changes into their own PR.

Thats fair. I rather keep it as is now and in the future create new PRs

devinleighsmith · 2023-11-22T00:35:19Z

...rontend/src/features/mapSideBar/acquisition/tabs/stakeholders/detail/stakeholderOrganizer.ts

@@ -0,0 +1,84 @@
+import { Api_AcquisitionFile } from '@/models/api/AcquisitionFile';


This looks a bit like unrelated tech-debt. I prefer this, but feel like this should have been done in a separate PR at least.

True, I refactored to leverage similar props being passed. It also contains the grouping logic instead of being part of the view

devinleighsmith · 2023-11-22T00:39:40Z

I'm seeing a lot of line coverage warnings for these changes. Can you confirm that this PR conforms to our minimum test coverage requirements:

one test per business logic.
each layer should be mocked, validate that mocked layers are called in the expected manner.

FuriousLlama · 2023-11-22T00:52:31Z

I'm seeing a lot of line coverage warnings for these changes. Can you confirm that this PR conforms to our minimum test coverage requirements:

one test per business logic.

each layer should be mocked, validate that mocked layers are called in the expected manner.

The backend logic has very thorough tests. I can add some to the front end too for the solver, but otherwise, it should have the same amount of tests.

github-actions · 2023-11-22T01:35:19Z

✅ No secrets were detected in the code.

github-actions · 2023-11-22T01:40:34Z

✅ No secrets were detected in the code.

devinleighsmith

Approved, but you are going to get a test reject from not displaying the warning tooltip in place of the edit icon.

FuriousLlama · 2023-11-22T17:06:55Z

Approved, but you are going to get a test reject from not displaying the warning tooltip in place of the edit icon.

Tooltip is there :)

github-actions · 2023-11-22T17:08:49Z

✅ No secrets were detected in the code.

github-actions · 2023-11-23T22:34:30Z

✅ No secrets were detected in the code.

github-actions · 2023-11-23T22:51:16Z

✅ No secrets were detected in the code.

* Added check status logic when updating acq and related entities | psp-7006 (#3602) * Added status checking to details, take, compensation and other acq file pages * Updated backend and updated tests * Fixed lint * Refactored solver to be simpler * Added tests * Pr comments * Added the sys-admin to edit acquisition fields * CI: Bump version to v4.0.0-67.24 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Added check status logic when updating acq and related entities | psp-7006 (#3602) * Added status checking to details, take, compensation and other acq file pages * Updated backend and updated tests * Fixed lint * Refactored solver to be simpler * Added tests * Pr comments * Added the sys-admin to edit acquisition fields * CI: Bump version to v4.0.0-67.24 * Updated placeholder display (#3620) * CI: Bump version to v4.0.0-67.25 * dialog correction. (#3629) Co-authored-by: Smith <Devin.Smith@quartech.com> * CI: Bump version to v4.0.0-67.26 * Keycloak Refactor (#3624) * Refactored keycloak sync to use the keycloak repository used by the API. ALso removed tools.Core and removed redundant models * pr fixes * CI: Bump version to v4.0.0-67.27 * System error modal | psp-7304 (#3628) * Updated Generic modal to use a variant * Improved System error modal * Lit fixes * Info fixes * CI: Bump version to v4.0.0-67.28 * psp-7346 correct bracket usage, ensure logic limited to file close only. (#3622) * psp-7346 correct bracket usage, ensure logic limited to file close only. * comment from code review. * test correction. --------- Co-authored-by: Smith <Devin.Smith@quartech.com> * CI: Bump version to v4.0.0-67.29 * requested source marked as nullable for validation. (#3630) Co-authored-by: Smith <Devin.Smith@quartech.com> * CI: Bump version to v4.0.0-67.30 * allow showing the edit screen for management activity while the popout is open. (#3632) Co-authored-by: Smith <Devin.Smith@quartech.com> * CI: Bump version to v4.0.0-67.31 * Pims api to Frontend ts generator and generated files (#3618) * Refactored concept models. Created a new library and cleaned dependencies between projects. Cleaned names of reused concepts and standarized concept namespace. Cleaned extension and help files * Fixed lints * Initial generator work * Cleaned up files and improved translation * Updated namespaces and path for the api models * Updated generator and cleaned up code * Added make command to generate ts api files and lint them. * First generation of the pims api. * Cleaned up generator and handled generic types. Improved header docks. * Fixed solution * Updated models for keycloak sync and fixed lint warnings * Fixed registering mappers * CI: Bump version to v4.0.0-67.32 * Bump DEV version - IS68 (#3636) * CI: Bump version to v4.0.0-68.1 * Manual merges and re-generated snaps * moved disposition models to the new api-models project * re-generated api models * Changed disposition property to property model type --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: devinleighsmith <41091511+devinleighsmith@users.noreply.github.com> Co-authored-by: Smith <Devin.Smith@quartech.com> Co-authored-by: Alejandro Sanchez <emailforasr@gmail.com>

* Added check status logic when updating acq and related entities | psp-7006 (#3602) * Added status checking to details, take, compensation and other acq file pages * Updated backend and updated tests * Fixed lint * Refactored solver to be simpler * Added tests * Pr comments * Added the sys-admin to edit acquisition fields * CI: Bump version to v4.0.0-67.24 * Updated placeholder display (#3620) * CI: Bump version to v4.0.0-67.25 * Added the disposition entry into the menu list --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

FuriousLlama added 4 commits November 17, 2023 09:46

Added status checking to details, take, compensation and other acq fi…

5b8aec9

…le pages

Merge branch 'dev' into features/PSP-7006

ab1b02c

Updated backend and updated tests

8fe12ba

Fixed lint

7c73745

FuriousLlama added the enhancement New feature or request label Nov 21, 2023

FuriousLlama self-assigned this Nov 21, 2023

Manual merge

ef23d36

Refactored solver to be simpler

023085f

Added tests

96ee3af

devinleighsmith reviewed Nov 21, 2023

View reviewed changes

devinleighsmith reviewed Nov 22, 2023

View reviewed changes

Pr comments

c4eb179

Merge branch 'dev' into features/PSP-7006

3a9a4eb

devinleighsmith approved these changes Nov 22, 2023

View reviewed changes

Merge branch 'dev' into features/PSP-7006

a117b77

Merge branch 'dev' into features/PSP-7006

294c885

Added the sys-admin to edit acquisition fields

f16a01c

FuriousLlama merged commit 1525026 into bcgov:dev Nov 24, 2023
8 of 9 checks passed

FuriousLlama deleted the features/PSP-7006 branch December 15, 2023 20:34

		@@ -0,0 +1,84 @@
		import { Api_AcquisitionFile } from '@/models/api/AcquisitionFile';

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

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

Conversation

FuriousLlama commented Nov 21, 2023

github-actions bot commented Nov 21, 2023

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

github-actions bot commented Nov 21, 2023

github-actions bot commented Nov 21, 2023

devinleighsmith commented Nov 21, 2023

devinleighsmith commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinleighsmith commented Nov 22, 2023

FuriousLlama commented Nov 22, 2023

github-actions bot commented Nov 22, 2023

github-actions bot commented Nov 22, 2023

devinleighsmith left a comment

Choose a reason for hiding this comment

FuriousLlama commented Nov 22, 2023

github-actions bot commented Nov 22, 2023

github-actions bot commented Nov 23, 2023

github-actions bot commented Nov 23, 2023

codecov bot commented Nov 21, 2023 •

edited

Loading