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 2021-12-06] [Pay on Tuesday 23rd Nov] Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending #5291

Closed
kavimuru opened this issue Sep 16, 2021 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@kavimuru
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Lunch the app
  2. Log in with any account
  3. Select any user
  4. Click on plus button
  5. Select Add Attachment
  6. Select Documents
  7. Select PDF with two and more pages, but don't send it
  8. Start scrolling the pages in Preview Page

Expected Result:

Able scroll PDF file more than 2 pages in Preview page before sending

Actual Result:

Unable scroll PDF file more than 2 pages in Preview page before sending

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.0.99-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5238841_Screen_Recording_20210915-203145_New_Expensify.mp4

Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label Sep 16, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@MelvinBot
Copy link

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Julesssss
Copy link
Contributor

Okay, I can reproduce this on Android. iOS Web and Desktop are fine. As the user is not blocked from sending PDFs and it only occurs on one platform I'm removing the deployBlocker label.

@Julesssss Julesssss added Weekly KSv2 Improvement Item broken or needs improvement. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Sep 16, 2021
@Julesssss
Copy link
Contributor

Viewing a previously shared attachment works fine, when sending an attachment you are able to scroll for about a second, and then it becomes slow and janky.

@Julesssss
Copy link
Contributor

I can't see any recent change which would have introduced this. This PR from a month ago fixed PDF pan and zoom and may have been the cause.

It was originally fixed here by @rdjuric, so maybe you would like to look into this issue too, once I make it external?

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Sep 16, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Sep 16, 2021
@isagoico isagoico changed the title Chat - -Unable to scroll PDF file more than 2 pages in Preview page before sending Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending Sep 17, 2021
@isagoico
Copy link

Issue reproducible during KI retests.

@JmillsExpensify
Copy link

Whoops, I've been in the trenches of N6. Just posted to Upwork: https://www.upwork.com/jobs/~01b0af8ac91b6afc7b

@MelvinBot MelvinBot removed the Overdue label Sep 21, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Sep 21, 2021
@mvtglobally
Copy link

Issue reproducible during KI retests.

@Julesssss
Copy link
Contributor

Still awaiting proposals

@MelvinBot MelvinBot removed the Overdue label Oct 27, 2021
@eVoloshchak
Copy link
Contributor

eVoloshchak commented Oct 30, 2021

The problem lies inside a Modal component used in AttachmentModal.
If attachment is PDF we pass CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE as Modal type, otherwise CONST.MODAL.MODAL_TYPE.CENTERED. This works correctly and passes 'centered_unswipeable' when we select PDF attachment. However, modal seems to ignore it, since scrolling is unavailable. If we just hardcode 'centered_unswipeable' as modal type, everything works as expected, scroll is available. I'm not sure what kind of magic is happening here, since in both cases we pass identical strings, but I wasn't able to track the root cause of this issue.
However, if we store modalType in state and update it every time file changes, scrolling starts working.

Proposal:

  1. Add modalType key to state
this.state = {
    isModalOpen: false,
    isConfirmModalOpen: false,
    file: null,
    sourceURL: props.sourceURL,
    modalType: CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE,
};
  1. Implement getModalType method inside AttachmentModal component
getModalType(sourceUrl, file) {
    const modalType = (sourceUrl
        && (Str.isPDF(sourceUrl) || (file && Str.isPDF(file.name || this.props.translate('attachmentView.unknownFilename')))))
        ? CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE
        : CONST.MODAL.MODAL_TYPE.CENTERED;
    return modalType;
}
  1. Update modal type inside displayFileInModal
displayFileInModal: ({file}) => {
    if (!this.isValidSize(file)) {
        this.setState({isConfirmModalOpen: true});
        return;
    }
    if (file instanceof File) {
        const source = URL.createObjectURL(file);
        const modalType = this.getModalType(source, file);
        this.setState({
            isModalOpen: true,
            sourceURL: source,
            file,
            modalType,
        });
    } else {
        const modalType = this.getModalType(file.uri, file);
        this.setState({
            isModalOpen: true,
            sourceURL: file.uri,
            file,
            modalType,
        });
    }
},
before.mp4
_after.mp4

@mvtglobally
Copy link

Issue reproducible during KI retests.

@mallenexpensify
Copy link
Contributor

@Julesssss can you review @eVoloshchak proposal above? #5291 (comment)

Created new job in Upwork, other had expired
Doubled price to $500 https://www.upwork.com/jobs/~01ff20b40c42f9bf74

@mallenexpensify
Copy link
Contributor

@Julesssss , please @eVoloshchak proposal above #5291 (comment)

We are placing all non-critical issues on hold while we're on a company offsite. The hold is expected to be lifted on 11/19 (but could go longer). For any PRs that are submitted before or during the hold, we will add a $250 bonus payment.

@Julesssss
Copy link
Contributor

Sorry Matt, I was OOO last week (but set to 10% and still was assigned issues).

@MelvinBot MelvinBot removed the Overdue label Nov 8, 2021
@Julesssss
Copy link
Contributor

Hi @eVoloshchak, your proposal looks great. Let's get you hired in UpWork.

@eVoloshchak
Copy link
Contributor

Hi @eVoloshchak, your proposal looks great. Let's get you hired in UpWork.

Hi, I've submitted a proposal

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 8, 2021
@mallenexpensify
Copy link
Contributor

Hired @eVoloshchak in Upworkk and assigned issue to them 🎉

@Julesssss
Copy link
Contributor

Merged, awaiting payment.

@Julesssss Julesssss changed the title Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending [Pay on Tuesday 23rd Nov] Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending Nov 16, 2021
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@mallenexpensify
Copy link
Contributor

Paid @eVoloshchak in Upwork! Added Company Offsite Hold bonus too

@MelvinBot MelvinBot removed the Overdue label Nov 25, 2021
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 29, 2021
@botify botify changed the title [Pay on Tuesday 23rd Nov] Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending [HOLD for payment 2021-12-06] [Pay on Tuesday 23rd Nov] Chat - Unable to scroll PDF file more than 2 pages in Preview page before sending Nov 29, 2021
@botify
Copy link

botify commented Nov 29, 2021

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

If no regressions arise, payment will be issued on 2021-12-06. 🎊

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 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants