[CRITICAL] Incorrect previousReportActionID for whispers from workspace invite

[roryabraham]

Problem

Inviting someone to a workspace will lead the inviter to repeatedly attempt to load whisper actions they can’t access (see the first image from Taras’ post here)

Reproduction:

Must be tested in connection with #30269

• Create a workspace as User A.
• Invite User B.
• Navigate to the created Workspace Chat as User B.
• Observe User B has a whisper message.
• Send a few messagse as User B inside the workspace chat.
• Navigate to Chat as User A.
• Observe User A does not see any whisper messages nor have them in their Onyx data.
• Look at Onyx data as User A and observe the first comment from User B has a previousReportActionID equal to the whisper message meant only for User B.
• User A will see a continuous loading state because we see a “gap” and we are trying to backfill a “missing report action” (i.e. the whisper) that User A should not be able to access.

Expectation:

• Continuous loading does not happen and we are able to see the CREATED action for this workspace report instead of an infinite load.

Important note: This is but one example, but we are assuming the same problem would exist anywhere a whisper exists.

Observations:

Deleted comments do not suffer the same problem because all users can still access the report action itself.
Whispers can be “hidden” from the front end if the current user’s accountID is not present in the reportAction.message.whisperedTo accountID array.

Solution:

Stop filtering out whisper messages on a per user level and treat them similar to deleted report actions (i.e. everyone can access them so they pose no problems for Comment Linking).

More context here about this decision is in this Slack thread.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d5dec33974b22a01
• Upwork Job ID: 1749864653925597184
• Last Price Increase: 2024-01-23

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @fedirjh (Internal)

[marcaaron]

I can grab this one.

[marcaaron]

Following the trail and it should be getting the previous action from here in Auth.

This appears to work fine for me locally. @roryabraham do you have any more information about this?

[marcaaron]

Gonna swap this one for #34982 for now as I don't quite understand what we are trying to fix here and the other one is much clearer to me.

[marcaaron]

Found this thread and left a comment there.

[marcaaron]

Seems like we are down to two possible solutions here. Not sure if we are ready to move forward yet.

[marcaaron]

The path I went down is likely wrong. Seeing if we can pivot. Asked for help in Slack here.

[marcaaron]

There was some more discussion here today. I am not sure if we have reached a 100% clear consensus - will bump some people for additional feedback on Monday.

The latest proposal is to ensure that whisper reportActions are always sent to all participants.

I did a quick audit of where all of the different kinds of whispers we have originate in Auth (we will need to ensure that if the Onyx updates are sent from Auth we send them to all participants)...

CompleteSplitBill

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/command/CompleteSplitBill.cpp#L145-L146

SettleModerationDecision

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/command/SettleModerationDecision.cpp#L99-L100

ShareReport

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/command/ShareReport.cpp#L117-L126

StartSplitBill

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/command/StartSplitBill.cpp#L122-L123

createActionableMentionWhisper()

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/lib/Report.cpp#L414-L440

(likely used in more places)

buildWhisperAction()

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/lib/Report.cpp#L6601-L6615

(likely used in more places)

Report previews:

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/lib/Report.cpp#L6881-L6891

(Used in many places)

IOU report action:

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/lib/Report.cpp#L6956-L6957

CreateMoneyRequest:

https://github.com/Expensify/Auth/blob/e3f820c2dc2c2eb30100083189eb4b977e3a0ad9/auth/lib/Transaction.cpp#L1781-L1782

That breaks out to a lot of stuff that we are going to have to audit and ensure that the correct updates are happening from within Auth. Luckily, we are mainly sending updates via Web-Expensify which are easy enough to spot by looking at the Web-Expensify repo search results here.

My recommendation would be to break this up into several smaller issues so we can audit one flow at a time, write tests, and ensure the whisper messages are sent to all clients regardless of whether they are in the whisperedTo message data.

Another hacky shortcut creative solution could be to somewhere somehow intercept an Onyx updates in both Auth and PHP and make sure we always include the rest of the participants. Or maybe @roryabraham said something about using the "report channel" for Onyx updates. Don't have a lot of context on how that works or if it's ready yet.

I picked this up thinking it would be much less involved than it was. After looking into this for a bit I don't think I have the bandwidth to do it all. But happy to help keep pushing it towards a solution for now!

[marcaaron]

Great conversation happening in #engineering-chat for this one. Put it to a vote here - I think the right path is to start sending whispers to everyone, but it's a big change so I'd like a bit more consensus before we dive down the 🐰 🕳️ .

[marcaaron]

Alright so, quick update here. This issue has turned out to be significantly more challenging than initially anticipated. I'm going to step out of the way for now since I already have a large project I'm working on.

This was a fun investigation though so hopefully what I have to share makes sense and can easily be picked up by someone (feel free to ask me questions if not).

My recommendation based on the initial research is that we do these things in this order:

• Unwind any code that might be returning incorrect previousReportActionID e.g. there is some low level code in Report::getActionList() that has some modifiers like filterByActions and skipWhispers. Those will need to get rolled back first because whenever they are used the previousReportActionID will be incorrect.

• Following that - we should update anywhere we request a list of report actions so it will not filter out whispers (or any other kinds of actions for that matter). Anytime this happens we could end up with a potentially inaccurate previousReportActionID. Maybe filtering is fine if we don't use the previousReportActionID - but it seems like it gets returned with the actions. So, I assume it can be wrong at least sometimes.

• We might also need to track down all the places where whispers are created. Sometimes these happen before the report actions list is fetched. In those cases, the requestor would get the action - but we may possibly need to also send Onyx updates to any participants of the chat (since everyone should be getting the whispers). I say "maybe" because we might be able to avoid this due to the nature of how Comment Linking works. If we fail to send an update for a whisper there will be a "gap" and then get filled in by the client with the empty whisper message and the client should mostly be unaware that this has happened.

• Follow up: Once clients are storing whisper report actions that they do not own we may need to ensure that the content is not accessible to them. I don't think this is a blocker for Comment Linking, but do think that it might require some more consideration.

I think if we do all that we should be in a pretty good spot.

[roryabraham]

Going to unassign @michaelhaxhiu and @fedirjh to make it clear this issue is currently not assigned to an internal engineer who can push it forward.

[cristipaval]

last status on Slack here

[cristipaval]

@Beamanator approved the App PR, but because perf-tests were failing, his approval was dismissed when I pushed a fix.

[melvin-bot]

@johncschuster @cristipaval this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[cristipaval]

The App PR is merged, once it hits production, we need to deprecate the older App versions and proceed with merging the Web-E PR

[cristipaval]

App PR is still in staging.
Web-E PR is ready to review. Tagged the reviewers to look at it asap to address all eventual change requests before the App PR hits production.

[cristipaval]

Update:

While reviewing the Web-E PR, @marcaaron raised a great point that we need to mute the push notifications for whispers targeted to others in the front end for web and desktop Apps. Here is another App PR that fixes that, is merged, and deployed to staging. Once that hits production, the Web-E PR can merge whenever approved.

[cristipaval]

Update:

• The App PR is still in staging
• Web-E PR is ready for a new round of reviews, after addressing some amazing feedback from @marcaaron
• Auth PR still held on Web-E PR

[cristipaval]

I hope that the App PR will hit production today.
Web-E PR is approved and ready to merge once the App PR is into prod.
I'll make sure that Auth PR is also ready to merge in the meantime. I mean, it's been ready, but I'll push to get reviews and feedback before Web-E gets into prod.

[melvin-bot]

@johncschuster @cristipaval this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

[cristipaval]

This is HOLD this week, we'll merge Web-E PR after the merge freeze

[cristipaval]

We'll deprecate the App version today and merge the Web-E PR

[cristipaval]

Web-E PR is merged

[cristipaval]

Update: Web-E PR is in staging. I'm actively addressing all the feedback on the Auth PR and doing my best to have it deployed tomorrow.

[cristipaval]

This is now in production