remove REPORT_NAME_VALUE_PAIRS from Onyx.merge part 1#79028
remove REPORT_NAME_VALUE_PAIRS from Onyx.merge part 1#79028danieldoglas merged 12 commits intoExpensify:mainfrom
Conversation
|
@ZhenjaHorbach 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
97f396f to
f974722
Compare
|
@DylanDylann Pls review this PR. Thanks |
src/libs/OptionsListUtils/index.ts
Outdated
|
|
||
| // Set properties that are used in SearchOption context | ||
| result.private_isArchived = reportNameValuePairs?.private_isArchived; | ||
| result.private_isArchived = reportNameValuePairsForReport?.private_isArchived; |
There was a problem hiding this comment.
If we only use private_isArchived, let's use useArchivedReportsIdSet, it's designed for this case
There was a problem hiding this comment.
@DylanDylann, useArchivedReportsIdSet is used to get the set of archived report IDs, but in this case we just need 1 private_isArchived so I don't think we should use it. ANW, I think we can just pass the single private_isArchived intead of the whole reportNameValuePairs.
|
Product review not required. Removing myself and unsubscribing |
src/libs/ReportNameUtils.ts
Outdated
| allReportNameValuePairs?: OnyxCollection<ReportNameValuePairs>, | ||
| personalDetailsList?: PersonalDetailsList, | ||
| reportActions?: OnyxCollection<ReportActions>, | ||
| reportNameValuePair?: OnyxEntry<ReportNameValuePairs>, |
There was a problem hiding this comment.
I think passing the isArchived parameter is enough
| const participants = selectedParticipants.map((participant) => { | ||
| const participantAccountID = participant?.accountID ?? CONST.DEFAULT_NUMBER_ID; | ||
| return participantAccountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, reportAttributesDerived); | ||
| return participantAccountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, privateIsArchived, reportAttributesDerived); |
| return participant; | ||
| } | ||
| return participant.accountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, reportAttributesDerived, reportDrafts); | ||
| return participant.accountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, privateIsArchived, reportAttributesDerived, reportDrafts); |
|
@dukenv0307 Typescript check is failed |
|
@DylanDylann I addressed all your comments, it's ready for review again |
| const participants = selectedParticipants.map((participant) => { | ||
| const participantAccountID = participant?.accountID ?? CONST.DEFAULT_NUMBER_ID; | ||
| return participantAccountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, derivedReports); | ||
| return participantAccountID ? getParticipantsOption(participant, personalDetails) : getReportOption(participant, !!reportNameValuePairs?.private_isArchived, derivedReports); |
| type ReportNameValuePairs = OnyxCommon.OnyxValueWithOfflineFeedback<{ | ||
| /** Whether the report is an archived room */ | ||
| private_isArchived?: string; | ||
| private_isArchived?: string | boolean; |
There was a problem hiding this comment.
I don't think we should edit this type, private_isArchived in rNVP should be string (save the deleted time)
There was a problem hiding this comment.
Oh, I think it can be Boolean as defined in
App/src/libs/actions/Policy/Member.ts
Line 579 in 232cb11
and
App/src/libs/actions/Policy/Member.ts
Line 534 in 232cb11
There was a problem hiding this comment.
@dukenv0307 Hmmm, Could you check again the response from BE to confirm if it could be boolean or not? We should use same type from BE
There was a problem hiding this comment.
I think BE will return a string, but it's safe to be a boolean because:
- We already used it as I mentioned above
- We use
useArchivedReportsIdSetto get the list of reportID that are archived reports, so we can easily getprivate_isArchivedas a boolean.
What do you think?
There was a problem hiding this comment.
We already used it as I mentioned above
It's bad practice as I think. It may cause problems in the future that I also don't know 😄 Anw, Why do we need to do it here?
There was a problem hiding this comment.
@DylanDylann We used it here
In the downstream tasks, we just use it as a boolean (like !!result.private_isArchived), then we can leverage useArchivedReportsIdSet. If it must be a string, we need to create another selector to get private_isArchived
There was a problem hiding this comment.
I also think we need to use private_isArchived instead of a boolean flag in this case. Happy to wait for @tgolen's perspective on this before moving forward.
There was a problem hiding this comment.
We should be try to keep it as consistent as we can from the BE (makes all of our lives easier). I verified in the BE that the value is a timestamp, so let's try to keep it that way as much as we can.
|
@DylanDylann It's ready for review again |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-01-15.at.15.09.04.movAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 9.3.5-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.5-7 🚀
|
Explanation of Change
Refactor
getReportOptionandgetReportDisplayOptionto usereportNameValuePairfrom component instead of using it from Onyx.connectFixed Issues
$ #66380
PROPOSAL:
Tests
Test 1:
Test 2 (Native only)
Offline tests
QA Steps
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
Screen.Recording.2026-01-08.at.10.50.47.mov
Android: mWeb Chrome
Screen.Recording.2026-01-08.at.10.59.30.mov
iOS: Native
Screen.Recording.2026-01-08.at.10.44.18.mov
Screen.Recording.2026-01-08.at.10.47.58.mov
iOS: mWeb Safari
Screen.Recording.2026-01-08.at.10.59.51.mov
MacOS: Chrome / Safari
Screen.Recording.2026-01-08.at.10.59.14.mov