PSP-7296 : Create/Edit Sales Details (Disposition) #3731

eddherrera · 2024-01-19T16:57:51Z

# Conflicts: # source/backend/api/Areas/Disposition/Controllers/DispositionFileController.cs # source/backend/api/Services/DispositionFileService.cs # source/backend/api/Services/IDispositionFileService.cs # source/backend/dal/Repositories/DispositionFileRepository.cs # source/backend/dal/Repositories/Interfaces/IDispositionFileRepository.cs # source/frontend/src/components/common/form/FastDateYearPicker.tsx # source/frontend/src/features/mapSideBar/disposition/router/DispositionRouter.tsx # source/frontend/src/features/mapSideBar/disposition/tabs/offersAndSale/OffersAndSaleContainerView.tsx

github-actions · 2024-01-19T16:59:43Z

✅ No secrets were detected in the code.

codecov · 2024-01-19T17:05:27Z

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (e44fd1e) 75.02% compared to head (78a7b43) 75.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3731      +/-   ##
==========================================
+ Coverage   75.02%   75.21%   +0.18%     
==========================================
  Files        1441     1447       +6     
  Lines       37194    37498     +304     
  Branches     6977     7060      +83     
==========================================
+ Hits        27905    28204     +299     
- Misses       9011     9014       +3     
- Partials      278      280       +2

Flag	Coverage Δ
unittests	`75.21% <78.69%> (+0.18%)`	⬆️

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

Files	Coverage Δ
...rce/backend/api/Services/DispositionFileService.cs	`75.90% <100.00%> (+1.84%)`	⬆️
...els/Concepts/DispositionFile/DispositionFileMap.cs	`100.00% <ø> (ø)`
...Concepts/DispositionFile/DispositionFileSaleMap.cs	`100.00% <100.00%> (ø)`
...ispositionFile/DispositionSalePurchaserAgentMap.cs	`100.00% <ø> (ø)`
...pts/DispositionFile/DispositionSalePurchaserMap.cs	`100.00% <ø> (ø)`
...sitionFile/DispositionSalePurchaserSolicitorMap.cs	`100.00% <ø> (ø)`
...kend/dal/Repositories/DispositionFileRepository.cs	`95.20% <100.00%> (+0.20%)`	⬆️
...tion/tabs/offersAndSale/OffersAndSaleContainer.tsx	`72.41% <100.00%> (ø)`
...leContactDetails/DispositionSaleContactDetails.tsx	`100.00% <100.00%> (ø)`
...spositionSale/form/DispositionSaleFormYupSchema.ts	`100.00% <100.00%> (ø)`
... and 14 more

... and 1 file with indirect coverage changes

github-actions · 2024-01-19T18:38:48Z

✅ No secrets were detected in the code.

devinleighsmith · 2024-01-22T18:55:34Z

source/backend/api/Areas/Disposition/Controllers/DispositionFileController.cs

+
+            return new JsonResult(_mapper.Map<DispositionFileSaleModel>(updatedSale));
+        }
+


Adding unit tests for controllers is pretty quick, can you please add the minimal amount to allow for test coverage?

devinleighsmith · 2024-01-22T18:57:37Z

source/backend/apimodels/Models/Concepts/DispositionFile/DispositionFileSaleModel.cs


        /// <summary>
-        /// get/set - A list of disposition Sale Purchaser(s) Solicitors.
+        /// get/set - Disposition Sale Purchaser(s)'s Solicitor.


Purchaser(s)'s?

devinleighsmith · 2024-01-22T19:34:15Z

source/frontend/src/features/mapSideBar/disposition/models/DispositionSaleFormModel.ts

@@ -47,22 +75,34 @@ export class DispositionSaleFormModel {
      finalConditionRemovalDate: emptyStringtoNullable(this.finalConditionRemovalDate),
      saleCompletionDate: emptyStringtoNullable(this.saleCompletionDate),
      saleFiscalYear: emptyStringtoNullable(this.saleFiscalYear),
-      finalSaleAmount: this.finalSaleAmount,
-      realtorCommissionAmount: this.realtorCommissionAmount,
+      finalSaleAmount: this.finalSaleAmount ? parseFloat(this.finalSaleAmount.toString()) : null,


why is this necessary?

When a value it's removed from the Input, it's being passed as an Empty String, (although it should have been parsed as a null, it doesn't, so a check is needed.)

devinleighsmith · 2024-01-22T19:36:16Z

source/frontend/src/features/mapSideBar/disposition/models/DispositionSaleFormModel.ts

  }

-  static fromApi(entity: Api_DispositionFileSale) {
-    const model = new DispositionSaleFormModel(entity.id, entity.dispositionFileId);
+  static fromApi(entity: Api_DispositionFileSale): DispositionSaleFormModel {


This is a pretty complex model, it looks like a good candidate for model unit tests. See some of our other model unit tests for example (they are quite simple).

devinleighsmith · 2024-01-22T19:47:59Z

This difference in primary contact display doesn't seem correct:

devinleighsmith · 2024-01-22T19:53:04Z

...ar/disposition/tabs/offersAndSale/dispositionSale/hooks/useCalculateNetProceedsBeforeSPP.tsx

+  const netBookAmount = getIn(values, 'netBookAmount');
+  const sppAmount = getIn(values, 'sppAmount');
+
+  useEffect(() => {


This code is a bit odd, why is being touched a requirement to display the summed totals? wouldn't the user want to see the totals regardless of whether every field has been touched?

I think this should be changed. If I come back to edit a sale, I wouldn't expect to have to touch every field in order to see my new total. What if I just want to change one field?

the amounts are update when any of the fields are touched. not when all are touched.

devinleighsmith · 2024-01-22T19:58:02Z

setting isGstRequired to true sends the string value "true" to the backend which causes a 400 error.

devinleighsmith · 2024-01-22T20:01:00Z

...pSideBar/disposition/tabs/offersAndSale/dispositionSale/form/DispositionSaleFormYupSchema.ts

+/* eslint-disable no-template-curly-in-string */
+import * as yup from 'yup';
+
+export const DispositionSaleFormYupSchema = yup.object().shape({


these are all strings, but in actuality a lot of these values are numbers or dates, please fix.
Make sure to enforce number size limits.
Also, it probably makes sense to limit the fiscal year to 4 digits.

devinleighsmith · 2024-01-22T20:02:22Z

...Bar/disposition/tabs/offersAndSale/dispositionSale/form/DispositionSalePurchasersSubForm.tsx

+        <>
+          {values.dispositionPurchasers.map(
+            (purchaser: DispositionSaleContactModel, index: number) => (
+              <React.Fragment key={`purchaser-${index}`}>


Please don't do this unless there is no readily available unique identifier. See React docs for rationale:

devinleighsmith · 2024-01-22T20:05:41Z

...tures/mapSideBar/disposition/tabs/offersAndSale/dispositionSale/form/DispositionSaleForm.tsx

+    const taxCollectedAmountValue = getIn(formikProps.values, 'gstCollectedAmount');
+
+    if (!isGstRequired) {
+      setModalContent({


this looks like it isn't covered in tests, but is one of the more complex workflows in this logic, probably should be covered.

devinleighsmith · 2024-01-22T20:08:21Z

...ar/disposition/tabs/offersAndSale/dispositionSale/hooks/useCalculateNetProceedsBeforeSPP.tsx

+        sppTouched) &&
+      !isSubmitting
+    ) {
+      if (gstDecimal && isGstEligible) {


this may not be safe, as if gstDecimal is 0 all of this logic will be skipped.

devinleighsmith

see comments.

github-actions · 2024-01-23T17:56:34Z

✅ No secrets were detected in the code.

FuriousLlama · 2024-01-24T19:20:37Z

source/frontend/src/features/mapSideBar/disposition/models/DispositionSaleFormModel.ts

@@ -38,6 +52,20 @@ export class DispositionSaleFormModel {

    model.netProceedsBeforeSppAmount = calculateNetProceedsBeforeSppAmount(entity);
    model.netProceedsAfterSppAmount = calculateNetProceedsAfterSppAmount(entity);
+
+    model.dispositionPurchasers = entity.dispositionPurchasers?.map(


not sure why this is not barking, but the result of this could be an undefined, that does not match the type in the model

FuriousLlama · 2024-01-24T19:24:22Z

...tures/mapSideBar/disposition/tabs/offersAndSale/dispositionSale/form/DispositionSaleForm.tsx

+          }}
+        />
+      </SectionField>
+      <SectionField label=" Realtor commission ($)" labelWidth="5" contentWidth="5">


nit space on the label

FuriousLlama · 2024-01-24T19:28:20Z

source/frontend/src/features/mapSideBar/disposition/models/DispositionSaleFormModel.ts

+
+    model.dispositionPurchaserAgent = entity.dispositionPurchaserAgent
+      ? DispositionSaleContactModel.fromApi(entity.dispositionPurchaserAgent)
+      : new DispositionSaleContactModel(null, entity.id, 0, null);


this constructor is being called with the same hardcoded parameters, that look like a codesmell. instead how about using a static method that builds the object using only the entity id?

I would rather just simplify the constructor instead of adding another method.

FuriousLlama · 2024-01-24T19:28:43Z

...Bar/disposition/tabs/offersAndSale/dispositionSale/form/DispositionSalePurchasersSubForm.tsx

+              const purchaserContact = new DispositionSaleContactModel(
+                null,
+                dispositionSaleId,
+                0,
+                null,
+              );
+              arrayHelpers.push(purchaserContact);


FuriousLlama · 2024-01-24T19:29:56Z

...apSideBar/disposition/tabs/offersAndSale/dispositionSale/update/UpdateDispostionSaleView.tsx

+        onSubmit={async (values: DispositionSaleFormModel, formikHelpers) => {
+          try {
+            const sale = await onSave(values.toApi());
+            if (sale && sale.id) {


note that the sale id could be zero

that's ok, when Zero it would mean the Api request call wasn't successful.

under that logic it should check for a zero as well

FuriousLlama · 2024-01-24T19:30:43Z

...apSideBar/disposition/tabs/offersAndSale/dispositionSale/update/UpdateDispostionSaleView.tsx

+const StyledFormWrapper = styled.div`
+  display: flex;
+  flex-direction: column;
+  flex-grow: 1;
+  text-align: left;
+  height: 100%;
+  overflow-y: auto;
+  padding-bottom: 1rem;
+`;
+
+const StyledContent = styled.div`
+  background-color: ${props => props.theme.css.filterBackgroundColor};
+`;
+
+const StyledFooter = styled.div`
+  margin-right: 1rem;
+  padding-bottom: 1rem;
+  z-index: 0;
+`;


any chance this is duplicated?

Very likely, we could create a story to define a new css definition for these kind of components.

FuriousLlama

based on comments

github-actions · 2024-01-24T23:55:02Z

✅ No secrets were detected in the code.

github-actions · 2024-01-25T00:11:40Z

✅ No secrets were detected in the code.

github-actions · 2024-01-25T18:24:36Z

✅ No secrets were detected in the code.

github-actions · 2024-01-25T18:53:51Z

✅ No secrets were detected in the code.

github-actions · 2024-01-25T20:01:47Z

✅ No secrets were detected in the code.

FuriousLlama · 2024-01-25T22:34:30Z

source/backend/api/Services/DispositionFileService.cs

@@ -246,6 +246,34 @@ public PimsDispositionSale GetDispositionFileSale(long dispositionFileId)
            return _dispositionFileRepository.GetDispositionFileSale(dispositionFileId);
        }

+        public PimsDispositionSale UpdateDispositionFileSale(long dispositionFileId, long saleId, PimsDispositionSale dispositionSale)


saleId and dispositionfileid are no longer necessary

FuriousLlama · 2024-01-25T22:37:02Z

source/backend/api/Services/DispositionFileService.cs

+            return updatedSale;
+        }
+
+        public PimsDispositionSale AddDispositionFileSale(long dispositionFileId, PimsDispositionSale dispositionSale)


does the dispositionsale have an invalid dispositionfileId? otherwise the file id does not need to be passed separately into the service

github-actions · 2024-01-26T00:14:07Z

✅ No secrets were detected in the code.

* Refactoring of modals and changes on Smoke Test Automation Test Cases * Automation refactoring - modals, updating fields on Compensation, Digital Docs for Property Management * Refactoring modals * Automation refactoring - modals, updating fields on Compensation, Digital Docs for Property Management * Refactoring modals * Automation updates * Automation updates * Contacts elements update * Digital Documents subtitles fix * CI: Bump version to v5.0.0-71.25 * Version bump (#3729) * CI: Bump version to v5.0.0-72.1 * Disposition Files initial Automation * CI: Bump version to v5.0.0-72.2 * Psp 7611 - ensure undefined/null handled correctly by contact parsers. (#3730) * handle null and undefined contact fields. * remove debugger; * CI: Bump version to v5.0.0-72.3 * PSP-7578 Remove Project and Product from Header of Disposition File (#3734) * Fix runtime error when user guid is invalid * Remove Project and Product from Header of Disposition File * Test corrections * CI: Bump version to v5.0.0-72.4 * psp-7630 ensure that properties added directly to inventory have the … (#3728) * psp-7630 ensure that properties added directly to inventory have the correct flags set. * test correction. Signed-off-by: devinleighsmith <[email protected]> --------- Signed-off-by: devinleighsmith <[email protected]> * CI: Bump version to v5.0.0-72.5 * psp-7609 ensure that changing the status of an acq file generates a note. (#3726) * CI: Bump version to v5.0.0-72.6 * lint correction. (#3733) * CI: Bump version to v5.0.0-72.7 * IS-72.00 Database Schema (#3716) * IS-72.00 Database Schema PSP_PIMS S72.00 Sprint 72 2024-Jan-16 - Altered tables: - PIMS_PROPERTY - PIMS_PROPERTY_HIST - Altered build scripts: - 002_PSP_PIMS_Build.sql - 020_DML_PIMS_STATIC_VARIABLE.sql - 148_DML_PIMS_DSP_CHKLST_ITEM_TYPE.sql - Added test scripts: - 071_PIMS_PROPERTY_CONTACT.sql - Altered test scripts: - 005_DML_LEASE_PROPERTY.sql - Requires additional metadata to meet standards * fixed build not matching alter up result * aligned latest folder --------- Co-authored-by: Manuel Rodriguez <[email protected]> * CI: Bump version to v5.0.0-72.8 * generated scaffold and removed lingering files (#3738) * CI: Bump version to v5.0.0-72.9 * Disposition checklist automation * fixed the disposition bug 7582 * CI: Bump version to v5.0.0-72.10 * Added Reordering of the DISPLAY_ORDER Columns (#3742) Reordered the display order following the insertion/deletion of the PIMS_DSP_CHKLST_ITEM_TYPE table. * CI: Bump version to v5.0.0-72.11 * PSP-7296 : Create/Edit Sales Details (Disposition) (#3731) * CI: Bump version to v5.0.0-72.12 * PSP-7639 PIMS HIST Table fix commit (#3743) Fix on missing Historical rows that were identified on 3 tables in UAT database 1.PIMS_INSURANCE 2.PIMS_LEASE_TENANT 3.PIMS_PROPERTY_LEASE * CI: Bump version to v5.0.0-72.13 * PSP-7509 : Allow Contractor to Create or Edit Disposition File (#3735) Co-authored-by: Eduardo Herrera <[email protected]> * CI: Bump version to v5.0.0-72.14 * Disposition File details, documents and notes. Refactoring on File Properties * PSP-7510 : Disallow Contractor to Remove Themself from Disposition File (#3745) Co-authored-by: Eduardo Herrera <[email protected]> * CI: Bump version to v5.0.0-72.15 * correct disposition list view file number. (#3736) * CI: Bump version to v5.0.0-72.16 * Offers and sales tab automation * disposition offers and sales tab * Psp 7614, 7657, 7626, 7610 assorted text corrections. (#3737) * text corrections. * correct title text. * psp-7657 fix modal buttons. * test updates. * lint fixes. * CI: Bump version to v5.0.0-72.17 * CI: Bump version to v5.0.0-72.18 * Refactoring and Disposition file automation * Merge conflicts between local and remote * Disposition files - Offers and sale create and update * Deleting AcquisitionProperties class, replaced by SharedFileProperties * Text changes (#3752) * CI: Bump version to v5.0.0-72.19 * correct message text. (#3749) * CI: Bump version to v5.0.0-72.20 * Refactoring LeaseTenants class * CI: Bump version to v5.0.0-72.21 * PSP-6668 FT: Acquisition File: Compensation: Show error modal instead of toast when total allowable compensation value is less than total compensation value (#3755) * Show modal popup with warning message for total allowable compensation * Test corrections * CI: Bump version to v5.0.0-72.22 * PSP-7664 FT-REG: When cancelling, the confirmation modal should be consistent around different types of files (#3747) * Increase the default width of modals * Rename file * Refactor duplicate code * Verbiage update * Update snapshots * Text updates * Test corrections * CI: Bump version to v5.0.0-72.23 * psp-7680 add agreement status type to agreement export. (#3751) * add agreement status type to agreement export. * test corrections. * CI: Bump version to v5.0.0-72.24 * Pipeline updates (#3756) * add keycloak sync to all pipelines. add uat hotfix and pre-release hotfix pipelines. update builds/keycloak sync to support new configuration. * correct uat secret name * add prod secrets for keycloak sync * correct typo. * CI: Bump version to v5.0.0-72.25 --------- Signed-off-by: devinleighsmith <[email protected]> Co-authored-by: Sue Tairaku <[email protected]> Co-authored-by: Sue Tairaku <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: praveen <[email protected]> Co-authored-by: devinleighsmith <[email protected]> Co-authored-by: Doug Filteau <[email protected]> Co-authored-by: Manuel Rodriguez <[email protected]> Co-authored-by: pravkuma14 <[email protected]> Co-authored-by: Eduardo <[email protected]> Co-authored-by: ap-quartech <[email protected]> Co-authored-by: Eduardo Herrera <[email protected]>

Eduardo Herrera added 8 commits January 12, 2024 11:34

PSP-7296 : Create/Edit Sales Details (Disposition)

f0b82c4

- updates

22bab57

- css fixes

924c1c7

- styling fixes

43c16ed

- test updates

b5a75ce

- linting

1fbd115

Merge branch 'dev' into psp-7296

3cab88b

eddherrera added the enhancement New feature or request label Jan 19, 2024

eddherrera requested a review from devinleighsmith January 19, 2024 16:57

eddherrera self-assigned this Jan 19, 2024

- add confirmation for clearing GST

214137d

devinleighsmith reviewed Jan 22, 2024

View reviewed changes

devinleighsmith requested changes Jan 22, 2024

View reviewed changes

Eduardo Herrera added 2 commits January 22, 2024 14:44

Merge branch 'dev' into psp-7296

276ad39

- updates

cefc418

FuriousLlama reviewed Jan 24, 2024

View reviewed changes

FuriousLlama requested changes Jan 24, 2024

View reviewed changes

Eduardo Herrera added 2 commits January 24, 2024 12:49

- updates

9828c1b

- updated to new API models

76a2207

eddherrera requested a review from FuriousLlama January 24, 2024 23:53

Eduardo Herrera added 2 commits January 24, 2024 15:57

Merge branch 'dev' into psp-7296

1f117ad

- test updates

4137221

- updates

d86a132

Merge branch 'dev' into psp-7296

305ea7a

- update

5a0dafc

FuriousLlama reviewed Jan 25, 2024

View reviewed changes

- updated

78a7b43

eddherrera requested a review from FuriousLlama January 26, 2024 00:19

FuriousLlama approved these changes Jan 26, 2024

View reviewed changes

eddherrera merged commit 1434326 into bcgov:dev Jan 26, 2024
9 checks passed

eddherrera deleted the psp-7296 branch January 26, 2024 01:07

devinleighsmith mentioned this pull request Aug 19, 2024

Pull requests report(19/08/2024 21:02) #4274

Open


		return new JsonResult(_mapper.Map<DispositionFileSaleModel>(updatedSale));
		}

PSP-7296 : Create/Edit Sales Details (Disposition) #3731

PSP-7296 : Create/Edit Sales Details (Disposition) #3731

Conversation

eddherrera commented Jan 19, 2024

github-actions bot commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 19, 2024

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 Jan 22, 2024

devinleighsmith Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinleighsmith commented Jan 22, 2024

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 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2024

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

FuriousLlama left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024

github-actions bot commented Jan 25, 2024

github-actions bot commented Jan 25, 2024

github-actions bot commented Jan 25, 2024

github-actions bot commented Jan 25, 2024

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

github-actions bot commented Jan 26, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

devinleighsmith Jan 22, 2024 •

edited

Loading