Skip to content
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-10] [$250] Onboarding tasks - Error appears when renaming onboarding tasks #46781

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 3, 2024 · 51 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 3, 2024

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.16
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
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Sign in with a new account
  3. On the onboarding modal select "Manage my team's expenses"
  4. Finish the onboarding flow by filling out business name and first name
  5. Open expensify chat
  6. Click on a task with a description. E.g "Set up categories"
  7. Click on the title and rename it to something else

Expected Result:

The title of the task changes successfully

Actual Result:

"Task description too long" error appears and the title of the task is not renamed but a system generated message shows up noting that the task title has been changed

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6554799_1722095847062.4_5848162808948069272.mp4

shot_1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a97f2cc7f0719d28
  • Upwork Job ID: 1820481612178741666
  • Last Price Increase: 2024-08-12
  • Automatic offers:
    • ZhenjaHorbach | Reviewer | 103598933
Issue OwnerCurrent Issue Owner: @isabelastisser
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 3, 2024
Copy link

melvin-bot bot commented Aug 3, 2024

Triggered auto assignment to @isabelastisser (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.

@lanitochka17
Copy link
Author

@isabelastisser 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

@kpadmanabhan
Copy link

kpadmanabhan commented Aug 4, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error message displayed upon editing task title. There is also a success message displayed stating that task updated.

What is the root cause of that problem?

Calling EditTask API returns code 666 with message Task description is too long and type Expensify\\Libs\\Error\\ExpError.
Front end recognizes the API response and displays the error message in the UI which is returned in the API response.
However we do not check if the error message is displayed or not and irrespective of that we display the message that updated the task title to XXX.

Update:

  1. Report description limit in CONST.ts to be updated to 1000 characters as per changes in backend.
  2. If backend returns error in API response, do not show success message. To be handled in ReportUtil.ts

What changes do you think we should make in order to solve the problem?

2 fixes needed here.

  1. Backend to check and fix the EditTask API and do not return message field if processing is successful. If error, then more meaningful message to be returned.
  2. In frontend, we need to correlate between processing of error response and displaying of success status. Display success only if there are no errors while processing.

What alternative solutions did you explore? (Optional)

N.A.

@daledah
Copy link
Contributor

daledah commented Aug 4, 2024

Edited by proposal-police: This proposal was edited at 2024-08-06 08:11:31 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

"Task description too long" error appears and the title of the task is not renamed but a system generated message shows up noting that the task title has been changed

What is the root cause of that problem?

When we call API EditTask we'll send both new title and description created when we finish onboarding
It'll return code 666 and the message Task description is too long

What changes do you think we should make in order to solve the problem?

Because the Back End has updated the description limit to 1000, we also need to update the description limit of the App to ensure consistency.

Update limit here, here to 1000

App/src/CONST.ts

Lines 184 to 186 in 5e5bff3

REPORT_DESCRIPTION: {
MAX_LENGTH: 500,
},

App/src/CONST.ts

Line 2485 in 5e5bff3

DESCRIPTION_LIMIT: 500,

What alternative solutions did you explore? (Optional)

@nyomanjyotisa
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error appears when renaming onboarding tasks

What is the root cause of that problem?

on EditTask API call, if the description exceeds the allowed length, the API will respond with error message 'Task description is too long.'
And we allow edit for onboarding tasks

What changes do you think we should make in order to solve the problem?

Onboarding tasks title and description shouldn't be editable, and user shouldn't be able to mark the onboarding tasks to complete manually, since the task will be marked as complete automatically once the user completed the task steps. For example, "Invite your team" task will be automatically completed when user invited member to their workspace

Add the following code on canModifyTask function here

    if (allPersonalDetails?.[getTaskOwnerAccountID(taskReport) ?? 0]?.login === CONST.EMAIL.CONCIERGE) {
        return false;
    }

What alternative solutions did you explore? (Optional)

If we only want to make the title and description not editable add the following code here

    const isOnboardingTask = personalDetails?.[report.ownerAccountID ?? 0]?.login === CONST.EMAIL.CONCIERGE;
    const isDisableInteractive = !canModifyTask || !isOpen || isOnboardingTask;

@isabelastisser isabelastisser added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 5, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01a97f2cc7f0719d28

@melvin-bot melvin-bot bot changed the title Onboarding tasks - Error appears when renaming onboarding tasks [$250] Onboarding tasks - Error appears when renaming onboarding tasks Aug 5, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach (External)

@ZhenjaHorbach
Copy link
Contributor

I will check proposals today or tomorrow

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Aug 6, 2024

Hmmmm
Actually, I'm also a little confused about why we can edit system tasks

@isabelastisser
We have several proposals to disable the ability to change the title and description of system tasks
What do you think? Does this make sense?

Copy link

melvin-bot bot commented Aug 9, 2024

@isabelastisser, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2024
@isabelastisser
Copy link
Contributor

Looking.

@isabelastisser
Copy link
Contributor

We have several proposals to disable the ability to change the title and description of system tasks
What do you think? Does this make sense?

@ZhenjaHorbach, I think there's some confusion here. Users should be able to rename the onboard tasks. The issue reported concerns the error message that appears when the task name is updated. We're throwing an error message even though the task is being successfully renamed.

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Aug 9, 2024

We have several proposals to disable the ability to change the title and description of system tasks
What do you think? Does this make sense?

@ZhenjaHorbach, I think there's some confusion here. Users should be able to rename the onboard tasks. The issue reported concerns the error message that appears when the task name is updated. We're throwing an error message even though the task is being successfully renamed.

Thanks for the answer 😃

Actually, the issue stems from a 500-character limit on task descriptions.
Some onboarding tasks exceed this limit because they were created on the backend without such a restriction.
As a result, when we attempt to modify the task name, we encounter an error due to the description already surpassing the allowed character count.

So I think we'll need some help from an internal engineer

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2024
@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Aug 9, 2024

Thanks everyone for proposals !

I agree with @kpadmanabhan proposal that we first need to update the BE

As I can see only @daledah updated the proposal to new requirements (#46781 (comment))
So I'm happy to go with #46781 (comment)

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 9, 2024

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@trjExpensify
Copy link
Contributor

Are we sure we're okay with them renaming onboarding tasks, @anmurali @danielrvidal?

@trjExpensify trjExpensify moved this from Release 1: Spring 2024 (May) to Polish in [#whatsnext] #wave-collect Aug 9, 2024
@mountiny
Copy link
Contributor

I kinda feel like we should not allow it

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 20, 2024
Copy link

melvin-bot bot commented Aug 20, 2024

📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Aug 20, 2024

📣 @daledah You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@mountiny
Copy link
Contributor

Gonna go with @daledah per the c+ recommendation

@daledah what is your eta for the pr?

@daledah
Copy link
Contributor

daledah commented Aug 20, 2024

I'll raise PR today

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 20, 2024
@daledah
Copy link
Contributor

daledah commented Aug 20, 2024

@ZhenjaHorbach This PR is ready for review.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@ZhenjaHorbach
Copy link
Contributor

Looks like production deploy automation failed: This should be on [HOLD for Payment 2024-09-06] according to prod deploy checklist, confirmed in slack

@ZhenjaHorbach
Copy link
Contributor

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:

  • [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

It's more like a new feature

  • [ @ZhenjaHorbach] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

NA

  • [@ZhenjaHorbach] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

NA

  • [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
  • [@ZhenjaHorbach] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  • Login with a new account
  • On onboarding modal, select "Manage my team's expenses"
  • Finish the onboarding flow
  • Open expensify chat
  • Click on a task with a description. E.g "Set up categories"
  • Rename task
  • Verify that task is renamed, no error appears

Do we agree 👍 or 👎

@mountiny
Copy link
Contributor

mountiny commented Sep 9, 2024

I think we can add this test as non-critical

@mountiny mountiny changed the title [$250] Onboarding tasks - Error appears when renaming onboarding tasks [HOLD for payment 2024-09-10] [$250] Onboarding tasks - Error appears when renaming onboarding tasks Sep 9, 2024
@mountiny mountiny added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Weekly KSv2 labels Sep 9, 2024
@isabelastisser
Copy link
Contributor

@daledah, what is your Upwork profile? Can you please apply for this job there?

https://www.upwork.com/jobs/~01a97f2cc7f0719d28

@melvin-bot melvin-bot bot removed the Overdue label Sep 9, 2024
@isabelastisser
Copy link
Contributor

Payment summary:

@ZhenjaHorbach, $250 for C+ review. Paid in Upwork.
@daledah, $250 for the Contributor proposal. Payment pending in Upwork.

@isabelastisser
Copy link
Contributor

Bump @daledah so I can process the payment in Upwork. Thanks!

@daledah
Copy link
Contributor

daledah commented Sep 11, 2024

@isabelastisser Here's my Upwork profile https://www.upwork.com/freelancers/~0138d999529f34d33f, please help send the offer. TIA

@isabelastisser
Copy link
Contributor

@daledah, thanks! I sent you the offer in Upwork now.

all set!

@daledah
Copy link
Contributor

daledah commented Sep 11, 2024

@isabelastisser I accepted thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants