[HOLD for payment 2022-11-18] [$250] Attachments - Preview is broken for Protected PDF file (iOS and Android)

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Launch the app
2. Log in with any account
3. Attach any protected PDF file
4. Open the PDF file

Expected Result:

Preview is not broken for Protected PDF file

Actual Result:

Preview is broken for Protected PDF file

Workaround:

Unknown

Platform:

Where is this issue occurring?

• iOS
• Android

Version Number: 1.2.24.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

RPReplay_Final1667585840.MP4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[syedsaroshfarrukhdot]

Proposal

The issue is in PDFView index.native.js as we need to pass height and width to KeyboardAvoidingView

App/src/components/PDFView/index.native.js

Lines 145 to 153 in 38b1d84

	{this.state.shouldRequestPassword && (
	<KeyboardAvoidingView>
	<PDFPasswordForm
	onSubmit={this.attemptPdfLoadWithPassword}
	onPasswordUpdated={() => this.setState({isPasswordInvalid: false})}
	isPasswordInvalid={this.state.isPasswordInvalid}
	shouldShowLoadingIndicator={this.state.shouldShowLoadingIndicator}
	/>
	</KeyboardAvoidingView>

We need to pass styles style={[styles.w100, styles.h100]} to KeyboardAvoidingView

After Fixing :-

[0xmiros]

Proposal

This is a regression from #11586 which replaced our own KeyboardAvoidingView with default react native KeyboardAvoidingView.
And react-native KeyboardAvoidingView isn't vertically stretched by default.

Solution:
https://github.com/Expensify/App/blob/main/src/components/PDFView/index.native.js

            <View style={containerStyles}>
                {this.state.shouldAttemptPdfLoad && (
                    <TouchableWithoutFeedback style={touchableStyles}>
                        <PDF
                            trustAllCerts={false}
                            renderActivityIndicator={() => <FullScreenLoadingIndicator />}
                            source={{uri: this.props.sourceURL}}
                            style={pdfStyles}
                            onError={this.initiatePasswordChallenge}
                            password={this.state.password}
                            onLoadComplete={this.finishPdfLoad}
                        />
                    </TouchableWithoutFeedback>
                )}
                {this.state.shouldRequestPassword && (
-                    <KeyboardAvoidingView>
+                    <KeyboardAvoidingView style={styles.flex1}>
                        <PDFPasswordForm
                            onSubmit={this.attemptPdfLoadWithPassword}
                            onPasswordUpdated={() => this.setState({isPasswordInvalid: false})}
                            isPasswordInvalid={this.state.isPasswordInvalid}
                            shouldShowLoadingIndicator={this.state.shouldShowLoadingIndicator}
                        />
                    </KeyboardAvoidingView>
                )}
            </View>

NOTE:

We don't need to stretch horizontally (styles.w100) because:

• When isSmallScreenWidth = true, containerStyles has it already
  

  App/src/components/PDFView/index.native.js

  Lines 125 to 127 in 0724337

  	const containerStyles = this.state.shouldRequestPassword && this.props.isSmallScreenWidth
  	? [styles.w100, styles.flex1]
  	: [styles.alignItemsCenter, styles.flex1];

• When isSmallScreenWidth = false, styles.alignItemsCenter added but in PDFPasswodForm containerStyle width manually set to 350
  

  App/src/components/PDFView/PDFPasswordForm.js

  Lines 98 to 100 in 0724337

  	const containerStyle = this.props.isSmallScreenWidth
  	? [styles.flex1, styles.w100]
  	: styles.pdfPasswordForm.wideScreenWidth;

[conorpendergrast]

• The bug is not a duplicate report of an existing GH
• The bug is reproducible, following the reproduction steps: Reproduced on iOS
• The GH template is filled out as fully as possible -- this means the GH body and title are clear
• The GH was created by an Expensify employee or a QA tester.
• If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification.
• Decide if the GH should be resolved by an External contributor or Internal engineer: External!

Testing file, password is Password:
Password Protected.pdf

[melvin-bot]

Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

[melvin-bot]

Triggered auto assignment to @thienlnam (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[conorpendergrast]

Internal job posting: https://www.upwork.com/ab/applicants/1589438303439740928/job-details
External job posting: https://www.upwork.com/jobs/~0123bb377c6009399a

[conorpendergrast]

@parasharrajat Two proposals for you to review already!

[parasharrajat]

The proposals look good. I will share my review tomorrow morning CST.

[parasharrajat]

@0xmiroslav 's proposal flex1 sounds better.

cc: @thienlnam

🎀 👀 🎀 C+ reviewed

[0xmiros]

@parasharrajat I already fixed it in this PR because without this fix, cannot test pdf password form.

[tgolen]

I like the flex1 proposal as well. Can someone explain for my own knowledge, why the style is applied to KeyboardAvoidingView rather than the first child component, in this case, PDFPasswordForm? I have a few other questions, mainly dealing with code consistency.

This KeyboardAvoidingView in BaseScreenWrapper:

App/src/components/ScreenWrapper/BaseScreenWrapper.js

Line 79 in 2410290

<KeyboardAvoidingView style={[styles.w100, styles.h100]} behavior={this.props.keyboardAvoidingViewBehavior}>

I see it has the styles.w100, styles.h100 styles, which is probably where the first proposal in this issue got the idea. Can this component also be changed to just using a flex1 style?

Should we maybe have something like styles.keyboardAvoidingView which is a style applied to every KeyboardAvoidingView component so they are all consistent? (Maybe not the one in SigninPageContent since that has some crazy styles already)

[parasharrajat]

Should we maybe have something like styles.keyboardAvoidingView which is a style applied to every KeyboardAvoidingView component so they are all consistent?

I like this one.

I see it has the styles.w100, styles.h100 styles, which is probably where the first proposal in this issue got the idea. Can this component also be changed to just using a flex1 style?

Previously, we had styles.w100, styles.h100 styles on the KeyboardAvoidingView. I guess it used to fit well everywhere. If this can be replaced with flex1. That would be great.
@0xmiroslav Any thoughts?

[0xmiros]

why the style is applied to KeyboardAvoidingView rather than the first child component, in this case, PDFPasswordForm

Except ScrollView (SectionList, FlatList are also ScrollView), react-native views (View, KeyboardAvoidingView) don't fully stretch height as default and height depends on child views' height sum (similar to wrap_content in android).
You can easily confirm this by removing children inside KeyboardAvoidingView and setting background color:

• <KeyboardAvodingView style={{backgroundColor: 'red'}} />: no background on screen
• <KeyboardAvodingView style={{flex: 1, backgroundColor: 'red'}} />: screen is filled with red

[0xmiros]

Previously, we had styles.w100, styles.h100 styles on the KeyboardAvoidingView

width: '100%' is needed when parent view of KeyboardAvoidingView has style of alignItems: 'center' with flexDirection: 'column'.
Or adding alignSelf: 'stretch' to KeyboardAvoidingView is alternative.

Here's good example of height: '100%' vs flex: 1. Hope you get the difference clearly from this video:
https://drive.google.com/file/d/1I1jpB0qqZ2xasS3p2ushB8VmbnohsnO-/view?usp=share_link

[parasharrajat]

Thanks for the video. So I think we can create a follow-up PR to replace the styles.w100, styles.h100 styles with styles.flex1, styles.w100 then move this to its own style styles.keyboardAvoidingView. Replace everywhere the styles with this.

Will that work?

[0xmiros]

yes will work. but everywhere is just 2 cases.
Currently there are only 3 usages of KeyboardAvoidingView throughout the app:

• BaseScreenWrapper
• PDFView
• SignInPageContent (Maybe not the one in SigninPageContent since that has some crazy styles already)

[parasharrajat]

Yeah, that is fine to setup a norm.

[tgolen]

Yeah, that sounds great!

That video shows it well. Thanks!

[parasharrajat]

@tgolen Do you want to assign this to @0xmiroslav?

[melvin-bot]

📣 @0xmiroslav You have been assigned to this job by @thienlnam!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[conorpendergrast]

The job:

• Internal job posting: https://www.upwork.com/ab/applicants/1589438303439740928/job-details
• External job posting: https://www.upwork.com/jobs/~0123bb377c6009399a

Ok, I'm following this process to hire via Upwork. I'm hiring:

• The Contributor+: @parasharrajat
• The contributor fixing the issue: @0xmiroslav

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.26-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix issues in pdf password form #12547

If no regressions arise, payment will be issued on 2022-11-18. 🎊

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[tgolen]

Hi all, due to the work I did in #11586, which probably forced a regression in this area, please don't hold any regressions against the contributor.

[JmillsExpensify]

Ok great, thanks for the heads up! Noted, though we'll still wait for the regression period to pass.

[conorpendergrast]

Thanks Tim! Note to myself: Two days until we pay out :👍

[conorpendergrast]

It is time, so I shall pay using this SO: How do I pay an external contributor via Upwork?

I am paying these two people: #12480 (comment)

Assignment date: 2022-11-09 #12480 (comment)
Merge date: 2022-11-09 #12547 (comment)

That fits: Merged PR within 3 business days - 50% bonus

So that makes base of $250 each plus 50% bonus of $125 = $375.

Pay $375 and end contract on:

• The Contributor+: @parasharrajat
• The contributor fixing the issue: @0xmiroslav

And finally:

• Close the job

All done!