Update attachment link URL replacing logic #14463

kidroca · 2023-01-23T09:14:37Z

Details

Update existing logic that changes URLs to resolve from API ROOT to handle more URL patterns.
URLs coming from these origins should be updated to resolve from API_ROOT

staging
prod
absolute urls (URLs starting with a / that have no origin part)

Fixed Issues

$ #14227
PROPOSAL: #14227 (comment)

Tests

Verify staging and prod urls are resolved from API ROOT
- Follow the steps from the issue description [HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from #14227 (comment) in order to create attachment links from staging and prod.
- Inspect the Network tab or the HTML source (Chrome Dev tools -> Elements tab) to verify any attachment url (staging or prod) is resolved against the API ROOT of the current environment
Verify absolute urls are resolved from API ROOT
- At the moment the backend is not yet saving attachments using the absolute path, so we need to simulate the result
- We need to inspect and modify Onyx data
- Find an attachment action and modify the html of the message so that the <img src="https://{env}.expensify.com/chat-attachments/{path}" /> part becomes <img src="/chat-attachments/{path}" />
  Example
  In Chrome dev console:
```
Onyx.connect({ key: 'reportActions_71955477', callback: actions => {
    const action = actions[932];
    action.message[0].html = action.message[0].html.replace('https://staging.expensify.com/', '/');
    Onyx.merge('reportActions_71955477', { 932: action });
}});
```
- Now refresh the browser. The (modified) attachment should resolve and load successfully from API ROOT
Verify all attachment usages are covered. All these places should resolve the attachment from API ROOT
- attachment thumbnail in chat message comments
- attachment preview modal
- attachment download button
- use different attachment types - image formats / document formats

Offline tests

Expected Offline behavior

Images that were seen recently should be visible and render instantly (from cache)
Images that haven't been already loaded would be blank

Expected slow internet behavior

Images that were seen recently should be visible and render instantly (from cache)
Other images would display a spinner / loader animation for a while until they load

QA Steps

Verify loading attachments still works
- Follow the steps to from the issue description [HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from #14227 (comment) in order to create attachment links from staging and prod.
- Inspect the Network tab or the HTML source (Chrome Dev tools -> Elements tab) to verify any attachment url (staging or prod) is resolved against the API ROOT of the current environment
- Example:
  1. Create attachment on PROD
  2. It would be saved with a https://www.expensify.com/... url
  3. Open staging web (and have Chrome dev tools open)
  4. See the same attachment
  5. It should be resolved from staging.expensify.com
Upload different attachment types using staging and prod and verify they work
- E.g. save a PDF, DOC, JPG, PNG on staging, save the same on prod. Verify they load successfully from both staging and prod
Verify all attachment usages are covered. All these places should load and render successfully
- attachment thumbnail in chat message comments
- attachment preview modal
- attachment download button
- use different attachment types - image formats / document formats

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot · 2023-01-24T08:30:03Z

@mountiny @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

mountiny

Code looks good to me in general! Great job! @mananjadhav will you be able to give this one a test?

src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

mananjadhav · 2023-01-24T14:40:13Z

Hey hey!!! Not sure what went wrong here, I missed all the conversation on the issue 😥

@mountiny I'll review the conversation and also review the PR. Sorry for missing the proposal review.

mountiny · 2023-01-24T16:04:44Z

@mananjadhav no problem at all, Kidroca brought this issue up and we went with his proposal so everything is fine.

… prod

mountiny

Thanks @kidroca, waiting for the testing now

mountiny · 2023-01-30T22:54:33Z

@mananjadhav Will you be bale to do the reviewer checklist as well?

mananjadhav · 2023-01-31T15:59:56Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-attachment-urls.mov

Mobile Web - Chrome

mweb-chrome-attachment-urls.mov

Mobile Web - Safari

mweb-safari-attachment-urls.mov

Desktop

desktop-attachment-urls.mov

iOS

ios-attachment-urls.mov

Android

android-attachment-urls.mov

@mountiny Completed the review list, attachment testing took some time.

mountiny

Thank you @kidroca @mananjadhav

OSBotify · 2023-01-31T17:55:04Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2023-01-31T18:23:09Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start nativeLaunch	18.600 ms → 18.935 ms (+0.335 ms, +1.8%)
App start regularAppStart	0.014 ms → 0.015 ms (+0.001 ms, +7.8%)
App start TTI	693.080 ms → 688.594 ms (-4.486 ms, -0.6%)
Open Search Page TTI	611.652 ms → 602.265 ms (-9.387 ms, -1.5%)
App start runJsBundle	197.594 ms → 186.903 ms (-10.691 ms, -5.4%)

Show details

Name	Duration
App start nativeLaunch	Baseline Mean: 18.600 ms Stdev: 1.227 ms (6.6%) Runs: 17 17 17 17 17 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 21 22 Current Mean: 18.935 ms Stdev: 1.795 ms (9.5%) Runs: 17 17 17 17 18 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 21 21 21 23 23 24
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (4.9%) Runs: 0.012614000122994184 0.012695000041276217 0.012695000041276217 0.012777000200003386 0.013142999727278948 0.013427999801933765 0.013508999720215797 0.01355000026524067 0.01375299971550703 0.013794000260531902 0.013915999792516232 0.013915999792516232 0.013957000337541103 0.01399700017645955 0.0139979999512434 0.014038000255823135 0.01411899970844388 0.014200999867171049 0.014240999706089497 0.014281999785453081 0.014322999864816666 0.01436399994418025 0.014607999939471483 0.014607999939471483 0.014607999939471483 0.01464799977838993 0.0148930000141263 0.015015000011771917 0.015056000091135502 Current Mean: 0.015 ms Stdev: 0.001 ms (6.8%) Runs: 0.013142999727278948 0.013264999724924564 0.013305999804288149 0.013631999958306551 0.014078999869525433 0.014119000174105167 0.014119999948889017 0.014281999785453081 0.014322999864816666 0.01436399994418025 0.014485999941825867 0.014607999939471483 0.014649000018835068 0.01501399977132678 0.015054999850690365 0.01509599993005395 0.015137000009417534 0.015298999845981598 0.01534100016579032 0.015380000229924917 0.015381000004708767 0.015664999838918447 0.01599099999293685 0.0159919997677207 0.016032000072300434 0.016113999765366316 0.01615400006994605 0.0163569999858737 0.0165200000628829 0.0165200000628829 0.0168869998306036
App start TTI	Baseline Mean: 693.080 ms Stdev: 35.456 ms (5.1%) Runs: 626.5636900002137 637.6922829998657 640.0184050002135 652.5137919997796 656.7209950000979 658.9662139997818 660.9063200000674 663.7737779999152 667.6600179998204 668.1336710001342 674.150117999874 674.9677320001647 675.5072340001352 676.4257820001803 685.1585809998214 690.349100000225 691.8598839999177 695.6721350001171 699.4195699999109 703.5023139999248 708.1721740001813 710.1368370000273 714.0408149999566 719.0693470002152 722.9836099999957 727.8737280000933 729.5988289997913 737.6799780000001 746.1574479998089 752.3865729998797 754.7385339997709 755.7632909999229 Current Mean: 688.594 ms Stdev: 21.055 ms (3.1%) Runs: 646.3322629998438 649.5617309999652 650.1125639998354 671.3310409998521 672.5855499999598 672.8042100002058 673.935097000096 674.0692879999988 676.6732379999012 678.3361100000329 678.3983800001442 680.2782530002296 681.5425630002283 682.9134920001961 687.6747869998217 688.2811870002188 690.2996180001646 692.5525369998068 693.7643760000356 694.0008439999074 696.3701430000365 700.9543229998089 706.6043830001727 708.0103980000131 708.2968290001154 712.6765689998865 715.3480659998022 715.9583490001969 722.8692180002108 735.2824209998362
Open Search Page TTI	Baseline Mean: 611.652 ms Stdev: 24.009 ms (3.9%) Runs: 565.3296719999053 572.9123539999127 574.5101729999296 575.8517259997316 579.1163329998963 580.7605389999226 591.1553139998578 595.3953450000845 596.9836430000141 598.6813570000231 601.3015540000051 604.8442799998447 605.7128909998573 610.1095789996907 611.3234459999949 612.9497479996644 615.0923660001718 616.4284260002896 617.3512780000456 618.3805339997634 618.5791420000605 623.0364580000751 623.6985679999925 628.2163090002723 631.8578699999489 632.4415690000169 635.9315590001643 638.184326000046 638.8039549998939 639.894328000024 658.71875 659.3092450001277 Current Mean: 602.265 ms Stdev: 19.093 ms (3.2%) Runs: 578.1209319997579 580.5354410000145 581.4763190001249 582.9656179999001 583.6535649998114 583.8728029998019 584.3013920001686 584.5055340002291 584.623128999956 586.055989000015 590.6280519999564 595.4514569998719 596.9394539999776 597.0887449998409 597.4442549999803 597.9624430001713 599.3940019998699 600.0806480003521 600.5074880002066 602.7712409999222 603.708862000145 604.6792000001296 608.6924239997752 610.3208010001108 612.504313999787 617.2628580001183 617.819213999901 624.1710620000958 628.5196529999375 636.1796480002813 646.3859869996086 653.8622650001198
App start runJsBundle	Baseline Mean: 197.594 ms Stdev: 24.341 ms (12.3%) Runs: 155 170 170 171 174 174 177 178 178 178 183 184 184 185 186 187 188 188 202 203 207 208 212 223 224 224 226 233 233 237 238 243 Current Mean: 186.903 ms Stdev: 15.111 ms (8.1%) Runs: 161 163 165 170 170 173 177 178 178 179 181 181 182 183 184 186 187 188 188 188 189 189 193 198 200 200 203 205 211 220 224

OSBotify · 2023-01-31T20:32:26Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.63-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mvtglobally · 2023-02-01T22:52:49Z

We are facing issue with this one. Not sure if something we are doing is not right, but images are opened from the wrong Environment
PDF which uploaded from staging not possible to open on the Prod

Screen.Recording.2023-02-02.at.00.16.55.1.mov

thienlnam · 2023-02-01T23:05:01Z

Yes I'm running into the same issue - failure to open a PDF uploaded from staging in prod

Though other file types seem to work fine

mountiny · 2023-02-01T23:08:41Z

@mvtglobally @thienlnam Can we confirm this was not malfunctioning before already? I know there were some issues with these.

thienlnam · 2023-02-01T23:11:20Z

Not sure how we'd do that because now staging has the change and we can't check the previous version of staging. Though I'm going to just create another issue for it #14739 and move forward with deploy

mvtglobally · 2023-02-01T23:27:41Z

this PR supposed to fix it, no?

mountiny · 2023-02-01T23:41:03Z

@mvtglobally do you get anything in the console? Or any error with that PDF which is not able to load.

Yeah this PR should be loading the attachments from appropriate endpoints.

OSBotify · 2023-02-01T23:59:04Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.63-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

kidroca · 2023-02-02T14:17:29Z

@thienlnam @mountiny

Can we confirm this was not malfunctioning before already? I know there were some issues with these.

I had this issue from before the PR, I've posted about it here: #14227 (comment)

I think at the moment only staging would behave correctly

I should have described it in the testing steps, but since the fix is not yet on prod we can't expect that prod would behave correctly until the PR is deployed
e.g. if you upload something on staging and view it from prod, it would still be loaded relative to staging, because the PR with the fix is not yet deployed on prod

mountiny · 2023-02-02T14:50:27Z

@kidroca Its deployed not #14463 (comment)

@mvtglobally Would you be able to re-test with the latest App version, please? Thanks!

mountiny · 2023-02-02T14:51:16Z

Oh @kidroca tested here and it seems to work now! Great 🎉

kidroca mentioned this pull request Jan 23, 2023

Image Web/Desktop: Add support for http headers #13036

Merged

56 tasks

kidroca marked this pull request as ready for review January 24, 2023 08:29

kidroca requested a review from a team as a code owner January 24, 2023 08:29

melvin-bot bot requested review from mananjadhav and mountiny and removed request for a team January 24, 2023 08:30

mountiny assigned kidroca Jan 24, 2023

mountiny reviewed Jan 24, 2023

View reviewed changes

src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js Outdated Show resolved Hide resolved

kidroca added 3 commits January 25, 2023 13:36

ImageRenderer - handle attachments url from staging being rendered on…

de376df

… prod

Extract replaceSourceOrigin.js to libs and refactor usages

5930b4a

replaceSourceOrigin - update to cover for absolute URLs

482e13f

kidroca force-pushed the kidroca/fix/attachment-link-urls branch from e3e7e38 to 04c2e21 Compare January 25, 2023 11:38

Documentation and better naming for tryResolveUrlFromApiRoot

72a2950

kidroca force-pushed the kidroca/fix/attachment-link-urls branch from 04c2e21 to 72a2950 Compare January 25, 2023 11:43

mountiny reviewed Jan 25, 2023

View reviewed changes

mananjadhav approved these changes Jan 29, 2023

View reviewed changes

mountiny approved these changes Jan 31, 2023

View reviewed changes

mountiny merged commit ce50a01 into Expensify:main Jan 31, 2023

OSBotify mentioned this pull request Jan 31, 2023

Deploy Checklist: New Expensify 2023-01-31 #14709

Closed

14 tasks

kidroca deleted the kidroca/fix/attachment-link-urls branch February 1, 2023 12:22

thienlnam mentioned this pull request Feb 1, 2023

Error loading PDF uploaded from staging in production #14739

Closed

6 tasks

melvin-bot bot mentioned this pull request Feb 1, 2023

[HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from #14227

Closed

1 task

kidroca mentioned this pull request Feb 16, 2023

Refactor: Create libs/ApiUtils #15178

Merged

74 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update attachment link URL replacing logic #14463

Update attachment link URL replacing logic #14463

kidroca commented Jan 23, 2023 •

edited

Loading

melvin-bot bot commented Jan 24, 2023

mountiny left a comment

mananjadhav commented Jan 24, 2023

mountiny commented Jan 24, 2023

mountiny left a comment

mountiny commented Jan 30, 2023

mananjadhav commented Jan 31, 2023

mountiny left a comment

OSBotify commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

OSBotify commented Jan 31, 2023

mvtglobally commented Feb 1, 2023

thienlnam commented Feb 1, 2023

mountiny commented Feb 1, 2023

thienlnam commented Feb 1, 2023

mvtglobally commented Feb 1, 2023

mountiny commented Feb 1, 2023

OSBotify commented Feb 1, 2023

kidroca commented Feb 2, 2023

mountiny commented Feb 2, 2023

mountiny commented Feb 2, 2023

Update attachment link URL replacing logic #14463

Update attachment link URL replacing logic #14463

Conversation

kidroca commented Jan 23, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 24, 2023

mountiny left a comment

Choose a reason for hiding this comment

mananjadhav commented Jan 24, 2023

mountiny commented Jan 24, 2023

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Jan 30, 2023

mananjadhav commented Jan 31, 2023

Reviewer Checklist

Screenshots/Videos

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Jan 31, 2023

mvtglobally commented Feb 1, 2023

thienlnam commented Feb 1, 2023

mountiny commented Feb 1, 2023

thienlnam commented Feb 1, 2023

mvtglobally commented Feb 1, 2023

mountiny commented Feb 1, 2023

OSBotify commented Feb 1, 2023

kidroca commented Feb 2, 2023

mountiny commented Feb 2, 2023

mountiny commented Feb 2, 2023

kidroca commented Jan 23, 2023 •

edited

Loading