Optimistically create policy expense chats for new members of workspaces #16764

robertKozik · 2023-03-30T18:08:23Z

Details

Add creating optimistic policy expense chats while adding new members to the workspace, so created chats would pop up while user is offline.

Fixed Issues

$ #14594
PROPOSAL: #14594 (comment)

Tests

Same as QA steps

Verify that no errors appear in the JS console

Offline tests

Same as the QA steps. Workspace chats for each member should appear optimistically

QA Steps

Log in with account that has access to the policy-expense-chat beta
Go offline by navigating to [profile icon] > Preferences > and enable Force offline
Create a new workspace.
Invite some people to the workspace. Verify that workspace chats for each member appear optimistically.
Go online by navigating to [profile icon] > Preferences > disable Force online. Verify that workspace chats created for each members are updated accordingly

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-03-30.at.12.58.35.mov

Mobile Web - Chrome

RPReplay_Final1680175838.MP4

Mobile Web - Safari

Screen.Recording.2023-03-30.at.13.23.18.mov

Desktop

Screen.Recording.2023-03-30.at.13.34.57.mov

iOS

Screen.Recording.2023-03-30.at.13.13.12.mov

Android

Screen.Recording.2023-03-30.at.15.50.22.mov

MelvinBot · 2023-03-30T18:08:29Z

@bondydaa @0xmiroslav One of you needs to 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]

github-actions · 2023-03-30T18:08:42Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

robertKozik · 2023-03-30T18:11:49Z

I have read the CLA Document and I hereby sign the CLA

bondydaa

Just one small thing from me that you can change or leave as is, I don't have a strong opinion.

Tested on desktop and 👍 :chef-kiss:

bondydaa · 2023-04-06T20:38:04Z

src/libs/actions/Policy.js

    const membersListKey = `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`;
    const logins = _.map(memberLogins, memberLogin => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin));

+    // create onyx data for policy expense chats for each new member
+    const membersChats = createPolicyExpenseChats(policyID, logins, betas);


NAB maybe a better variable name would be like membersChatData

0xmiros · 2023-04-06T20:39:37Z

oh sorry forgot this PR. reviewing now

bondydaa · 2023-04-06T20:40:07Z

all you @0xmiroslav let me know if you need help with the checklist.

0xmiros · 2023-04-06T20:40:43Z

@robertKozik can you please merge from main since branch is very behind

robertKozik · 2023-04-06T21:39:39Z

@0xmiroslav rebased, so should be up to date

0xmiros · 2023-04-06T23:00:43Z

BUG: multiple workspace chats are created for the same user

bug.mov

0xmiros · 2023-04-06T23:08:03Z

@bondydaa should we also cross-out workspace chat when remove user from workspace offline?

0xmiros · 2023-04-06T23:14:42Z

src/libs/actions/Policy.js

+        onyxSuccessData: [],
+        onyxOptimisticData: [],
+        optimisticReportIDs: {},


no need failureData here?

0xmiros · 2023-04-06T23:17:11Z

src/libs/actions/Policy.js

+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
+            value: {
+                isLoadingReportActions: false,


Is this necessary?

I was modelling that on creating report inside Report.js file in function openReport (393:400 lines). If you think it can be ommited I'll remove it

If not needed, let's remove

Looks like false is the default here

App/src/pages/home/ReportScreen.js

Line 96 in 64f4a78

isLoadingReportActions: false,

and it's used here

App/src/pages/home/ReportScreen.js

Line 228 in 64f4a78

const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;

as well as here

src/pages/home/report/ReportActionsList.js 180: if (this.props.report.isLoadingReportActions && lastReportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED) { src/pages/home/report/ReportActionsView.js 134: if (nextProps.report.isLoadingReportActions !== this.props.report.isLoadingReportActions) {

Seems like it can probably be removed.

bondydaa · 2023-04-07T15:30:00Z

should we also cross-out workspace chat when remove user from workspace offline?

Hmm I didn't considered that scenario initially.

I think let's try to keep the scope of this PR small and only focused on the AddMembersToWorkspace API method since I'm assuming the removing of a user is a different API command? We can spin up a new issue to ensure the offline behavior for removing a workspace member is handled properly.

0xmiros · 2023-04-07T15:34:31Z

BUG: multiple workspace chats are created for the same user

@bondydaa are you able to reproduce this one?

bondydaa · 2023-04-07T15:44:24Z

I didn't see more than 1 when I tested yesterday on desktop but let me try again. Did you only add 1 member while offline? Or multiple? I think I tested adding 2 users but not just a single user so I'll try both.

0xmiros · 2023-04-07T15:47:21Z

I didn't see more than 1 when I tested yesterday on desktop but let me try again. Did you only add 1 member while offline? Or multiple? I think I tested adding 2 users but not just a single user so I'll try both.

I tried only one. Please try to repeat adding/removing the same user.

bondydaa · 2023-04-07T16:02:09Z

Hmm yeah I am seeing some weird things when you remove the user while offline.

Steps are like:

Go offline
add new member to workspace, see optimistic report added to LHN
remove user from workspace, optimistic report stays in LHN
Go online
optimistic report in LHN becomes "real chat", if you open that chat it then disappears from LHN and you see it's archived.

Though I think this is related more to removing a user vs adding a user so I think we handle that in the follow up GH for DeleteMembersFromWorkspace API command

robertKozik · 2023-04-07T16:06:35Z

@0xmiroslav @bondydaa I came up with quicker fix, as after removing the user chat remains as archived. We can look for this report and just change it's status to open as API will return it's ID as new chat. What you think about such an approach?

edit: @bondydaa I was testing it with following steps:

Go offline
add new member to workspace, see optimistic report added to LHN
Go online
Optimistic chat is replaced with real one
remove user from workspace, optimistic report stays in LHN
Go offline
Add member simmilar to the one from step 1

I came across one more problem though @bondydaa . Now we are passing only the reportID and It's merging into one as it should. But as we are not passing the created report action ID, when user clicks the created chat after coming back online we have double welcome notice. I think we should also pass the reportActionID as it is done when creating normal chat
Here are steps to repo this:

Go offline
Invite new member to workspace (optimistic creation of report is doing fine)
Go online
click on created chat and wait for the data to come from server

bondydaa · 2023-04-07T16:20:45Z

I created this #17137 for the follow up issue. Let's keep adding reproduce steps for things we see related to removing a user from the workspace / DeleteMembersFromWorkspace APi command to that as we come across them.

bondydaa · 2023-04-26T22:51:27Z

hmm nope still nodda locally for me, let me switch my new dot to staging as well and see if that does it since the workspace name change offline still reverted back.

0xmiros · 2023-04-26T22:58:58Z

@bondydaa please add Ready To Build label. I will test on release build.

bondydaa · 2023-04-26T23:01:56Z

yeah even against staging I'm still seeing the optimistic Expense Policy Chats being created.

2023-04-26_17-01-00.mp4

bondydaa · 2023-04-26T23:16:48Z

okay kicked off a build for this, workflow is here https://github.com/Expensify/App/actions/runs/4814233404. I guess a QR code should show up in like an hour or so?

0xmiros · 2023-04-26T23:18:38Z

okay kicked off a build for this, workflow is here https://github.com/Expensify/App/actions/runs/4814233404. I guess a QR code should show up in like an hour or so?

QR code should show up in a few hrs. I will test this tomorrow.

OSBotify · 2023-04-27T00:07:10Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/16764/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/16764/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/16764/NewExpensify.dmg	https://16764.pr-testing.expensify.com

0xmiros · 2023-04-27T16:33:24Z

All tests well but one issue:
Workspace admin (myself) is not in members list until online

members.mov

robertKozik · 2023-04-27T21:24:25Z

Great catch, fixed 🔧

scree-bug.mov

bondydaa · 2023-04-27T21:38:48Z

src/libs/actions/Policy.js

+    }
+
+    _.each(members, (login) => {
+        const oldChat = ReportUtils.getChatByParticipantsAndPolicy([sessionEmail, login], policyID);


NAB for now since I think workspaces can only allow 1 admin anyways but as we expand to allow multiple admins this will break and only add the admin of the current user.

I think the members variable we pass to this method should have all users on the workspace right? I wonder if we could run something to check if they are a workspace admin and then add them to this array?

members variable stores only users which are going to be add to the workspace. But we can get the policy members from Onyx store and filter out members to get all admins. The downside is that while being offline there is possibility of not having this info stored, so we would still fallback to the [newMember, ourMail] situation like here.

It shouldn't be difficult to implement, so If you think it should support multiple admins already, I'm happy to help

Hmm I think this is probably better as is for now, if/when we start allowing more than 1 admin on a workspace we can update this code as needed.

0xmiros

Looks good, tests well on all possible cases except the known workspace name issue on production backend. This is fixed in staging backend.
Anyway, not a blocker for this PR.

workspace.name.mov

OSBotify · 2023-04-28T13:35:36Z

🚀 Deployed to staging by https://github.com/bondydaa in version: 1.3.8-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-05-02T19:50:19Z

🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.8-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

bondydaa · 2023-05-03T22:21:58Z

hmm testing this on staging and production right now isn't working anymore 😢

bondydaa · 2023-05-03T22:27:53Z

I think I remember seeing something about us changing the invite member page so my guess it's involved here.

looking at the the logs

bondydaa · 2023-05-03T22:35:50Z

oh think i might have figured it out, testing more, hold on to your butts

bondydaa · 2023-05-03T22:38:35Z

yep confirmed, this #15672 broke it.

roryabraham · 2023-05-04T01:56:17Z

Following up on @bondydaa's comment here, I am seeing that the workspace chats appear to be created optimistically:

Then successfully created when coming back online:

Testing on dev. Let me make sure my Web-E & Auth are up-to-date...

roryabraham · 2023-05-04T01:58:36Z

Oh, maybe I lied and there was an error set on those chats...

roryabraham · 2023-05-04T02:02:59Z

Would love it if we could follow-up with some automated tests for this btw... might have prevented it from breaking

Gonals · 2024-06-06T12:41:36Z

This PR caused this bug. A user can be part of a workspace's room (if invited), but not part of the workspace. Inviting those users to the workspace would cause an error, as we'd wrongly identify the room as the Workspace chat.

robertKozik requested a review from a team as a code owner March 30, 2023 18:08

melvin-bot bot requested review from 0xmiros and bondydaa and removed request for a team March 30, 2023 18:08

robertKozik mentioned this pull request Mar 30, 2023

[HOLD for payment 2023-05-09] LHN - Workspace chats for each member appears only going back to online #14594

Closed

6 tasks

bondydaa assigned robertKozik Apr 6, 2023

bondydaa previously approved these changes Apr 6, 2023

View reviewed changes

optimistically create policy expense chats for new members of workspaces

f3eb8ac

robertKozik dismissed bondydaa’s stale review via f3eb8ac April 6, 2023 21:38

robertKozik force-pushed the optimistic-policy-expense-chats branch from e6d32a5 to f3eb8ac Compare April 6, 2023 21:38

bondydaa previously approved these changes Apr 6, 2023

View reviewed changes

0xmiros reviewed Apr 6, 2023

View reviewed changes

add onyx failure data for creating policy members chats

e928f1d

bondydaa mentioned this pull request Apr 7, 2023

[Hold] Update DeleteMembersFromWorkspace to work with optimistically creates chats offline #17137

Closed

6 tasks

bondydaa added the Ready To Build label Apr 26, 2023

add admin as workspace policy chat member

29aa65f

robertKozik dismissed bondydaa’s stale review via 29aa65f April 27, 2023 21:22

bondydaa reviewed Apr 27, 2023

View reviewed changes

bondydaa approved these changes Apr 27, 2023

View reviewed changes

0xmiros approved these changes Apr 27, 2023

View reviewed changes

bondydaa merged commit ffae56c into Expensify:main Apr 28, 2023

OSBotify mentioned this pull request Apr 28, 2023

Deploy Checklist: New Expensify 2023-04-28 #18132

Closed

30 tasks

bondydaa mentioned this pull request May 3, 2023

Implement Invite message page #15672

Merged

55 tasks

abdulrahuman5196 mentioned this pull request May 4, 2023

Followup fixes on invite message page implementation #18395

Merged

57 tasks

s77rt mentioned this pull request May 10, 2023

[$1000] [HOLD for payment 2023-05-23] [Manual Requests] Fix participants on the IOUConfirmation page in the case when an admin requests money from the workspace #18708

Closed

Gonals mentioned this pull request Jun 6, 2024

[HOLD for payment 2024-06-05] Existing policy expense chats are not being correctly found when inviting users to policies #42499

Closed

6 tasks

Optimistically create policy expense chats for new members of workspaces #16764

Optimistically create policy expense chats for new members of workspaces #16764

Conversation

robertKozik commented Mar 30, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 30, 2023

github-actions bot commented Mar 30, 2023 • edited Loading

robertKozik commented Mar 30, 2023

bondydaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmiros commented Apr 6, 2023

bondydaa commented Apr 6, 2023

0xmiros commented Apr 6, 2023

robertKozik commented Apr 6, 2023

0xmiros commented Apr 6, 2023

0xmiros commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertKozik Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bondydaa commented Apr 7, 2023

0xmiros commented Apr 7, 2023

bondydaa commented Apr 7, 2023

0xmiros commented Apr 7, 2023

bondydaa commented Apr 7, 2023

robertKozik commented Apr 7, 2023 • edited Loading

bondydaa commented Apr 7, 2023

bondydaa commented Apr 26, 2023

0xmiros commented Apr 26, 2023

bondydaa commented Apr 26, 2023

bondydaa commented Apr 26, 2023

0xmiros commented Apr 26, 2023

OSBotify commented Apr 27, 2023

0xmiros commented Apr 27, 2023

robertKozik commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmiros left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 28, 2023

OSBotify commented May 2, 2023

bondydaa commented May 3, 2023

bondydaa commented May 3, 2023

bondydaa commented May 3, 2023

bondydaa commented May 3, 2023

roryabraham commented May 4, 2023

roryabraham commented May 4, 2023

roryabraham commented May 4, 2023

Gonals commented Jun 6, 2024

robertKozik commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

robertKozik Apr 7, 2023 •

edited

Loading

robertKozik commented Apr 7, 2023 •

edited

Loading