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

iOS app rejected due to not making remote push notifications optional #827

Closed
asiripanich opened this issue Nov 2, 2022 · 25 comments
Closed

Comments

@asiripanich
Copy link
Member

asiripanich commented Nov 2, 2022

@shankari yesterday I submitted a new release of my custom app of e-mission to Apple and got the response below about a violation of Guideline 4.5.4. Do you have the same problem at your end? How did you fix this?


Guideline 4.5.4 - Design - Apple Sites and Services

We noticed that your app requires push notifications in order to function.

Next Steps

Push notifications must be optional and must obtain the user's consent to be used within the app.

Resources

For information on working with push notifications, review User Notifications framework and Technical Note TN2265: Troubleshooting Push Notifications.

Please see attached screenshots for details.

@shankari
Copy link
Contributor

shankari commented Nov 2, 2022

Nope, but I haven't submitted an update for a couple of months.
However, we do ask the user for permission to send notifications - the status screen asks users to turn on notifications.
I assume you have the status screen in the custom app?

What are the screenshots that they are complaining about?

@asiripanich
Copy link
Member Author

This is the screenshot they shared:

image

@shankari
Copy link
Contributor

shankari commented Nov 2, 2022

I am not sure whether they are complaining about the "optional" part, or the "user consent" part.

We do clearly ask for user consent.

It would be very challenging to make them be completely optional, because, in case the visit API does not work, we use the silent push notifications to detect when a user has stopped moving and end the trip. For a concrete example of this, see your email from Aug 14 2021 titled "Issues with trip detection and UI"

And we indicate this to the user upfront.

Maybe ask them which part they are complaining about and clarify that the "optional" is challenging given that we use silent push notifications.

I think they want us to use the new provisional notifications feature, but I don't think we can do that for silent notifications since the user will not see them anyway...

@shankari
Copy link
Contributor

shankari commented Nov 2, 2022

I should also be submitting an update to NREL OpenPATH at the end of the week, so can report a second set of communications...

@asiripanich
Copy link
Member Author

Maybe ask them which part they are complaining about and clarify that the "optional" is challenging given that we use silent push notifications.

Can we just let the user continue to the next page even if they don't allow the notification? Maybe we can add one ⚠️ warning before letting them through?

@shankari
Copy link
Contributor

shankari commented Nov 2, 2022

That would definitely make the notifications optional. But then our silent push notifications would not go through and we would get emails like the one that you sent out on Aug 14 2021 titled "Issues with trip detection and UI" Or people will click on "deny" by mistake when prompted to allow location access in the background, and not be prompted to turn it on again.

And then we would have to ask people to turn on their notifications instead of having them just do it upfront.

Again, you can make that change in your custom app - but I want to argue a bit with Apple first, because the option they are pushing us towards (provisional notifications), do not seem to really work with silent push notifications.

For the record: the HTML code is in www/templates/appstatus/permissioncheck.html; if you comment out everything around overallNotificationStatusClass, we won't even show it to the user.

Or if you want to show it, but handle the rejection differently, the code is in www/js/appstatus/permissioncheck.js
right now,

        let fixPerms = function() {
            console.log("fix and refresh notification permissions");
            return checkOrFix(appAndChannelNotificationsCheck, $window.cordova.plugins.BEMDataCollection.fixShowNotifications,
                $scope.recomputeNotificationStatus, showError=true);
        };

we call checkOrFix, which will prompt until it is fixed, you can change that to only prompt once.

@asiripanich
Copy link
Member Author

@shankari how can we make it prompts only one on iOS but keep the same behavior on Android?

@shankari
Copy link
Contributor

shankari commented Nov 3, 2022

Do these examples help?
https://github.com/e-mission/e-mission-phone/search?q=ionicPlatform

@asiripanich
Copy link
Member Author

but I want to argue a bit with Apple first, because the option they are pushing us towards (provisional notifications), do not seem to really work with silent push notifications.

Do you mind providing me a response that I can pass on to them and see how they react? Unfortunately I don't really know the low-level of this as much as you do.

@asiripanich
Copy link
Member Author

Or this is enough at this stage?

Maybe ask them which part they are complaining about and clarify that the "optional" is challenging given that we use silent push notifications.

@shankari
Copy link
Contributor

shankari commented Nov 3, 2022

Yup something like this is what I had in mind, along with some details of why we use silent push notifications.

Or this is enough at this stage?

Maybe ask them which part they are complaining about and clarify that the "optional" is challenging given that we use silent push notifications.

@asiripanich
Copy link
Member Author

I have decided to check the platform variable in recomputeOverallStatus and allows the user to progress if they use iPhone and even if they don't allow notifications.

$scope.recomputeOverallStatus = function(platform) {
        if ($scope.platform.toLowerCase() == "android") {
            $scope.overallstatus = $scope.overallLocStatus
                && $scope.overallFitnessStatus
                && $scope.overallNotificationStatus
                && $scope.overallBackgroundRestrictionStatus;
        } else if ($scope.platform.toLowerCase() == "ios") {
            $scope.overallstatus = $scope.overallLocStatus
                && $scope.overallFitnessStatus
                // && $scope.overallNotificationStatus
                && $scope.overallBackgroundRestrictionStatus;
        } else {
            alert("Unknown platform");
        }
    }

@asiripanich
Copy link
Member Author

@shankari this is not looking good.


Hello,

Thank you for providing this information.

Regarding guideline 4.5.4, push notifications should not be required for your app to function. They should also not be used to send sensitive personal or confidential information.

Please revise your app to allow push notifications to be fully optional for your users and allow your app to function normally if push notifications are not enabled.

Additionally, upon further review, we found that your submission does not comply with the following guidelines:

Guideline 5.1.5 - Legal - Privacy - Location Services

We noticed that your app does not request and obtain the user's consent with a permission access request prior to accessing their location data, which is not allowed on the App Store.

It is not appropriate to circumvent permission access requests for user data by directing users to iOS Settings.

Next Steps

To resolve this issue, please revise your app to include a permission access request for the user's location data and ensure the features are still functional if the user initially opts out.

Please resubmit your app for review once you have revised your app to allow push notifications to be fully optional and implemented a permission access request for users' location data with an appropriate purpose string that clearly explains your app's need for location data access and provides a specific example of how users' data will be used.

We look forward to reviewing your resubmitted app.

Best regards,

App Store Review

@shankari
Copy link
Contributor

shankari commented Nov 6, 2022

  • They are still not making a distinction between visible push notifications and silent push notifications, but I guess your workaround iOS app rejected due to not making remote push notifications optional #827 (comment) will get you past this step
  • wrt location services, I really don't understand what they want. If we don't have access to location data, we cannot build a travel diary. So it is not clear what "ensure the features are still functional if the user initially opts out." means.

I also checked 5.1.5, https://developer.apple.com/app-store/review/guidelines/#location and it says that we should "Use Location services in your app only when it is directly relevant to the features and services provided by the app. " and that "If your app uses location services, be sure to explain the purpose in your app;"

Didn't you also say that you have noticed that other apps redirect to iOS settings to turn on the location settings?

@shankari
Copy link
Contributor

shankari commented Nov 6, 2022

I am just waiting for #831 to be fixed before I submit my own update, so you could also wait and see what happens when I submit. My most recent accepted version was from Sept 1, so not that long ago. If my update is accepted, you might want to change the text to make the app purpose more clear.

@shankari
Copy link
Contributor

shankari commented Nov 6, 2022

@asiripanich Since you have a pending data collection that you want to have the updated app for, you might want to work around this as well without waiting for me to finish the change and then finish the argument with Apple.

You already have a workaround for the notifications.

For the location services change, you know how to open your native version of the app (from platforms/ios/<app name>/<app name>.xcworkspace) so that you can archive and submit, right?

Before submitting, open SensorControlForegroundDelegate.m and remove these lines

    if (IsAtLeastiOSVersion(@"13.0")) {
        NSLog(@"iOS 13+ detected, launching UI settings to easily enable always");
        // we want to leave the registration in the prompt for permission, since we don't want to register callbacks when we open the app settings for other reasons
        [[TripDiaryStateMachine delegate] registerForegroundDelegate:self];
        [[TripDiaryStateMachine instance].locMgr startUpdatingLocation];
        [self openAppSettings];
    }
    else {

Make sure to remove the last } for the else as well.

As you can see, this will then prompt for always permission, but on iOS13+, users have to select "when in use" first and then select "always" after some period of tracking, as they used to in #483 before the status screen changes.

@asiripanich
Copy link
Member Author

Thank you so much @shankari for suggesting these workarounds. I will submit an updated app today and let you know how it goes.

@asiripanich
Copy link
Member Author

@shankari i have a suggestion. I think we should show a warning message on the diary tab if the are any settings not configured correctly. Tapping on the message would show a pop up window that explain how to enable the permission with a shortcut to settings.

Warning message Message pop up

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

We already check the settings every hour and generate a notification if the settings are not configured correctly. Clicking on the notification automatically opens the NREL status page (from Profile -> App Status), which allows the users to see which settings are misconfigured and need to be fixed.

This allows us to be proactive about telling users about the settings that need to be fixed instead of waiting for users to launch the app. In general, we expect that users are not necessarily launching the app multiple times a day.

@asiripanich
Copy link
Member Author

You wouldn't be able to notify the user if they don't allow notifications right? That is why I think showing them what settings are not correct when they open the app is the best way to inform them.

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

Well, that's why I think that we need notifications 😄
Note that these are not the push notifications that Apple is complaining about, but local notifications generated in the app.

wrt in-app UI, we already do have the app status screen (Profile -> App Status) for users to find out what is wrong with the app. If you would like to contribute functionality to make the app status more visible in other screens, that would be great.

It is a bit tricky to get it right given that the diary is already fairly cluttered and complicated, and given that OpenPATH is configurable, the main screen is likely to be different for different instances.

For example, we will likely make the dashboard be the launch screen for the open access instance, while the e-bike trip logger instances will want to stay with the label screen as primary.

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

So ideally, you would make this a directive that we can <ng-if into each screen if it is the primary screen.

@shankari
Copy link
Contributor

shankari commented Nov 7, 2022

And again, the direction that we are going in from the NREL side is towards more passive data collection with potentially some intermittent labels. So there is no guarantee that people will open the app every day, and in fact, in the programs we see that people typically label once a week.

@asiripanich
Copy link
Member Author

Update. Apple has approved the new version of my custom app that I modified using your recommendations here.

@shankari
Copy link
Contributor

shankari commented Dec 7, 2022

FYI, the OpenPATH app was approved with the original status screen. I did not change the status screen for the new version, so I think that they let it go as already approved.
https://apps.apple.com/us/app/nrel-openpath/id1628058068

I will keep this in mind during the update because it may come up later if they do a more comprehensive review, or for new apps using the platform.

@asiripanich you might also want to see if you can change your app description to better match OpenPATH.

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

No branches or pull requests

2 participants