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

psp-8905 | Contact summary model standarization #4394

Merged
merged 3 commits into from
Oct 24, 2024
Merged

psp-8905 | Contact summary model standarization #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4545ebe
Updated backend and frontend to use the concept model for contact sum…
FuriousLlama Oct 11, 2024
725cfae
Merge branch 'dev' into features/psp-8905
FuriousLlama Oct 11, 2024
ab5bab9
Merge branch 'dev' into features/psp-8905
asanchezr Oct 24, 2024
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
1 change: 1 addition & 0 deletions source/backend/api/Areas/Contacts/Controllers/SearchController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Pims.Api.Helpers.Exceptions;
using Pims.Api.Helpers.Extensions;
using Pims.Api.Models.Base;
using Pims.Api.Models.Concepts.Contact;
using Pims.Api.Policies;
using Pims.Api.Services.Interfaces;
using Pims.Dal.Entities.Models;
Expand Down
7 changes: 3 additions & 4 deletions ...eas/Contacts/Mapping/Search/ContactMap.cs → ...els/Models/Concepts/Contact/ContactMap.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
using Mapster;
using Entity = Pims.Dal.Entities;
using Model = Pims.Api.Areas.Contact.Models.Search;

namespace Pims.Api.Areas.Contact.Mapping.Search
namespace Pims.Api.Models.Concepts.Contact
{
public class ContactMap : IRegister
public class ContactSummaryMap : IRegister
{
public void Register(TypeAdapterConfig config)
{
config.NewConfig<Entity.PimsContactMgrVw, Model.ContactSummaryModel>()
config.NewConfig<Entity.PimsContactMgrVw, ContactSummaryModel>()
.Map(dest => dest.Id, src => src.Id)
.Map(dest => dest.PersonId, src => src.PersonId)
.Map(dest => dest.Person, src => src.Person)
Expand Down
14 changes: 6 additions & 8 deletions ...acts/Models/Search/ContactSummaryModel.cs → ...s/Concepts/Contact/ContactSummaryModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
namespace Pims.Api.Areas.Contact.Models.Search
using Pims.Api.Models.Concepts.Organization;
using Pims.Api.Models.Concepts.Person;

namespace Pims.Api.Models.Concepts.Contact
{
public class ContactSummaryModel
{
Expand All @@ -14,19 +17,14 @@ public class ContactSummaryModel
/// </summary>
public long? PersonId { get; set; }

public Pims.Api.Models.Concepts.Person.PersonModel Person { get; set; }
public PersonModel Person { get; set; }

/// <summary>
/// get/set - The primary key to identify the organization.
/// </summary>
public long? OrganizationId { get; set; }

public Pims.Api.Models.Concepts.Organization.OrganizationModel Organization { get; set; }

/// <summary>
/// get/set - The concurrency row version.
/// </summary>
public long RowVersion { get; set; }
public OrganizationModel Organization { get; set; }

/// <summary>
/// get/set - Either the person name or the organization name.
Expand Down
8 changes: 5 additions & 3 deletions source/frontend/src/components/contact/ContactManagerView/ContactManagerView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { defaultFilter } from './ContactFilterComponent/ContactFilterComponent';
import ContactManagerView from './ContactManagerView';
import { MockedFunction } from 'vitest';
import { MaybeMocked } from '@vitest/spy';
import { ApiGen_Concepts_ContactSummary } from '@/models/api/generated/ApiGen_Concepts_ContactSummary';
import { getEmptyContactSummary } from '@/mocks/contacts.mock';

// mock auth library

Expand All @@ -44,7 +46,7 @@ const setup = (renderOptions: RenderOptions = {}) => {
return { searchButton, ...utils };
};

const setupMockSearch = (searchResults?: IContactSearchResult[]) => {
const setupMockSearch = (searchResults?: ApiGen_Concepts_ContactSummary[]) => {
const results = searchResults ?? [];
const len = results.length;
getContacts.mockResolvedValue({
Expand All @@ -58,7 +60,8 @@ const setupMockSearch = (searchResults?: IContactSearchResult[]) => {
});
};

const defaultPersonSearchResult: IContactSearchResult = {
const defaultPersonSearchResult: ApiGen_Concepts_ContactSummary = {
...getEmptyContactSummary(),
id: '1',
summary: 'summary',
mailingAddress: '123 mock st',
Expand All @@ -68,7 +71,6 @@ const defaultPersonSearchResult: IContactSearchResult = {
municipalityName: 'city',
provinceState: 'province',
isDisabled: false,
provinceStateId: 0,
};

const defaultPagedFilter = {
Expand Down
7 changes: 4 additions & 3 deletions source/frontend/src/components/contact/ContactManagerView/ContactManagerView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import { Claims } from '@/constants/claims';
import { useApiContacts } from '@/hooks/pims-api/useApiContacts';
import { useKeycloakWrapper } from '@/hooks/useKeycloakWrapper';
import { useSearch } from '@/hooks/useSearch';
import { IContactSearchResult } from '@/interfaces/IContactSearchResult';
import { fromContactSummary, IContactSearchResult } from '@/interfaces';
import { ApiGen_Concepts_ContactSummary } from '@/models/api/generated/ApiGen_Concepts_ContactSummary';

import {
ContactFilterComponent,
Expand Down Expand Up @@ -70,7 +71,7 @@ const ContactManagerView = ({
setCurrentPage,
setPageSize,
loading,
} = useSearch<IContactSearchResult, IContactFilter>(
} = useSearch<ApiGen_Concepts_ContactSummary, IContactFilter>(
initialFilter,
getContacts,
'Search returned no results',
Expand Down Expand Up @@ -110,7 +111,7 @@ const ContactManagerView = ({
<div>
<ContactResultComponent
loading={loading}
results={results}
results={results.map(fromContactSummary)}
sort={sort}
pageSize={pageSize}
pageIndex={currentPage}
Expand Down
5 changes: 4 additions & 1 deletion ...ponents/contact/ContactManagerView/ContactResultComponent/ContactResultComponent.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ const defaultPersonSearchResult: IContactSearchResult = {
municipalityName: 'city',
provinceState: 'province',
isDisabled: false,
provinceStateId: 0,
personId: 1,
person: null,
middleNames: null,
organizationName: null,
};

const mockResults: IContactSearchResult[] = [
Expand Down
1 change: 0 additions & 1 deletion ...c/components/contact/ContactManagerView/ContactResultComponent/ContactResultComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export interface IContactResultComponentProps {

/**
* Display a react table with the results filtered by the {@link ContactFilter}
* @param {IContactSearchResult} props
*/
export function ContactResultComponent(props: IContactResultComponentProps) {
const { results, sort = {}, setSort, setPageSize, setPageIndex, ...rest } = props;
Expand Down
4 changes: 2 additions & 2 deletions source/frontend/src/components/contact/ContactManagerView/ContactResultComponent/columns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { InlineFlexDiv } from '@/components/common/styles';
import { ColumnWithProps } from '@/components/Table';
import { Claims } from '@/constants/claims';
import { useKeycloakWrapper } from '@/hooks/useKeycloakWrapper';
import { IContactSearchResult, isPersonResult } from '@/interfaces';
import { IContactSearchResult, isPersonSummary } from '@/interfaces';
import { stringToFragment } from '@/utils';

const columns: ColumnWithProps<IContactSearchResult>[] = [
Expand All @@ -35,7 +35,7 @@ const columns: ColumnWithProps<IContactSearchResult>[] = [
width: 20,
maxWidth: 20,
Cell: (props: CellProps<IContactSearchResult>) =>
isPersonResult(props.row.original) ? (
isPersonSummary(props.row.original) ? (
<StatusIndicators className={props.row.original.isDisabled ? 'inactive' : 'active'}>
<FaRegUser size={20} />
</StatusIndicators>
Expand Down
4 changes: 2 additions & 2 deletions ...ntend/src/components/contact/ContactManagerView/ContactResultComponent/summaryColumns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { FaRegBuilding, FaRegUser } from 'react-icons/fa';
import { CellProps } from 'react-table';

import { ColumnWithProps } from '@/components/Table';
import { IContactSearchResult, isPersonResult } from '@/interfaces';
import { IContactSearchResult, isPersonSummary } from '@/interfaces';
import { isValidId } from '@/utils';

const summaryColumns: ColumnWithProps<IContactSearchResult>[] = [
Expand All @@ -28,7 +28,7 @@ const summaryColumns: ColumnWithProps<IContactSearchResult>[] = [
width: 80,
maxWidth: 120,
Cell: (props: CellProps<IContactSearchResult>) =>
isPersonResult(props.row.original) ? (
isPersonSummary(props.row.original) ? (
<strong>{props.row.original.firstName + ' ' + props.row.original.surname}</strong>
) : (
<span></span>
Expand Down
4 changes: 3 additions & 1 deletion source/frontend/src/features/contacts/list/ContactListView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ const defaultPersonSearchResult: IContactSearchResult = {
municipalityName: 'city',
provinceState: 'province',
isDisabled: false,
provinceStateId: 0,
person: null,
middleNames: null,
organizationName: null,
};

const defaultPagedFilter = {
Expand Down
8 changes: 2 additions & 6 deletions source/frontend/src/features/contacts/list/__snapshots__/ContactListView.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
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 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).

</div>
/>
<div
class="td"
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=""
>
last
</div>
/>
<div
class="td"
role="cell"
Expand Down
30 changes: 21 additions & 9 deletions .../src/features/leases/detail/LeasePages/stakeholders/AddLeaseStakeholderContainer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Claims } from '@/constants';
import { LeaseContextProvider } from '@/features/leases/context/LeaseContext';
import { LeaseFormModel } from '@/features/leases/models';
import { useApiContacts } from '@/hooks/pims-api/useApiContacts';
import { IContactSearchResult } from '@/interfaces';
import { IContactSearchResult, fromContactSummary } from '@/interfaces';
import {
getEmptyPerson,
getMockContactOrganizationWithMultiplePeople,
Expand Down Expand Up @@ -127,7 +127,9 @@ describe('AddLeaseTenantContainer component', () => {
await setup({});

await act(async () => {
viewProps.setSelectedStakeholders([getMockContactOrganizationWithOnePerson()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithOnePerson()),
]);
});
expect(getPersonConcept).toHaveBeenCalledTimes(1);
expect(viewProps.selectedStakeholders[0].organizationPersons).toHaveLength(1);
Expand All @@ -138,8 +140,8 @@ describe('AddLeaseTenantContainer component', () => {

await waitFor(() => {
viewProps.setSelectedStakeholders([
getMockContactOrganizationWithOnePerson(),
getMockContactOrganizationWithOnePerson(),
fromContactSummary(getMockContactOrganizationWithOnePerson()),
fromContactSummary(getMockContactOrganizationWithOnePerson()),
]);
expect(getPersonConcept).toHaveBeenCalledTimes(1);
expect(viewProps.selectedStakeholders[0].organizationPersons).toHaveLength(1);
Expand Down Expand Up @@ -182,14 +184,18 @@ describe('AddLeaseTenantContainer component', () => {

//setup
await waitFor(() => {
viewProps.setSelectedStakeholders([getMockContactOrganizationWithOnePerson()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithOnePerson()),
]);
expect(getPersonConcept).toHaveBeenCalledTimes(1);
expect(viewProps.selectedStakeholders).toHaveLength(1);
});

await act(async () => {
//act
viewProps.setSelectedStakeholders([getMockContactOrganizationWithOnePerson()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithOnePerson()),
]);
});
//assert
expect(getPersonConcept).toHaveBeenCalledTimes(1);
Expand All @@ -199,7 +205,9 @@ describe('AddLeaseTenantContainer component', () => {
await setup({});

await waitFor(() => {
viewProps.setSelectedStakeholders([getMockContactOrganizationWithMultiplePeople()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithMultiplePeople()),
]);
expect(viewProps.selectedStakeholders).toHaveLength(1);
});
await act(async () => {
Expand All @@ -215,7 +223,9 @@ describe('AddLeaseTenantContainer component', () => {
await setup({});

await act(async () => {
viewProps.setSelectedStakeholders([getMockContactOrganizationWithMultiplePeople()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithMultiplePeople()),
]);
});
expect(viewProps.selectedStakeholders).toHaveLength(1);
await act(async () => {
Expand Down Expand Up @@ -278,7 +288,9 @@ describe('AddLeaseTenantContainer component', () => {

//setup
await waitFor(() => {
viewProps.setSelectedStakeholders([getMockContactOrganizationWithMultiplePeople()]);
viewProps.setSelectedStakeholders([
fromContactSummary(getMockContactOrganizationWithMultiplePeople()),
]);
expect(viewProps.selectedStakeholders).toHaveLength(1);
expect(viewProps.saveCallback).not.toBeNull();
});
Expand Down
15 changes: 7 additions & 8 deletions ...ntend/src/features/leases/detail/LeasePages/stakeholders/AddLeaseStakeholderContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const AddLeaseStakeholderContainer: React.FunctionComponent<
const [handleSubmit, setHandleSubmit] = useState<(() => void) | undefined>(undefined);

const { getPersonConcept } = useApiContacts();
const { execute } = useApiRequestWrapper({
const { execute: executeGetPerson } = useApiRequestWrapper({
requestFunction: getPersonConcept,
requestName: 'get person by id',
});
Expand Down Expand Up @@ -114,7 +114,7 @@ export const AddLeaseStakeholderContainer: React.FunctionComponent<
);

// fetch any person ids that we do not have person information for.
const personQueries = unprocessedPersons.map(person => execute(person.personId));
const personQueries = unprocessedPersons.map(person => executeGetPerson(person.personId));
const personResponses = await Promise.all(personQueries);
const allPersons = personResponses.concat(processedPersons);

Expand All @@ -127,14 +127,13 @@ export const AddLeaseStakeholderContainer: React.FunctionComponent<
op.person = matchingPerson;
}
});
stakeholder.stakeholderType = stakeholder?.stakeholderType
? stakeholder.stakeholderType
: isPayableLease
? 'OWNER'
: 'TEN';
return stakeholder;
}) ?? [];
const formTenants = tenantsWithPersons?.map(t => new FormStakeholder(undefined, t)) ?? [];
const stakeholderType = isPayableLease ? 'OWNER' : 'TEN';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, the point remains about the testing team, please make sure the scope of the changes included in this story are clear.

const formTenants =
tenantsWithPersons?.map(
t => new FormStakeholder(undefined, { contact: t, stakeholderType }),
) ?? [];
setStakeholders([...formTenants, ...matchingExistingTenants]);
};

Expand Down
Loading
Loading