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

148 - Prominent disclosure for storage permission #250

Merged
merged 7 commits into from
Apr 5, 2022

Conversation

latin-panda
Copy link
Contributor

@latin-panda latin-panda commented Mar 21, 2022

Description

This PR:

  • Adds a prominent disclosure for when requesting storage permission
  • Updates location prominent disclosure to use intent's callback contracts.
  • Handle "never ask again" or when permission is denied for second time or when the permission is denied from the App's Settings page, at this point we can't request the grant automatically so the user is redirected to the app's settings to manually grant the permission.

Tested in: Android 6, 9 and 11.

Ticket: #148

Screenshots

Location prominent disclosure looks the same:

New storage prominent disclosure:

Redirection to app's settings when we can't get the permission grant automatically:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@latin-panda
Copy link
Contributor Author

Hey @jkuester, this is a draft, I still need to work the test coverage and see compatibility with older android devices.
But following up out conversation from before, I think I found a solution using callback contracts but also adapted to balance our new code vs the old code. I'm interested to know your thoughts on this and also the Java code 😬

The flow is like this:

The feature(s) makes a regular startActivityForResult, this is to connect with the old code.

Then our new activity will display a disclosure explaining why we need the permission and if the user agrees then it launches another activity to request permissions, once the permission is granted it will execute the callback contract (we're saving 1 request code by doing this).

The callback will resolve the activity and give control back to the EmbeddedBrowserActivity, the old code, to determine how it resumes the action .

EmbeddedBrowserActivity extends from a very basic Activity class, that doesn't support callback contracts, I'm suggesting to change this later, so we can control better the resume of actions by using callbacks and reduce the usage of request code.

When looking into this solution, I tried to keep the "resume of the action" inside the feature class (CHTExternalAppHandler, FilePickerHandler) by making the feature an action, but I couldn't find a way of passing objects as intent extras, I wanted to pass these parameters. And since the feature action's intent needs to be started by EmbeddedBrowserActivity, it was becoming too much structure, so I decided to drop it for now.

@latin-panda
Copy link
Contributor Author

I've tested the solution in devices with Android 6, 9 and 11. Working fine!

Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

This is great! Chaining the intents via the PermissionActivity classes cleans up the logic flow a lot.

Just a few minor suggestions/questions inline.

@latin-panda latin-panda marked this pull request as ready for review March 24, 2022 15:18
@latin-panda latin-panda requested a review from jkuester March 24, 2022 15:45
@latin-panda
Copy link
Contributor Author

@jkuester can you please have a look again? Thanks!

Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple more minor suggestions/questions. I made it through all the impl code, but have not looked at the tests yet.

@latin-panda latin-panda requested a review from jkuester March 28, 2022 10:11
@latin-panda
Copy link
Contributor Author

Thanks for the review! @jkuester I've fixed all the feedback. Just this pending comment, I'm okay removing that flag in this PR or in another, what do you think? :)

Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Tests look great! Very thorough. This is ready to go (pending what you want to do with the SettingsStore changes). 🚀

@latin-panda latin-panda linked an issue Mar 29, 2022 that may be closed by this pull request
@latin-panda latin-panda merged commit ef3cf1a into master Apr 5, 2022
@latin-panda latin-panda deleted the 148-prominent_disclosure_read_storage branch April 5, 2022 09:46
melema120 pushed a commit to livinggoods/medic-android that referenced this pull request Apr 27, 2022
Ticket: medic#148

This commit:
- Adds a prominent disclosure for when requesting storage permission
- Updates location prominent disclosure to use intent's callback contracts.
- Handle "never ask again" or when permission is denied for second time or when the permission is denied from the App's Settings page, at this point we can't request the grant automatically so the user is redirected to the app's settings to manually grant the permission.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prominent disclosure when "handling users' Files"
2 participants