Created a test for permissionPermanentlyDenied

[itsymbal]

The method call requires an activity, so I've added that (and an Application class which it requires and test configuration which that requires)

The test is failing.

The test is correct. The underlying library method does not work correctly.

[googlebot]

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 (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[samtstern]

@itsymbal thank you for sending this PR! If you sign the CLA, happy to merge.

[samtstern]

Could you please add one-line JavaDoc comments to this class and to TestApplication to specify why we need them? When reading this PR it's obvious, but it may not be obvious to future code readers.

[SUPERCILEX]

@samtstern the test is failing though according to @itsymbal. Also, that's making a system call so do we really need to test it? (And is it even possible?)

[samtstern]

Oh I didn't notice that the test was failing ... well yes I would want it to pass first! It should be possible to test this system call, we test a similar system call in the other test in the same file.

@itsymbal what was your main motivation for adding this test to the library?

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[itsymbal]

Added javadoc as requested.

@SUPERCILEX The test is exercising your library call EasyPermissions.permissionPermanentlyDenied(). I'm not sure what you mean by "that's making a system call so do we really need to test it?"

@samtstern My intention was to demonstrate a problem with the library itself. The call to 'permissionPermanentlyDenied()' method calls through to PermissionsHelper.permissionPermanentlyDenied() which calls to platform method shouldShowRequestPermissionRationale. That's not a correct implementation.
The platform method returns false in two cases - 1 - permission has not been requested, not even once, and 2 - permission has been denied, WITH the 'do not show again' (i.e, truly permanently denied). You can't tell between these two cases using platform methods alone - the platform permissions API is not well thought out.
The only way to tell for sure if a permission has been permanently denied is to check that you've asked for the permission (you have to persist that fact yourself, using, e.g. Preferences) AND that shouldShowRequestPermissionRationale is FALSE (meaning, you don't have this permission, and don't even bother showing rationale for it).

So, as I mentioned, the test is correct.
The fact that it's failing highlights a problem with the easypermissions library itself.

[SUPERCILEX]

Ohhhhhhhhh, so you're just saying you'd like us to perform the permission check for you? That would have been so much clearer if you made an issue 😂. Oh well, @samtstern do you want a PR or should we stay aligned with system behavior? I think what @itsymbal is saying makes sense so I'm for the PR.

[itsymbal]

Well, I'm happy to make an issue also. But how does one know an issue is resolved without making a failing test into a passing test, right?
Also, I thought I'd contribute something useful, and a test is as useful a thing as any.
FWIW my own Permissions implementation is here .
[here] (https://github.com/itsymbal/op-boilerplate/blob/master/app/src/main/java/com/orangepenguin/boilerplate/util/PermissionUtil.kt)

[SUPERCILEX]

Yeah, true. Do you want to go ahead and make your test pass then if @samtstern agrees with this change?

[samtstern]

Ah @itsymbal I get it now, thank you for expressing the problem as a failing test. I do appreciate how thorough that is!

I was aware of this issue, that's why the docs only suggest making this call after the permission has been denied:
https://github.com/googlesamples/easypermissions#required-permissions

As you said the platform API does not allow any way to determine this, but I don't think EasyPermissions should start persisting permission request information (feels like it would be error-prone).

I think the best thing to do would be to add a strongly-worded warning to both the README and the JavaDoc about exactly when this method is effective. What do you think about that?

[itsymbal]

Thanks, @samtstern.
Certainly, I would agree that persisting permission request information would be error-prone. I wouldn't advocate doing that without giving it some strong consideration.

I must admit I haven't considered your suggested approach - ask for permission first, and if denied, check 'shouldShowRequestPermissionRationale()' - and if false, this would tell you both things you need to know to determine that the perm's been denied permanently. I believe this approach would work(*). The asterisk being that it would work in situations where it's appropriate to ask for permissions without showing any rationale first.

But let's consider a different, entirely reasonable, scenario.
Let's say you need a perm, but it is not clear to the user from the context why you require it. It's not triggered by user action but needed at startup, or needed by the nature of what the app does. Let's say you're a News app and want to surface local weather along with the news headlines and so you need Location. Or you're a B.Good (restaurant) app (not mine, I'm merely a happy customer) and you have a function to let the user send a coupon or a gift to a contact - so you need to let the user search contacts (that's how it currently works). So you want to show rationale, before even asking the user to grant permission. (I'm having difficulty coming up with another real-world example but I distinctly remember being annoyed for being asked for permission on some app startup, without a rationale given, for reasons I didn't understand - so denying the perm, with prejudice)

So the app logic is something like:

if have permission, do the thing
else - if permanently denied - send to Settings (don't want the user to paint themselves into a corner, and never be able to do the thing ever again)
else - show rationale, then ask for permission.

If I use the approach you suggest, when checking for permanent denial (ask permission then check 'shouldShowRequestPermissionRationale()') - I violate the requirement to show rationale before asking for perm.
If I do show rationale first, then ask for perm, (and the user denied with "Don't ask again") - then the user has the incongruous experience of seeing the rationale, expecting to be prompted to grant permission, but the prompt never comes.
In fact, that's exactly how the B.Good app works - they show rationale before asking for perm, whether I denied it temporarily or permanently, but if I denied permanently, the perm request dialog never shows up. I've painted myself into a corner. Next time I start the app they will show rationale again, but no permission request.

So in this type of scenario, the suggested solution of asking for permission first, doesn't work well.

So, I am back to suggesting that the fact of having requested a permission be persisted - error-prone though it may be. I'm also suggesting a robust test coverage to detect some of the potential errors.

I do wonder though. With you being a Google insider, do you have the ear of the platform team, for the purpose of requesting a feature to check if the perm's been permanently denied? That's information the system has access to, it merely needs to be surfaced via an api call...

[samtstern]

@itsymbal thanks again for the detailed discussion. I think there are two kinds of "rationale" and only one of them is supported by EasyPermissions.

In this library we use "rationale" to mean "a small amount of text shown in a dialog when an app requests a previously denied permission". The idea is that the first request happens without rationale, but if the user denies it and the app wants to request again it has to give rationale first. This is just following the UX recommended by the platform team and the results coming back from system calls.

The second kind of rationale, which many polished apps use, is the kind shown before any request, For example you'd show a full screen prompt saying that "In order to help you share restaurants with your friends, the app will need access to your contacts" to make the prompt not-scary. In this case the prompt normally only happens after an explicit user click.

Unfortunately EasyPermissions does not deal with that kind of rationale at all, as it's something that should be designed to fit within the app flow and I don't believe we'd be able to provide a suitable one-size-fits-all solution.

So here's what I think we should do:

• Update the docs here to make sure that we set the right expectations for the "permanently denied" method
• I will file the feature request for an official, always-correct version of this method with the platform / support lib team (here https://issuetracker.google.com/77960408)

For now I don't want to get into the business of request persistence, even if we may be able to do it correctly with careful coding and testing.

[itsymbal]

@samtstern
Very well then. I can relate to not wanting to increase the scope of a project.
Thanks for creating a feature request. And yes, do add a strong warning to the documentation.