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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: privacy manifest collection purposes type #3275

Closed
wants to merge 2 commits into from
Closed

fix: privacy manifest collection purposes type #3275

wants to merge 2 commits into from

Conversation

FelixHerrmann
Copy link
Contributor

馃摐 Description

Change NSPrivacyCollectedDataTypePurposes type from string to array.

Note: Using the Property List representation of Xcode will use tabs instead of spaces under the hood, that's why the git diff is not super readable...

馃挕 Motivation and Context

We have started configuring the privacy manifest for our app. The generated Privacy Report from the Xcode Organizer presented us the following error:
Screenshot 2023-09-11 at 18 11 07

Checking Sentry's manifest file confirmed it, the NSPrivacyCollectedDataTypePurposes (Collection Purposes) had the wrong type, string instead of array.

馃挌 How did you test it?

I haven't tested it by generating another report but Apple's docs clearly define it as an array of strings and the Xcode Property List representation shows the correction display names instead of the underlying key (Item 1 is fixed, Item 2 is original):
Screenshot 2023-09-11 at 17 55 44

馃摑 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

馃敭 Next steps

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #3275 (0af9234) into main (881a955) will decrease coverage by 0.005%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3275       +/-   ##
=============================================
- Coverage   89.223%   89.218%   -0.005%     
=============================================
  Files          501       502        +1     
  Lines        53949     54250      +301     
  Branches     19170     19453      +283     
=============================================
+ Hits         48135     48401      +266     
+ Misses        4958      4880       -78     
- Partials       856       969      +113     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 881a955...0af9234. Read the comment docs.

@armcknight
Copy link
Member

Thanks @FelixHerrmann for the fix! I pushed a changelog fix and let's see how CI does on that commit.

For easier diffing, here're the significant changes not including whitespace:
image

@FelixHerrmann
Copy link
Contributor Author

For easier diffing, here're the significant changes not including whitespace:

Yes, much better to read, thanks for sending this!

@armcknight
Copy link
Member

I'm going to close this one in favor of #3276, I think there's a permission error with the CI jobs on this one. Thanks again for the PR!

@armcknight armcknight closed this Sep 11, 2023
@FelixHerrmann
Copy link
Contributor Author

All good, thanks for picking this up so quickly!

@FelixHerrmann FelixHerrmann deleted the fix/privacy-manifest-collection-purposes-type branch September 11, 2023 21:44
@FelixHerrmann
Copy link
Contributor Author

I think there's a permission error with the CI jobs on this one.

@armcknight do you think thats a general issue or just with this particular PR? I have another small fix for you but I can also open an issue, let me know!

@armcknight
Copy link
Member

I think it's because it's in a fork. If you push a branch directly to this remote for a PR, we should be able to authorize CI jobs containing sensitive info. Thanks for all your efforts @FelixHerrmann 馃檹馃徎

@FelixHerrmann
Copy link
Contributor Author

Gotcha, will try next time! I just saw that the issue already got fixed in #3270, it's just not released yet 馃憤馃徎

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.

None yet

2 participants