Frenkield 8595 pdf password #9141

frenkield · 2022-05-23T23:51:28Z

Details

Added password validation form for viewing password-protected PDFs.

Fixed Issues

$ #8595

Tests

Normal PDF with Send button (regression)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a normal PDF (not password-protected)
Verify that the PDF renders correctly
Click Send button
Verify that the PDF is sent

Normal PDF with enter key (regression web and desktop)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a normal PDF (not password-protected)
Verify that the PDF renders correctly
Press enter key on keyboard
Verify that the PDF is sent

Protected PDF with send button

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Press Confirm button
Verify that password-required message appears
Enter an invalid password and press Confirm button
Verify that invalid-password message appears
Enter correct password and press Confirm button
Verify that the PDF renders correctly
Click Send button
Verify that the PDF is sent

Protected PDF with enter key (web and desktop)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Press enter key
Verify that password-required message appears
Enter an invalid password and press enter key
Verify that invalid-password message appears
Enter correct password and press enter key
Verify that the PDF renders correctly
Press enter key again
Verify that the PDF is sent

Protected PDF that was already sent

Open a chat where a protected PDF attachment was sent or received
Click on the icon for the PDF attachment
Verify that the password form appears
Press enter key
Verify that password-required message appears
Enter an invalid password and press enter key
Verify that error message appears
Enter correct password and press enter key
Verify that the PDF renders correctly

Protected PDF without viewing

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Click Send button
Verify that the PDF is sent

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Normal PDF with Send button (regression)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a normal PDF (not password-protected)
Verify that the PDF renders correctly
Click Send button
Verify that the PDF is sent

Normal PDF with enter key (regression web and desktop)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a normal PDF (not password-protected)
Verify that the PDF renders correctly
Press enter key on keyboard
Verify that the PDF is sent

Protected PDF with send button

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Press Confirm button
Verify that password-required message appears
Enter an invalid password and press Confirm button
Verify that invalid-password message appears
Enter correct password and press Confirm button
Verify that the PDF renders correctly
Click Send button
Verify that the PDF is sent

Protected PDF with enter key (web and desktop)

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Press enter key
Verify that password-required message appears
Enter an invalid password and press enter key
Verify that invalid-password message appears
Enter correct password and press enter key
Verify that the PDF renders correctly
Press enter key again
Verify that the PDF is sent

Protected PDF that was already sent

Open a chat where a protected PDF attachment was sent or received
Click on the icon for the PDF attachment
Verify that the password form appears
Press enter key
Verify that password-required message appears
Enter an invalid password and press enter key
Verify that error message appears
Enter correct password and press enter key
Verify that the PDF renders correctly

Protected PDF without viewing

Open a chat, click the plus button to the left of the message input, and select "Add attachment"
Select a password-protected PDF (there's one in a comment in this PR)
Verify that the password form appears
Click Send button
Verify that the PDF is sent

Verify that no errors appear in the JS console

Videos

Web

web_8595.mp4

mWeb - iOS Safari

ios_safari_8595.mp4

mWeb - Android Chrome

android_chrome_8595.mp4

Desktop

macos_desktop_8595.mp4

iOS

ios_native_8595.mp4

Android

android_native_8595.mp4

…andards

…ressOnEnter

…end-on-enter

…gray

…coding standards

frenkield · 2022-05-23T23:55:50Z

Here's a password-protected PDF for testing. The password is 1234

password-protected.pdf

tgolen · 2022-05-24T18:11:32Z

src/components/PDFView/PDFPasswordForm.js

+
+PDFPasswordForm.propTypes = propTypes;
+PDFPasswordForm.defaultProps = defaultProps;
+PDFPasswordForm.displayName = 'PDFPasswordForm';


I think this is only required for a functional component, but I'm not 100% sure when it comes to a PureComponent. If it's not necessary, would you mind removing it?

Ah yeah, displayName is only needed for functional components. I'll remove it.

tgolen · 2022-05-24T18:11:47Z

src/components/PDFView/PDFPasswordForm.js

+    isPasswordInvalid: false,
+};
+
+class PDFPasswordForm extends PureComponent {


Why use a PureComponent as opposed to just a regular component?

I did that just because PDFView/index.js uses PureComponent. I'll change it to a regular compoment.

tgolen · 2022-05-24T18:23:02Z

src/components/PDFView/index.js

+        const PasswordResponses = {
+            NEED_PASSWORD: 1,
+            INCORRECT_PASSWORD: 2,
+        };


I think it would be better to move these into CONST and just keep a comment with them about where they come from.

Ok I'll move it.

tgolen · 2022-05-24T18:23:38Z

src/components/PDFView/index.js

+
+    /**
+     * Send submitted password to react-pdf via its callback so it can attempt to
+     * open the password-protected PDf.


Suggested change

* open the password-protected PDf.

* open the password-protected PDF.

I'll fix that.

tgolen · 2022-05-24T18:25:13Z

src/components/PDFView/index.js

+     *
+     * @param {String} password The password entered in the form
+     */
+    openPdfWithPassword(password) {


Since this method only does one thing, and it passes the same arguments, I think the method is unnecessary and it should just reference this.onPasswordCallback directly.

Thanks, I'll remove that.

tgolen · 2022-05-24T18:25:46Z

src/components/PDFView/index.js

+    renderPasswordForm() {
+        return (
+            <PDFPasswordForm
+                onSubmit={this.openPdfWithPassword}
+                isPasswordInvalid={this.state.isPasswordInvalid}
+            />
+        );


Please leave all rendering in the render() method.

Ok I'll move that.

tgolen · 2022-05-24T18:27:58Z

src/components/PDFView/index.js

+                            Array.from(
+                                new Array(this.state.numPages),
+                                (el, index) => (


I'm not quite sure what this is doing. Why is Array.from() used and also new Array()?

We try to standardize by using _ methods, is it possible to use that here instead?

If not, would a simple for loop be better?

This is how the code was originally. I didn't touch it. Do you want me to refactor that?

Yes, please. I'm glad you didn't add it, but let's always be improving the code quality when we find things like this.

tgolen · 2022-05-24T18:34:40Z

src/components/PDFView/index.native.js

+     *
+     * @param {String} message
+     */
+    initiatePasswordChallenge({message}) {


It feels like a lot of this code is duplicated between native and web. Can it be abstracted better so there isn't so much duplication?

Ok sure, I can abstract that a bit.

I don't see a component where there's something like that. What should I name the base component file? Like index.base.js or BasePDFView.js?

Yes create a new BasePDFView.js

Ok thanks, @Santhosh-Sellavel.

And sorry, I do see examples like this.

Just move only codes that can be kept common for all platforms. The rest should be in platform-specific files.

Ok sounds good.

Hey so sorry, I don't see a nice way to do this. If I understand correctly we should not extend components. So doing something like in BaseTextInput.js, for example, doesn't seem possible to me.

The main issue is that things need to be handled quite differently on web and on native in order to acheive a proper password challenge experience:

On web, the react-pdf component must not be conditionally rendered - and thus hidden via styles when the password form is visible. This is because it receives the password via a callback function and so the PDF component needs to be persistent.

Whereas on native, the react-native-pdf must be conditionally rendered. This is because it doesn't detect when its props.password is updated.

Both versions of my code (web/native) use similar state variables (shouldRequestPassword and isPasswordInvalid) but otherwise the code is totally different.

Can someone maybe provide some specific direction on how to refactor this?

It seems fine to me. I don't feel the need to abstraction as both of them have different approaches.

OK, thank you for that explanation. I agree with you now that this is probably fine the way it is.

Santhosh-Sellavel

Just adding me as a reviewer

frenkield · 2022-05-24T19:20:19Z

I have to step out for a couple hours. I'll work on the changes later today. Thanks, All!

tgolen · 2022-05-24T21:24:39Z

I will be going OOO so I will be reassigning this for now.

frenkield · 2022-05-24T23:01:26Z

I pushed most of the changes. I'll push the BasePDFView.js change later today.

frenkield · 2022-08-27T21:27:42Z

PR updated, @parasharrajat.

parasharrajat

LGTM. Good work. There were lots of ups and downs, I appreciate your patience.

cc: @tgolen

PR 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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
- Any functional components have the displayName property
- 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

parasharrajat · 2022-08-27T22:10:02Z

src/components/PDFView/index.js

+     * The PasswordResponses constants used below were copied from react-pdf
+     * because they're not exported in entry.webpack.
+     *
+     * @param {*} callback Callback used to send password to react-pdf


Not a Blocker

Suggested change

* @param {*} callback Callback used to send password to react-pdf

* @param {Function} callback Callback used to send password to react-pdf

parasharrajat · 2022-08-31T07:37:25Z

Bump @tgolen, awaiting your review. Thanks.

tgolen

Looks great! I just noticed this one thing.

tgolen · 2022-08-31T08:50:04Z

src/components/PDFView/PDFPasswordForm.js

+            shouldShowForm: false,
+        };
+        this.submitPassword = this.submitPassword.bind(this);
+        this.validate = this.validate.bind(this);


This method is only called from submitPassword() and validateAndNotifyPasswordBlur() which are already bound to this, so this line can be removed.

frenkield · 2022-08-31T18:04:06Z

PR updated, @parasharrajat and @tgolen.

parasharrajat · 2022-09-05T08:06:42Z

Bump @tgolen

parasharrajat · 2022-09-05T10:05:45Z

@tgolen let's merge this. https://expensify.slack.com/archives/C02NK2DQWUX/p1662371689856979?thread_ts=1662366794.882629&cid=C02NK2DQWUX

melvin-bot · 2022-09-05T12:47:30Z

@tgolen 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.

tgolen · 2022-09-05T12:48:04Z

OK, there it goes! 🎉

Nice work getting through this one @frenkield

tgolen · 2022-09-05T12:54:41Z

It's OK, Melvin. The test is failing because this is an older PR where the comment formats don't match what the test is expecting regarding the reviewer checklist.

OSBotify · 2022-09-05T13:17:52Z

🚀 Deployed to staging by @tgolen in version: 1.1.97-2 🚀

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

frenkield · 2022-09-05T16:13:06Z

Thanks, @tgolen and @parasharrajat.

MonilBhavsar · 2023-01-23T09:18:56Z

src/components/PDFView/index.js

+                        onSubmit={this.attemptPdfLoad}
+                        onPasswordUpdated={() => this.setState({isPasswordInvalid: false})}
+                        isPasswordInvalid={this.state.isPasswordInvalid}
+                        shouldAutofocusPasswordField={!this.props.isSmallScreenWidth}


@tgolen @frenkield @parasharrajat @Santhosh-Sellavel
We're coming from this issue. Basically a request to autofocus password field on native devices. Any idea why this condition was added. Just want to clarify before we proceed with the linked issue. cc @sobitneupane

I tried to remember but didn't find the exact reason why we conditionally enabled it. But the main reason to disable the autofocus on the web was to avoid UX confusion with two buttons 'Confirm' and Send.

They both were laying over the keyboard which looked bad. #9141 (comment)

I don't specifically remember either.

hungvu193 · 2023-07-20T15:00:09Z

This PR introduced a regression: #22519
We should use translateKey instead of translation text to make sure it will be updated when we changed the language.

allroundexperts · 2023-07-31T01:47:47Z

src/components/PDFView/index.js

+        // Use window height changes to toggle the keyboard. To maintain keyboard state
+        // on all platforms we also use focus/blur events. So we need to make sure here
+        // that we avoid redundant keyboard toggling.
+        if (!this.state.isKeyboardOpen && this.props.windowHeight < prevProps.windowHeight) {


This line caused #22364 because it did not factor in the offline indicator.

cubuspl42 · 2023-09-02T08:24:47Z

src/components/PDFView/PDFPasswordForm.js

+                        )}
+                        <Button
+                            text={this.props.translate('common.confirm')}
+                            onPress={this.submitPassword}


Coming from #25570

We should have ensured that the virtual keyboard stays open on mWeb when a wrong password is provided.

I don't think that counts as a regression rather feature request.

frenkield added 13 commits May 19, 2022 08:19

feat: added pdf password form for web

b91a215

feat: pdf password form - added invalid password message

8f204c0

feat: pdf password form - use style to show/hide react-pdf

80eff1a

feat: added pdf password form for ios/android

7a0b368

feat: pdf password form - converted native PDFView to pure component

11a2151

feat: pdf password form - changed background color to white

caaa16c

feat: pdf password form - added more comments for native

d4cd9aa

feat: pdf password form - some updates to better conform to coding st…

fa7e2fd

…andards

feat: pdf password form - updated Button to handle changes to props.p…

b7260d9

…ressOnEnter

feat: pdf password form - added enable/disable option to attachment s…

eed5363

…end-on-enter

feat: pdf password form - updated text and changed confirm button to …

e756d42

…gray

feat: pdf password form - updated handler names to better conform to …

05f99a2

…coding standards

feat: pdf password form - removed unnecessary memo use in password form

5905124

frenkield marked this pull request as ready for review May 24, 2022 00:33

frenkield requested a review from a team as a code owner May 24, 2022 00:33

melvin-bot bot requested review from tgolen and removed request for a team May 24, 2022 00:33

frenkield mentioned this pull request May 24, 2022

[HOLD for payment 2022-09-15] [$500] Feature Request - Attachment- Unable to cancel the requested password for PDF file #8595

Closed

tgolen requested changes May 24, 2022

View reviewed changes

Santhosh-Sellavel reviewed May 24, 2022

View reviewed changes

tgolen requested a review from a team May 24, 2022 21:24

melvin-bot bot requested review from Santhosh-Sellavel and tgolen and removed request for a team May 24, 2022 21:24

tgolen removed their request for review May 24, 2022 21:24

feat: pdf password form - a few small changes requested in pull request

9d83ce1

feat: pdf password form - refactored password field blur handler

c391379

parasharrajat previously approved these changes Aug 27, 2022

View reviewed changes

feat: pdf password form - updated type for parameter comment

2bc6031

frenkield dismissed parasharrajat’s stale review via 2bc6031 August 27, 2022 22:16

tgolen requested changes Aug 31, 2022

View reviewed changes

feat: pdf password form - removed unecessary method bind

bfbf6f4

tgolen approved these changes Sep 5, 2022

View reviewed changes

tgolen merged commit dc6a69a into Expensify:main Sep 5, 2022

melvin-bot bot added the Emergency label Sep 5, 2022

OSBotify mentioned this pull request Sep 5, 2022

Deploy Checklist: New Expensify 2022-09-05 #10808

Closed

33 tasks

kbecciv mentioned this pull request Sep 5, 2022

Android - Attachment - PDF password form displayed lower than expected #10822

Closed

s77rt mentioned this pull request Jan 21, 2023

[HOLD for payment 2023-02-08] [$1000] password field not auto focused while opening password protected PDF #14265

Closed

2 tasks

MonilBhavsar reviewed Jan 23, 2023

View reviewed changes

hungvu193 mentioned this pull request Jul 20, 2023

[HOLD for payment 2023-07-24] [$1000] PDFPasswordForm: Error message is not translated to Spanish when changing language. #22519

Closed

6 tasks

allroundexperts reviewed Jul 31, 2023

View reviewed changes

allroundexperts mentioned this pull request Jul 31, 2023

[HOLD for payment 2023-07-28] [$1000] Chat - Send button disappears when 'No internet connection' banner appears on top of browser #22364

Closed

6 tasks

cubuspl42 reviewed Sep 2, 2023

View reviewed changes

cubuspl42 mentioned this pull request Sep 2, 2023

[HOLD for payment 2023-09-08] [$1000] mWeb - Chat - Hide the keyboard on click of confirm button #25570

Closed

6 tasks

tienifr mentioned this pull request Jun 4, 2024

[HOLD for payment 2024-06-28] [$250] mWeb - Attachment - Invisible send button above keyboard when PDF password focused #42667

Closed

6 tasks

	* open the password-protected PDf.
	* open the password-protected PDF.

	* @param {*} callback Callback used to send password to react-pdf
	* @param {Function} callback Callback used to send password to react-pdf

Frenkield 8595 pdf password #9141

Frenkield 8595 pdf password #9141

Conversation

frenkield commented May 23, 2022 • edited by AndrewGable Loading

Details

Fixed Issues

Tests

Normal PDF with Send button (regression)

Normal PDF with enter key (regression web and desktop)

Protected PDF with send button

Protected PDF with enter key (web and desktop)

Protected PDF that was already sent

Protected PDF without viewing

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Normal PDF with Send button (regression)

Normal PDF with enter key (regression web and desktop)

Protected PDF with send button

Protected PDF with enter key (web and desktop)

Protected PDF that was already sent

Protected PDF without viewing

Videos

Web

mWeb - iOS Safari

mWeb - Android Chrome

Desktop

iOS

Android

frenkield commented May 23, 2022

Choose a reason for hiding this comment

frenkield May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenkield May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

frenkield commented May 24, 2022

tgolen commented May 24, 2022

frenkield commented May 24, 2022

frenkield commented Aug 27, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

Choose a reason for hiding this comment

parasharrajat commented Aug 31, 2022

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenkield commented Aug 31, 2022

parasharrajat commented Sep 5, 2022

parasharrajat commented Sep 5, 2022

melvin-bot bot commented Sep 5, 2022

tgolen commented Sep 5, 2022

tgolen commented Sep 5, 2022

OSBotify commented Sep 5, 2022

frenkield commented Sep 5, 2022

Choose a reason for hiding this comment

frenkield commented May 23, 2022 •

edited by AndrewGable

Loading

frenkield May 24, 2022 •

edited

Loading

frenkield May 25, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading