-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Suggested Follow-ups][R2] Disable onboarding completion list when landing in #admins #80707
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
[Suggested Follow-ups][R2] Disable onboarding completion list when landing in #admins #80707
Conversation
|
@mananjadhav 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] |
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4365ea2c
ℹ️ 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".
src/libs/ReportUtils.ts
Outdated
|
|
||
| const optimisticData: Array<TupleToUnion<typeof tasksForOptimisticData> | OnyxUpdate<typeof ONYXKEYS.COLLECTION.POLICY>> = [...tasksForOptimisticData]; | ||
| const lastVisibleActionCreated = welcomeSignOffCommentAction.created; | ||
| const lastVisibleActionCreated = shouldPostTasksInAdminsRoom ? textCommentAction.created : welcomeSignOffCommentAction.created; |
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.
Avoid updating lastVisibleActionCreated without an action
For Manage Team/Track Workspace signups that post in #admins (no '+' email), you now skip adding textCommentAction but still set lastVisibleActionCreated to textCommentAction.created. That makes the #admins report look updated/unread and can reorder the sidebar even though no visible action exists (the last message text remains the old one) until the backend posts something. Consider leaving lastVisibleActionCreated untouched (or tying it to a real action) when shouldPostTasksInAdminsRoom is true.
Useful? React with 👍 / 👎.
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.
I don't think that's confirmed by manual tests
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.
|
|
@mananjadhav do you have time do check this one on a short notice? |
|
I can help if urgent |
Lets give @mananjadhav 10 more minutes to react since it auto assigned to him, otherwise will appriciate your help @mkhutornyi 😃 Will have #80918 later on today as well if you want |
|
Thanks @mkhutornyi ! Yes would highly appreciate the quick help here as the last few days 🙏 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariBefore fix: before.movAfter fix: web.mov |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7e128e3a8
ℹ️ 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".
| const optimisticData: Array<TupleToUnion<typeof tasksForOptimisticData> | OnyxUpdate<typeof ONYXKEYS.COLLECTION.POLICY>> = [...tasksForOptimisticData]; | ||
| const lastVisibleActionCreated = welcomeSignOffCommentAction.created; | ||
| const lastVisibleActionCreated = shouldUseFollowupsInsteadOfTasks ? textCommentAction.created : welcomeSignOffCommentAction.created; | ||
| optimisticData.push( |
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.
Avoid updating lastVisibleActionCreated without an action
When shouldUseFollowupsInsteadOfTasks is true (admins room in non‑production), this path skips adding the text/sign‑off report actions, but still sets lastVisibleActionCreated to textCommentAction.created. That makes the admins report look like it has a new action even though no visible action exists, so the report can jump to the top/unread with an empty action list until backend followups arrive. Consider leaving lastVisibleActionCreated unchanged (or only updating it when you actually insert the corresponding report action) in the followups path.
Useful? React with 👍 / 👎.
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.
This makes sense to me since we don't create textCommentAction optimistically.
| it('should not add anything to guidedSetupData when posting into the admin room', async () => { | ||
| const adminsChatReportID = '1'; | ||
| // Not having `+` in the email allows for `isPostingTasksInAdminsRoom` flow | ||
| await Onyx.merge(ONYXKEYS.SESSION, {email: 'test@example.com'}); |
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.
Can we also add test case of email having +?
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.
added
| const {onboardingMessages} = getOnboardingMessages(); | ||
| const expectedManageTeamDefaultTasksCount = onboardingMessages[CONST.ONBOARDING_CHOICES.MANAGE_TEAM].tasks.length - 3; | ||
| // We do not pass tasks to `#admins` channel in favour of backed generated followup-list | ||
| const expectedManageTeamDefaultTasksCount = 0; |
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.
Can we also add test case of email having +?
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.
actually this test result will not be affected by + in the email. We are only checking for #admins channel actions that are empty either way now. Would skip adding this one, it will duplicate a lot of code :(
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #80690 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
marcochavezf
left a comment
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.
![]()
|
✋ 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/Beamanator in version: 9.3.11-16 🚀
|
Explanation of Change
This PR is a part of Suggested Follow-ups project.
It disables optimistic generation of onboarding messages and task list for the cases where the list was supposed to be posted in the
#adminsroom (MANAGE_TEAM and TRACK_WORKSPACE onboarding actions, except for emails that have a '+'.)This change is gated to NON PRODUCTION envs.
Fixed Issues
$ #80690
PROPOSAL:
Tests
This change is gated to NON PRODUCTION envs
+in itMANAGE_TEAMwhen prompted by onboarding modals#adminsor concierge chats.Offline tests
QA Steps
Same as tests.
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
0089.android.native.mov
Android: mWeb Chrome
0089.android.chrome.mov
iOS: Native
0089.ios.native.mov
iOS: mWeb Safari
0089.ios.safari.mov
MacOS: Chrome / Safari
0089.chrome.desktop.mov