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-11-13] [HOLD for payment 2024-11-11] Room – Unable to save room description with numbers only #51789

Closed
2 of 8 tasks
lanitochka17 opened this issue Oct 31, 2024 · 27 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 31, 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.56-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
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. Go to https://staging.new.expensify.com/
  2. Log
  3. Create a workspace
  4. Create a room without description
  5. Open room chat
  6. Click on header, open description
  7. Enter few numbers into description and save

Expected Result:

Description is saved

Actual Result:

Description is empty

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6651150_1730381345048.bandicam_2024-10-31_15-20-54-852.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlockerCash This issue or pull request should block deployment and removed Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 31, 2024
Copy link

melvin-bot bot commented Oct 31, 2024

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

Copy link

melvin-bot bot commented Oct 31, 2024

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

Copy link

melvin-bot bot commented Oct 31, 2024

💬 A slack conversation has been started in #expensify-open-source

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Oct 31, 2024
Copy link
Contributor

👋 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.

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@lanitochka17
Copy link
Author

Production:

bandicam.2024-10-31.09-39-05-128.mp4

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 31, 2024

Regression PR: #50838

It's because when we set the room description to a number e.g 2222222 and when displaying the room description, we use getReportDescription function for the report description value

Inside this function we will parse the report.description using JSON parse and return the html value, if there's an error meaning the value could not be parse or something else, we will simply return report.description

App/src/libs/ReportUtils.ts

Lines 4261 to 4271 in 6c33bba

function getReportDescription(report: OnyxEntry<Report>): string {
if (!report?.description) {
return '';
}
try {
const reportDescription = report?.description;
const objectDescription = JSON.parse(reportDescription) as {html: string};
return objectDescription.html ?? '';
} catch (error) {
return report?.description ?? '';
}

When we set the room description something like testsd not a number, it will work because the JSON parse cannot parse string value which will run this return statement

App/src/libs/ReportUtils.ts

Lines 4269 to 4271 in 6c33bba

} catch (error) {
return report?.description ?? '';
}

But when we set a number value inside the room description, the JSON parse did not throw an error instead returning the report description number in type of number not string

So when there's no error this catch block will not run

App/src/libs/ReportUtils.ts

Lines 4269 to 4271 in 6c33bba

} catch (error) {
return report?.description ?? '';
}

But instead it will run this

App/src/libs/ReportUtils.ts

Lines 4267 to 4268 in 6c33bba

const objectDescription = JSON.parse(reportDescription) as {html: string};
return objectDescription.html ?? '';

Since it's number so when we return objectDescription.html ?? '' it will return empty string


Instead of returning objectDescription.html ?? '' we can return this:

const reportDescription = report?.description;
const objectDescription = JSON.parse(reportDescription) as {html: string};
return objectDescription.html ?? report?.description ?? '';

@MuaazArshad
Copy link
Contributor

Proposal

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

Unable to save room description with numbers only.

What is the root cause of that problem?

We are passing only strings for the room description; if we receive a number, it cannot be processed as a string.
here

App/src/libs/ReportUtils.ts

Lines 4265 to 4271 in 6c33bba

try {
const reportDescription = report?.description;
const objectDescription = JSON.parse(reportDescription) as {html: string};
return objectDescription.html ?? '';
} catch (error) {
return report?.description ?? '';
}

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

We should cast numbers to string before processing like this

try {
        const reportDescription = report?.description;
        const objectDescription = JSON.parse(reportDescription) as {html: string};
        const newObjectDescription = {html:String(objectDescription)};
        return newObjectDescription.html ?? '';
    } catch (error) {
        return report?.description ?? '';
    }

What alternative solutions did you explore? (Optional)

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 1, 2024

@Gonals I can reproduce this, can this go external? Is it a blocker?

@Beamanator
Copy link
Contributor

@NJ-2020 -thanks for finding what looks like the regression, i pinged the PR authors in slack 🙏

@melvin-bot melvin-bot bot added the Overdue label Nov 4, 2024
@truph01
Copy link
Contributor

truph01 commented Nov 4, 2024

Working on it ...

@Beamanator
Copy link
Contributor

PR looks great, I'll merge & CP very soon 👍

@Beamanator
Copy link
Contributor

Beamanator commented Nov 4, 2024

Note to @Christinadobrzyn : It looks like we're using @NJ-2020 's solution from here, so I believe they deserve some compensation even though they didn't make the PR

@Beamanator
Copy link
Contributor

PR is being CP'd

@Beamanator
Copy link
Contributor

Beamanator commented Nov 4, 2024

Fixed in staging 👍 Not closing b/c #51789 (comment)

@Beamanator Beamanator removed Reviewing Has a PR in review DeployBlockerCash This issue or pull request should block deployment labels Nov 4, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 4, 2024
@melvin-bot melvin-bot bot changed the title Room – Unable to save room description with numbers only [HOLD for payment 2024-11-11] Room – Unable to save room description with numbers only Nov 4, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.56-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-11. 🎊

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Nov 6, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-11-11] Room – Unable to save room description with numbers only [HOLD for payment 2024-11-13] [HOLD for payment 2024-11-11] Room – Unable to save room description with numbers only Nov 6, 2024
Copy link

melvin-bot bot commented Nov 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.57-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-13. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Nov 13, 2024

Payment Summary

Upwork Job

BugZero Checklist (@Christinadobrzyn)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@abdulrahuman5196
Copy link
Contributor

Unassigning myself since i didn't work on this PR.

@abdulrahuman5196 abdulrahuman5196 removed their assignment Nov 13, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 14, 2024

Payouts due:

Do we need a regression for this?

@NJ-2020
Copy link
Contributor

NJ-2020 commented Nov 14, 2024

Thank you @Christinadobrzyn @Beamanator

Sure here's my Upwork profile: https://www.upwork.com/freelancers/~01ff73ff788ecf5f2e

@truph01
Copy link
Contributor

truph01 commented Nov 14, 2024

@Christinadobrzyn I create a follow-up PR, so no payment is needed for me

@Christinadobrzyn
Copy link
Contributor

Thanks @NJ-2020! Can you please accept the Upwork offer so I can pay you? Thanks!

@NJ-2020
Copy link
Contributor

NJ-2020 commented Nov 14, 2024

Done accepted

@Christinadobrzyn
Copy link
Contributor

Awesome! Thanks @NJ-2020 - I sent the payment through Upwork.

It does seem like a regression test would be good for this and since there was no C+ review, I'm discussing the best options to create one with QA. https://expensify.slack.com/archives/C9YU7BX5M/p1731570034074179

@Christinadobrzyn
Copy link
Contributor

Sounds like QA already has a test for this and they are going to treat this as an edge case so no need to create a regression test. So we're good to close!

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 Engineering Weekly KSv2
Projects
Development

No branches or pull requests

8 participants