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

Use MIMEType for image download extension if not present in resource URL #6591

Merged
merged 11 commits into from
Oct 1, 2020

Conversation

akivab
Copy link
Contributor

@akivab akivab commented Sep 29, 2020

Recommend adding so that we can be creative about extension (without forcing the user to explicitly set in the URL provided, as it might be provided in storage metadata)

Addresses the following issue
#6590

Recommend adding so that we can be creative about extension (without forcing the user to explicitly set in the URL provided, as it might be provided in storage metadata)
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@akivab
Copy link
Contributor Author

akivab commented Sep 29, 2020

@chliangGoogle I could move some of this logic to another function to add tests, let me know what your preference would be. Thanks!

@paulb777
Copy link
Member

@akivab Thanks for the issue and PR! Routing to @chliangGoogle for review.

@akivab
Copy link
Contributor Author

akivab commented Sep 29, 2020

EDIT: Was able to get tests to work using the Simulator target instead of Mac thanks to Paul's comment below.

Original comment:
I have tried adding some tests but having trouble with automatic signing issues for the AppHost-FirebaseMessaging-Unit-Tests target (ld: file not found: /Users/akivab/Library/Developer/Xcode/DerivedData/FirebaseMessaging-hkzirhinkydyvnexsqhhikscqmru/Build/Products/Debug-maccatalyst/AppHost-FirebaseMessaging-Unit-Tests.app/Contents/MacOS/AppHost-FirebaseMessaging-Unit-Tests)

Attaching the diff so that it might serve as an example of the tests I'm planning to add (but that I didn't want to add due to the concern of being unable to run locally)

This is the git diff between what is in this PR and what I have suggested for tests.

Thanks so much!

changes_with_tests.txt

@paulb777
Copy link
Member

It might be easier to test on iOS than Catalyst. You can generate an iOS dev environment with:

pod gen FirebaseMessaging.podspec --auto-open --local-sources=./ --platforms=ios

@akivab
Copy link
Contributor Author

akivab commented Sep 29, 2020

Thanks so much @paulb777 -- I've updated my PR with some simple unit tests. @chliangGoogle open to any feedback.

Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you for doing this!

FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m Outdated Show resolved Hide resolved
FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m Outdated Show resolved Hide resolved
FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m Outdated Show resolved Hide resolved
@akivab
Copy link
Contributor Author

akivab commented Sep 30, 2020

Thanks @chliangGoogle, addressed comments.

@paulb777 paulb777 added this to the Firebase 7 milestone Sep 30, 2020
Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

one last nit bit.

Can you also update the changelog in FirebaseMessaging/CHANGELOG?
#unreleased
[fixed] Fixed the issue that failed to download image if no extension but MIME type is set. (#6590)

  • [fixed] Fix issue :3

@akivab
Copy link
Contributor Author

akivab commented Sep 30, 2020

Thanks @chliangGoogle -- added to the CHANGELOG.md and calling teardown on the mock now

Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

Sorry, my wording is bad, I corrected mine again.

Thank you very much doing this. I will approve this when it's okay to merge master. Our team is doing maintenance work at the moment. Once it's cleared, I will approve and you can merge to master.

FirebaseMessaging/CHANGELOG.md Show resolved Hide resolved
@akivab
Copy link
Contributor Author

akivab commented Sep 30, 2020

Perfect, sounds good. Contributing here has been a great experience, hoping to do more in the future.

@akivab akivab changed the title Use MIMEType if file does not have the right extension Use MIMEType for image download extension if not present in resource URL Sep 30, 2020
@akivab
Copy link
Contributor Author

akivab commented Oct 1, 2020

Hi @chliangGoogle, I don't have write access to this repo so expecting that you might be able to merge in. Thanks so much.

@paulb777
Copy link
Member

paulb777 commented Oct 1, 2020

I'll merge it now - to make sure that none of the upcoming major changes introduce a conflict.

Thanks for the PR @akivab !

@paulb777 paulb777 merged commit 49afa26 into firebase:master Oct 1, 2020
@akivab akivab deleted the patch-1 branch October 1, 2020 04:47
@firebase firebase locked and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants