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.

Sign up for GitHub

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

Jump to bottom

feat: add GAID fetching and saving to google_ad_id #93

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: add GAID fetching and saving to google_ad_id #93

wants to merge 6 commits into from

Conversation

comfrt1k
Copy link
Contributor

@comfrt1k comfrt1k self-assigned this Dec 24, 2024
@comfrt1k
Copy link
Contributor Author

  1. В первом пункте задания сказано, что надо запрашивать пермишны:

Для запроса свойства нужно запросить пермишны. Это должна быть опциональная конструкция, которую разработчик клиента может добавить в инициализацию SDK, чтобы мы могли получать этот ID и отправлять к себе.

На оф сайте написано это:

Кроме того, при обновлении приложений до Android 13 или более поздней версии необходимо указать в файле манифеста обычное разрешение Google Play следующим образом:
<uses-permission android:name="com.google.android.gms.permission.AD_ID"/>

И это:

Некоторые SDK, например Google Mobile Ads SDK (play-services-ads), могут уже объявлять это разрешение в манифесте библиотеки SDK. Если ваше приложение использует эти SDK в качестве зависимостей, разрешение AD_ID из манифеста библиотеки SDK будет по умолчанию объединено с основным манифестом вашего приложения, даже если вы явно не объявляете это разрешение в основном манифесте вашего приложения.

То есть, в рантайме запрашивать ничего не надо.
Вышесказанное также решает проблему, описанную во втором пункте:

При этом магазин может уже установить этот запрос прав без нашего участия (в крупных магазинах этот кейс будет в 100% случаев), поэтому наш SDK не должен конфликтовать с этой настройкой, но должен уметь забирать AD_ID.

В таком случае манифесты просто объединятся, а ad_id будет нам доступен

  1. GAID получается при каждой инициализации sdk, тк получать актуальный можно только так (пользователь может его менять и удалять)

  2. Ну и пруфы на скринах ниже:

Если пользователь не удалил gaid или не запретил его использование:
image

Если gaid удален :
image

@comfrt1k comfrt1k marked this pull request as ready for review December 25, 2024 11:31
@TorinAsakura TorinAsakura removed their request for review December 25, 2024 11:58
@comfrt1k comfrt1k requested a review from looee1q December 25, 2024 12:02
looee1q
looee1q reviewed Dec 26, 2024
View reviewed changes
personalization-sdk/src/main/kotlin/com/personalization/SDK.kt Outdated Show resolved Hide resolved
...ization-sdk/src/main/kotlin/com/personalization/sdk/data/repositories/gaid/GaIdDataSource.kt Outdated
import com.google.android.gms.ads.identifier.AdvertisingIdClient

class GaIdDataSource {
internal fun fetchGaId(context: Context): String =
Copy link
Contributor

Choose a reason for hiding this comment

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

Не уверен, что есть смысл в этом классе DataSource. Кажется проще сразу вызвать в репозитории нужную функцию. Я понимаю, что у нас для некоторых других составляющих имеется DataSource, и в части этих мест от него, на мой взгляд, отсутствует какой-либо толк. По крайней мере, я не понимаю необходимость наличия таких классов в ряде случаев.

Общая проблема в том, что у нас отсутсвует единая архитектурная карта приложения, которой мы могли бы следовать при добавлении новой функциональности.
А текущие архитектурные решения, если я правильно понял Андрея, не являются удовлетворительными.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Удалила data source, перенесла логику получения gaid в репозиторий
  • Репозиторий переименовала на AdvertisingRepository

Copy link
Member

Choose a reason for hiding this comment

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

advertising.repository.kt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

advertising.repository.kt

Что это значит?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TorinAsakura

Кодстайл котлина говорит о том, что если в файле единственный класс/интерфейс, то название файла должно быть таким же, как и имя класса/интерфейса. А так же название файла должно быть с большой буквы и в CameCase стиле.

Мы отходим от каждого из принятых правил кодстайла котлина для нэйминга файлов? И называем их с малькой буквы, а все слова разделяем через точку?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Я не против нейминга котлина, однако, предпочитаю camelCase, а у тебя PascalCase, так как у нас вся кодовая база приводится к данному контракту. Далее
  2. Если вы научитесь именовать классы "говорящими" - я не против, называйте файлы также как классы
  3. Не всё разделяем точками, только специфики, которые не относятся напрямую к неймингу, вот пример из той же доки https://kotlinlang.org/docs/coding-conventions.html#multiplatform-projects

...sdk/src/main/kotlin/com/personalization/sdk/domain/usecases/userSettings/FetchGaIdUseCase.kt Outdated Show resolved Hide resolved
...sdk/src/main/kotlin/com/personalization/sdk/domain/usecases/userSettings/FetchGaIdUseCase.kt Outdated Show resolved Hide resolved
...ization-sdk/src/main/kotlin/com/personalization/sdk/data/repositories/gaid/GaIdDataSource.kt Outdated Show resolved Hide resolved
@comfrt1k comfrt1k requested a review from looee1q December 26, 2024 12:07
looee1q
looee1q reviewed Dec 26, 2024
View reviewed changes
...in/kotlin/com/personalization/sdk/data/repositories/advertising/AdvertisingRepositoryImpl.kt Outdated Show resolved Hide resolved
...sdk/src/main/kotlin/com/personalization/sdk/domain/usecases/userSettings/FetchGaIdUseCase.kt Outdated Show resolved Hide resolved
...ain/kotlin/com/personalization/sdk/domain/usecases/userSettings/FetchAdvertisingIdUseCase.kt Outdated
import com.personalization.sdk.domain.repositories.UserSettingsRepository
import javax.inject.Inject

class FetchAdvertisingIdUseCase @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Этот юзкейс можно назвать InitializeAdvertisingIdUseCase. Он будет выполнять роль получения юзкейса при инициализации SDK и сохранения его в SharedPref.

При этом нужен еще один юзкейс - GetAdvertisingIdUseCase, который уже будет позволять получить юзкейс из SharedPref. Именно оттуда и будут брать токен при работе с приложением.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Переименовала usecase и добавила в sdk метод для получения gaid

Copy link
Member

Choose a reason for hiding this comment

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

*.usecase.kt

@comfrt1k comfrt1k requested a review from looee1q December 27, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TorinAsakura TorinAsakura TorinAsakura left review comments

@looee1q looee1q Awaiting requested review from looee1q

At least 1 approving review is required to merge this pull request.

Assignees

@comfrt1k comfrt1k

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@comfrt1k @TorinAsakura @looee1q