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.

Sign up for GitHub

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

Jump to bottom

[FIX 17237] Updated usages of source. #19107

Merged
merged 10 commits into from
May 22, 2023
Merged

[FIX 17237] Updated usages of source. #19107

Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
84367a8
[FIX 17237] Updated usages of source.
DanutGavrus May 17, 2023
e204aab
Merge branch 'main' into fix/17237-old-picture-in-preview
DanutGavrus May 18, 2023
c5f09fb
[FIX 17237] Reverted usage before attachment view.
DanutGavrus May 18, 2023
9591c83
[FIX 17237] Reverted usage before attachment view.
DanutGavrus May 18, 2023
9b164ae
Merge branch 'main' into fix/17237-old-picture-in-preview
DanutGavrus May 18, 2023
005cc6c
[FIX 17237] Unified back all usages of source in render.
DanutGavrus May 18, 2023
022e526
[FIX 17237] Downloaded image could not be opened afterwards.
DanutGavrus May 18, 2023
239115f
Merge branch 'main' into fix/17237-old-picture-in-preview
DanutGavrus May 19, 2023
b293467
Merge branch 'main' into fix/17237-old-picture-in-preview
DanutGavrus May 20, 2023
a4a6569
[FIX 17237] Run prettier.
DanutGavrus May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ class AttachmentModal extends PureComponent {
}

render() {
const source = this.state.source;
const source = this.props.source || this.state.source;
return (
<>
<Modal
Expand All @@ -274,7 +274,7 @@ class AttachmentModal extends PureComponent {
title={this.props.headerTitle || this.props.translate('common.attachment')}
shouldShowBorderBottom
shouldShowDownloadButton={this.props.allowDownload}
onDownloadButtonPress={() => this.downloadAttachment(source)}
onDownloadButtonPress={() => this.downloadAttachment(this.state.source)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

source from here was previously set to this.state.source and the download functionality was working properly. By updating this, after I was downloading an image, I couldn't open it locally because it was wrongly downloaded. Updated it back to fix this & updated the QA steps with this use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate more on the issue ? how is it "wrongly downloaded" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to open the file after downloading it shows me this:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see it , the downloadAttachment require urls to be singed ( with auth signature ) , if url isn’t signed you can’t download them

onCloseButtonPress={() => this.setState({isModalOpen: false})}
/>
<View style={styles.imageModalImageCenterContainer}>
Expand All @@ -286,8 +286,7 @@ class AttachmentModal extends PureComponent {
onToggleKeyboard={this.updateConfirmButtonVisibility}
/>
) : (
Boolean(this.state.source) &&
this.state.shouldLoadAttachment && (
Boolean(source) && this.state.shouldLoadAttachment && (
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed, I was thinking to update the usage to source there and let this.state.shouldLoadAttachment be managed for uploading files. I may revert it back if you say so, maybe it's unnecessary. Let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: as we are using source={source} 3 lines below in the AttachmentView which shows only if the previous conditions are true, I think it's ok to update it like this.

<AttachmentView
containerStyles={[styles.mh5]}
source={source}
Expand Down