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

Add ability to download statement PDF #7873

Merged
merged 12 commits into from
Mar 2, 2022
Merged

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented Feb 23, 2022

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/192002

Due to CSP / configuration issues, this can't easily be tested on dev mobile and will need to be prodQA

Tests

  1. Follow the setup steps from this PR
  2. Press the download button on the header bar
  3. Make sure the PDF downloads
  • Verify that no errors appear in the JS console

QA Steps

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2022-02-24 at 11 15 40 AM
Screen Shot 2022-02-24 at 11 16 05 AM

Mobile Web

Desktop

iOS

Android

@thienlnam thienlnam self-assigned this Feb 23, 2022
@thienlnam thienlnam requested a review from nickmurray47 March 1, 2022 02:08
@thienlnam thienlnam marked this pull request as ready for review March 1, 2022 23:33
@thienlnam thienlnam requested a review from a team as a code owner March 1, 2022 23:33
@MelvinBot MelvinBot requested review from deetergp and removed request for a team March 1, 2022 23:33
@thienlnam thienlnam added the InternalQA This pull request required internal QA label Mar 1, 2022
@thienlnam
Copy link
Contributor Author

Added a note to the description but we're having a difficult time testing it on mobile due to some certificate/CSP issues when loading oldDot so this will have to be a prodQA. This feature is not being used so it should not impact any functionality

nickmurray47
nickmurray47 previously approved these changes Mar 2, 2022
Copy link
Contributor

@nickmurray47 nickmurray47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/pages/wallet/WalletStatementPage.js Outdated Show resolved Hide resolved
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

@deetergp deetergp merged commit 1c05621 into main Mar 2, 2022
@deetergp deetergp deleted the jack-generatePDFStatement branch March 2, 2022 17:58
@MelvinBot
Copy link

Triggered auto assignment to @aldo-expensify (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

@botify
Copy link

botify commented Mar 2, 2022

@deetergp looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

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

@thienlnam
Copy link
Contributor Author

Tests passed?? Not sure why Emergency label is being added

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @deetergp in version: 1.1.42-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants