psp-8905 | Contact summary model standarization #4394
Conversation
FuriousLlama
changed the title
FuriousLlama
added
enhancement
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
3 similar comments
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
| @@ -1237,17 +1237,13 @@ exports[`Contact List View > matches snapshot 1`] = ` | |||
| role="cell" | |||
| style="box-sizing: border-box; flex: 60 0 auto; min-width: 30px; width: 60px; justify-content: left; text-align: left; flex-wrap: wrap; align-items: center; display: flex;" | |||
| title="" | |||
| > | |||
| first | |||
There was a problem hiding this comment.
It looks like the test may be failing your new isPersonSummary test and returning an empty string. Can you please fix the test model so that it is not ignored (and potentially add an invalid model for testing that does return an empty result).
| return stakeholder; | ||
| }) ?? []; | ||
| const formTenants = tenantsWithPersons?.map(t => new FormStakeholder(undefined, t)) ?? []; | ||
| const stakeholderType = isPayableLease ? 'OWNER' : 'TEN'; |
There was a problem hiding this comment.
I think this may cause issues if a user has owners/tenants and then swaps the lease from payable to not-payable.
Just a note - let's be careful about the scope of these "refactors" - if we don't have existing tests, and the refactor doesn't include tests to validate the before/after functionality, than these are code changes and not refactors, since we are altering functionality. That is potentially fine - but we need to make sure that the FT team is aware of the scope of these changes - otherwise when this is merged the FT team will not know what to re-test. (this story is psp-8905, which talks about contact summary model changes, I don't think it would be obvious to the FT team that lease stakeholder functionality would be affected).
There was a problem hiding this comment.
I agree that refactors should be minimal. In this case, the refactor exposed an improper use of a common model for something that is quite specific (the lease tenants). In an ideal world, the code would have been separated making the changes here much smaller. Fixing this is part of maintaining the code base and cleaning shortcuts that where taken at the time (tech debt). Also, all code changes introduce some risk into the application, and no changes make the application harder to enhance in the future, so there is always a tradeoff.
There was a problem hiding this comment.
Fair enough, the point remains about the testing team, please make sure the scope of the changes included in this story are clear.
|
|
||
| this.summary = selectedContactModel.summary; | ||
| this.email = selectedContactModel.email; | ||
| this.mailingAddress = { streetAddress1: selectedContactModel.mailingAddress } as FormAddress; | ||
| this.municipalityName = selectedContactModel?.municipalityName; | ||
| this.note = selectedContactModel.note; | ||
| this.note = undefined; |
There was a problem hiding this comment.
Hmm, can you help me understand this change? Wouldn't these values already be undefined? Why were we populating these fields previously but not any more?
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
1 similar comment
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4394 |
No description provided.