App
App copied to clipboard
Expensify
[$500] Expensify comment to enter the details for the deleted expense request
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.36-5 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 Expensify/Expensify Issue URL: Issue reported by: @danieldoglas Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1707168112041529
Action Performed:
- smartscan invalid receipts and wait for to fail
- Delete th requests
Expected Result:
No message saying that Receipt scanning failed. Enter the details manually.
Actual Result:
hey now had a comment from Expensify saying that Receipt scanning failed. Enter the details manually. . Because of those comments, the expense policy chat is now always showing as a GBR, when it shouldn't show anything since I can't act on it.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [ ] Android: Native
- [ ] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/203bc721-8ab8-4650-8b02-c7cc9f729e6c
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~018bc064bf2f244ac3
- Upwork Job ID: 1754669805080993792
- Last Price Increase: 2024-02-06
Job added to Upwork: https://www.upwork.com/jobs/~018bc064bf2f244ac3
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)
Proposal
Please re-state the problem that we are trying to solve in this issue.
hey now had a comment from Expensify saying that Receipt scanning failed. Enter the details manually. . Because of those comments, the expense policy chat is now always showing as a GBR, when it shouldn't show anything since I can't act on it.
What is the root cause of that problem?
Currently we only delete the money request if there's no visible report action here. This doesn't work in the case where in the money request there're only actions from Expensify Notification account because those are considered as visible report action.
What changes do you think we should make in order to solve the problem?
Extend the check whether delete the money request to check if the report actions are all from Expensify Notification, we can do this by, for example, counting the report actions from Expensify Notification (those will have actorAccountID of CONST.ACCOUNT_ID.NOTIFICATIONS) and check if it's equal to the childVisibleActionCount. In this case, we'll still delete the money request thread.
We might want to fix the back-end to follow the same logic too.
What alternative solutions did you explore? (Optional)
An alternative is to looping through the list of report actions to see if there's any non-Expensify-Notification visible report actions (like comment, attachment, ...). If yes, delete the money request, if not, keep the money request
@CortneyOfstad Could you kindly confirm the expectation here?
Should we not show the failure message or not show the whole request itself? I am not sure what we should expect here.
@danieldoglas since you reported the issue, I would love your input here. I'm leaning towards not showing the whole request, but wondered if you had a preference. Thanks!
We should dismiss the GBR or RBR if we cancel the request. There's nothing else to do there, so it doesn't make sense to keep it alerting.
Sounds good @danieldoglas! Does that make sense for you @abdulrahuman5196?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Hi, @CortneyOfstad sorry for the delay. Will check on the review in my morning.
This is the expected from @danieldoglas comment
We should dismiss the GBR or RBR if we cancel the request. There's nothing else to do there, so it doesn't make sense to keep it alerting.
@dukenv0307 proposal targets to delete the request in case of it only having expensify notifications. But the expectation here is to not show the green indicator alone. So the proposal is invalid at this point.
@danieldoglas Do correct me if misunderstood the requirements in any way.
@danieldoglas The steps in the OP currently do not result in a GBR/RBR showing, neither is the video attached in the OP. Can you double check to see if it's correct?
CortneyOfstad: I'm leaning towards not showing the whole request, but wondered if you had a preference. Thanks!
@danieldoglas What do you think about this? IMO we should delete the money request completely if there's only "Expensify Notification" in it, there's no point keeping the money request in that case, especially when the user accidentally uploaded a wrong receipt and tried to delete that incorrect request.
We already do the same (remove the request completely) if there's only description changed/amount changed/other money request update message.
The problem with deleting everything is in case we have messages that are not related. Let's say for example that you add a comment in a money request, then delete the money request. The comment will still be there, and we need to show it as available.
What's happening here is that we're not removing the GBG or RBR if we delete the money request.
We already do the same (remove the request completely)
Those are different types of reportActions though - we can't delete those messages specifically because they're not currently considered system messages.
The problem with deleting everything is in case we have messages that are not related. Let's say for example that you add a comment in a money request, then delete the money request. The comment will still be there, and we need to show it as available.
@danieldoglas This case will not be impacted, in this case the money request will be kept. What I suggested is that if the report actions are all from Expensify Notification, the money request can be removed, if there's even 1 comment from the user, the money request will be kept.
What do you think? Is there any case where we should not delete the money request if there's only Expensify Notification actions in the request?
@CortneyOfstad @abdulrahuman5196 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!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@danieldoglas Just checking to see if you had a follow-up to @dukenv0307's comment here?
I agree with @dukenv0307's line of thinking that if there is a comment, the request should remain (or at least a message indicating the request was deleted/removed), but interested to hear your thoughts! Thanks!
One edge I see here, currently as of this issue only expensify notification comment is Receipt scan failed notification. But what if in future we started having different kind of expensify notification itself and we don't want to delete the request even if it had those notification(for eg: like some bank related notification from coincerge, just think out load)
Is there a property to just say if the expensify notification comment is the Receipt scan failed one? and we can only delete the report if only had the specific expensify notification comments like receipt failed? @dukenv0307
Is there a property to just say if the expensify notification comment is the Receipt scan failed one? and we can only delete the report if only had the specific expensify notification comments like receipt failed? @dukenv0307
@abdulrahuman5196 There isn't unfortunately. However I don't think that'd be necessary, even if the request thread is kept when the request is deleted, there won't be any information left on the request, only the [Deleted request] message, so even if there's other notifications like some bank related notification from coincerge, it wouldn't have much value (if at all), so I think it'd be safe to delete.
Also if the notification is from Concierge, it won't be affected by the suggested change, only the notification from Expensify Notification account is affected.
Ok, I agree with that as well. Thanks @dukenv0307
Hi @danieldoglas , Wanted to confirm again, since the last decision seems to have changed.
With the recent discussion we are fine with proceeding in the expectation mentioned by @dukenv0307 - https://github.com/Expensify/App/issues/35861#issuecomment-1928662194, that is to delete the request if we only have expensify notification messages.
I think the proposal is technically reasonable, so if we confirm on the above, I can go ahead with the next steps.
@CortneyOfstad @abdulrahuman5196 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!