-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Update report option when personal detail is changed #40167
Changes from 9 commits
26810a9
7f41cf8
8cef454
16603a0
2cfca31
f3abe81
65b9aa3
e351997
1fecd97
9f166ff
d726f3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = { | ||
|
@@ -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>({ | ||
|
@@ -45,6 +51,7 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp | |
}); | ||
|
||
const personalDetails = usePersonalDetails(); | ||
const prevPersonalDetails = usePrevious(personalDetails); | ||
const prevReports = usePrevious(reports); | ||
|
||
/** | ||
|
@@ -108,14 +115,45 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp | |
return; | ||
} | ||
|
||
const newReportOptions: Array<{ | ||
replaceIndex: number; | ||
newReportOption: OptionsListUtils.SearchOption<Report>; | ||
}> = []; | ||
|
||
Object.keys(personalDetails).forEach((accoutID) => { | ||
const prevPersonalDetail = prevPersonalDetails?.[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; | ||
}); | ||
|
||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nkdengineer can you help me understand why we added this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. little bump @nkdengineer on above comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bondydaa I added a comment, please help to check again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. little bump @bondydaa for final review |
||
}, [personalDetails]); | ||
|
||
const loadOptions = useCallback(() => { | ||
|
There was a problem hiding this comment.
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
😄)There was a problem hiding this comment.
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!