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

Fix missing photo permission message in image block #19577

Closed
wants to merge 2 commits into from

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Nov 10, 2022

Fixes #19576

MediaPicker-iOS handles the display of a message when the app lacks photo permission. By implementing a delegate method, the app was overriding this and presenting the prompt from the block editor screen which is hidden behind the media picker screen, leading to this error:

2022-11-10 00:01:35.281384-0700 Jetpack[7225:42366] [Presentation] Attempt to present <UIAlertController: 0x7fa85aa9d200> on <WordPress.AztecNavigationController: 0x7fa849195400> (from <WordPress.GutenbergViewController: 0x7fa849030800>) whose view is not in the window hierarchy.

To fix this, I removed the delegate method. This means that the media picker doesn't try to call the delegate method (see code and instead calls wpm_showAlertWithError, which in turn shows the same permission alert that's shown when adding media via My Site → Menu → Media (which presents the alert on the media picker view controller, which is on-screen).

To test:

  1. Ensure the app does not have access to photos:
    a. Go My Site tab → Menu → Media
    b. Go to add media, but deny the app permission
  2. Now open a new post
  3. Add the image block
  4. Tap "Choose from device"
  5. Notice the app now shows an alert prompting users to enable the missing permission

Regression Notes

  1. Potential unintended areas of impact

Image block error handling could be inadvertently affected by this change.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

I tested adding images via the image block both with and without the photo permission.

  1. What automated tests I added (or what prevented me from doing so)

There were no existing tests I could easily build upon.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@guarani guarani requested a review from fluiddot November 10, 2022 07:45
@guarani guarani changed the title Fix missing photo permission msg in image block Fix missing photo permission message in image block Nov 10, 2022
@guarani guarani added this to the 21.2 milestone Nov 10, 2022
@guarani
Copy link
Contributor Author

guarani commented Nov 10, 2022

Hi @fluiddot, I wasn't sure who to add here as a reviewer.

@guarani guarani marked this pull request as ready for review November 10, 2022 07:55
@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19577-5587e49 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19577-5587e49 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@guarani guarani modified the milestones: 21.2, 21.3 Nov 10, 2022
@guarani guarani marked this pull request as draft November 10, 2022 08:39
@guarani guarani removed the request for review from fluiddot November 10, 2022 08:40
@guarani
Copy link
Contributor Author

guarani commented Nov 10, 2022

@fluiddot, I noticed this change causes #18139 to regress, so I converted this back to a draft and removed you as reviewer. Sorry for the noise.

@oguzkocer oguzkocer modified the milestones: 21.3, 21.4 Nov 28, 2022
@spencertransier spencertransier modified the milestones: 21.4, 21.5 Dec 16, 2022
@mokagio
Copy link
Contributor

mokagio commented Jan 9, 2023

Moving this to the 21.6 milestone as I'm running the 21.5 code freeze today and this is in draft state.

@mokagio mokagio modified the milestones: 21.5, 21.6 Jan 9, 2023
@guarani guarani removed this from the 21.6 milestone Jan 13, 2023
@kean
Copy link
Contributor

kean commented Aug 25, 2023

I'm going to close it because it's not obsolete thanks to the changes made in #21190

@kean kean closed this Aug 25, 2023
@guarani
Copy link
Contributor Author

guarani commented Aug 25, 2023

Thanks for closing this, @kean!

@jkmassel jkmassel deleted the fix/missing-photo-permission-msg branch July 26, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank screen when selecting media when photo permission is missing
6 participants