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 2023-09-12] [HOLD for payment 2023-09-11] [$1000] [Distance] - App crashes when opening IOU thread and IOU shows a red dot #26585

Closed
3 of 6 tasks
lanitochka17 opened this issue Sep 2, 2023 · 98 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Sep 2, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Issue found when executing PR #25707

Action Performed:

  1. Launch New Expensify app
  2. Go to workspace chat
  3. Go to + > Request money > Distance
  4. Create a distance request
  5. Right after the request is created (when it shows $0.00), click on the IOU to open IOU thread and wait for a while
  6. Relaunch New Expensify app
  7. Go to the created distance IOU

Expected Result:

In Step 5, app does not crash
In Step 7. the distance IOU does not have a red dot

Actual Result:

In Step 5, the app crashes
In Step 7. the distance IOU shows a red dot

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.62-1

Reproducible in staging?: Yes

Reproducible in production?: No

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

Bug6186242_Screen_Recording_20230902_231428_New_Expensify.mp4

aaaaaaaa.txt

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team and also @situchan for a separate issue solved by the PR for this issue

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014333c9073e9221c1
  • Upwork Job ID: 1698167577340440576
  • Last Price Increase: 2023-09-03
  • Automatic offers:
    • pradeepmdk | Contributor | 26478625
    • Pujan92 | Contributor | 26480728
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Sep 2, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Sep 2, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2023

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@pradeepmdk
Copy link
Contributor

pradeepmdk commented Sep 3, 2023

Proposal

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

App crashes when opening IOU thread and IOU shows a red dot

What is the root cause of that problem?

in the latest main red dot is already fixed.
URI is passing as number because because local import should be passed with without uri key.
Screenshot 2023-09-03 at 5 14 12 AM

why because it fetching local image means here file name is empty when the CreateDistanceRequest API called
Screenshot 2023-09-03 at 6 21 03 AM
App/src/components/ReportActionItem/MoneyRequestPreview.js here expecting the filename
but in the API we have receiptFilename and filename is missing

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

form BC add fileName in the api

or

images={[ReceiptUtils.getThumbnailAndImageURIs(props.transaction.receipt.source, props.transaction.filename)]}

here we need add
props.transaction.filename || props.transaction.receiptFilename

Alternative Solution

here in ReportActionItemImage uri will doesn't support local assets file path.

 return (
        <Image
            source={{uri: image}}
            style={[styles.w100, styles.h100]}
        />
    );

so we need 'ReportActionItemImage' one more props if we are trying to load assets image means

 return (
        <Image
            source={assets ?  assets : {uri: image}}
            style={[styles.w100, styles.h100]}
        />
    );

and this function should return getThumbnailAndImageURIs {thumbnail: null, image: null , assets: localImagepath};

Or we can check the image as a string if string means we can assign with uri else without uri

I already added commented another PR #25841

please consider this as well

or else we can remove the local images from here

function getThumbnailAndImageURIs(path, filename) {

so that we can avoid the crash issue.

@hayata-suenaga
Copy link
Contributor

It's weekend and we need to fix this by Monday so I gonna make this external to get immediate proposals

@hayata-suenaga hayata-suenaga added Daily KSv2 External Added to denote the issue can be worked on by a contributor Hourly KSv2 and removed Hourly KSv2 Daily KSv2 labels Sep 3, 2023
@melvin-bot melvin-bot bot changed the title Distance - App crashes when opening IOU thread and IOU shows a red dot [$500] Distance - App crashes when opening IOU thread and IOU shows a red dot Sep 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014333c9073e9221c1

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@hayata-suenaga hayata-suenaga changed the title [$500] Distance - App crashes when opening IOU thread and IOU shows a red dot [$1000] Distance - App crashes when opening IOU thread and IOU shows a red dot Sep 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

Upwork job price has been updated to $1000

@spcheema
Copy link
Contributor

spcheema commented Sep 3, 2023

Might be related to this.

#26092

@pradeepmdk
Copy link
Contributor

IOU shows a red dot already fixed. here #26567

@JmillsExpensify
Copy link

@parasharrajat Do we need to add a regression test to prevent this from happening in the future?

@melvin-bot melvin-bot bot removed the Overdue label Sep 13, 2023
@JmillsExpensify
Copy link

JmillsExpensify commented Sep 13, 2023

In the meantime, payment summary is as follows:

  • Issue reporter: @situchan $50
  • Contributor: @Pujan92 $750 (incl. urgency bonus)
  • Contributor: @pradeepmdk $1,500 (incl. urgency bonus)
  • Contributor+: @situchan $1,000 (incl. urgency bonus; paid via NewDot)

@hayata-suenaga Can you confirm this summary is right, or are the contributors splitting the payment?

@hayata-suenaga
Copy link
Contributor

https://github.com/Expensify/App/issues/26585#issuecomment-1704400588#27221

@pradeepmdk made the PR but @Pujan92 helped us along the way. I think the half $750 is appropriate for @Pujan92's help

@situchan
Copy link
Contributor

@JmillsExpensify I C+ reviewed PR

@JmillsExpensify
Copy link

Ok thanks everyone! I've updated the payment summary once more. It should now be accurate based on @hayata-suenaga's comment above. We're ready for payments.

@JmillsExpensify
Copy link

@situchan offer sent via upwork. I didn't include the issue reporting, though feel free to update or I will pay that one when I pay/close the contract.

@pradeepmdk I've issued payment based on the summary.

@Pujan92 I've also issued payment based on the summary.

@situchan
Copy link
Contributor

@JmillsExpensify yes, I am also eligible for reporting based on #26585 (comment)

@parasharrajat
Copy link
Member

@JmillsExpensify I also reviewed the PR. I had fully reviewed the PR by the time @situchan was assigned. I am sure @situchan didn't find any issues after my review. It is just that It was too late for me to do so I couldn't approve the PR and @hayata-suenaga asked someone to complete the checklist on Slack and thus @situchan reviewed it.

I was the primary C+ here. I should be equally eligible for C+ payment here.

@parasharrajat
Copy link
Member

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:

  • [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: None, seems like backend caused this crash.

  • [@parasharrajat] 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

  • [@parasharrajat] 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

  • [@parasharrajat] Determine if we should create a regression test for this bug. Yes

  • [@parasharrajat] 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 Steps

  1. Launch the New Expensify app
  2. Go to any workspace chat
  3. Go to FAB (➕) > Request money > Distance Tab.
  4. Throttle the internet to 3G.
  5. Create a distance request.
  6. Immediately after the request is created (when it shows $0.00), click on the IOU to open the IOU thread and wait for a while.
  7. The app should not crash.
  8. Relaunch/reload the app
  9. Go to the created distance IOU
  10. Verify there is no crash.

Do you agree 👍 or 👎 ?

@JmillsExpensify
Copy link

@situchan offer sent via upwork. I didn't include the issue reporting, though feel free to update or I will pay that one when I pay/close the contract.

Yes agreed.

@JmillsExpensify
Copy link

@hayata-suenaga Can you please comment on @parasharrajat's post above? I'm not sure why we'd pay $1,500 for both of C+, just like we didn't do that for Contributor either.

@parasharrajat
Copy link
Member

parasharrajat commented Sep 18, 2023

I think it should be this way. This is what I have seen.

  1. Primary C+ issue price + bonus if applicable.
  2. other c+ normal/standard payout.

@melvin-bot melvin-bot bot added the Overdue label Sep 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

@JmillsExpensify, @Pujan92, @pradeepmdk, @parasharrajat, @hayata-suenaga Whoops! This issue is 2 days overdue. Let's get this updated quick!

@hayata-suenaga
Copy link
Contributor

waiting for payment

@melvin-bot melvin-bot bot removed the Overdue label Sep 25, 2023
@JmillsExpensify
Copy link

JmillsExpensify commented Sep 25, 2023

Updated payment summary:

Issue reporter: @situchan $50
Contributor: @Pujan92 $750 (incl. urgency bonus)
Contributor: @pradeepmdk $1,500 (incl. urgency bonus)
Contributor+: @situchan $1,000
Contributor+: @parasharrajat $1,500 (incl. urgency bonus; paid via NewDot)

@JmillsExpensify
Copy link

@situchan paid for reporting and C+. @Pujan92 paid for contributor work. @pradeepmdk paid for contributor work. @parasharrajat is paid via NewDot. Now that the regression test is created, I think we're all done here. Closing.

@parasharrajat
Copy link
Member

Payment requested as per #26585 (comment)

@JmillsExpensify
Copy link

$1,500 payment approved for @parasharrajat based on comment above.

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests