-
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
[Hold for payment: 2024-09-24] [$250] Task - Unable to remove task description #46111
Comments
Triggered auto assignment to @dylanexpensify ( |
@dylanexpensify 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 |
We think that this bug might be related to #vip-vsp |
ProposalPlease re-state the problem that we are trying to solve in this issue.Task - Unable to remove task description What is the root cause of that problem?Empty string will be falsy here Lines 502 to 503 in a79189f
so it will set it to the previous description What changes do you think we should make in order to solve the problem?change it to
What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Unable to remove task description What is the root cause of that problem?When description is empty string we are assigning the old description value ( Line 502 in a79189f
What changes do you think we should make in order to solve the problem?We should assign const newDescription = description ? ReportUtils.getParsedComment(description) : description ?? report.description; What alternative solutions did you explore? (Optional) |
Will repro today!
…On Wed, 24 Jul 2024 at 14:40, Night Coder ***@***.***> wrote:
Proposal Please re-state the problem that we are trying to solve in this
issue.
Unable to remove task description
What is the root cause of that problem?
When description is empty string we are assigning the old description
value (report.description) to newDescription. This will prevent new
description from being assigned undefined when user edits task title that
has an existing description, and remove existing description in the
process. This is because description is undefined when user edits task
title. But when the user removes existing description, the old description
value (report.description) is assigned to newDescription since empty
string is regarded as false when we use ? instead of ??.
https://github.com/Expensify/App/blob/a79189f2294a07a737bebbf5423f7d19ebf9d467/src/libs/actions/Task.ts#L502
What changes do you think we should make in order to solve the problem?
We should assign description to newDescription when description is an
empty string and assign report.description when description is undefined
const newDescription = description ? ReportUtils.getParsedComment(description) : description ?? report.description;
What alternative solutions did you explore? (Optional)
—
Reply to this email directly, view it on GitHub
<#46111 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOKVCAG26L6DBDLDUABTSS3ZN6VDFAVCNFSM6AAAAABLMP7PQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBXHE3DKMJVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@dylanexpensify Eep! 4 days overdue now. Issues have feelings too... |
@dylanexpensify Still overdue 6 days?! Let's take care of this! |
@dylanexpensify 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it! |
reviewing today! |
Confirming behavior |
@dylanexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@dylanexpensify Eep! 4 days overdue now. Issues have feelings too... |
Job added to Upwork: https://www.upwork.com/jobs/~01059741d391e013aa |
Forgot to hit external! |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allgandalf ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Unable to save empty description. What is the root cause of that problem?
When we save empty string, we essentially have Line 502 in a79189f
That is why the description is not updated if we save a empty string What changes do you think we should make in order to solve the problem?We do not need the check in place, we can directly set the newDescription to const newDescription = ReportUtils.getParsedComment(description); This is because we already validate if the prop passed is string when we call the util functions: App/src/pages/tasks/TaskDescriptionPage.tsx Lines 52 to 55 in b7f7f1b
What alternative solutions did you explore? (Optional) |
Thanks for proposals everyone 🙏 , all you guys have the correct RCA! Coming to solution: @etCoderDysto I feel your solution is a little over-engineering and not a correct way to approach. @FitseTLT , though your solution would solve this but the With that said, lets go with @eucool 's proposal here, their solution seems more appropriate for this bug 🎀👀🎀 C+ reviewed |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@deetergp, @dylanexpensify, @allgandalf Eep! 4 days overdue now. Issues have feelings too... |
Any updates @allgandalf? |
sorry for the delay, We can go with @FitseTLT 's proposal here, I didn't consider the case where we edit title, honest mistake on my part, sorry @deetergp @FitseTLT @etCoderDysto 🙇 If we edit the description then the type of description will be a So @FitseTLT 's solution will be better suited for this bug 🎀👀🎀 C+ reviewed |
Current assignee @deetergp is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
@allgandalf @FitseTLT's last edit and my proposal came at the same time (4:39). And my propsal has details of how description field becomes undefined when editing title. This is to ask if having a clear, and detailed RCA would make my proposal a better choice. cc: @deetergp |
@deetergp @dylanexpensify @allgandalf this issue is now 4 weeks old, please consider:
Thanks! |
@allgandalf can you please comment on my question here. Thanks! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@allgandalf @FitseTLT mind giving an update |
Automation failed here, the PR was deployed to production yesterday with the checklist: #48664 |
TY! |
Nice will pay today! |
can you pay this one please @dylanexpensify |
@dylanexpensify payment Ooooverdue here |
Payment summary: Contributor: @FitseTLT $250 via Upwork Please apply/request! |
Paid! |
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: 9.0.11-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4760280
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
User will be able to remove the task description
Actual Result:
User is unable to remove the task description
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6551397_1721815786409.20240724_180756.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @allgandalfThe text was updated successfully, but these errors were encountered: