-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Triggered auto assignment to @sophiepintoraetz ( |
Triggered auto assignment to @johncschuster ( |
Job added to Upwork: https://www.upwork.com/jobs/~01d20cdc896fd34ad2 |
Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts ( |
ProposalPlease 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: App/src/libs/actions/Policy.ts Line 1210 in 6d30216
we should remove this: App/src/libs/actions/Policy.ts Lines 1239 to 1253 in 6d30216
and this: App/src/libs/actions/Policy.ts Lines 1302 to 1320 in 6d30216
and this: App/src/libs/actions/Policy.ts Lines 1367 to 1376 in 6d30216
in https://github.com/Expensify/App/blob/main/src/libs/actions/Policy.ts we should create a 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 Lines 3450 to 3468 in 6d30216
and create a new function 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: App/src/libs/actions/Policy.ts Lines 607 to 610 in 6d30216
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 we should do the same for App/src/libs/actions/Policy.ts Line 1555 in 6d30216
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.mp4What alternative solutions did you explore? (Optional) |
ProposalPlease 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?
What alternative solutions did you explore? (Optional) |
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. |
Yes, I think it makes sense to follow what we did for |
Thanks and I have updated the draft PR to send across |
Web PR has been merged - should go to prod by tuesday. |
Web PR is on staging now! |
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. |
@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:
|
What's the announce room used for in that invoice case, @madmax330? |
Apparently nothing 😄 |
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. |
That's already the plan, isn't it? The App PR is in review: #48660 |
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? |
Yeah, I think we still need this App PR to be merged: #48660 |
Agreed. Can we update that PR to CRITICAL, mostly because it's affecting production users. What do ya'll think @NikkiWines @allroundexperts? |
Ah yeah, I commented in the PR already. I've escalated it to slack here: https://expensify.slack.com/archives/C02NK2DQWUX/p1727347525853669 |
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. |
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 🙇 |
Payment Summary:Contributor: @rojiphil paid $500 via Upwork - PAID! 🎉 Contributor+: @allroundexperts due $500 via NewDot
|
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. |
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:
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:
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: