-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$500] Task - [Deleted task] is not grayed out in task report when the task is deleted offline #28190
Comments
Triggered auto assignment to @Christinadobrzyn ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Task - [Deleted task] is not grayed out in task report when the task is deleted offline What is the root cause of that problem?When user canceled a task, we only added the pendingAction for taskAction and the parentAction here: Lines 768 to 769 in e14e9ee
Lines 807 to 810 in e14e9ee
Also we didn't wrap our canceled task view with OfflineWithFeedback, so we it won't grey out when task is deleted offline: App/src/pages/home/report/ReportActionItem.js Lines 522 to 534 in e14e9ee
What changes do you think we should make in order to solve the problem?Add Also clean up it here: {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
value: {
pendingAction: null
}
} Finally wrap our canceled task view with OfflineWithFeedback. if (ReportUtils.isCanceledTaskReport(props.report, parentReportAction)) {
return (
<OfflineWithFeedback pendingAction={props.report.pendingAction}>
</OfflineWithFeedback>
);
} What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.[Deleted task] is not grey when cancelling task offline What is the root cause of that problem?App/src/pages/home/report/ReportActionItem.js Lines 521 to 536 in b9a2f62
We don’t wrap [Deleted task] message by OfflineWithFeedback Component What changes do you think we should make in order to solve the problem?we should add OfflineWithFeedback Component as wrapper like this
|
My proposal post first in Slack |
Asking the team - https://expensify.slack.com/archives/C01SKUP7QR0/p1695748789919179 |
Follow up from Slack chat: https://expensify.slack.com/archives/C01SKUP7QR0/p1695748789919179 This is a polish job so something we should move forward with fixing. Expensify Tasks Design doc (not available for everyone) says we follow offline first. So that would indicate that the [Deleted Message] needs to be greyed out not bold. I'm not sure if this should be Internal or External so adding External. |
Job added to Upwork: https://www.upwork.com/jobs/~0140a361ebd43cf297 |
Triggered auto assignment to Contributor Plus for review of internal employee PR - @jjcoffee ( |
Current assignee @jjcoffee is eligible for the External assigner, not assigning anyone new. |
ProposalPlease re-state the problem that we are trying to solve in this issue.The [Delete task] message is not grayed out in the task report when the task was deleted offline. The message should be grayed out when the task is deleted offline. What is the root cause of that problem?The task report component is not re-rendering when a task is deleted offline. This could happen if the component is using another variable that is not updated when a task is deleted offline. What changes do you think we should make in order to solve the problem?We can use the state or props to render the tasks, and update them accordingly when a task is deleted offline. This way, the component will re-render and show the correct status of the tasks. What alternative solutions did you explore? (Optional) |
📣 @MichaelAwinda! 📣
|
Contributor details |
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
Unsure of how to handle a proposal being posted on Slack earlier than the first proposal on the issue, but I think since this issue wasn't created from the bug on Slack where the proposal was posted, that we unfortunately wouldn't take it into account in this case. That being said, I prefer @hungvu193's proposal anyway as it feels a bit tidier to add the extra 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@jjcoffee Could you help to explain this point? Because
App/src/pages/home/report/ReportActionItem.js Line 522 in b9a2f62
We also are using parentReportAction
|
@francoisl, @jjcoffee, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@DylanDylann I don't feel too strongly about it, it just makes sense to me to also update the |
@jjcoffee |
@hungvu193 Do you want to weigh in on this? |
I believe the task in LHN grey out is expected, because we're deleting the task not a specific action. |
One more thing, We want to grey out the first message of task report (It is a report action). IMO, It is more suitable if we use pending action of report action to grey out report action (this is way we use in other palces). we shouldn't use pending action of report to grey out the report action (don't use in other places). |
We still use the pendingAction of the report for action for few cases, for examples:
In this case, I just want to update pendingAction on the actual task report. |
@DylanDylann The task report is already greyed out in LHN for me on latest main. Remember that the point of |
Just a final note, some action to task report on offline will make the first message of task report is greyed out if we use pendingAction of report for it. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
hi @jjcoffee let me know if these proposals are looking good or if you'd like to see more! |
Looks like we just did some refactoring for tasks with #24601 / #27827 and I'm not sure this is still an issue... in particular, after canceling a task while offline, we now still display the regular task view, with the task name / checkbox, description and assignee - and no Screen.Recording.2023-10-03.at.9.06.04.PM.movAm I missing something, or is this no longer an issue? |
Hmm yeah, It seems we are no longer added [Deleted task] message. So this is not reproducible anymore. |
Sorry I missed that! We're good to close this then @francoisl |
I'll ask QA to test just to make sure this is not happening anymore - https://expensify.slack.com/archives/C9YU7BX5M/p1696434612063849 |
@Christinadobrzyn Issue is not reproduced on v1.3.77-5. We are seeing old UI task page instead of the new one. Is this expected? 20231005_001741.mp4 |
I believe your "New UI task page" is from the video in the original post on an older version? Anyway, looks like this is no longer an issue, going to close. Thanks for the proposals anyway everyone, and on to the next one 🚀 |
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:
Expected Result:
In Step 4, after deleting the task, [Deleted task] message is grayed out
Actual Result:
In Step 4, after deleting the task, [Deleted task] message is not grayed out in the task report, while [Deleted task] is grayed out in 1:1 DM in Step 5
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.74-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Bug6214084_20230926_023232.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: