-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-06-20] Stop highlighting report mentions in non policy rooms in the Composer #41597
Comments
ProposalPlease 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 App/src/hooks/useMarkdownStyle.ts Lines 52 to 55 in ab1e924
What changes do you think we should make in order to solve the problem?We'll add a new argument to
Updated logic for styles of
Pass
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
We'll do the same in index.native.tsx also:
|
Hi @rlinoz! I'll look into it |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
which its inputs are the
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.
What alternative solutions did you explore? (Optional)
|
I updated the proposal |
Sorry we aligned on Slack that this one should go to Robert since he was the one adding the styling to report mentions. |
@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 |
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 |
@robertKozik This feature is not implemented in staging, right? (So I think It will be handled in the future). Thanks for your information! |
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 😄 |
@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. |
Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.75-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-06-03. 🎊 For reference, here are some details about the assignees on this issue:
|
Fix was merged into |
|
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-06-20. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@mollfpr would you like to get us kicked off for the BZ checklist? |
Payment Summary
BugZero Checklist (@JmillsExpensify)
|
No offending PR.
The regression step should be enough.
|
Confirming the payment summary above for $250 for @mollfpr. Regression test is also added, so closing this issue. |
Contributor: @mollfpr due $250 via NewDot |
$250 approved for @mollfpr |
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.
Solution:
We need to stop applying the markdown style to report mentions in reports that don't belong to a group policy.
Issue Owner
Current Issue Owner: @JmillsExpensifyThe text was updated successfully, but these errors were encountered: