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-03-28] Make sure attachments are fetched from appropriate servers #15087

Closed
mountiny opened this issue Feb 13, 2023 · 35 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 Engineering

Comments

@mountiny
Copy link
Contributor

Coming from here

Problem

Chat attachments are loaded from API not matching the client environment. So you can be on Staging app but the chat attachment is loaded from production

Why is it important

Environment inconsistency can make it harder to find regressions and can introduce some unexpected ones in one deployment cycle

Solution

Make sure the production App loads the chat attachments from production api and staging from appropriate api server. cc @kidroca

@mountiny mountiny added Engineering Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 13, 2023
@mountiny mountiny self-assigned this Feb 13, 2023
@MelvinBot
Copy link

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 13, 2023
@MelvinBot
Copy link

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@Expensify Expensify unlocked this conversation Feb 13, 2023
@mountiny
Copy link
Contributor Author

@kidroca Do you want to get assigned to this and work hourly for this issue?

@kidroca
Copy link
Contributor

kidroca commented Feb 13, 2023

@kidroca Do you want to get assigned to this and work hourly for this issue?

Yes!

@zanyrenney
Copy link
Contributor

Chatted to @mountiny in DM - I can unassign as kidroca has their own contract with expensify for bug fixes!

@zanyrenney zanyrenney removed their assignment Feb 14, 2023
@kidroca
Copy link
Contributor

kidroca commented Feb 15, 2023

Found another discrepancy here:

Preferences for the Staging toggle on dev

Right now on DEV the staging toggle defaults to ON

on DEV this toggle does not have any effect - we're always using the PROD server

The Use Staging Server toggle only works on staging, where you can use it to toggle between the prod and the staging server

I think if the toggle does not work to change the underlying api (on DEV) we should not render it at all, or at least render it disabled

@mountiny
Copy link
Contributor Author

I think if the toggle does not work to change the underlying api (on DEV) we should not render it at all, or at least render it disabled

This is correct and it is expected although I agree it is misleading.

On dev you change contents of .env file to change the API. I think we should hide the toggle on dev personally

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Feb 15, 2023
@kidroca
Copy link
Contributor

kidroca commented Feb 17, 2023

PR is ready for review:

@MelvinBot
Copy link

@kidroca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@kidroca
Copy link
Contributor

kidroca commented Feb 27, 2023

PR ready for review

But we found that it has overlapping with another PR so we started a discussion there

@MelvinBot
Copy link

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

@MelvinBot
Copy link

@kidroca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@kidroca
Copy link
Contributor

kidroca commented Mar 7, 2023

The PR was on hold, but is unblocked now:

I have to address some minor items from the latest review

@MelvinBot
Copy link

@kidroca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@kidroca
Copy link
Contributor

kidroca commented Mar 15, 2023

PR is ready and reviewed

@mountiny I think some minor changes dismissed your approval

@s77rt
Copy link
Contributor

s77rt commented Mar 21, 2023

  • The PR that introduced the bug has been identified: I think this was implemented this way from the start
  • The offending PR has been commented on: n/a ^
  • A discussion in #expensify-bugs has been started: Given this is not a regression, should we open a discussion? @mountiny @kidroca

Regression Test Proposal

  1. Login using https://staging.new.expensify.com/
  2. Open any chat and send an attachment
  3. Verify that the attachment url starts with https://staging.new.expensify.com/chat-attachments/
  4. Login using https://new.expensify.com/
  5. Open same chat
  6. Verify that the attachment url starts with https://new.expensify.com/chat-attachments/

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Mar 27, 2023
@mountiny
Copy link
Contributor Author

Looking good, thanks! @zanyrenney would you be able to pay $1000 to @s77rt please?

@melvin-bot melvin-bot bot removed the Overdue label Mar 30, 2023
@mountiny
Copy link
Contributor Author

Bumping this one @zanyrenney

@melvin-bot melvin-bot bot added the Overdue label Apr 3, 2023
@mountiny
Copy link
Contributor Author

mountiny commented Apr 3, 2023

Zany is ooo so asking internally for someone to help complete the payment here.

@melvin-bot melvin-bot bot removed the Overdue label Apr 3, 2023
@dylanexpensify
Copy link
Contributor

On it - @s77rt here's job to apply to! https://www.upwork.com/jobs/~014a7129dead94e399

@s77rt
Copy link
Contributor

s77rt commented Apr 3, 2023

@dylanexpensify Applied.

@dylanexpensify
Copy link
Contributor

offer sent!!

@s77rt
Copy link
Contributor

s77rt commented Apr 3, 2023

Accepted

@dylanexpensify
Copy link
Contributor

Paid and contract ended! RT issue here! Thanks for playing y'all!

@lanitochka17
Copy link

Issue reproducible with Windows10/Chrome

Recording.2884.mp4

@lanitochka17 lanitochka17 reopened this Aug 9, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2023
@zanyrenney
Copy link
Contributor

hmm @kidroca could you review please and check why this is still happening? cc. @mountiny @s77rt

Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2023
@zanyrenney zanyrenney reopened this Aug 9, 2023
@zanyrenney
Copy link
Contributor

didn't mean to close the issue.

@s77rt
Copy link
Contributor

s77rt commented Aug 9, 2023

@lanitochka17 @zanyrenney I think this is working as expected, the url is pointing to the staging server. Please note that the url can either be www.expensify.com or staging.exepnsify.com depending on the environment but never staging.new.expensify.com nor new.expensify.com.

@kidroca
Copy link
Contributor

kidroca commented Aug 10, 2023

@zanyrenney

hmm @kidroca could you review please and check why this is still happening? cc. @mountiny @s77rt

What @s77rt said in their reply is correct (thank you ❤️):

@lanitochka17 @zanyrenney I think this is working as expected, the url is pointing to the staging server. Please note that the url can either be www.expensify.com or staging.exepnsify.com depending on the environment

@lanitochka17 when you are on staging.new.expensify.com attachments should be served from staging.expensify.com exactly how it happens in the video you shared: #15087 (comment)

The actual bug we fixed here was that staging attachments were served from www.expensify.com and not from staging.expensify.com and vice versa

@zanyrenney I think we can close this one

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@zanyrenney
Copy link
Contributor

Awesome. Thank you @kidroca for the detailed explanation here. Closing this out.

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
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 Engineering
Projects
None yet
Development

No branches or pull requests

7 participants