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

Jump to bottom

Update report option when personal detail is changed #40167

Merged
merged 11 commits into from
Apr 29, 2024
Merged

Update report option when personal detail is changed #40167

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
26810a9
Update report option when personal detail is changed
nkdengineer Apr 12, 2024
7f41cf8
remove unnecessary change
nkdengineer Apr 12, 2024
8cef454
fix selfDM case
nkdengineer Apr 12, 2024
16603a0
Merge branch 'main' into fix/39607
nkdengineer Apr 16, 2024
2cfca31
fix lint
nkdengineer Apr 16, 2024
f3abe81
run prettier
nkdengineer Apr 16, 2024
65b9aa3
Merge branch 'main' into fix/39607
nkdengineer Apr 17, 2024
e351997
safely get personal detail
nkdengineer Apr 17, 2024
1fecd97
update to use usePrevious hook
nkdengineer Apr 17, 2024
9f166ff
Merge branch 'main' into fix/39607
nkdengineer Apr 22, 2024
d726f3f
add explaination comment
nkdengineer Apr 22, 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
41 changes: 40 additions & 1 deletion src/components/OptionListContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as OptionsListUtils from '@libs/OptionsListUtils';
import type {OptionList} from '@libs/OptionsListUtils';
import * as ReportUtils from '@libs/ReportUtils';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Report} from '@src/types/onyx';
import type {PersonalDetails, Report} from '@src/types/onyx';
import {usePersonalDetails} from './OnyxProvider';

type OptionsListContextProps = {
Expand Down Expand Up @@ -37,6 +37,12 @@ const OptionsListContext = createContext<OptionsListContextProps>({
areOptionsInitialized: false,
});

const isEqualPersonalDetail = (prevPersonalDetail: PersonalDetails | null, personalDetail: PersonalDetails | null) =>
prevPersonalDetail?.firstName === personalDetail?.firstName &&
prevPersonalDetail?.lastName === personalDetail?.lastName &&
prevPersonalDetail?.login === personalDetail?.login &&
prevPersonalDetail?.displayName === personalDetail?.displayName;

function OptionsListContextProvider({reports, children}: OptionsListProviderProps) {
const areOptionsInitialized = useRef(false);
const [options, setOptions] = useState<OptionList>({
Expand All @@ -45,6 +51,7 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp
});

const personalDetails = usePersonalDetails();
const prevPersonalDetails = useRef(personalDetails);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we replace this by our existing hook usePrevious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hungvu193 I tested and we can use usePrevious hook. Updated to use this hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, let's me test again!

const prevReports = usePrevious(reports);

/**
Expand Down Expand Up @@ -108,14 +115,46 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp
return;
}

const newReportOptions: Array<{
replaceIndex: number;
newReportOption: OptionsListUtils.SearchOption<Report>;
}> = [];

Object.keys(personalDetails).forEach((accoutID) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up, this caused the regression #41275

(Also, accountID 😄)

Copy link
Contributor

@hungvu193 hungvu193 May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for handling it while I'm AFK, that's weird that I already tested logged out case but couldn't see the crash.
I should have tested it more careful next time!

const prevPersonalDetail = prevPersonalDetails.current?.[accoutID];
const personalDetail = personalDetails?.[accoutID];

if (isEqualPersonalDetail(prevPersonalDetail, personalDetail)) {
return;
}

Object.values(reports ?? {})
.filter((report) => Boolean(report?.participantAccountIDs?.includes(Number(accoutID))) || (ReportUtils.isSelfDM(report) && report?.ownerAccountID === Number(accoutID)))
.forEach((report) => {
if (!report) {
return;
}
const newReportOption = OptionsListUtils.createOptionFromReport(report, personalDetails);
const replaceIndex = options.reports.findIndex((option) => option.reportID === report.reportID);
newReportOptions.push({
newReportOption,
replaceIndex,
});
});
});

// since personal details are not a collection, we need to recreate the whole list from scratch
const newPersonalDetailsOptions = OptionsListUtils.createOptionList(personalDetails).personalDetails;

setOptions((prevOptions) => {
const newOptions = {...prevOptions};
newOptions.personalDetails = newPersonalDetailsOptions;
newReportOptions.forEach((newReportOption) => (newOptions.reports[newReportOption.replaceIndex] = newReportOption.newReportOption));
return newOptions;
});

prevPersonalDetails.current = personalDetails;
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

what other dependencies is this expecting that we're purposefully ignoring? it looks reports?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nkdengineer can you help me understand why we added this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bondydaa In this case we only want to update the personal detail option and the related report of the changed personal detail. So we're ignoring other dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, can you add that as a comment here then as well so future us can know exactly why/which dependencies we are okay with ignoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

little bump @nkdengineer on above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bondydaa I added a comment, please help to check again.
cc @hungvu193

Copy link
Contributor

Choose a reason for hiding this comment

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

little bump @bondydaa for final review

}, [personalDetails]);

const loadOptions = useCallback(() => {
Expand Down
Loading