Open lanitochka17 opened 1 month ago
Triggered auto assignment to @muttmuure (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
@muttmuure Huh... This is 4 days overdue. Who can take care of this?
Not overdue
A user can reply to a thread for messages in chat with Expensify by right clicking on a report action in the chat.
There should be no Reply in thread
and some other options in the right click context menu action popover.
Incidentally, the MiniReportActionContextMenu
that appears on hovering the report action, however, does not show these options.
https://github.com/Expensify/App/assets/102477862/4a4edbf3-027d-4e6c-a085-3646b19f690f
That is because we pass disabledActions
here to the MiniReportActionContextMenu
https://github.com/Expensify/App/blob/017605269c9f3a351522784aec04dd12644ec564/src/pages/home/report/ReportActionItem.tsx#L888
but we do not pass them here to the right click report action context menu
https://github.com/Expensify/App/blob/017605269c9f3a351522784aec04dd12644ec564/src/pages/home/report/ReportActionItem.tsx#L355
The disabledActions
include these here
https://github.com/Expensify/App/blob/017605269c9f3a351522784aec04dd12644ec564/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L553-L563
Pass the disabledActions
here
https://github.com/Expensify/App/blob/017605269c9f3a351522784aec04dd12644ec564/src/pages/home/report/ReportActionItem.tsx#L355
similar to this
https://github.com/Expensify/App/blob/017605269c9f3a351522784aec04dd12644ec564/src/pages/home/report/ReportActionItem.tsx#L888
Once we fix this a thread cannot be started and the other issue of changing notification preference does not exist anymore.
Additionally in the other issue, the notification preference changes optimistically when we select another option but backend returns a 404 error later saying report not found so the notification preference gets reset.
@muttmuure Huh... This is 4 days overdue. Who can take care of this?
Job added to Upwork: https://www.upwork.com/jobs/~01aaf20e189bef2fa2
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External
)
@hoangzinh @muttmuure 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!
@c3024's proposal https://github.com/Expensify/App/issues/43562#issuecomment-2184831241 looks good to me
πππ C+ reviewed
Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
That is because we pass disabledActions here to the MiniReportActionContextMenu but we do not pass them here to the right click report action context menu
Are we sure this is not intentional behavior? Did some PR introduce the regression?
It is not a recent PR but I think it was missed to be included in the right click Context Menu in this PR #40010. Here it was included only in the mini menu.
I think both Context Menus should have the same options. Can they be different? π€
I think both Context Menus should have the same options. Can they be different?
I'm not sure. It might be intentional. That's why I'm asking about it.
Can you maybe post what it looks like with all the options so that Design can review the change?
In Step 5, app should disable reply in thread to Expensify message
Also this is not really clear to me. Why should it be expected that we disable this? @muttmuure what's your interpretation of this problem? I feel like I'm probably missing what problem we're trying to solve with this one.
I watched the video and it looks like the bug is related to not being able to set a notification preference so not really sure what that has to do with context menu actions really. π€·ββοΈ
app should disable reply in thread to Expensify message
I think this is the part we should be focusing on. Confirming whether this is true or not? If there's no reason why someone wouldn't be able to reply in a thread then this looks like more of a backend problem. But tbh, not sure we should do anything about this right now.
Can you maybe post what it looks like with all the options so that Design can review the change?
Here are the options in the MiniReportActionContextMenu (menu on hover): QuickReactionEmojis, Emoji Picker, Mark as unread, Copy to clipboard, Copy link.
Here are the options in the ReportActionContextMenu (menu on right-click): QuickReactionEmojis, Emoji Picker, Mark as unread, Copy to clipboard, Copy link, Reply in thread, Join thread.
https://github.com/Expensify/App/assets/102477862/a4c83b60-6b17-4e66-b52a-5061321d16eb
Also this is not really clear to me. Why should it be expected that we disable this?
We do not display the composer for chats with Expensify
, and the footer prompts users to chat with Concierge
for setup.
This is due to canWriteInReport
being false for this report, among other reasons.
In my view, since messaging is not permitted in this report, creating threads should also be restricted. This is the behaviour in all cases as far as I remember. So, in this case, it seems the option was intentionally removed from the mini menu, but its removal from the right-click menu was missed.
I watched the video and it looks like the bug is related to not being able to set a notification preference so not really sure what that has to do with context menu actions really.
I think the thread should not have been created in the first place and backend error is also due to this forbidden creation π. That is why I suggested that fix.
@hoangzinh, @marcaaron, @muttmuure Huh... This is 4 days overdue. Who can take care of this?
In my view, since messaging is not permitted in this report, creating threads should also be restricted. This is the behaviour in all cases as far as I remember. So, in this case, it seems the option was intentionally removed from the mini menu, but its removal from the right-click menu was missed.
Ok I think that makes sense? I am not really too familiar the system chat. @francoisl does the proposal make sense to you? It sounds like this is polish for Build Stage 2 of Guided Setup.
both Context Menus should have the same options. Can they be different?
If this is true - let's define their options in the same place with the same logic so this error can't happen again.
Yes, the system DM is supposed to be read-only, so users shouldn't be able to create threads.
The proposal makes, but it sounds like we also missed updating something in the backend, because in that case we should have thrown an error. I can try to help look into what change we need in the backend later this week.
Sounds good, thanks for confirming!
π£ @hoangzinh π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @c3024 π 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 PR is ready!
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.5-13 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-17. :confetti_ball:
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:
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.6-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-22. :confetti_ball:
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:
@hoangzinh, @marcaaron, @muttmuure, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!
Are we blocked on anything here?
Nope. Awaiting payment from @muttmuure since 17-Jul. Automation incorrectly added another date.
@hoangzinh, @marcaaron, @muttmuure, @c3024 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
Web/Desktop
Reply in thread
and Join thread
options are not displayedmWeb/Native apps
Reply in thread
and Join thread
options are not displayedDo we agree π or π
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.82-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Issue found when executing PR https://github.com/Expensify/App/pull/41343
Action Performed:
Expected Result:
In Step 5, app should disable reply in thread to Expensify message
Actual Result:
In Step 5, user can reply in thread to Expensify message. Then the composer disappears after replying in thread (Step 6) In Step 8, after changing notification preference in thread message with Expensify, the preference reverts to "Immediately"
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/561bb658-7234-4449-935b-95c8eea2e8b7
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @muttmuure