kavimuru commented 1 year ago

Rollout Plan

[x] #18427
[x] https://github.com/Expensify/Expensify/issues/281934
[x] #19531
[x] #19529
[x] #19534
[x] #19535

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Go to any chat
Type a text
Long press and react to some emojis , Please do select 3 / 4 emojis ( using add emoji button)
Notice that you have now multiple emoji reactions on that typed text.
Now unselect those emojis

Notice that after unselecting all emojis, they reappear

Expected Result:

After unselecting emojis, they should not reappear

Actual Result:

When all emojis are unselected, usually one or all emojis are restored. Further, if new text is typed below. Such restoration doesn’t last.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native ( Device model : one plus nord)
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.2.87-1 Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/227727393-9e12dfb6-a7a9-403b-9feb-ffb7fc8e72a5.mp4

Expensify/Expensify Issue URL: Issue reported by: @ashimsharma10 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679675126743679

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016e24bec63a2e084f
Upwork Job ID: 1640357797486551040
Last Price Increase: 2023-03-27

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

stitesExpensify commented 1 year ago

This is the same as number 3 from my slack post here https://expensify.slack.com/archives/C049HHMV9SM/p1678286500121729

I'll leave this open and assigned to me since we didn't have an issue for it, but it is known

priyeshshah11 commented 1 year ago

@stitesExpensify Just FYI, this also happens when you add/remove reactions when offline & then go back online as it effectively is the same as reacting quickly as both requests will be sent in quick successions.

laurenreidexpensify commented 1 year ago

@stitesExpensify what is the latest on this one?

stitesExpensify commented 1 year ago

No update yet. This is a bug that is not specific to emojis and happens in other places like pins as well, so it will require a larger discussion. I've been focusing on the smaller emoji reaction polish, and plan on coming back to this next week when I have more time.

MelvinBot commented 1 year ago

@eVoloshchak @stitesExpensify @laurenreidexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

MelvinBot commented 1 year ago

@eVoloshchak, @stitesExpensify, @laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

laurenreidexpensify commented 1 year ago

@stitesExpensify is this merged now?

laurenreidexpensify commented 1 year ago

Note for myself: Haven't hired anyone in Upwork yet, waiting for confirmation that we're fixing this.

stitesExpensify commented 1 year ago

No, this has not been dealt with. I'm going to try to create a plan today. This issue is pretty complicated due to the interaction between optimistic onyx data, the server response, and the pusher data, so I need to figure out the best way to prevent regressions

laurenreidexpensify commented 1 year ago

@stitesExpensify any update on the plan?

stitesExpensify commented 1 year ago

There are 2 options as far as I can tell.

Don't send pusher updates to the client that reacted, but do send failure data
- Pros:
  - Directly solves the problem of your own reactions causing flickering
  - No front end trickery
- Cons:
  - As we saw with inviting to large group chats, deciding who to send pusher data to can be slow
  - Goes against the pattern of always pushing data back to the front end for writes
Always prefer the local changes for the user instead of accepting the server push
- Pros:
  - This will prevent flickering for both yourself, and multiple users reacting at once
- Cons:
  - Involves manipulating server data on the front end which is against our development philosophy
  - Uses the front end as source of truth instead of the server
  - Could cause problems with adding/removing emojis on other clients (how do we know when it's legit or not)
Some combination of both of the above (which might be necessary)

I plan on starting with 1 and testing it out because it is the most straightforward, and then I'll come back for 2/3 if necessary

MelvinBot commented 1 year ago

@eVoloshchak @stitesExpensify @laurenreidexpensify 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!

laurenreidexpensify commented 1 year ago

@stitesExpensify listed a plan above that we'll work on this week

stitesExpensify commented 1 year ago

So I almost immediately realized a problem with 1 which is that we still need to use pusher for the user that reacted, because we need to push out the reaction to the user's other devices.

I'm thinking the better way to do it will be to control the merge somehow. Looking into it today, this is my top priority now btw

MelvinBot commented 1 year ago

@eVoloshchak, @stitesExpensify, @laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

MelvinBot commented 1 year ago

@eVoloshchak @stitesExpensify @laurenreidexpensify 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!

laurenreidexpensify commented 1 year ago

@stitesExpensify any update? thanks!

stitesExpensify commented 1 year ago

I'm at a bit of a loss on this one tbh, my attempts to fix it haven't been successful. I've started asking around to see how (or if) we fixed this same issue with pinning reports to see if the same solution could be applied here.

stitesExpensify commented 1 year ago

The pinning issue is not fixed FWIW.

Another potential solution would be to basically batch all of the requests into a single API call with multiple emojis so that it all gets sent at once.

Some potential issues with that are:

If you are the first to react and do a bunch of reactions when someone else does one, you will get a push from someone else and you will now be the second person to react (your order will switch)
We will still have the same problem where if you send the batch and then react one more time right after, the flicker will still be there

laurenreidexpensify commented 1 year ago

Discussing with @tgolen here

tgolen commented 1 year ago

In our discussion, I realized that the Onxy.merge() isn't working as it should because the reactions are stored as an array, and therefore, that's what's causing the results from one API request to overwrite the other API request. In a world where the reactions are stored as an object, this wouldn't happen. I'm going to be taking over this issue and working on it as top priority to figure out what we need to do here.

tgolen commented 1 year ago

Yesterday a built a POC in NewDot for moving reactions to their own Onyx collection and stored the data as objects instead of arrays. This works well on the front-end. Today, I'm going to attempt to build that into the PHP API, and then eventually the Auth commands.

tgolen commented 1 year ago

OK, I think I've got an idea of how this is going to work. To start with, here is the data format that I'm looking at using. Are we OK with this @stitesExpensify @marcaaron @hannojg ?

message = {
    "html": "test message with emoji \ud83d\ude1c",
    "emojiReactions": {
        "<emojiCode>": {
            "createdAt": 123456789,
            "users": {
               "<accountID>": {
                    "createdAt": 123456789,
                    "skinTone": 1,
                },
            },
        },
    },
}

Old data format

``` message = { "html": "test message with emoji \ud83d\ude1c", "reactions": [{ "emoji": ":+1:", "users": [{ "accountID": 2352342, "skinTone": 1 }, { "accountID": 4532562, "skinTone": 2 }] }] } ```

A few notes for completeness:

The new property is called emojiReactions. I am not in love with this, but I couldn't think of any other way to migrate to a new data format without using a new property name. I'm open to ideas and suggestions here!
The emojiCode and accountID will now be the object keys
The createdAt timestamp will come from the front-end and it will be used in the UI to ensure that the emojis and users are sorted in the order of creation
I don't really think we need to consider a migration from the old format. This means we will lose all previous reactions, but I think that it is fine at this stage and I don't think trying to keep them is very high priority. At some point, we could maybe delete them to clean up the database, but it's a minimal amount of data IMO

marcaaron commented 1 year ago

Did we consider the tradeoffs of letting the format on the server continue to be an array and transform it into an object when handing off to the front end?

As for an alternative name I can offer... userReactions, participantReactions or emojiReactions

stitesExpensify commented 1 year ago

I think that the new data format makes sense!

tgolen commented 1 year ago

We could considering massaging the data in PHP, but IMO it's not worth it since this feature is so new. It seems like if we did that, then our 5 year-from-now selves would regret that and we should always try to keep things stored on the front-end roughly the same way they are on the backend.

tgolen commented 1 year ago

I like emojiReactions better than reactionObjects. @stitesExpensify do you have a preference?

stitesExpensify commented 1 year ago

Definitely emojiReactions

tgolen commented 1 year ago

Cool beans. I'll use that then. There was one more pieces I wanted to get some thoughts on.

With this new way of storing the data, PHP is able to send all of the proper Onyx updates without the response from Auth. I guess my question is... should we still continue to return something from Auth (like the full emojiReactions object) even if it's not necessary or should we just not return anything and only make sure that PHP handles any Auth errors?

tgolen commented 1 year ago

@stitesExpensify @marcaaron do you have thoughts on the above about Auth sending back a response at all?

stitesExpensify commented 1 year ago

I think that makes sense. As long as we are handling the errors, then it doesn't seem like we need to return the full list from auth

tgolen commented 1 year ago

@marcaaron that will affect some of the refactoring you were doing on the Auth commands since you were changing what the returned response would be.

I'm trying to think about how to roll this out with a minimal amount of breakage. This is what I'm thinking:

Roll out Auth so that both data formats are stored and updated in the existing commands
Add a parameter to the PHP endpoints for add/remove reactions called useObjectDataFormat. When it's true then it will process and return the new data format to NewDot
Update NewDot to use the new data format and pass useObjectDataFormat as true
Once ^ is deployed, remove the useObjectDataFormat parameter from PHP
Once ^ is deployed, remove the useObjectDataFormat parameter from NewDot
Finally, remove the code in Auth which is storing the old array data format

marcaaron commented 1 year ago

ah tbh, I don't know if I fully understood the question 😓

PHP is able to send all of the proper Onyx updates without the response from Auth

Which updates are we talking about specifically and how is PHP able to send anything without something from Auth? 🤔

tgolen commented 1 year ago

It's basically the same "optimistic" principle that we use on the front-end. In the case of reactions, PHP knows exactly what to send to update Onyx because it's a super simple merge() with the changes to the reactions. PHP doesn't need anything from Auth to:

Add a new emoji: Onyx.merge('reportActionsReactions_$reportID.$reportActionID', {<newEmojiCode>: {'users': {<accountID>: {skinTone: <skinTone>}}}})
Add a reaction to an emoji that already exists: same as previous

laurenreidexpensify commented 1 year ago

@stitesExpensify @marcaaron any thoughts on the above from @tgolen ^^

tgolen commented 1 year ago

OK, here is my plan. I'm going to make these into separate issues so it's easier to track the PRs and progress.

(moved plan to OG description)

tgolen commented 1 year ago

I submitted a PR to Auth on Friday, so it should be reviewed soon. Once it is merged, it will unblock the next step.

tgolen commented 1 year ago

I submitted a PR to PHP today. Once that's merged, I'll get the PR for NewDot ready.

tgolen commented 1 year ago

This is slowly moving forward. Things are just kind of hung up on deploys and we don't want to rush this since things have to be deployed in a specific order.

tgolen commented 1 year ago

The auth change caused a deploy blocker for web that is being fixed in https://github.com/Expensify/Auth/pull/7890

tgolen commented 1 year ago

I am currently working on the PHP and NewDot changes to support both the old and the new emoji formats. It will be my primary focus today

laurenreidexpensify commented 1 year ago

I don't think this is overdue, the weekend just confused things a bit. @tgolen continues to. be active on this.

tgolen commented 1 year ago

I did a lot of work on this yesterday and ran into several edge cases during testing that was giving me trouble. Hopefully, I'll be able to get the functionality working perfectly today, then I can get the PRs ready for review in PHP and NewDot.

laurenreidexpensify commented 1 year ago

@tgolen how did yesterday go? did you manage to get the functionality working okay?

tgolen commented 1 year ago

It went pretty well. There is one final piece I have to finish up which is ensuring that the reactions are always displayed in the order they were added. I need to complete my testing around it today. Next steps look like:

Finish testing in NewDot with the PHP changes
Remove the HOLD on the PHP PR
Get the NewDot PR ready and reviewed (but it will be on HOLD until the PHP change is done)

tgolen commented 1 year ago

I was able to do some further testing today and got the functionality implemented to sort the reactions in the right order.

Sadly though, I still keep running into strangeness when testing:

Sometimes it takes the front-end 10-15 seconds between clicking on an emoji, and the API request being made. I think this has something to do with Pusher authentication taking a while. It seems to only happen in the first 10-15 seconds after loading the app. Not a big problem because the network requests do end up happening, but it's a little annoying.
Sometimes after removing several emoji in a row, a phantom emoji comes back (same as the original issue)
Sometimes it takes 5-10 seconds after receiving the pusher event that an emoji has been added/removed before the UI updates (this seems to be a new problem)
Pusher events are duplicated (I asked about this in Slack here)

It doesn't seem like any of these are problems on the PHP side (except for 4, so that's good).

I can't help shake the feeling though that this is getting reactions into a state that is worse than (or the same as) what we had originally. The problems only seem to manifest when quickly adding/removing (even in the original case). So, it's frustrating that I haven't really been able to improve much from what we had originally.

@stitesExpensify If you had the time, could you spend maybe about 15 minutes testing this out and playing with it? You'll need to be on updated Auth, web-e and NewDot need to be on the branch tgolen-migrate-reactions. I've been testing it with two clients open and signed in as different users chatting with themselves. I'd like to know if you're seeing the same things I am (especially 2 & 3).

Expensify / App

Reaction emoji disappears/reappears after quickly adding/removing reactions #16506

Rollout Plan

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Bug0 Triage Checklist (Main S/O)