[Awaiting BugZero checklist and C+ newdot payment] [$500] Chat - Emoji reactions are not being displayed in order of who reacted first

[kbecciv]

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:

1. Have multiple users react to a message
2. Observe that the reactions are not ordered based on who reacted first

Expected Result:

Emoji reactions are ordered by timestamp so it shows all reactions in the correct order

Actual Result:

Emoji reactions don't look to be ordered by the reaction timestamp at all.

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.72.8 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

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0164915b9cf8bed4bf
• Upwork Job ID: 1704949352333058048
• Last Price Increase: 2023-10-12
• Automatic offers:
• paultsimura | Contributor | 27254185

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0164915b9cf8bed4bf

[melvin-bot[bot]]

Current assignee @kadiealexander is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[ZhenjaHorbach]

Proposal

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

The list of usernames should go according by time

What is the root cause of that problem?

We don't sort usernames by time

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

We have a function that formats the data for the reaction

https://github.com/Expensify/App/blob/65fd4350829a60399aa36693a4731ac69b8e2b70/src/components/Reactions/ReportActionItemEmojiReactions.js#L74-L111

And instead of getting reactionUsers by _.keys(usersWithReactions)

We can write function which will use dates from skinTones and then will return sorted list

For example

        function sortUsersBySkinTonesDates(users) {
            const userEntries = Object.entries(users);

            userEntries.sort((a, b) => {
                const aDates = _.values(a[1].skinTones);
                const bDates = _.values(b[1].skinTones);

                const aMinDate = Math.min(..._.map(aDates, (date) => new Date(date).getTime()));
                const bMinDate = Math.min(..._.map(bDates, (date) => new Date(date).getTime()));

                return aMinDate - bMinDate;
            });

            const sortedUserIds = _.map(userEntries, (entry) => entry[0]);
            return sortedUserIds;
        }

What alternative solutions did you explore? (Optional)

NA

[paultsimura]

Proposal

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

The user reactions are not sorted by the reaction timestamp.

What is the root cause of that problem?

There are 2 issues here:

1. User names are not sorted in the tooltip;
2. Reaction skin tones are not sorted;

For 1: While sorting the reaction, we only sort them by one oldest timestamp, to determine the order of the emojis in the line. But we don't sort the users of each specific reaction.

https://github.com/Expensify/App/blob/9d23ffbeee01bf47edeb93c23a6bd9b2392fa5d0/src/components/Reactions/ReportActionItemEmojiReactions.js#L49-L72

For 2: We are not sorting the reaction skin tone icons by their timestamp, only by uniqueness.

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

First of all, it's important to note that there are 2 places where the emojis are sorted and displayed: ReportActionItemEmojiReactions and BasePopoverReactionList. Both components have quite a similar code, so I've decided it'd be useful to move it to a util function in EmojiUtils and reuse it.

First, we'll add the following function. It will not be exported, but will be used inside another Util function: I could use a less verbose approach to get the oldestEmojiTimestamp, but this one takes less iterations and memory

/**
 * Given an emoji reaction object and its name, it populates it with the oldest reaction timestamps.
 * @param {Object} emoji
 * @param {String} emojiName
 * @returns {Object}
 */
const enrichEmojiReactionWithTimestamps = (emoji, emojiName) => {
    let oldestEmojiTimestamp = null;

    let usersWithTimestamps = _.chain(emoji.users)
        .pick(_.identity)
        .mapObject((user, id) => {
            const oldestUserTimestamp = lodashMin(_.values(user.skinTones));

            if (!oldestEmojiTimestamp || oldestUserTimestamp < oldestEmojiTimestamp) {
                oldestEmojiTimestamp = oldestUserTimestamp;
            }

            return {
                ...user,
                id,
                oldestTimestamp: oldestUserTimestamp
            }
        })
        .value();

    return {
        ...emoji,
        users: usersWithTimestamps,
        // Just in case two emojis have the same timestamp, also combine the timestamp with the
        // emojiName so that the order will always be the same. Without this, the order can be pretty random
        // and shift around a little bit.
        oldestTimestamp: (oldestEmojiTimestamp || emoji.createdAt) + emojiName
    };
};

Second, we'll move the reusable code from ReportActionItemEmojiReactions and BasePopoverReactionList into this function:

Also, here we are sorting the users by reaction timestamp, and solving issue 1.

/**
 * Given an emoji reaction and the current user's account ID, it returns the reusable details of the emoji reaction.
 * @param {Object} reaction
 * @param {String} currentUserAccountID
 * @returns {Object}
 */
/**
 * Given an emoji reaction and current user's account ID, it returns the reusable details of the emoji reaction.
 * @param {String} emojiName
 * @param {Object} reaction
 * @param {String} currentUserAccountID
 * @returns {Object}
 */
const getEmojiReactionDetails = (emojiName, reaction, currentUserAccountID) => {
    const { users, oldestTimestamp } = enrichEmojiReactionWithTimestamps(reaction, emojiName);

    const emoji = findEmojiByName(emojiName);
    const emojiCodes = getUniqueEmojiCodes(emoji, users);
    const reactionCount = lodashSum(_.map(users, (user) => _.size(user.skinTones)));
    const hasUserReacted = Report.hasAccountIDEmojiReacted(currentUserAccountID, users);
    const userAccountIDs = _.chain(users)
        .sortBy('oldestTimestamp')
        .map((user) => Number(user.id))
        .value();

    return {
        emoji,
        emojiCodes,
        reactionCount,
        hasUserReacted,
        userAccountIDs,
        oldestTimestamp,
    }
}

Third, We'll modify the getUniqueEmojiCodes function in order to sort the skintone reactions by timestamp:

const getUniqueEmojiCodes = (emojiAsset, users) => {
    const emojiCodes = _.reduce(users, (result, userSkinTones) => {
        _.each(lodashGet(userSkinTones, 'skinTones'), (createdAt, skinTone) => {
            const emojiCode = getPreferredEmojiCode(emojiAsset, skinTone);
            if (!!emojiCode && (!result[emojiCode] || createdAt < result[emojiCode])) {
                result[emojiCode] = createdAt;
            }
        });
        return result;
    }, {});

    return _.chain(emojiCodes)
        .pairs()
        .sortBy((entry) => new Date(entry[1])) // Sort by values (timestamps)
        .map((entry) => entry[0]) // Extract keys (emoji codes)
        .value();
};

Almost lastly, we'll make changes leveraging the reusable code:

In ReportActionItemEmojiReactions, we fully remove the first chain of building sortedReactions, and instead use the newly created Util function:

const formattedReactions = _.chain(props.emojiReactions)
        .map((emojiReaction, emojiName) => {
            const {
                emoji,
                emojiCodes,
                reactionCount,
                hasUserReacted,
                userAccountIDs,
                oldestTimestamp,
            } = EmojiUtils.getEmojiReactionDetails(emojiName, emojiReaction, props.currentUserPersonalDetails.accountID);

            if (reactionCount === 0) {
                return null;
            }
            totalReactionCount += reactionCount;

            const onPress = () => {
                props.toggleReaction(emoji);
            };

            const onReactionListOpen = (event) => {
                reactionListRef.current.showReactionList(event, popoverReactionListAnchors.current[emojiName], emojiName, reportActionID);
            };

            return {
                reactionEmojiName: emojiName,
                emojiCodes,
                userAccountIDs,
                reactionCount,
                hasUserReacted,
                onPress,
                onReactionListOpen,
                pendingAction: emojiReaction.pendingAction,
                oldestTimestamp: lodashGet(oldestTimestamp, 'oldestTimestamp'),
            };
        })
        // Each emoji is sorted by the oldest timestamp of user reactions so that they will always appear in the same order for everyone
        .sortBy('oldestTimestamp')
        .value();

And in BasePopoverReactionList:

/**
     * Get the reaction information.
     *
     * @param {Object} selectedReaction
     * @param {String} emojiName
     * @returns {Object}
     */
    getReactionInformation(selectedReaction, emojiName) {
        if (!selectedReaction) {
            return {
                emojiName: '',
                reactionCount: 0,
                emojiCodes: [],
                hasUserReacted: false,
                users: [],
            };
        }

        const {
            emojiCodes,
            reactionCount,
            hasUserReacted,
            userAccountIDs
        } = EmojiUtils.getEmojiReactionDetails(emojiName, selectedReaction, this.props.currentUserPersonalDetails.accountID);

        const users = PersonalDetailsUtils.getPersonalDetailsByIDs(userAccountIDs, this.props.currentUserPersonalDetails.accountID, true);
        return {
            emojiName,
            emojiCodes,
            reactionCount,
            hasUserReacted,
            users,
        };
    }

And here: https://github.com/Expensify/App/blob/0ab7332bb7e968fe16f24e6d8429d82e22940e6c/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js#L208

- const {emojiName, emojiCount, emojiCodes, hasUserReacted, users} = this.getReactionInformation(selectedReaction);
+ const {emojiName, emojiCount, emojiCodes, hasUserReacted, users} = this.getReactionInformation(selectedReaction, this.props.emojiName);

One more change: For the users list, we are fetching the personal details with this function (used only for emojis across the project):

https://github.com/Expensify/App/blob/bb6023e47a7b5627a18dbb45f4cfc9a711c4f460/src/libs/PersonalDetailsUtils.js#L38-L54

For some reason, it was built to ignore the order of accountIDs list. Moreover, it's inefficient because goes through the whole list of allPersonalDetails, while the requested list of accountIDs is always smaller or equal to all personal details list. We should rewrite this function:

function getPersonalDetailsByIDs(accountIDs, currentUserAccountID, shouldChangeUserDisplayName = false) {
    return _.chain(accountIDs)
        .filter((accountID) => !!allPersonalDetails[accountID])
        .map((accountID) => {
            const detail = allPersonalDetails[accountID];

            if (shouldChangeUserDisplayName && currentUserAccountID === detail.accountID) {
                return {
                    ...detail,
                    displayName: Localize.translateLocal('common.you'),
                };
            } else {
                return detail;
            }
        })
        .value();
}

Result:

As a result, we get the emoji flow working as expected, but also we get a reusable and much cleaner code.

https://github.com/Expensify/App/assets/12595293/c1c168b5-48b6-44eb-82fa-4a0c5f99264a

[Krishna2323]

Proposal

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

The initial function didn't correctly order emojis based on their timestamps. When duplicates were present, it kept the first occurrence instead of the most recent one. The goal is to fix this to prioritize the latest emojis and remove earlier duplicates.

What is the root cause of that problem?

The original function getUniqueEmojiCodes aimed to return a list of unique emojis. However, it didn't consider the order of emojis based on their timestamps, nor did it prioritize the retention of the most recent emoji in case of duplicates.

Root Cause:

1. The initial function did not take the timestamp into account when determining the uniqueness of emoji codes.
2. Duplicates were removed based on their occurrence in the initial array, not based on their timestamps.

https://github.com/Expensify/App/blob/b225b53829f8fa4ef5f7a75616469d1fc7d347b9/src/libs/EmojiUtils.js#L457-L468

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

The getUniqueEmojiCodes function is responsible for ensuring uniqueness among the emoji codes. In the process, if there are duplicate emoji codes with different timestamps, the function will retains the one with the most recent timestamp and discards earlier occurrences. As a result, the returned array of emoji codes is sorted such that the ones with the latest timestamps appear last.

Option 1

To address this issue, the following changes are recommended:

1. Incorporate Timestamps: Introduce a new array emojiCodesWithTimestamps that stores each emoji code alongside its respective timestamp. This ensures that when sorting, the function can consider the order of emojis based on their recency.

2. Sort by Timestamps: The emojiCodesWithTimestamps array is sorted based on timestamps to ensure that the emojis are ordered from oldest to newest.

3. Refined Uniqueness Check: Instead of pushing unique emojis to uniqueEmojiCodes on the fly, we will first store all emojis with timestamps, sort them, and then utilize lodash's _.uniq() method, which, when chained with a reverse(), ensures that the latest occurrence of any duplicate emoji is retained.

Updated Code

const getUniqueEmojiCodes = (emojiAsset, users) => {
    const emojiCodesWithTimestamps = []; // This will help us in sorting
    _.each(users, (userSkinTones) => {
        _.each(lodashGet(userSkinTones, 'skinTones'), (createdAt, skinTone) => {
            const emojiCode = getPreferredEmojiCode(emojiAsset, skinTone);
            emojiCodesWithTimestamps.push({
                emojiCode,
                timestamp: createdAt,
            });
        });
    });

    const sortedByTimestamp = _.sortBy(emojiCodesWithTimestamps, (item) => new Date(item.timestamp));

    // Reverse, remove earlier occurrences of duplicates, and then reverse again
    const sortedEmojiCodes = _.chain(sortedByTimestamp).reverse().map('emojiCode').uniq().reverse().value();

    return sortedEmojiCodes;
};

We should do sorting in getUniqueEmojiCodes function because we are already mapping through it to get emoji codes and to remove dupelicates.

If we want to keep the emojis in the order they were originally added, meaning the first emoji added should come first in the result, and we don't want later duplicates to change this order, then we can simply remove both instances of reverse() from your code.

By doing this, we ensure that the emojis remain in their original order, regardless of any duplicates added later. So, the order in which the emojis were first added will be maintained in the final result.

Option 2:

We can also pass sorted reaction.users to getUniqueEmojiCodes but thats not preferred. https://github.com/Expensify/App/blob/b225b53829f8fa4ef5f7a75616469d1fc7d347b9/src/components/Reactions/ReportActionItemEmojiReactions.js#L88

Result

https://github.com/Expensify/App/assets/85894871/09164f83-5bd3-4cc1-a2ee-fed76c436a00

[kadiealexander]

@mollfpr please check these proposals!

[mollfpr]

The list of usernames should go according by time

@ZhenjaHorbach Isn't the issue regarding the emoji order, not the username?

@kadiealexander Could you help confirm if the order should start from the oldest or the latest reaction?

[MariaHCD]

@mollfpr We want the reactions for the same emoji to be displayed by the reaction timestamp from the oldest to the newest.

[mollfpr]

Thanks @MariaHCD! I'll get the review to continue then.

[mollfpr]

@paultsimura I think your proposal is more focused on the order of each emoji instead of the order of the one emoji reacted by many users or skin tones.

Result from your solution


---

const sortedByTimestamp = _.sortBy(emojiCodesWithTimestamps, (item) => new Date(item.timestamp));

@Krishna2323 What is this variable for? I see that you edit your code, but it's never used.

Also, it still needs to be ordered from the oldest. The -1 skin tone should be placed first.

[Krishna2323]

@mollfpr, sorry, i made a minor mistake while editing my proposal, now updated. We should use that variable in _.chain

[melvin-bot[bot]]

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

[paultsimura]

@mollfpr sorry, I think you got it a bit wrong. My solution does both in one take: sorts the emojis by their order among each other, and sorts the users that reacted within each emoji by their oldest timestamp.

If you check your screenshot of my solution, you can see that the users list of each emoji is already sorted by timestamp. As well as the emojis are sorted by their oldest timestamp.

[mollfpr]

@paultsimura But should the other consider the timestamp from the skin tone?

[paultsimura]

@paultsimura But should the other consider the timestamp from the skin tone?

The skin tones are the only place where we have the timestamps of users reaction, aren't they?

[paultsimura]

The only tricky part is when one user has reacted with multiple skin tones. I have implemented to take into consideration the earliest reaction among those skin tone reactions (because we don't want to show one user twice). The same logic is currently used to sort the different emojis among each other.

[melvin-bot[bot]]

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

[Krishna2323]

@mollfpr, We can do little update in my proposal if we need this:

If we want to keep the emojis in the order they were originally added, meaning the first emoji added should come first in the result, and we don't want later duplicates to change this order, then we can simply remove both instances of reverse() from my code.

By doing this, we ensure that the emojis remain in their original order, regardless of any duplicates added later. So, the order in which the emojis were first added will be maintained in the final result.

[mollfpr]

I'll get this review again in the morning.

[paultsimura]

@mollfpr just to assure you that my proposal works correctly and efficiently: Here's an example of:

• multiple emojis;
• the :+1: reaction contains multiple users who reacted with it;
• The user "15664034" reacted with multiple skin tones;

As a result, in a single run, the sortedReactions object holds all the emojis sorted among each other, and all the users within each emoji are sorted by their oldest reaction time.

You can also make some simple component render time comparisons to verify that my solution works faster than the other proposals.

[melvin-bot[bot]]

@mollfpr @kadiealexander 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!

[melvin-bot[bot]]

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

[mollfpr]

Sorry, I didn't get a chance to review this. I'll get this done by tomorrow.

[mollfpr]

(because we don't want to show one user twice)

Do you mean we don't show the same reaction from users with multiple skin tones? After thinking again, we can ignore my feedback earlier.

I did test your solution again with multiple users reacting to a message. The sortedReactions seems correct, ordering the users from the oldest emoji to which they reacted. But I couldn't get a better result because you need to update formattedReactions to make it work.

@paultsimura

[paultsimura]

@mollfpr I mean we do show all user's skin tone reactions (multiple skin tones from one user are displayed), but we don't show the user name in the list multiple times (for each skin tone):

As for the updated formattedReactions – my bad, I will update my proposal with it. Tried to keep it as laconic as possible, but apparently that's a bad practice.

[Krishna2323]

@mollfpr, have you tested my proposal?

[paultsimura]

@mollfpr I have updated my proposal https://github.com/Expensify/App/issues/27961#issuecomment-1730347696, tried to make it as detailed as possible. Also, added some important notes and improvements.

[melvin-bot[bot]]

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

[Krishna2323]

Hey @paultsimura,

I've reviewed the recent changes in your proposal, and I noticed the usage of getUniqueEmojiCodes in your updates. If I recall correctly, that function was first presented in my initial proposal.

From my perspective, the modifications in both ReportActionItemEmojiReactions and BasePopoverReactionList seem to revolve around reusing logic. However, I believe the core of the issue can be addressed solely by altering the getUniqueEmojiCodes function, without needing to make changes to the other components.

[paultsimura]

Hey @paultsimura,

I've reviewed the recent changes in your proposal, and I noticed the usage of getUniqueEmojiCodes in your updates. If I recall correctly, that function was first presented in my initial proposal.

From my perspective, the modifications in both ReportActionItemEmojiReactions and BasePopoverReactionList seem to revolve around reusing logic. However, I believe the core of the issue can be addressed solely by altering the getUniqueEmojiCodes function, without needing to make changes to the other components.

Modifying getUniqueEmojiCodes does not solve the issue of displaying the users in a correct order, only the emoji icons, which I believe is only a partial solution for this issue.

[kadiealexander]

@mollfpr any updates here?

[mollfpr]

@Krishna2323 Your solution is only sorting the skin tones, but not with the user's name.

The last three order user's name order should be 27961 Six, 27961 Five , and 27961 Four.

[mollfpr]

@paultsimura I did copy-paste your diff and follow your suggestion change, but the user name's is not sorted.

Here's the report action and user details.

Could you try your proposal with more than four users reacting?

[paultsimura]

@mollfpr you are right!

Updated my proposal with the last extra step now and attached the screenshots of users' ordering.

To test this easier, you can just clone my branch, as it is a lot of changes to copy-paste: https://github.com/paultsimura/Expensify/tree/fix/27961-emoji-ordering

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[paultsimura]

Just reproduced in Prod:

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[kadiealexander]

@mollfpr any thoughts on https://github.com/Expensify/App/issues/27961#issuecomment-1755994407?

[mollfpr]

Sorry for the delay 🙏

I tested the latest proposal from @paultsimura, and it seems to resolve the sorting problems on the skin tones and the user names reacting.

Before


After


🎀 👀 🎀 C+ reviewed!

[mollfpr]

@kadiealexander Could you help pull the internal engineer here? I guess because I've been removed from the team, the bot won't trigger the 🎀 👀 🎀

[melvin-bot[bot]]

Triggered auto assignment to @lakchote (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[paultsimura]

Issue not reproducible during KI retests. (Second week)

Reproducible:

[lakchote]

Issue reproducible in prod.

See discussion here.

@paultsimura's proposal LGTM. Thanks for reviewing @mollfpr.

[melvin-bot[bot]]

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

Offer link 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 📖

[hoangzinh]

It seems Melvin assigned the wrong C+. @lakchote do you have permission to assign @mollfpr and unassign me as a reviewer from this PR https://github.com/Expensify/App/pull/29846? Thanks