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: Support GDPR (APSAnalytics 2.1.0), add missing native header #10051

Merged
merged 14 commits into from May 24, 2018

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn commented May 16, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26004

Adding unit test once CR approved.

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

I think we need to agree on the API specification for Ti.Analytics optOut because this is varying per implementation:

Ti.Analytics.optedOut // instead of Ti.Analytics.isOptedOut
Ti.Analytics.setOptedOut(boolean)
Ti.Analytics.getOptedOut()

For consistency like our other Titanium APIs

For example, the Windows implementation uses isOptOut:
https://github.com/appcelerator/titanium_mobile_windows/pull/1238

And Android uses optedOut:
#10058

@hansemannn
Copy link
Collaborator Author

@garymathews @ypbnv @infosia I have updated the iOS-PR to add the property optedOut with it's getter getOptedOut() and setter setOptedOut(true|false). I have also added the docs for iOS and Android - I think the "windows" platform tag needs to be added via the titanium_mobile_windows repo?

@build build added the docs label May 18, 2018
@infosia
Copy link
Contributor

infosia commented May 18, 2018

Updated Windows PR: https://github.com/appcelerator/titanium_mobile_windows/pull/1238 Updated API doc for Windows too.

@hansemannn
Copy link
Collaborator Author

@garymathews The PR was updated, can you re-review? I have also added @vijaysingh-axway for another CR.

@hansemannn hansemannn removed the request for review from sgtcoolguy May 23, 2018 09:58
Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

@hansemannn Can you address the 2 points which I have mentioned. Thanks!


- (void)optedOut
{
@([[APSAnalytics sharedInstance] isOptedOut]);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Isn't it good if we make this method's return type to (NSNumber *) for more readability and return it.
  2. Following test case crashing, if I close the app and run again. Can this not be a test case ?
    `var win = Ti.UI.createWindow({
    backgroundColor: '#fff'
    });

var btn = Ti.UI.createButton({
title: 'Trigger'
});

btn.addEventListener('click', function() {
Ti.Analytics.setOptedOut(true);
});

win.add(btn);
win.open();
`

var should = require('./utilities/assertions');

describe('Titanium.Analytics', function () {
it.androidMissing('.optedOut', function () {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@garymathews @ypbnv Once Android is merged, we can update this one to work or all platforms.

@infosia I think this works on Windows already, since your tests have been pretty extensive before merging. Any objections? :-)

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

CR passed.

@hansemannn hansemannn dismissed garymathews’s stale review May 24, 2018 08:21

Dismissing review since the comments have been addressed earlier.

@hansemannn hansemannn merged commit e937a54 into tidev:master May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants