Add application install #40
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @KarlsonComplete, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces the ApplicationInstallation entity and its corresponding repository, along with related events and interfaces. The goal is to manage the lifecycle and status changes of application installations within the Bitrix24 ecosystem. The entity includes properties such as IDs, timestamps, statuses, and associated Bitrix24 account and partner information. The repository provides methods for saving, retrieving, and deleting application installations.
Highlights
- ApplicationInstallation Entity: Defines the
ApplicationInstallationentity with properties like ID, creation/update timestamps, Bitrix24 account/partner IDs, status, and external ID. It includes methods for changing the status of an application installation (e.g., installed, uninstalled, blocked, active) and updating associated contact persons and partner information. - ApplicationInstallationsRepository: Implements the
ApplicationInstallationRepositoryInterfaceusing Doctrine ORM. It provides methods for saving, retrieving by ID and Bitrix24 account ID, and deletingApplicationInstallationentities. It also includes validation to ensure the application installation exists and is in the correct status before deletion. - Domain Validation: The pull request also includes a change to the
Domainvalue object, specifically modifying the regex pattern used to validate domain names. The pattern^[A-Za-zА-Яа-яЁё0-9-]{1,63}(\.[A-Za-zА-Яа-яЁё0-9-]{1,63}){0,2}$limits the number of domain segments to a maximum of 3.
Changelog
Click here to see the changelog
- src/ApplicationInstallations/Entity/ApplicationInstallation.php
- Introduces the
ApplicationInstallationentity with properties for managing application installation details. - Implements methods for changing contact persons, partner information, and application status.
- Adds methods for marking applications as installed, uninstalled, active, or blocked, triggering corresponding events.
- Includes validation to ensure status transitions are valid.
- Introduces the
- src/ApplicationInstallations/Infrastructure/Doctrine/ApplicationInstallationsRepository.php
- Creates the
ApplicationInstallationsRepositoryfor persisting and retrievingApplicationInstallationentities. - Implements methods for saving, retrieving by ID and Bitrix24 account ID, and deleting application installations.
- Adds validation to ensure the application installation exists and is in the correct status before deletion.
- Adds methods for finding application installations by Bitrix24 account ID and external ID.
- Creates the
- src/Bitrix24Accounts/ValueObjects/Domain.php
- Modifies the regex pattern for domain validation to limit the number of domain segments to a maximum of 3.
- The pattern
^[A-Za-zА-Яа-яЁё0-9-]{1,63}(\.[A-Za-zА-Яа-яЁё0-9-]{1,63}){0,2}$enforces this limit.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A new entity takes its place,
Application installs, a digital space.
With statuses to track,
And events on the right track,
In Bitrix's realm, a structured embrace.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces the ApplicationInstallation entity and its repository, along with necessary interfaces and events. The code appears well-structured, but there are a few areas that could be improved for clarity and robustness. Specifically, I have a question about the readonly properties in the entity, and suggest some minor improvements to the repository.
Merge Readiness
The code is generally well-structured and implements the required functionality. However, I recommend addressing the questions and suggestions raised in the review comments before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Entity/ApplicationInstallation.php
Outdated
Show resolved
Hide resolved
- Добавил билдер - И тесты на репозиторий
config/xml/Bitrix24.Lib.ApplicationInstallations.Entity.ApplicationInstallation.dcm.xml
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Infrastructure/Doctrine/ApplicationInstallationRepository.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Infrastructure/Doctrine/ApplicationInstallationRepository.php
Outdated
Show resolved
Hide resolved
src/ApplicationInstallations/Infrastructure/Doctrine/ApplicationInstallationRepository.php
Outdated
Show resolved
Hide resolved
- Поправил сущность
- Поправил домен
- Написал UseCase на установку приложения
- Поправил handler для установки приложения - Тест на установку (включающий в себя аккаунт битрикс 24) -Тест на метод ChangePortalLicenseFamily
| */ | ||
|
|
||
| /* | ||
| * Еще вопрос написано деактивация приложения и аккаунтов связанных с приложением это soft delete ? |
There was a problem hiding this comment.
Во всех промышленных приложениях у тебя soft-delete.
- Всегда можно воссстановить
- В случа чего можно найти концы, кто и что было
- У тебя сущность может быть завязано на много других сущностей и каскадное удаление - трудозатратная операция.
|
|
||
| /* | ||
| В документации написано, что нужно деактивировать аккаунтЫ связанные с приложением. Но у нас связь у приложения идет только с 1 аккаунтом. | ||
| Отсюда вопрос у нас аккаунт у приложения один ? Или если несколько то как их получить ? |
There was a problem hiding this comment.
Аккаунтов у приложения может быть несколько. Аккаунт = авторизация в Битрикс24, поэтому если приложение допустим что-то делает для конкретного юзера, то оно долно делать это с его уровнями доступа.
Как создать еще 1 аккаунт у приложения – тема отдельного разговора.
| $bitrix24AccountId = $activeApplicationInstallation->getBitrix24AccountId(); | ||
|
|
||
| /* | ||
| В документации написано, что нужно деактивировать аккаунтЫ связанные с приложением. Но у нас связь у приложения идет только с 1 аккаунтом. |
There was a problem hiding this comment.
Связь 1:M, у 1 приложения может быть много аккаунтов.
|
|
||
|
|
||
| $oldBitrix24Account = $this->bitrix24AccountRepository->getById($bitrix24AccountId); | ||
| // Тут возникает вопрос. Для деинсталяции(если деактивация означает это) нам нужно токен прокидывать в команду, мне кажется бредово. |
There was a problem hiding this comment.
В жизни у нас может быть два варианта:
1. мы поймали событие удаления приложения с портала пришедшее от Б24 с токеном
Тогда мы берём токен и переводим аккаунт Б24 в состояние удалён.
Выбрасывается корректное доменное событие, т.к. мы однозначно понимаем, что удаление произошло и пришедший от Б24 токен в событии это подтверждает.
2. мы не поймали событие удаления (сервер лежал)
Вот тут начинается тонкий лёд, т.к. у нас разошлись состояния систем:
2.1. в б24 у нас приложение уже удалено, обработчики удалены, токены все инвалидированы, все запросы с токенами по этому приложению будут падать с ошибкой (приложение удалено) @KarlsonComplete пометь себе поставить локальное приложение и убедиться, что SDK выбрасывает эксепшен
https://github.com/bitrix24/b24phpsdk/blob/main/src/Core/ApiLevelErrorHandler.php - тут не уверен, если там пишется однозначно, что приложение удалено, то должен быть типизированный эксепшен. Если нет, то ставь ишью.
2.2. у нас на сервере приложение до сих пор в статусе активно и он не знает про удаление.
Токена мы уже от Б24 не получим.
Узнать о том, что приложение удалено можно:
- выполнив запрос к Б24 и получив внятное сообщение об ошибке из-за удаления приложения
- получив запрос на инсталляцию (нельзя установить приложение которое уже стоит на портале)
Т.е. в этот момент нам надо перевести приложение в состояние удалено, но без токена, т.е. это не штатный на мой взгляд метод ну или действительно сделать ручку получения токена из сущности и подпихивать его в метод удаления будто он пришёл от Б24
@camaxtly что думаешь?
There was a problem hiding this comment.
Давайте неявное сделяем явным.
-
Команда это ответ на внешний "пинг". В предметной области у нас приходит событие и мы его обрабатываем. Тут именно реакция на удаление приложения. Это п1 из описанного выше. Здесь токен должен быть в команде, и помимо токена должен быть АПП ИД, или другие данные идентифицирующие приложение.
-
Все что описано в п2 это "неидеальность" этого мира, которую мы опытным путем решаем. По косвенным признакам мы понимаем и/или догадываемся, что приложение удалено. Это не реакция на собитие, а скорее всего наш фоновый чек статуса – ок или не ок (а может и не фоновый). Как вариант выделить в отдельный сценарий (тоже под вопросом, а надо ли).
Я бы смотрел на код. И как вариант, что для п2 мешает нам прокидывать токен как "пустую" строку?
There was a problem hiding this comment.
@mesilov а дай линку, где у нас зафиксированы структуры данных, приходящих от Битрикс24 – установка/удаление
There was a problem hiding this comment.
2.1. в б24 у нас приложение уже удалено, обработчики удалены, токены все инвалидированы, все запросы с токенами по этому приложению будут падать с ошибкой (приложение удалено) @KarlsonComplete пометь себе поставить локальное приложение и убедиться, что SDK выбрасывает эксепшен https://github.com/bitrix24/b24phpsdk/blob/main/src/Core/ApiLevelErrorHandler.php - тут не уверен, если там пишется однозначно, что приложение удалено, то должен быть типизированный эксепшен. Если нет, то ставь ишью.
@mesilov я создал локальное приложение ,
и не особо понимаю что мне теперь делать ? Удалить его и просто попробовать к нему подключиться с несуществующими параметрами ? (clien_id,client_secret)
| $applicationInstallation = $this->getEntityManager()->getRepository(ApplicationInstallation::class)->find($uuid); | ||
|
|
||
| if (null === $applicationInstallation) { | ||
| throw new ApplicationInstallationNotFoundException( |
There was a problem hiding this comment.
Здесь скорее всего это не надо. Нет такого сценария, где ты на вход получаешь УИД, а потом его не находишь в БД.
| } | ||
|
|
||
| #[\Override] | ||
| public function delete(Uuid $uuid): void |
There was a problem hiding this comment.
У меня пока нет сценария для того, чтоб этот метод существовал. Или привести сценарий, или удалить
| $command->isBitrix24UserAdmin, | ||
| $command->memberId, | ||
| $command->domain->value, | ||
| Bitrix24AccountStatus::new, |
There was a problem hiding this comment.
Статус тоже по идее не надо пробрасывать.
| $this->flusher->flush($bitrix24Account); | ||
|
|
||
| $applicationInstallation = new ApplicationInstallation( | ||
| $command->uuid, |
| use Bitrix24\SDK\Core\Credentials\Scope; | ||
| use Symfony\Component\Uid\Uuid; | ||
|
|
||
| readonly class Command |
There was a problem hiding this comment.
Убираем сценарий "переустановки", как отдельный сценарий. У нас остается сценарий "установка" и внутри него мы уже смотрим, это новая-чистая установка или повторная.
…ия / статус) из конструктора . Объявляем теперь при создании.
| true | ||
| ); | ||
|
|
||
| $applicationToken = Uuid::v7()->toRfc4122(); |
There was a problem hiding this comment.
Ты не должен придумывать applicationToken, его тебе дает б24 либо на первом шаге, либо чуть позже - асинхронно в событии!
No description provided.