Refactor ONYXKEYS.NVP_ACTIVE_POLICY_ID in src/libs/OptionsListUtils.ts#70137
Refactor ONYXKEYS.NVP_ACTIVE_POLICY_ID in src/libs/OptionsListUtils.ts#70137parasharrajat wants to merge 4 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
DylanDylann
and removed request for
a team
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| const personalDetailsWithoutDMs = filteredPersonalDetailsOfRecentReports(options.recentReports, options.personalDetails); | ||
| const reportsAndPersonalDetails = options.recentReports.concat(personalDetailsWithoutDMs); | ||
| return orderOptions({recentReports: reportsAndPersonalDetails, personalDetails: []}, searchInputValue, orderReportOptionsConfig); | ||
| return orderOptions({recentReports: reportsAndPersonalDetails, personalDetails: []}, activePolicyID ?? '', searchInputValue, orderReportOptionsConfig); |
There was a problem hiding this comment.
Do not default string IDs to any value
There was a problem hiding this comment.
This parameter is required, so how do we manage that? I had to make this parameter required because of the different types this function support.
There was a problem hiding this comment.
Do not default string IDs to any value
This is lint error message 😄
There was a problem hiding this comment.
But as I said, the param to the function is required but the activePolicyID is optional so we need to make sure it is not undefined but the value of this param can be empty which is a fine case.
So to satisfy ts, we need to pass a default value.
There was a problem hiding this comment.
@DylanDylann What do you suggest on how we handle this?
Here are 2 variants of this function.
function orderOptions(options: ReportAndPersonalDetailOptions, activePolicyID?: string): ReportAndPersonalDetailOptions;
/**
* Sorts reports and personal details independently, but prioritizes the search value.
*/
function orderOptions(options: ReportAndPersonalDetailOptions, activePolicyID: string, searchValue: string, config?: OrderReportOptionsConfig): ReportAndPersonalDetailOptions;
function orderOptions(options: ReportAndPersonalDetailOptions, activePolicyID?: string, searchValue?: string, config?: OrderReportOptionsConfig): ReportAndPersonalDetailOptions {
There was a problem hiding this comment.
@parasharrajat Why do we need to make activePolicyID as required while It could be undefined?
There was a problem hiding this comment.
In the second function declaration above, third param searchValue is required so we have to make second param required and as we need activePolicyID in all 3 declarations, I have to position it as second param.
There was a problem hiding this comment.
Do you agree with this?
There was a problem hiding this comment.
@parasharrajat Sorry for the delay. Thanks for bump
There was a problem hiding this comment.
Initially, this function overload provided type safety and improved IntelliSense. However, in this PR, the returned type was unified across the overloads.
Because of that, the overload now seems redundant. I suggest removing it, which would also resolve the issue we’re facing here. Let me know what you think.
| const optionList = getAttendeeOptions(options.reports, options.personalDetails, betas, attendees, recentAttendees ?? [], iouType === CONST.IOU.TYPE.SUBMIT, true, false, action); | ||
| if (isPaidGroupPolicy) { | ||
| const orderedOptions = orderOptions(optionList, searchTerm, { | ||
| const orderedOptions = orderOptions(optionList, activePolicyID ?? '', searchTerm, { |
There was a problem hiding this comment.
Do not default string IDs to any value.
|
@parasharrajat Could you please fix failed GH actions? |
|
I will update this PR next week. |
|
@parasharrajat Any update? |
Co-authored-by: DylanDylann <141406735+DylanDylann@users.noreply.github.com>
|
@parasharrajat is this PR ready for review? |
|
Yes, it is ready. |
|
@parasharrajat Could you please merge latest main? I see there are lots of failed GH actions |
|
@parasharrajat Also please add QA steps |
|
I will update this PR soon. It might take 1-2 days due to some other things I am prioritizing over this. |
|
@parasharrajat Kindly bump |
|
Thanks for pjng. I will catch up here soon |
|
Going to break this PR into multiples. |
|
@parasharrajat Let's close this PR 😄 or make it draft |
2 participants
Explanation of Change
Fixed Issues
$ #66382
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop