Skip to content

iOS : Notification and Calendar permission issue resolution#2833

Merged
kumarpalsinh25 merged 2 commits intomainfrom
kumar/ios-permission-issue
Apr 16, 2025
Merged

iOS : Notification and Calendar permission issue resolution#2833
kumarpalsinh25 merged 2 commits intomainfrom
kumar/ios-permission-issue

Conversation

@kumarpalsinh25
Copy link
Contributor

@kumarpalsinh25 kumarpalsinh25 commented Apr 15, 2025

Fixes, #2820

As we are using permission_handler for permission checking and management, based on their official documentation, we need to declared permissions thing in iOS/Podfile to make it work properly.

If we don't declared it it library will alway given permissionDenied status which was the core issue in our code.

@github-actions
Copy link
Contributor

Hey there 👋,
and thanks for the contribution. But it seems like you forgot to

  • 📰 Add a markdown file in .changes/ explaining what changed

@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 36.57%. Comparing base (1acba89) to head (82a219a).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
.../lib/common/utils/device_permissions/calendar.dart 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2833      +/-   ##
==========================================
+ Coverage   36.55%   36.57%   +0.01%     
==========================================
  Files         831      831              
  Lines       52442    52441       -1     
==========================================
+ Hits        19171    19180       +9     
+ Misses      33271    33261      -10     
Flag Coverage Δ
integration-test 45.38% <ø> (+0.04%) ⬆️
unittest 27.40% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines -42 to -44
final deviceCalendar = DeviceCalendarPlugin();
final hasPermission = await deviceCalendar.hasPermissions();
return hasPermission.data ?? false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This permission checking was managed by device_calendar code which was always buggy and not giving proper status so I replaced it with permission_handler which we are using in other places of the app as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this idea that "device_calendar code which was always buggy" come from? It worked fine for at least a bunch of cases, including some iOS testing (I had done here before). Replacing this raises the question: has this been tested on other platforms than iOS? And: is this change really necessary for the fix?

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

I don't understand what the actual problem was nor how these things are supposed to fix them.

The calendar events plugin request used to work fine, replacing it without a good reason risks us breaking several other cases that worked before. And the additional supposed settings have already been set in the Info.plist.

What actually is the issue and what change in here fixes it? Additionally: has this been tested on other supported platforms?

Comment on lines -42 to -44
final deviceCalendar = DeviceCalendarPlugin();
final hasPermission = await deviceCalendar.hasPermissions();
return hasPermission.data ?? false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this idea that "device_calendar code which was always buggy" come from? It worked fine for at least a bunch of cases, including some iOS testing (I had done here before). Replacing this raises the question: has this been tested on other platforms than iOS? And: is this change really necessary for the fix?

Comment on lines +39 to +54
target.build_configurations.each do |config|
# You can remove unused permissions here
# for more information: https://github.com/Baseflow/flutter-permission-handler/blob/main/permission_handler_apple/ios/Classes/PermissionHandlerEnums.h
# e.g. when you don't need camera permission, just add 'PERMISSION_CAMERA=0'
config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] ||= [
'$(inherited)',
## dart: PermissionGroup.calendar
'PERMISSION_EVENTS=1',

## dart: PermissionGroup.calendarFullAccess
'PERMISSION_EVENTS_FULL_ACCESS=1',

## dart: PermissionGroup.notification
'PERMISSION_NOTIFICATIONS=1',
]
end
Copy link
Contributor

@gnunicorn gnunicorn Apr 15, 2025

Choose a reason for hiding this comment

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

Is this the actual fix then? I don't understand why this would fix anything. Clearly notifications worked before without this, so whatever this is doing, it isn't overwriting the Info.plist settings. And as said before, the same is true for calendar events - it is used to work fine before, why is this necessary now?

I think this requires more documentation than a link to a list of things, like maybe a github issue or readme describing that doing this is necessary?!?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a huge explanation but I still don't know where this code comes from. It is not on the permission_handler setup README as implied. If this is not the setup usually needed for permission_handler, why do we need it?

@kumarpalsinh25
Copy link
Contributor Author

kumarpalsinh25 commented Apr 16, 2025

To understand and fixed the issue, I need to dd bit of R&D and then implemented this code. Let me try to explain everything address your queries.


General Awareness:

Now a days, all plugins are managing permission handling as built in part of it. Like camera, photos, calendar, notification etc permissions are managed by their respective plugins.


Permission Handler:

  • When we wanted to handle permission externally from our cod then permission_handler is the plugin to manage all the OS and device specific permission.
  • On Notification and Calendar permission info screen, we need to check permission first and then show Dialog screen if permission is not granted.
  • So to check whether permission is granted or not, we have used permission_handler which give use proper status of permission. (Granted, Denied, PermanentlyDenied)
  • So if permission is granted then we are doing respective logical code else we will ask use to give permission.

Side-notes:

  • permission_handler plugin was already part of Acter project from a while and we have just used in in Image Picker for storage permission asking ( final status = await Permission.storage.request();) and it was for android only.
    Screenshot 2025-04-16 at 1 20 13 PM
  • But we haven't properly configured permission_handler plugin for iOS setup guide writtern on https://pub.dev/packages/permission_handler
  • Till now everything is working as we aren't using permission_handler on any iOS code as different library as managing permission things inbuilt within it

Current Issue Point:

Ref : Comment

I don't understand what the actual problem was nor how these things are supposed to fix them.

Answer:

  • As show-cased in iOS : Notification and Calendar is already granted still asking for give permission dialog #2820, on iOS device when I want to turn on Notification or Calendar settings then it is showing permission screen even though permissions is already granted.
  • This is working fine on Android device as permission_handler plugin doesn't require additional configure but in iOS it does requires in PodFile which correction I did.
  • await Permission.notification.status; // await Permission.calendarFullAccess.request(); always giving PermissionDenied status in iOS if we don't configure PodFile as per documentation.

Permission handler for calendar permission:

Ref : Comment

The calendar events plugin request used to work fine, replacing it without a good reason risks us breaking several other cases that worked before. And the additional supposed settings have already been set in the Info.plist.

Where does this idea that "device_calendar code which was always buggy" come from? It worked fine for at least a bunch of cases, including some iOS testing (I had done here before). Replacing this raises the question: has this been tested on other platforms than iOS? And: is this change really necessary for the fix?

Answer:

  • As explain above, generally plugin are managing permission as inbuilt and same things device_calendar is doing.
  • However now we are managing permissions with additional info screens and where we wanted to check Granted, Denied, and PermentelyDenied status and act accordingly.
  • device_calendar's inbuilt permission code not giving proper status for PermentelyDenied case due their internal bug.
  • We never check for this type of cases earlier so that we haven't faced any issue.
  • However with the new code we are checking for all the cases and also informing and redirecting user to go for settings screen to enable permission if it is permentally disabled.
  • So do avoid this thing I used proper permission management with permission_handler for calendar.
  • Moreover I haven't changed any internal logic of device_calendar apart from permission asking code.

Additional Platforms Things:

Ref Comment:

Additionally: has this been tested on other supported platforms?

  • Well, we have calendar support for only mobile platform and there was issue in iOS as described above. Android already works fine.
  • If you wanted test it then we you do it from main branch code for idea.

Reference Video:

  1. Issue Point:
Issue.Point.mov
  1. Now after this PR:
Expected.Behavior.When.permissioned.enabled.mov

@gnunicorn
Copy link
Contributor

gnunicorn commented Apr 16, 2025

When I am asking about "what is the actual issue", I didn't mean to expand on the large parts of the ins and outs, but that

device_calendar's inbuilt permission code not giving proper status for PermanentelyDenied case due their internal bug.

is the actual interesting part - yet it lacks a link. Is there some issue on their repo or a link of their code or anyone else explaining the bug? I can't find any in their repo. So if not, what's the evidence for a bug in their code and not ours? If there is a bug there and it isn't reported yet, why don't we report it?

Well, we have calendar support for only mobile platform and there was issue in iOS as described above. Android already works fine.

But this PR changes that. It changes the code that is run on Android to one we haven't used there before. So it hasn't been tested whether the new code works on Android then? Why not?

I am not suggesting it doesn't work. But it code effecting both platforms and it is kinda critical for the calendar sync to work. So if we changed it to fix it one platform, we still need to confirm whether that new code works on the other as well...

@gnunicorn
Copy link
Contributor

To be clear, I am not claiming it doesn't fix the bug in question, nor am I suggesting this is wrong code. What I am asking why does it fix it. And I have troubles understanding two concrete parts, not the big picture:

  1. there is a claim -- lacking evidence -- that device calendar isn't doing something right. what that something is and what it should be doing instead, is unclear to me -- due to a lack of evidence. Leading to a code change that hasn't yet been tested on all required platforms
  2. there is a change in Podfile, that isn't clear why it is needed or where it comes from -- the package, that is supposedly needed that change says nothing of that sort. It is unclear to me what this is for.

Neither of these two specific remarks has been addressed.

@kumarpalsinh25
Copy link
Contributor Author

When I am asking about "what is the actual issue", I didn't mean to expand on the large parts of the ins and outs, but that

Sorry but I am not getting you. Isn't video shown in this issue #2820 enough to get idea of issue?

is the actual interesting part - yet it lacks a link. Is there some issue on their repo or a link of their code or anyone else explaining the bug? I can't find any in their repo. So if not, what's the evidence for a bug in their code and not ours? If there is a bug there and it isn't reported yet, why don't we report it?

I just come across issue on Monday my iPhone and felt that will block things so before next Thursday release I want to fix it without waiting from other people to report and then resolve the issue. It was all about permission checking so used permission_handler.n. I didn't had any other solution to move on with this bugs solition.

But this PR changes that. It changes the code that is run on Android to one we haven't used there before. So it hasn't been tested whether the new code works on Android then? Why not?

Yes of course, tested new code with android. If want then will share video reference of the same. (Side-not : This is just simple permission checking code that's why haven't shared android video reference in PR Description but happy to share that)

there is a claim -- lacking evidence -- that device calendar isn't doing something right. what that something is and what it should be doing instead, is unclear to me -- due to a lack of evidence. Leading to a code change that hasn't yet been tested on all required platforms

I am getting difficulty in explaining things in words. If you can just test Notification and Calendar permission scenarios with Granted, Denied and PermanentlyDenied cases then probably you will get better understanding on what wrong on what things and what can be solution. Also sharing again, I have tested code on Android and iOS both platforms.

there is a change in Podfile, that isn't clear why it is needed or where it comes from -- the package, that is supposedly needed that change says nothing of that sort. It is unclear to me what this is for.

It is clearly mentioned on https://pub.dev/packages/permission_handler document about configuration requirement.

@gnunicorn
Copy link
Contributor

It is clearly mentioned on https://pub.dev/packages/permission_handler document about configuration requirement.

Ah, sorry, my bad. It is behind an expand so even a ctrl+f on the page doesn't find it unless you opened it. Okay, great, that's all I wanted to know!

Yes of course, tested new code with android. If want then will share video reference of the same.

No, need. All I asked was whether it was tested on Android and didn't get a straight answer. If you say you did, that's enough for me.

I am getting difficulty in explaining things in words. If you can just test Notification and Calendar permission scenarios with Granted, Denied and PermanentlyDenied cases then probably you will get better understanding on what wrong on what things and what can be solution.

So... I am only talking about the Calendar Permissions. My question here was: has the old code been tested with the permission changes made in the podfile? Maybe the changes in the podfile were all that is needed?

The answer is no. Digging deeper into the issue, finding the reference in the changelog of permissions handler of permission changes for calendar by apple in iOS 17+, which lead me to this bug report on device calendar for which there seem to be a fix, which just hasn't been merged due to support for older iOS. Arguably, we could probably just use that fix, as we don't intend to support iOS 13, but this is what I call the root cause -- the evidence.

There is a bug in device calendar and there is a report and even a fix for it. This is our problem.

Now, you chose the path to go over flutter permissions handler instead which has wider and better support, and that is fine. I was only lacking context for the specific change in question.

@kumarpalsinh25 kumarpalsinh25 merged commit 90297ba into main Apr 16, 2025
21 of 23 checks passed
@kumarpalsinh25 kumarpalsinh25 deleted the kumar/ios-permission-issue branch April 16, 2025 10:51
@github-project-automation github-project-automation bot moved this from Backlog to Recently Done in Product Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants