Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

Stop highlighting report mentions in non policy rooms in the Composer #41597

Open rlinoz opened 2 weeks ago

rlinoz commented 2 weeks ago

Problem: While typing a report mention in the composer in a DM or group chat the mention is highlighted, even though it is not possible to mention a report in this types o chats.

image

Solution: We need to stop applying the markdown style to report mentions in reports that don't belong to a group policy.

ShridharGoel commented 1 week ago

Proposal

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

Need to stop highlighting report mentions in non policy rooms.

What is the root cause of that problem?

As of now, mentionReport is always using theme.mentionText and theme.mentionBG in the markdown style which leads to the highlighting.

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/hooks/useMarkdownStyle.ts#L52-L55

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

We'll add a new argument to useMarkdownStyle.

function useMarkdownStyle(message: string | null = null, shouldUseMentionReport = true)

Updated logic for styles of mentionReport:

mentionReport: {
    color: shouldUseMentionReport ? theme.mentionText : null,
    backgroundColor: shouldUseMentionReport ? theme.mentionBG : null,
},

Pass isGroupPolicyReport as a prop to Composer via ComposerWithSuggestions.

isGroupPolicyReport={isGroupPolicyReport}

We'll update Composer function signature in both index.tsx and index.native.tsx.

Now, we can pass the same to useMarkdownStyle as the value of shouldUseMentionReport by updating this code: Link.

const markdownStyle = useMarkdownStyle(value, isGroupPolicyReport);

We'll do the same in index.native.tsx also:

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/components/Composer/index.native.tsx#L37

robertKozik commented 1 week ago

Hi @rlinoz! I'll look into it

nkdengineer commented 1 week ago

Proposal

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

What is the root cause of that problem?

  1. In policy room chat, when typing any room name that does not belong to the policy, the typed text is still highlighted. Then send a message. That message is still highlighted until the API AddComment is successful.

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

    • We should create a new function to check if the typed text matches the suggestedMentions.
  1. Then update this to:

            mentionReport: doesRoomExisted(message, listRoomName) ? {
                color: theme.mentionText,
                backgroundColor: theme.mentionBG,
            }: {},

    this step is straightforward so I do not implement the details. The main idea of this step is we will check if the typed text exists in the list room that can be mentioned. If true, apply the highlight style. Also, we need to apply the change in .native.ts file

  2. Now our main bug is fixed. But there is another issue that I mentioned in the RCA section. For example in the above example is room-4, that message is highlighted for a while before the AddComment is called successfully. It is because in here, we apply the mention-report tag in case of isGroupPolicyReport. We should use doesRoomExisted along with doesRoomExisted

What alternative solutions did you explore? (Optional)

nkdengineer commented 1 week ago

I updated the proposal

rlinoz commented 1 week ago

Sorry we aligned on Slack that this one should go to Robert since he was the one adding the styling to report mentions.

nkdengineer commented 1 week ago

@rlinoz I found another related bug: In policy room chat, when typing any room name that does not belong to the policy, the typed text is still highlighted. Then send a message. That message is still highlighted until the API AddComment is successful.

Can you confirm if it is a bug? Thanks

robertKozik commented 1 week ago

Hi @nkdengineer I think it's not actually a bug - If there is no rrom with such a name, we will send actionable whisper with an option to create it. Checking for the room existance is not a valid one in this case. CMIIW

nkdengineer commented 1 week ago

we will send actionable whisper with an option to create it

@robertKozik This feature is not implemented in staging, right? (So I think It will be handled in the future).

Thanks for your information!

robertKozik commented 1 week ago

I believe frontend implementation is already merged, only backend part is not yet in staging. But I'm not so up-to-date on it 😄

robertKozik commented 1 week ago

@rlinoz I've created draft PR for this one. I went with more general solution and implement the rule excluding mechanism. Feel free to test and comment - I'll add the remaining screenshots and the rest later today.

melvin-bot[bot] commented 3 days ago

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