[No QA][Part 3g] Remove Onyx.connect from ModifiedExpenseMessage#83931
Conversation
melvin-bot
bot
requested review from
heyjennahay and
mjasikowski
and removed request for
a team
|
@mjasikowski 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/libs/ReportNameUtils.ts
Outdated
| reportPolicy: Policy | undefined, | ||
| parentReport: Report | undefined, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| _policyTags?: OnyxEntry<PolicyTagLists>, |
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
The _policyTags parameter is added to computeReportNameBasedOnReportAction but is never used inside the function body (it is prefixed with _ and guarded by an eslint-disable for @typescript-eslint/no-unused-vars). This function handles action types like SUBMITTED, FORWARDED, HOLD, UNHOLD, etc. -- none of which need policy tags. The policyTags data is only needed by computeChatThreadReportName, which already receives it separately in the computeReportName caller.
Remove _policyTags from computeReportNameBasedOnReportAction's signature and stop passing it from the callers:
function computeReportNameBasedOnReportAction(
translate: LocalizedTranslate,
parentReportAction: ReportAction | undefined,
report: Report | undefined,
reportPolicy: Policy | undefined,
parentReport: Report | undefined,
): string | undefined {In computeReportName, just remove policyTags from the computeReportNameBasedOnReportAction call:
const parentReportActionBasedName = computeReportNameBasedOnReportAction(translateLocal, parentReportAction, report, reportPolicy, parentReport);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| import EXPENSIFY_ICON_URL from '@assets/images/expensify-logo-round-clearspace.png'; | ||
| import * as AppUpdate from '@libs/actions/AppUpdate'; | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| import {translateLocal} from '@libs/Localize'; |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line @typescript-eslint/no-deprecated on the translateLocal import lacks a justification comment. Other newly added eslint-disables in this same PR include justification (e.g., -- Onyx.connectWithoutView is being removed in ...).
Add a justification explaining why the deprecated API is used:
// eslint-disable-next-line @typescript-eslint/no-deprecated -- translateLocal is deprecated; BrowserNotifications is non-React code that cannot use the translate hook
import {translateLocal} from '@libs/Localize';Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }: LocalNotificationModifiedExpensePushParams) { | ||
| const title = reportAction.person?.map((f) => f.text).join(', ') ?? ''; | ||
| const body = getForReportAction({ | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line @typescript-eslint/no-deprecated for the translate: translateLocal usage lacks a justification comment.
Add a justification:
// eslint-disable-next-line @typescript-eslint/no-deprecated -- translateLocal is deprecated; BrowserNotifications is non-React code that cannot use the translate hook
translate: translateLocal,Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/libs/ReportUtils.ts
Outdated
| const movedFromReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(parentReportAction, CONST.REPORT.MOVE_TYPE.FROM)}`]; | ||
| const movedToReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(parentReportAction, CONST.REPORT.MOVE_TYPE.TO)}`]; | ||
| const modifiedMessage = getForReportAction({ | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line @typescript-eslint/no-deprecated for the translate: translateLocal usage in getReportName lacks a justification comment. The other new eslint-disable in this file (line 1081) includes a justification with a link to the tracking issue.
Add a justification:
// eslint-disable-next-line @typescript-eslint/no-deprecated -- getReportName is itself deprecated; will be removed as part of the Onyx.connect migration
translate: translateLocal,Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
leshniak
changed the title
leshniak
changed the title
leshniak
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab483e744
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| policy, | ||
| movedFromReport, | ||
| movedToReport, | ||
| policyTags, | ||
| currentUserLogin: currentUserLogin ?? '', |
There was a problem hiding this comment.
This path now always uses getForReportAction with whatever policy, policyTags, and currentUserLogin were passed into getLastMessageTextForReport, but several options-list callers invoke this helper without those fields. In that common case, modified-expense previews lose workspace-specific tag labels and policy-aware attribution (e.g., policy-rules/MCC context) and fall back to generic text. The previous fallback branch (getForReportAction with policyID) still resolved that context from Onyx for these callers.
Useful? React with 👍 / 👎.
src/libs/ReportNameUtils.ts
Outdated
| reportAction: parentReportAction, | ||
| policyID, | ||
| movedFromReport, | ||
| movedToReport, | ||
| policyTags, | ||
| currentUserLogin: '', |
There was a problem hiding this comment.
The new call to getForReportAction omits policy and hardcodes currentUserLogin to an empty string, so policy-dependent branches in ModifiedExpenseMessage cannot run in thread-name generation. For modified-expense actions with policy-rules changes (and MCC attribution requiring admin context), this degrades the computed thread name to generic output instead of the detailed policy-aware message that the previous policyID-based path produced.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
leshniak
changed the title
leshniak
force-pushed
the
extract-remove-onyx-connect-final-cleanup
branch
from
2ab483e to
5cd0ea3
Compare
leshniak
force-pushed
the
extract-remove-onyx-connect-final-cleanup
branch
5 times, most recently
from
0909c1d to
c704eb3
Compare
leshniak
changed the title
leshniak
force-pushed
the
extract-remove-onyx-connect-final-cleanup
branch
from
c704eb3 to
ec241de
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
@leshniak sorry, forgot to merge and there's one more conflict |
|
@mjasikowski no worries, I'll resolve them tomorrow. |
Resolve conflicts adapting to upstream refactoring: - computeReportName now uses object params (ComputeReportName type) - computeChatThreadReportName gained currentUserLogin param; use it instead of '' - getAlternateText gained policy? param from upstream; keep our currentUserAccountID change - SearchFiltersChatsSelector: add policy extraction from allPolicies - reportAttributes: use object-param call syntax + allPolicyTags: policyTags Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@mjasikowski this should be merged after #84029 |
mjasikowski
changed the title
|
@leshniak if it needs to be merged after something else, it's always good to put |
… in Report/index.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
…sernotifications-chain' into extract-remove-onyx-connect-final-cleanup # Conflicts: # src/libs/Notification/LocalNotification/BrowserNotifications.ts # src/libs/OptionsListUtils/index.ts # src/libs/actions/Report/index.ts
leshniak
force-pushed
the
extract-remove-onyx-connect-final-cleanup
branch
from
5782819 to
d0d0c9c
Compare
leshniak
changed the title
|
I'll resolve conflicts tomorrow. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Tags and currentUserLogin Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eMessageTest Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Conflicts resolved. Also FYI @tgolen |
|
🚧 @mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ 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/mjasikowski in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
5 participants
Explanation of Change
Final cleanup step: removes the old
getForReportActionimplementation that relied on module-levelOnyx.connectWithoutViewsubscriptions, and promotesgetForReportActionTemptogetForReportAction.getForReportAction(~320 lines) that keptallPolicyTagsandstoredCurrentUserLoginviaOnyx.connectWithoutViewgetForReportActionTemp→getForReportActioneverywhere it is usedThis PR is part of the work split from #75562.
Fixed Issues
$#66336
PROPOSAL:
Tests
Offline tests
N/A — pure refactor, no network calls added or removed.
QA Steps
[No QA] — no user-visible behavior change. This is a pure internal cleanup that removes dead code.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
N/A — no UI changes
Android: mWeb Chrome
N/A — no UI changes
iOS: Native
N/A — no UI changes
iOS: mWeb Safari
N/A — no UI changes
MacOS: Chrome / Safari
N/A — no UI changes