Skip to content
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

[Hold #41442] [Guided Setup Stage 3][$500] Don't create default #announce rooms until a workspace has 3 or more members #34929

Closed
JmillsExpensify opened this issue Jan 22, 2024 · 98 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Jan 22, 2024

Problem

While we originally envisioned default rooms to streamline the a whole host of team collaboration use cases – including announcements to your employees, or chatting with other admins - as we've gained experience onboarding users to NewDot we've realized that this has had the effect of making onboarding harder. More specifically, your LHN starts out with a host of rooms and messages, though it's not clear which you should start with first:

  • Expensify (planned)
  • Concierge
  • Workspace chat
  • #admins
  • #announce

Even further, plenty of sign-ups are first and foremost looking for an expense and financial management app, and they're just one person, so this ends up confusing them that they've downloaded the wrong app, costing us valuable bottom-up chances that we'd otherwise keep around as long as sign-ups can effectively find what they're looking for.

Solution

Part of this solution is more targeted onboarding, and that's already being worked on in #wave9-collect-signups. Though equally, let's not assume group collaboration is what sign-ups are after – especially if they're the only member of the workspace. A good example is the #announce room, which is only relevant when a workspace has several members.

Thus, in an effort to simplify what users are exposed to post-sign-up, and better target our functionality to their intended use case, let's:

  • Not create a default #announce room when the workspace is created
  • Instead, let's only create #announce rooms when the third member is added to the workspace (after all, otherwise two members could use a DM without issue).
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d20cdc896fd34ad2
  • Upwork Job ID: 1755964549843316736
  • Last Price Increase: 2024-02-16
  • Automatic offers:
    • rojiphil | Contributor | 0
@JmillsExpensify JmillsExpensify added Daily KSv2 NewFeature Something to build that is a new item. labels Jan 22, 2024
Copy link

melvin-bot bot commented Jan 22, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 22, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 31, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 1, 2024
@strepanier03 strepanier03 added Overdue NewFeature Something to build that is a new item. and removed NewFeature Something to build that is a new item. labels Feb 1, 2024
Copy link

melvin-bot bot commented Feb 1, 2024

@melvin-bot melvin-bot bot removed the Overdue label Feb 1, 2024
@anmurali anmurali added the External Added to denote the issue can be worked on by a contributor label Feb 9, 2024
@melvin-bot melvin-bot bot changed the title Don't create default #announce rooms until a workspace has 3 or more members [$500] Don't create default #announce rooms until a workspace has 3 or more members Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

Copy link

melvin-bot bot commented Feb 9, 2024

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

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Feb 9, 2024

Proposal

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

Don't create default #announce rooms until a workspace has 3 or more members

What is the root cause of that problem?

This is a new feature

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

When we create a policy we are getting optimistic workspace chats here:

} = ReportUtils.buildOptimisticWorkspaceChats(policyID, workspaceName);

we should remove this:

App/src/libs/actions/Policy.ts

Lines 1239 to 1253 in 6d30216

{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
value: {
pendingFields: {
addWorkspaceRoom: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
...announceChatData,
},
},
{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
value: announceReportActionData,
},

and this:

App/src/libs/actions/Policy.ts

Lines 1302 to 1320 in 6d30216

{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
value: {
pendingFields: {
addWorkspaceRoom: null,
},
pendingAction: null,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
value: {
[announceCreatedReportActionID]: {
pendingAction: null,
},
},
},

and this:

App/src/libs/actions/Policy.ts

Lines 1367 to 1376 in 6d30216

{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
value: null,
},
{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
value: null,
},

in https://github.com/Expensify/App/blob/main/src/libs/actions/Policy.ts we should create a createAnnounceRoom function that use the previous removed logic.

function createAnounceChat(policyName = '', policyID= ''): string {
    const workspaceName = policyName;

    const {
        announceChatReportID,
        announceChatData,
        announceCreatedReportActionID,
        announceReportActionData,
    } = ReportUtils.buildOptimisticAnnounceChat(policyID, workspaceName);

    const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: {
                pendingFields: {
                    addWorkspaceRoom: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
                },
                ...announceChatData,
            },
        },
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: announceReportActionData,
        },

    ];

    const successData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: {
                pendingFields: {
                    addWorkspaceRoom: null,
                },
                pendingAction: null,
            },
        },
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: {
                [announceCreatedReportActionID]: {
                    pendingAction: null,
                },
            },
        },


    ];

    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: null,
        },
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: null,
        },

    ];

    const params: CreateAnnounceChatParams = {
        policyID,
        announceChatReportID,
        announceCreatedReportActionID,

    };

    API.write(WRITE_COMMANDS.CREATE_WORKSPACE, params, {optimisticData, successData, failureData});

    return announceChatReportID;

}

we should also remove the logic of creating the announce room in buildOptimisticWorkspaceChats, here:

App/src/libs/ReportUtils.ts

Lines 3450 to 3468 in 6d30216

const announceChatData = buildOptimisticChatReport(
currentUserAccountID ? [currentUserAccountID] : [],
CONST.REPORT.WORKSPACE_CHAT_ROOMS.ANNOUNCE,
CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE,
policyID,
CONST.POLICY.OWNER_ACCOUNT_ID_FAKE,
false,
policyName,
undefined,
undefined,
// #announce contains all policy members so notifying always should be opt-in only.
CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY,
);
const announceChatReportID = announceChatData.reportID;
const announceCreatedAction = buildOptimisticCreatedReportAction(CONST.POLICY.OWNER_EMAIL_FAKE);
const announceReportActionData = {
[announceCreatedAction.reportActionID]: announceCreatedAction,
};

and create a new function buildOptimisticAnnounceRoom in https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.ts that implement the removed logic (returning announceChatReportID, announceChatData, announceReportActionData, announceCreatedReportActionID)

function buildOptimisticAnnounceChat(policyID: string, policyName: string): OptimisticAnnounceChat {
        const announceChatData = buildOptimisticChatReport(
            currentUserAccountID ? [currentUserAccountID] : [],
            CONST.REPORT.WORKSPACE_CHAT_ROOMS.ANNOUNCE,
            CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE,
            policyID,
            CONST.POLICY.OWNER_ACCOUNT_ID_FAKE,
            false,
            policyName,
            undefined,
            undefined,

            // #announce contains all policy members so notifying always should be opt-in only.
            CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY,
        );
        const announceChatReportID = announceChatData.reportID;
        const announceCreatedAction = buildOptimisticCreatedReportAction(CONST.POLICY.OWNER_EMAIL_FAKE);
        const announceReportActionData = {
            [announceCreatedAction.reportActionID]: announceCreatedAction,
        };

        return {
            announceChatReportID,
            announceChatData,
            announceReportActionData,
            announceCreatedReportActionID: announceCreatedAction.reportActionID,    
        }
}

when adding members to a workspace here:

/**
* Adds members to the specified workspace/policyID
*/
function addMembersToWorkspace(invitedEmailsToAccountIDs: Record<string, number>, welcomeNote: string, policyID: string) {

here we should check number of members of the workspace, if it is 3 members or more and there is no announce chat for the workspace we should call createAnnounceRoom

the condition is:

    let doesAnnounceChatExists = false;
    const policyReports = ReportUtils.getAllPolicyReports(policyID);
    policyReports.forEach((report) => {
        if (ReportUtils.isAnnounceRoom(report)) {
            doesAnnounceChatExists = true;
        }
    });
    if(!doesAnnounceChatExists && Object.keys(allPolicyMembers?.[membersListKey] ?? {}).length >= 2){
            const policyName = ReportUtils.getPolicy(policyID)?.name;
            createAnounceChat(policyName, policyID);
    }

The condition prevents duplicate createAnounceChat function calls when members are removed and new ones are added, by verifying the absence of an existing announce room before creation.

we should do the same for createWorkspaceFromIOUPayment function here as we did for createWorkspace

function createWorkspaceFromIOUPayment(iouReport: Report | EmptyObject): string | undefined {

Backend changes are also needed from internal team after we do frontend changes (we can test the changes offline before making changes to the backend).

Result:

Recording.2024-02-09.174210.mp4

What alternative solutions did you explore? (Optional)

@shahinyan11
Copy link
Contributor

Proposal

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

Don't create default #announce rooms until a workspace has 3 or more members

What is the root cause of that problem?

New feature

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

  1. Remove this constants and everything depends on these form createWorkspace and createWorkspaceFromIOUPayment
  2. Create new buildOptimisticAnounceChat function in ReportUtils and move this part of the code there.
  3. Create new CreateAnnounceChat endpoint in backend side
  4. Create new createAnnounceChat function similar to createWorkspace. E.g Below
function createAnnounceChat( policyName, policyID ) {
    const {
        announceChatReportID,
        announceChatData,
        announceReportActionData,
        announceCreatedReportActionID,

    } = ReportUtils.buildOptimisticAnounceChat(policyID, policyName);

    const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${policyID}`,
            value: {
                [sessionAccountID]: {
                    role: CONST.POLICY.ROLE.ADMIN,
                    errors: {},
                },
            },
        },
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: {
                pendingFields: {
                    addWorkspaceRoom: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
                },
                ...announceChatData,
            },
        },
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: announceReportActionData,
        },
    ];

    const successData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: {
                pendingFields: {
                    addWorkspaceRoom: null,
                },
                pendingAction: null,
            },
        },
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: {
                [announceCreatedReportActionID]: {
                    pendingAction: null,
                },
            },
        },
    ];

    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT}${announceChatReportID}`,
            value: null,
        },
        {
            onyxMethod: Onyx.METHOD.SET,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceChatReportID}`,
            value: null,
        },
    ];

    const params = {
        policyID,
        announceChatReportID,
        announceCreatedReportActionID,
    };

    API.write("CreateAnnounceChat", params, {optimisticData, successData, failureData});
}
  1. Add below condition in addMembersToWorkspace function
    if(allPolicyMembers?.[membersListKey]?.length === 2){
        createAnounceChat(policiName, policyID)
    }

What alternative solutions did you explore? (Optional)

@quinthar
Copy link
Contributor

quinthar commented Feb 9, 2024

I think this would be done on the back end, wouldn't it be? We would create the announce room when the 3rd person is added to the workspace, which is something we should probably do inside auth given that there are so many ways to invite someone.

@rayane-djouah
Copy link
Contributor

I think this would be done on the back end, wouldn't it be? We would create the announce room when the 3rd person is added to the workspace, which is something we should probably do inside auth given that there are so many ways to invite someone.

@quinthar This feature requires updates on both the frontend and backend. We can handle the frontend adjustments, while the internal team should develop a new CreateAnnounceChat endpoint on the backend to support the creation of announce rooms.

@rojiphil
Copy link
Contributor

rojiphil commented Sep 5, 2024

@NikkiWines I have created a draft PR that addresses most of the issues, but we need to agree on how FE can optimistically generate the announce room. And, at the same time, the BE also seamlessly considers that without resulting in two #announce rooms in FE. The following test video demonstrates the problem for the draft PR.

Can BE handle this if we pass announce room chat data along with AddMembersToWorkspace request and also additionally pass the params announceChatReportID and announceCreatedReportActionID here as we do for createWorkspaceChat here or do you see any other approach to resolve this?

34929-duplicate-announce.mp4

@anmurali
Copy link

anmurali commented Sep 9, 2024

@NikkiWines - can we take care of this issue? It would be really great to get this done as it is what stands between Stage 2 onboarding (within the scope of which we said we would clean this up) being actively worked on vs. done.

@NikkiWines
Copy link
Contributor

Can BE handle this if we pass announce room chat data along with AddMembersToWorkspace request and also additionally pass the params announceChatReportID and announceCreatedReportActionID here as we do for createWorkspaceChat here or do you see any other approach to resolve this?

Yes, I think it makes sense to follow what we did for createWorkspaceChat, and I can update the backend for AddMembersToWorkspace to take in the announceChatReportID and announceCreatedReportActionID.

@rojiphil
Copy link
Contributor

Yes, I think it makes sense to follow what we did for createWorkspaceChat, and I can update the backend for AddMembersToWorkspace to take in the announceChatReportID and announceCreatedReportActionID.

Thanks and I have updated the draft PR to send across announceChatReportID and announceCreatedReportActionID as params in AddMembersToWorkspace request. I look forward to testing this out once BE is ready.

@NikkiWines
Copy link
Contributor

NikkiWines commented Sep 10, 2024

The backend PRs (one and two) are up and in review

@NikkiWines
Copy link
Contributor

The Auth PR has been merged, once that's deployed we can merge the Web PR. And when the Web PR is live then we can get #48660 over the line

@NikkiWines
Copy link
Contributor

NikkiWines commented Sep 13, 2024

Web PR has been merged - should go to prod by tuesday.

@NikkiWines
Copy link
Contributor

NikkiWines commented Sep 16, 2024

Web PR is on staging now!

@NikkiWines
Copy link
Contributor

Backend changes are live, we should be able to move forward with the App PR now

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 19, 2024
@rojiphil
Copy link
Contributor

Backend changes are live, we should be able to move forward with the App PR now

Thanks @NikkiWines and yeah, BE is full of life and works like a charm too.
PR is ready for review @allroundexperts

@madmax330
Copy link
Contributor

@JmillsExpensify there's an issue here: https://github.com/Expensify/Expensify/issues/427525

For invoicing we expect the announce room to exist, but when you send an invoice to someone and they chose to pay as a business, we create a workspace for them where they are the only member, so the annouce room doesn't get created (since they're the only memeber)

I'm not sure what we should do in this case. Should we:

  1. create the announce room with < 3 members for this case
  2. use another room for invoicing
  3. something else?

@trjExpensify
Copy link
Contributor

What's the announce room used for in that invoice case, @madmax330?

@madmax330
Copy link
Contributor

Apparently nothing 😄
Or if something I can't find it.
But I think it's mainly because we create the room optimistically, so when the API doesn't create it it causes a bug on the front-end. So maybe we should just stop creating it altogether?

@anmurali
Copy link

https://fsty.io/v/6X2rBN3K

I have seen this in about a dozen sessions. The #announce room shows up but when clicked shows a not here error. Is this because the room is created optimistically? I think we should just stop creating it. Create it when there are 3+ members in the workspace.

@trjExpensify
Copy link
Contributor

That's already the plan, isn't it? The App PR is in review: #48660

@danielrvidal
Copy link
Contributor

I'd vote we stop creating it altogether. I saw this in 3 full stories impacting customers trying to adopt. Can we prioritize as CRITICAL?

@trjExpensify
Copy link
Contributor

Yeah, I think we still need this App PR to be merged: #48660

@JmillsExpensify
Copy link
Author

Agreed. Can we update that PR to CRITICAL, mostly because it's affecting production users. What do ya'll think @NikkiWines @allroundexperts?

@trjExpensify
Copy link
Contributor

Ah yeah, I commented in the PR already. I've escalated it to slack here: https://expensify.slack.com/archives/C02NK2DQWUX/p1727347525853669

Copy link

melvin-bot bot commented Sep 30, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@NikkiWines
Copy link
Contributor

This can be closed I think - #48660 has been on prod for a few weeks now - not sure why the issue wasn't updated with the payment details.

@johncschuster can we issue payment to @rojiphil and @allroundexperts for the PR and C+ review - thank you 🙇

@johncschuster
Copy link
Contributor

johncschuster commented Oct 14, 2024

Payment Summary:

Contributor: @rojiphil paid $500 via Upwork - PAID! 🎉

Contributor+: @allroundexperts due $500 via NewDot

Upwork job here! Please apply

Copy link

melvin-bot bot commented Oct 22, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests