[No QA] refactor createTransactionThreadReport to use 1 param#89517
Conversation
|
@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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03368aaf45
ℹ️ 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".
| let isTaskAutoCompleted: boolean = task.autoCompleted; | ||
|
|
||
| const onboardingSelfTourViewed = isSelfTourViewed ?? onboarding?.selfTourViewed; | ||
| const onboardingSelfTourViewed = isSelfTourViewed; |
There was a problem hiding this comment.
Preserve Onyx fallback for self-tour completion
prepareOnboardingOnyxData() now derives onboardingSelfTourViewed only from the isSelfTourViewed argument, but openReport() still allows callers to omit that field (OpenReportActionParams.isSelfTourViewed?), and several call sites do so (for example AuthScreensInitHandler calls openReport({reportID, introSelected, betas})). In those paths, isSelfTourViewed is undefined even when NVP_ONBOARDING.selfTourViewed is already true, so the VIEW_TOUR task is no longer auto-completed and users can see an incorrect pending onboarding task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ah we can't remove onboarding?.selfTourViewed since some places are still using it. I updated this PR to refactor them first
03368aa to
52d7f9b
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Reviewer Checklist
|
|
@mjasikowski looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @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. |
|
There was a checklist, I don't know why the emergency label was applied |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.73-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes required. This PR is a purely internal code refactoring that converts the |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.73-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.73-4 🚀
|
Explanation of Change
Fixed Issues
$ #66424
PROPOSAL:
Tests
Offline tests
QA Steps
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari