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

Ask permission to use notifications after onboarding #429

Merged

Conversation

ivan-magda
Copy link
Member

@ivan-magda ivan-magda commented Dec 11, 2018

Задача: #APPS-2160

Описание:
Запрос разрешения на использование нотификаций после прохождения онбординга для всех пользователей.

@ivan-magda ivan-magda self-assigned this Dec 11, 2018
@ivan-magda ivan-magda requested review from kvld and Ostrenkiy December 11, 2018 14:25
@ivan-magda ivan-magda mentioned this pull request Dec 12, 2018
5 tasks
Copy link
Contributor

@kvld kvld left a comment

Choose a reason for hiding this comment

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

Правда, что здесь просто аккуратно откатили тест? Если да, то можно мерджить сразу наверное

@ivan-magda
Copy link
Member Author

Да, откатили тест и убедились в том, что на iOS 9.0 запрос пушей происходит после онбординга, как и на iOS 10.0*.

@@ -64,8 +64,8 @@ struct NotificationAlertsAnalytics {
return "streak after submission - \(shownCount)"
case .personalDeadline:
return "create personal deadline"
case .abAppLaunch:
return "ab subscribe on app launch"
case .onboarding:
Copy link
Contributor

Choose a reason for hiding this comment

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

В будущем надо будет подумать о том, чтобы информация об ab в такие проперти не проникала и их не надо было менять.
В идеале вообще применение/откат a/b было бы делать только в паре мест, связанных с самим a/b тестом - проще и меньше потенциальных багов.

} else {
self.notificationsRegistrationService.registerForRemoteNotifications()
}
self.notificationsRegistrationService.registerForRemoteNotifications()
Copy link
Contributor

Choose a reason for hiding this comment

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

Здесь мы это делаем чтобы предлагать пользователю в настройки перейти?

Copy link
Member Author

Choose a reason for hiding this comment

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

Да, будет алерт с предложением перейти в настройки

} else {
self.notificationsRegistrationService.registerForRemoteNotifications()
}
self.notificationsRegistrationService.registerForRemoteNotifications()
Copy link
Contributor

Choose a reason for hiding this comment

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

Как и здесь

@ivan-magda ivan-magda merged commit 388835d into dev Dec 12, 2018
@ivan-magda ivan-magda deleted the feature/request-notifications-permission-after-onboarding branch December 12, 2018 14:12
@kvld kvld added this to the 1.74 milestone Dec 13, 2018
@kvld kvld mentioned this pull request Dec 17, 2018
@ivan-magda ivan-magda added the a/b Pull requests that working with A/B testing label Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/b Pull requests that working with A/B testing main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants