[HOLD for payment 2024-12-11] [$250] Remove pinned SelfDM for new users with "introSelected": "newDotManageTeam"

[danielrvidal]

Proposal: Remove pinned SelfDM for new users with "introSelected": "newDotManageTeam" (slack)

Background: We pinned the SelfDM to all user's cases, thinking it might give them a place to make notes and try submitting an expense to themselves before someone else. I think we left the SelfDM pinned for all users by default to see if it causes issues while seeing if some people might use it as intended. But I don’t think we’ve really seen that.

Problem: The SelfDM adds noise and confusion to users looking to get their team set up

Solution: Remove SelfDM from being pinned for users with this introSelection so they don’t see it unless they track an expense.

Below is what the LHN looks like today. We would not pin or show the SelfDM going forward.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021860980169165781817
• Upwork Job ID: 1860980169165781817
• Last Price Increase: 2024-11-25

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021860980169165781817

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)

[twilight2294]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove pinned SelfDM for new users with "introSelected": "newDotManageTeam"

What is the root cause of that problem?

Improvement

What changes do you think we should make in order to solve the problem?

1. We would need to introduce new prop selfDMReportID to completeOnboarding:
   

   App/src/libs/actions/Report.ts

   Line 3892 in 51e25af

   function completeOnboarding(

2. We would also need to pass this prop to prepareOnboardingOptimisticData:
   

   App/src/libs/actions/Report.ts

   Line 3473 in 51e25af

   function prepareOnboardingOptimisticData(

3. Then In BaseOnboardingAccounting:
   

   App/src/pages/OnboardingAccounting/BaseOnboardingAccounting.tsx

   Line 38 in 51e25af

   function BaseOnboardingAccounting({shouldUseNativeStyles, route}: BaseOnboardingAccountingProps) {

we will get the self-dm reportID as follows and pass it to completeOnboarding here:

                    const selfDMReportID = useMemo(() => ReportUtils.findSelfDMReportID(), []);
                    
                    Report.completeOnboarding(
                        onboardingPurposeSelected,
                        CONST.ONBOARDING_MESSAGES[onboardingPurposeSelected],
                        undefined,
                        undefined,
                        onboardingAdminsChatReportID ?? undefined,
                        onboardingPolicyID,
                        undefined,
                        onboardingCompanySize,
                        userReportedIntegration,
                        undefined,
                        selfDMReportID,
                    );

Then in prepareOnboardingOptimisticData we need to optimistically set the isPinned to false in case the introSelected is newDotManageTeam:

    if(engagementChoice === CONST.ONBOARDING_CHOICES.MANAGE_TEAM) {
        optimisticData.push(
            {
                onyxMethod: Onyx.METHOD.MERGE,
                key: `${ONYXKEYS.COLLECTION.REPORT}${selfDMReportID}`,
                value: {
                    isPinned: false,
                },
            },
        )
    }

Note that BE changes will also be required in this case.

One question:

It would look a little weird that first we have the self-dm pinned and then if the choice is manage team we remove it, one thing i would propose is that when the account is created set isPinned to false while returning from backend, and then in prepareOnboardingOptimisticData we can check if engagementChoice !== CONST.ONBOARDING_CHOICES.MANAGE_TEAM and if this is true then set isPinned to true.

Note that this is only a suggestion, I can work with the originally expected result too

[twilight2294]

@JmillsExpensify can you please read the one question I wrote in my proposal and give feedback?

Note

@suneox just noting that the basic idea is the same whether you pass a prop or do it in the function itself, All the core idea is there in my proposal, also from #52409 (comment) , I hope the same logic is applied here, so i hope you will be fair while selecting the proposal as you were in that other issue🙏

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-11-25 14:44:40 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove pinned SelfDM for new users with "introSelected": "newDotManageTeam"

What is the root cause of that problem?

We don't update the selfDM to unpinned after the onboarding is complete and backend also doesn't update it.

What changes do you think we should make in order to solve the problem?

In prepareOnboardingOptimisticData, we need to update selfDM to unpin in optimisticData and reset it in failureData if the engagementChoice is newDotManageTeam. We also need the same update on backend side in CompleteGuidedSetup API

const selfDMReportID = ReportUtils.findSelfDMReportID();
const selfDMReport = ReportConnection.getReport(selfDMReportID ?? '-1');

if (engagementChoice === CONST.ONBOARDING_CHOICES.MANAGE_TEAM) {
    optimisticData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${selfDMReportID}`,
        value: {
            isPinned: false
        }
    })

    failureData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${selfDMReportID}`,
        value: {
            isPinned: selfDMReport?.isPinned
        }
    })
}

App/src/libs/actions/Report.ts

Line 3474 in ba65563

function prepareOnboardingOptimisticData(

We need to always do this in prepareOnboardingOptimisticData because we also have another case completeOnboarding here with onboardingPurpose is newDotManageTeam here. This is the case that the new user is invited to a workspace before the first time login. In this case, the normal onboarding flow is skipped and the first time the user selects a payment method, the onboarding is completed in completePaymentOnboarding

App/src/libs/actions/IOU.ts

Line 7415 in ba65563

function completePaymentOnboarding(paymentSelected: ValueOf<typeof CONST.PAYMENT_SELECTED>, adminsChatReportID?: string, onboardingPolicyID?: string) {

What alternative solutions did you explore? (Optional)

We can unpin the selfDM by default from backend side and then update it to pinned if we select an onboardingPurpose isn't newDotManageTeam.

[suneox]

Thank you for all the proposals.

We have a different solution from @twilight2294 proposal, which only handle use case complete OnboardingAccounting by adding a new prop. However, there are several other use cases, such as updating onboardingPurpose when completing OnboardingPersonalDetails or PaymentOnboarding at KYCWall.

@nkdengineer proposal identifies these additional use cases and suggests always retrieving the value from prepareOnboardingOptimisticData instead of passing props. Additionally, this proposal also handles reverting the value in failureData, which is LGTM.

Overall, we can proceed with @nkdengineer proposal.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[danielrvidal]

Thank you team! Looking forward to the PRs!

[MonilBhavsar]

Looks good to me.
Once we have App draft up. I'll test backend PR and submit it for review

[melvin-bot]

❌ There was an error making the offer to @suneox for the Reviewer role. The BZ member will need to manually hire the contributor.

[melvin-bot]

❌ There was an error making the offer to @nkdengineer for the Contributor role. The BZ member will need to manually hire the contributor.

[danielrvidal]

@mallenexpensify something didn't work with the roles. Do you mind fixing?

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[trjExpensify]

Yo yo! Noticed this in a FullStory session. I can reproduce it reliably, see below:

2024-11-29_14-37-43.mp4

The selfDM appears, and then disappears which is pretty jarring. Is that because the auth PR is merged, but the frontend PR is still outstanding?

[MonilBhavsar]

Yes, App PR will fix this issue

[trjExpensify]

Great stuff, thanks for confirming!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.70-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: remove pinned SelfDM for new users #53193

If no regressions arise, payment will be issued on 2024-12-11. 🎊

For reference, here are some details about the assignees on this issue:

• @suneox requires payment (Needs manual offer from BZ)
• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot]

@suneox @mallenexpensify @suneox The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[flaviadefaria]

Hey team, migrated users are still having the self-DM pinned when this change should also have accounted for them. Do we need to update the logic?

[suneox]

Hey team, migrated users are still having the self-DM pinned when this change should also have accounted for them. Do we need to update the logic?

I think we can incorporate the update logic into issue #53405.

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @suneox paid $(AMOUNT) via Upwork (LINK)
• ROLE: @nkdengineer paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1860980169165781817/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[suneox]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other: Improvement

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other: All enviroment

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: This is an improvement, so we don’t have an offending PR.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
  N/A: It isn't an impactful bug

[suneox]

Thank you! I’ve accepted an offer.

[nkdengineer]

@mallenexpensify TY, I applied to that job

[mallenexpensify]

Contributor: @nkdengineer paid $250 via Upwork
Contributor+: @suneox paid $250 via Upwork.

@nkdengineer Please accept the job and reply here once you have?
upwork.com/jobs/~021860980169165781817

[nkdengineer]

@mallenexpensify Done!

[mallenexpensify]

@nkdengineer paid, payment breakdown updated above.

I created a testcase and provided the reasoning below. We'll see what QA wants to do, if anything

This one's a lil weird, I'm unsure if we need a test case for it but.. if this feature stops working correctly, we'd like to know. IF there's not a test case then someone would have to report. ¯_(ツ)_/¯