Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.35k stars 2.77k forks source link

[READY FOR PAYMENT][$125] Report fields - List type report field shows "List is required" when there is an initial value #46223

Open lanitochka17 opened 2 months ago

lanitochka17 commented 2 months ago

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: 9.0.12-0 Reproducible in staging?: Y Reproducible in production?: N 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/44139

Action Performed:

Precondition:

Expected Result:

List type report field will not display "List is required" violation because there is an initial value

Actual Result:

List type report field displays "List is required" violation when there is an initial value

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/2c1c330f-3552-43a2-8c2f-f9a2e2a65e8f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fa66587c84d7c165
  • Upwork Job ID: 1823770004376025525
  • Last Price Increase: 2024-08-14
  • Automatic offers:
    • nkdengineer | Contributor | 103527627
Issue OwnerCurrent Issue Owner: @JmillsExpensify
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 2 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
francoisl commented 2 months ago

@thienlnam is this also something about report fields we can polish with a follow-up PR?

Beamanator commented 2 months ago

Assigning jack since this is part of your Report Field Violations Project πŸ™

Beamanator commented 2 months ago

Also if this is under beta we can demote, is there a report fields beta? reportFieldsFeature i think?

thienlnam commented 2 months ago

Yeah, we definitely can address - @jnowakow another one to fix

thienlnam commented 2 months ago

@Beamanator That beta is for setting up report fields in NewDot - slightly unrelated to showing them, but I don't think this needs to be a blocker

nkdengineer commented 1 month ago

Proposal

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

List type report field displays "List is required" violation when there is an initial value

What is the root cause of that problem?

After we reset the cache the field list has value is undefined since we use the defaultValue when we create the expense report.

https://github.com/Expensify/App/blob/04741f4b9c2ec7191d16abbc1ab2fe67085afb83/src/libs/actions/IOU.ts#L862-L867

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

We should return early if the defaultValue is not empty

if (excludedFields.includes(field.fieldID) || !!field.value || !!field.defaultValue) { 
   return; 
}

https://github.com/Expensify/App/blob/04741f4b9c2ec7191d16abbc1ab2fe67085afb83/src/libs/actions/IOU.ts#L862-L867

Or BE should return the value of fieldList as defaultValue after we create the expense report and don't change any report field.

What alternative solutions did you explore? (Optional)

thienlnam commented 1 month ago

πŸ‘‹ @war-in Could we also address this issue with https://github.com/Expensify/App/issues/46215?

melvin-bot[bot] commented 1 month ago

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

war-in commented 1 month ago

Hi πŸ‘‹ @thienlnam sure! I'll take care of it

war-in commented 1 month ago

I went with the same approach @nkdengineer proposed (FE one, I don't know if we want to modify the backend)

cc @thienlnam

war-in commented 1 month ago

PR is ready for C+ review :)

cc @allroundexperts

nkdengineer commented 1 month ago

@thienlnam Can I get the compensation here since we went with my proposal https://github.com/Expensify/App/issues/46223#issuecomment-2257983537, thanks.

thienlnam commented 1 month ago

Since this was a regression to be fixed by the original authors and was not intended to be an external issue, does half payment ($125) work for you?

nkdengineer commented 1 month ago

@thienlnam That work for me.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

thienlnam commented 1 month ago

cc @JmillsExpensify Could we please get a payment to @nkdengineer for $125?

nkdengineer commented 1 month ago

@thienlnam Do we need to assign me here for payment?

melvin-bot[bot] commented 1 month ago

πŸ“£ @nkdengineer πŸŽ‰ 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 πŸ“–

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 3 weeks ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

thienlnam commented 3 weeks ago

Bump payment for this one cc @JmillsExpensify https://github.com/Expensify/App/issues/46223#issuecomment-2289371246

melvin-bot[bot] commented 1 week ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 week ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 week ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 5 days ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer 10 days overdue. I'm getting more depressed than Marvin.

melvin-bot[bot] commented 3 days ago

@JmillsExpensify, @thienlnam, @war-in, @nkdengineer 12 days overdue. Walking. Toward. The. Light...

melvin-bot[bot] commented 5 hours ago

This issue has not been updated in over 14 days. @JmillsExpensify, @thienlnam, @war-in, @nkdengineer eroding to Weekly issue.