-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Chat - Attachment from old dot chat shown empty rectangles in ND #44445
Comments
Triggered auto assignment to @twisterdotcom ( |
@twisterdotcom 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 |
Sorry, I didn't get around to testing this again this week, my bad. Monday I will and get External on it I assume. |
Okay, I can recreate, but I wonder if this is something that will just be resolved by sending the correct OD > ND onyx updates. Also, I didn't turn my Mic on for this, but I'm not redoing it. 44445.mp4Asking internally: https://expensify.slack.com/archives/C03SSAQ7P/p1719851330833789 |
Okay, Onxy should be working for this, so going to make it external. |
Job added to Upwork: https://www.upwork.com/jobs/~017e4b531ab757c124 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 ( |
@twisterdotcom, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too... |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@twisterdotcom @dukenv0307 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@twisterdotcom, @dukenv0307 Still overdue 6 days?! Let's take care of this! |
@twisterdotcom, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it! |
ProposalPlease re-state the problem that we are trying to solve in this issue.attachments from old expensify cannot be previewed What is the root cause of that problem?What changes do you think we should make in order to solve the problem?We add a CORS header to the back-end to allow new.expensify.com What alternative solutions did you explore? (Optional) |
How is this solution @dukenv0307? |
@twisterdotcom Hi, I can't reproduce this bug. I think it's fixed |
@dukenv0307 new images are being loaded correctly, but old ones still same issue. Seeing that expensify and new.expensify are essentially same, we should add the header to the back-end to avoid any future CORS issues as well. |
@luacmartins or @justinpersaud - any more thoughts on this? |
FWIW I still can't find anything wrong with the headers. Also FWIW I can reproduce this issue on staging, but not on production. Screen.Recording.2024-09-30.at.5.12.23.PM.mov |
I'm also unable to reproduce on dev. So this seems to be affecting staging only. @twisterdotcom were you on production when you reproduced the issue? |
I cannot reliably reproduce this. I have seen it once, but never again. Even on staging. |
I put a PR up that I think may help here. |
I can still reproduce the issue on staging 😞 |
@luacmartins can you write out the full steps you're using? |
On staging:
I can't reproduce on production though |
Do you know if we changed anything with how we're fetching the image from the client in NewDot? If you copy that request as cURL and paste it in your browser with all the headers, you will see the response contains the headers it says are missing:
|
I'm not aware of any changes to that. I'll ask in open-source. So you're saying that we're missing the |
No, the headers should be in the response. But I don't understand this. Are you 100% able to reproduce it on staging now every time? I still cannot for some reason. Only that one time and then yesterday when we tried the new PR but then after revert, I couldn't reproduce again. |
Yea, it's been pretty consistent for me, but only on staging (can't repro on dev nor prod) |
Agree, this is only on staging for me too, so perhaps it's not super pressing. |
Yea, this will only affect internal employees and contributors for now. We should still fix it, but maybe the priority is a bit lower since it doesn't affect customers |
I don't remember seeing any/many bugs that are 'stuck' in staging and never hit production. Is this something we need to document somewhere? ie. in the main BZ SO, do we want to add something like "If a bug is stuck in staging and isn't affecting real-world users on production, feel free to treat it as lower priority' |
@luacmartins can you copy the request that is failing as curl or fetch and share it here? I tried repro steps again on staging, still can't reproduce it. I need the full headers, request method and URL that is failing. |
Ha now I can't reproduce using these steps anymore 🙃 I did notice that now the first request to get the attachment is consistently cancelled and the 2nd one succeeds. ![]() |
For preflight requests, the first request is supposed to be an OPTIONS request and we respond back with a 204, and then it is supposed to request the actual image. |
Closing as discussed here |
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.1-0
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): rybkina+062524old@gmail.com
Issue reported by: Applause - Internal Team
Issue found when executing PR https://github.com/Expensify/Web-Expensify/pull/42420
Action Performed:
Expected Result:
Attachment from old dot chat shown in ND with image
Actual Result:
Attachment from old dot chat shown empty rectangles in ND
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6524592_1719352523426.old_to_new.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @luacmartinsThe text was updated successfully, but these errors were encountered: