[HOLD for payment 2023-09-04] [$1000] Chat - Not update frequently used emoji when pasting emoji into composer

kbecciv commented 1 year ago

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:

Enter :smile: in composer
Notice that it is converted to emoji and updated frequently used emoji
Paste a emoji to compose
Open emoji picker and notice this

Expected Result:

App should update frequent used emoji when pasting a emoji into composer

Actual Result:

App doesn't update frequent used emoji when pasting a emoji into composer

Workaround:

Unknown

Platforms:

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

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.31-3 Reproducible in staging?: y Reproducible in production?: y 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: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/f9878c06-82a1-41d3-affb-18914aa896a6

https://github.com/Expensify/App/assets/93399543/785c7782-1742-4d47-9ba4-0591ccb9ae6f

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0183125281d2c3d055
Upwork Job ID: 1674388658583044096
Last Price Increase: 2023-07-13

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

dukenv0307 commented 1 year ago

Proposal

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

Chat - Not update frequently used emoji when pasting emoji into composer

What is the root cause of that problem?

When pasting emoji we don't have any logic to update frequently used emoji

https://github.com/Expensify/App/blob/59973575da78597ccac89781b4d7117110b82078/src/pages/home/report/ReportActionCompose.js#L728-L733

In the updateComment function, we only replace text to emoji (like :smile: to 😄) and update frequently used emoji. If there are emojis in the comment like 😃 we will not add them into frequently used emojis

https://github.com/Expensify/App/blob/59973575da78597ccac89781b4d7117110b82078/src/pages/home/report/ReportActionCompose.js#L731-L733

The emojis in line 731 only contain emojis that is converted from text (like :smile:)

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

we should get all emojis from the comment and update frequently used emoji. For example, if the comment is

hello `:smile:` oke 🌵

We will update both 🌵 and 😄 to frequently used emoji list

What alternative solutions did you explore? (Optional)

bfitzexpensify commented 1 year ago

Agreed that we should be updating the list of Frequently Used emojis for any emoji used, whether via the picker or paste.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0183125281d2c3d055

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

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

bfitzexpensify commented 1 year ago

@dukenv0307 did you mark your comment https://github.com/Expensify/App/issues/21786#issuecomment-1612421841 as off-topic? Trying to determine whether that's a legit proposal or not.

parasharrajat commented 1 year ago

I was about to ask the same question. Also,

we should get all emojis from the comment and update frequently used emoji.

How will we do that?

spcheema commented 1 year ago

I was about to ask the same question. Also,

we should get all emojis from the comment and update frequently used emoji.

How will we do that?

I have experience any app yet where emoji from text are considered as frequently used.

bfitzexpensify commented 1 year ago

Interesting, so this might be tricky from a technical perspective. I just tested in Slack and it doesn't update the most frequently used when pasting the emoji in from elsewhere.

The behaviour does work on iOS - I tested copying different emojis that weren't in the frequently used list from Notes into WhatsApp, FB Messenger, iMessage and Instagram messaging, and each time the frequently used list updated after pasting in the emoji and sending it. But that seems like an OS thing rather than an app thing - the emoji picker there carries across apps, rather than being app-specific.

@parasharrajat do you have an Android you can test the same on? I can test if not but it will take me a little longer to set up and sign into these apps to test.

Since iOS does do this somehow, I think we can leave this open to see if we can get any proposals that will be able to solve for it. If not, it's not a major thing and we can just close it out.

jeet-dhandha commented 1 year ago

Proposal (Updated).

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

Pasted emojis in compser is not counted in frequently used emojis.

What is the root cause of that problem?

There's no such implementation in the code to get emojis if they are pasted in composer. As they are in unicode format and thus we cannot use replaceEmojis directly.

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

First we will create a map of all the emojis and their corresponding object.

const emojiMap = {
  "😀": {
    name: "grinning",
    code: "😀",
    keywords: ["smile", "happy", "grinning", "face", "grin"],
  },
  // ... rest emojis (this is made using `emojis.js`)
}

Create a function getEmojiCodeFromText which will take unicode as input and return the emoji object from the map, in file EmojiUtils.js.

Code Changes:

```js import emojiMap from "../assets/emojiMap"; ... rest code function getEmojiCodeFromText(text) { // For example, replace "Some comment with emoji 👉" const emojis = []; if(!text || typeof text !== "string") { return emojis; } let parseEmojis = text.match(CONST.REGEX.EMOJI); // EMOJI -> /(\u00a9|\u00ae|[\u2000-\u3300]|\ud83c[\ud000-\udfff]|\ud83d[\ud000-\udfff]|\ud83e[\ud000-\udfff])/gi if(!parseEmojis) { return emojis; } parseEmojis = [...new Set(parseEmojis)]; for (let i = 0; i < parseEmojis.length; i++) { const character = parseEmojis[i]; if(emojiMap[character]) { emojis.push(emojiMap[character]); } } return emojis; } ```

Now use getEmojiCodeFromText function in replaceEmojis function to get the emojis from the text.

Code Changes:

```diff function replaceEmojis(text, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE) { let newText = text; - const emojis = []; + const emojis = [...getEmojiCodeFromText(text)]; const emojiData = text.match(CONST.REGEX.EMOJI_NAME); if (!emojiData || emojiData.length === 0) { return {text: newText, emojis}; } ```

Finally use replaceEmojis function and update frequetly used emojis in Composer's paste function.

Code Changes

    paste(text) {
        try {
            document.execCommand('insertText', false, text);
            this.updateNumberOfLines();

+           const {emojis = []} = EmojiUtils.replaceEmojis(text, this.props.preferredSkinTone);
+           if (!_.isEmpty(emojis)) {
+               User.updateFrequentlyUsedEmojis(EmojiUtils.getFrequentlyUsedEmojis(emojis));
+           }

            // Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.
            this.textInput.blur();
            this.textInput.focus();
            // eslint-disable-next-line no-empty
        } catch (e) {}
    }

After Changes

Output

https://github.com/Expensify/App/assets/78416198/864c6179-ccef-4e71-8d5e-25050bc8e3f6

What alternative solutions did you explore? (Optional)

N/A

----- CHANGES LOG -------

Updated code for getEmojiCodeFromText.

parasharrajat commented 1 year ago

@bfitzexpensify I tested WhatsApp on Android and it does not update the emoji recently when we paste an emoji copied from a website.

jeet-dhandha commented 1 year ago

@bfitzexpensify @parasharrajat - Any updates on my proposal ?

parasharrajat commented 1 year ago

@dukenv0307 Bump https://github.com/Expensify/App/issues/21786#issuecomment-1613058834

dukenv0307 commented 1 year ago

@parasharrajat Yes. I marked it as off because I think this seems to be a not enough good proposal.

parasharrajat commented 1 year ago

@jeet-dhandha Although your proposal seems like it works, my mind is not fully convinced if that is the best solution.

Do you mind explaining all the scenarios where an emoji can be used in a composer and explaining what will be the outcome of all?

How will you handle emoji face tones?

jeet-dhandha commented 1 year ago

Updated PROPOSAL : https://github.com/Expensify/App/issues/21786#issuecomment-1613176046

jeet-dhandha commented 1 year ago

@parasharrajat

Do you mind explaining all the scenarios where an emoji can be used in a composer and explaining what will be the outcome of all? Emoji can be added by:

Pasting Text

Adding using Composer's emoji button.

Could have been in draft before this change.

How will you handle emoji face tones?

Upto my knowledge we are just rendering them as different skin tone. But storing them in suggestion is based on the base code only.

PS: I updated the proposal and also optimised the function to not loop through each text, instead use regex to get all the emojis and also use Set to remove duplicates.

jeet-dhandha commented 1 year ago

@parasharrajat Any thoughts on the above answer.

melvin-bot[bot] commented 1 year ago

@parasharrajat, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

parasharrajat commented 1 year ago

Will update you.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

bfitzexpensify commented 1 year ago

@parasharrajat reminder to check https://github.com/Expensify/App/issues/21786#issuecomment-1615382576 when you get a moment - thanks!

parasharrajat commented 1 year ago

Sure. I will. Need to test a few things on the App.

ShogunFire commented 1 year ago

Proposal

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

When we paste emojis they are not added to the frequently used emojis

What is the root cause of that problem?

We are not currently handling emojis that are pasted

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

In replaceEmojis before replacing the text by emojis here: https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/libs/EmojiUtils.js#L279

we can add the already existing emojis to the array that we return like this:

_.forEach(allEmojis, (item) => {
    if(text.includes(item.code)){
        emojis.push({
            name: item.name,
            code: item.code,
            types: item.types,
        });
    }
});

allEmojis being declared like this: import {default as allEmojis} from '../../assets/emojis/common';

Then they will be added to the frequent emojis here: https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/pages/home/report/ReportActionCompose.js#L756

Result:

https://github.com/Expensify/App/assets/19537677/71877a94-c950-43e1-9bf0-239edf53625a

What alternative solutions did you explore? (Optional)

For more clarity we could put this code in another function and then merge the two arrays before updating the frequent emojis

melvin-bot[bot] commented 1 year ago

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

parasharrajat commented 1 year ago

Thanks for the proposal. Will check this tomorrow.

melvin-bot[bot] commented 1 year ago

@parasharrajat @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

bfitzexpensify commented 1 year ago

@parasharrajat looks like we have the updated proposal from @jeet-dhandha and also a proposal from @ShogunFire ready for review when you get a chance

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

parasharrajat commented 1 year ago

Expect an update tomorrow. I am done for today. Thanks.

bfitzexpensify commented 1 year ago

@parasharrajat reminder to have a look here today when you get a chance, thank you!

parasharrajat commented 1 year ago

I looked at a few things and I think we can go with @jeet-dhandha's proposal with following changes.

we already have a map so use emojiCodeTable.
Do not pollute replaceEmoji function. Instead get the emojis code in the parent function updateComment and then pass both emojies array(the one we just got and one from replaceemoji) to the next line https://github.com/Expensify/App/blob/afb94208707a78583ddae88dd391f78423e917a3/src/pages/home/report/ReportActionCompose.js#L797.
There is no need to handle this in paste event.
I see that a few other functions are responsible for updating the emojis as well. let's make sure we find the best place to add our logic and test them all before raising the PR.

Thanks @ShogunFire for the proposal. Although, your proposal is different the base idea is the same as the earlier proposal so I didn't see any reason to reject the existing proposals based on the choice of the construct.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

ShogunFire commented 1 year ago

Humm the base idea is the same maybe but I think my proposal adds a lot to @jeet-dhandha 's proposal who was doing complex and unecessary code. On the 4 points that you want to add to his proposal, 3 are in my proposal. Usually proposals are not accepted just because they have the base idea. I really think my proposal is better :/

jeet-dhandha commented 1 year ago

@ShogunFire can you try copying these "😊😊😊" emojis and pasting them in composer and check if that works or not.

ShogunFire commented 1 year ago

Sorry I don't really have time right now and I don't know if that would change reviewers choice. But I don't see why that wouldn't work

jeet-dhandha commented 1 year ago

@parasharrajat I found two places where we will need to make the change:

ReportActionCompose.js https://github.com/Expensify/App/blob/96d077cfd7894c9f148049b85957bbc5652a6e0a/src/pages/home/report/ReportActionCompose.js#L790-L798
ReportActionItemMessageEdit.js https://github.com/Expensify/App/blob/96d077cfd7894c9f148049b85957bbc5652a6e0a/src/pages/home/report/ReportActionItemMessageEdit.js#L163-L170

jeet-dhandha commented 1 year ago

Also can we go with creating this common function such that there's not much change in the above given files.

Common Function

function replaceAndExtractEmojis(text, preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE, lang = CONST.LOCALES.DEFAULT) {
const {text: newDraft = '', emojis = []} = replaceEmojis(text, preferredSkinTone, lang);

return {
    text: newDraft,
    emojis: emojis.concat(getEmojiCodeFromText(text)),
};
}

ReportActionCompose.js

const {text: newComment = '', emojis = []} = 
-  EmojiUtils.replaceEmojis(comment, this.props.preferredSkinTone, this.props.preferredLocale);
+  EmojiUtils.replaceAndExtractEmojis(comment, this.props.preferredSkinTone, this.props.preferredLocale);

ReportActionItemMessageEdit.js

const {text: newDraft = '', emojis = []} = 
-  EmojiUtils.replaceEmojis(newDraftInput, props.preferredSkinTone, props.preferredLocale);
+  EmojiUtils.replaceAndExtractEmojis(newDraftInput, props.preferredSkinTone, props.preferredLocale);

jeet-dhandha commented 1 year ago

Also a question - Do we count frequently used emojis from Title's or Descriptions of Task or any other inputs ?

melvin-bot[bot] commented 1 year ago

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

📣 @jeet-dhandha 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 year ago

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Upwork job

parasharrajat commented 1 year ago

Do we count frequently used emojis from Title's or Descriptions of Task or any other inputs ?

No I guess.

jeet-dhandha commented 1 year ago

Then above changes should suffice.

jeet-dhandha commented 1 year ago

Does this code looks good: https://github.com/Expensify/App/pull/23153

jeet-dhandha commented 1 year ago

@parasharrajat Waiting for you to confirm.

parasharrajat commented 1 year ago

Please put up a PR for review after testing everything and we can take care of the code during review.

jeet-dhandha commented 1 year ago

That's Cool 😎👍

Expensify / App