Implement Bitrix24Partners support (issue #70) #71
Conversation
This commit implements comprehensive support for Bitrix24Partners following the established DDD/CQRS patterns in the codebase. ## Features Implemented: ### Core Domain - Bitrix24Partner entity with full interface implementation - Repository with all required query methods - Doctrine XML mapping with unique constraint on bitrix24PartnerId - PhoneNumber Doctrine custom type for phone number storage ### Use Cases (CQRS) - Create: Create new partner records - Update: Update partner information - Delete: Soft-delete partners (mark as deleted) - MarkAsBlocked: Block partner accounts - MarkAsActive: Reactivate blocked partners ### CLI Tools - ScrapePartnersCommand: Web scraper for https://www.bitrix24.ru/partners/ - Parses partner data from HTML - Generates CSV output - Configurable URL and output path - ImportPartnersCsvCommand: CSV import utility - Bulk import partners from CSV files - Phone number parsing with libphonenumber - Error handling with --skip-errors option ### Testing - Unit tests for entity (extends SDK contract tests) - Unit tests for Create command validation ## Technical Details: - Follows existing bounded context patterns - Uses value objects where appropriate - Implements domain events for all state changes - Full validation in command constructors - Comprehensive error handling Resolves: #70
Summary of ChangesHello @mesilov, 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! This pull request delivers a complete solution for managing Bitrix24 partners within the application, adhering to DDD/CQRS principles. It establishes the core domain model, provides a full set of transactional use cases for partner lifecycle management, and introduces practical command-line utilities for data acquisition and import. The changes are designed to be robust, testable, and integrated seamlessly with existing architectural patterns. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for Bitrix24Partners, following established DDD/CQRS patterns. The changes include a new entity, repository, use cases for CRUD operations, and CLI tools for scraping and importing partner data. The implementation is well-structured. However, I've identified a few issues that should be addressed: a bug in the CSV import logic that could lead to data corruption, a security vulnerability in the web scraper, and some opportunities for code refinement to improve consistency and maintainability.
| curl_setopt($ch, CURLOPT_URL, $url); | ||
| curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); | ||
| curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true); | ||
| curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false); |
There was a problem hiding this comment.
Disabling SSL certificate verification by setting CURLOPT_SSL_VERIFYPEER to false introduces a significant security vulnerability. This makes the cURL request susceptible to man-in-the-middle (MitM) attacks, where an attacker could intercept the connection and tamper with the data. This option should be set to true to enforce secure communication. If the target server uses a self-signed certificate, the proper way to handle it is by specifying a custom CA bundle using CURLOPT_CAINFO.
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, true);| public function setPhone(?PhoneNumber $phone): void | ||
| { | ||
| $this->phone = $phone; | ||
| $this->updatedAt = new CarbonImmutable(); | ||
|
|
||
| $this->events[] = new Bitrix24PartnerPhoneChangedEvent( | ||
| $this->id, | ||
| new CarbonImmutable() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The setPhone method dispatches a Bitrix24PartnerPhoneChangedEvent every time it's called, regardless of whether the phone number has actually changed. This is inconsistent with other setters in the class (like setTitle and setSite) which check for changes before raising an event. This can lead to unnecessary event processing. It's better to add a condition to only dispatch the event when the phone number is modified.
public function setPhone(?PhoneNumber $phone): void
{
if ($this->phone != $phone) {
$this->phone = $phone;
$this->updatedAt = new CarbonImmutable();
$this->events[] = new Bitrix24PartnerPhoneChangedEvent(
$this->id,
new CarbonImmutable()
);
}
}| $partner = $this->getEntityManager()->getRepository(Bitrix24Partner::class) | ||
| ->createQueryBuilder('p') | ||
| ->where('p.id = :id') | ||
| ->andWhere('p.status != :status') | ||
| ->setParameter('id', $uuid) | ||
| ->setParameter('status', Bitrix24PartnerStatus::deleted) | ||
| ->getQuery() | ||
| ->getOneOrNullResult() | ||
| ; |
There was a problem hiding this comment.
In this method, you're calling $this->getEntityManager()->getRepository(Bitrix24Partner::class) to start a query. Since this class is already the designated repository for the Bitrix24Partner entity, you can simplify this by calling $this->createQueryBuilder('p') directly. This makes the code more concise and readable. This same improvement can be applied to other methods in this class like findByBitrix24PartnerId, findByTitle, and findByExternalId.
$partner = $this->createQueryBuilder('p')
->where('p.id = :id')
->andWhere('p.status != :status')
->setParameter('id', $uuid)
->setParameter('status', Bitrix24PartnerStatus::deleted)
->getQuery()
->getOneOrNullResult()
;![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
config/xml/Bitrix24.Lib.Bitrix24Partners.Entity.Bitrix24Partner.dcm.xml
Outdated
Show resolved
Hide resolved
- Add symfony/http-client dependency to composer.json - Update ScrapePartnersCommand to use HttpClientInterface via DI - Replace curl_* functions with HttpClient API - Improve testability and follow Symfony best practices Benefits: - Better error handling and exceptions - Easier to mock in tests - Consistent with Symfony ecosystem - Type safety with readonly properties
config/xml/Bitrix24.Lib.Bitrix24Partners.Entity.Bitrix24Partner.dcm.xml
Outdated
Show resolved
Hide resolved
config/xml/Bitrix24.Lib.Bitrix24Partners.Entity.Bitrix24Partner.dcm.xml
Outdated
Show resolved
Hide resolved
| public ?string $site = null, | ||
| public ?PhoneNumber $phone = null, | ||
| public ?string $email = null, | ||
| public ?int $bitrix24PartnerId = null, |
There was a problem hiding this comment.
@claude параметр $bitrix24PartnerId обязательный
| public ?string $site = null, | ||
| public ?PhoneNumber $phone = null, | ||
| public ?string $email = null, | ||
| public ?int $bitrix24PartnerId = null, |
There was a problem hiding this comment.
@claude параметр $bitrix24PartnerId обязательный
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Bitrix24\Lib\Bitrix24Partners\UseCase\MarkAsBlocked; |
This commit implements several improvements based on code review: 1. **Database schema improvements:** - Renamed table from `bitrix24partner` to `bitrix24_partners` - Made `b24_partner_id` required (non-nullable) with unique constraint 2. **Entity improvements:** - Generate UUID internally in constructor (removed UUID parameter) - Made bitrix24PartnerId required (int instead of ?int) - Removed isEmitBitrix24PartnerCreatedEvent parameter, always emit creation event - Fixed setPhone() to only emit event when phone actually changes - Updated getBitrix24PartnerId() return type from ?int to int 3. **CSV parsing fixes:** - Fixed bug where '0' was incorrectly treated as empty - Improved whitespace handling - now properly trims before validation - Added explicit validation for required bitrix24PartnerId field - More robust parsing logic using isset() and explicit empty string checks 4. **Use case updates:** - Updated Create/Command to make bitrix24PartnerId required second parameter - Updated Create/Handler to work with new constructor signature - Improved validation messages 5. **Test updates:** - Updated entity test to work with new constructor (ignores passed UUID) - Updated command test with corrected parameter order and validation messages These changes improve type safety, data integrity, and eliminate edge cases in CSV parsing that could lead to incorrect data import.
This commit implements several major improvements and additions: ## 1. Enhanced CSV Import with league/csv - Added `league/csv` dependency for robust CSV parsing - Replaced manual fgetcsv() with league/csv Reader - Implemented progress bar for import operations - Better error handling and iterator management - Named column access instead of index-based ## 2. Progress Bars for Long Operations - Added ProgressBar to ImportPartnersCsvCommand - Shows progress, percentage, time elapsed/estimated, and memory usage - Proper cleanup on error and success ## 3. Immutable bitrix24PartnerId - Removed bitrix24PartnerId from Update\Command - Removed setBitrix24PartnerId() call from Update\Handler - Marked setBitrix24PartnerId() as @deprecated with TODO - Added TODO to create issue in b24phpsdk repository ## 4. Phone Number Type Refactoring - Removed custom src/Infrastructure/Doctrine/PhoneNumberType.php - Added odolbeau/phone-number-bundle dependency - Updated EntityManagerFactory to use bundle's PhoneNumberType - Leverages mature, well-tested phone number handling ## 5. Comprehensive Test Coverage - Added Bitrix24PartnerRepositoryTest extending SDK contract tests - Added Create\HandlerTest for functional testing - Tests follow established patterns from Bitrix24Accounts - Full integration with EntityManager and event dispatching ## Dependencies Added - league/csv: ^9.0 - Professional CSV handling - odolbeau/phone-number-bundle: ^3.0 - Phone number Doctrine integration These changes improve code quality, testability, and user experience with better progress feedback during long-running operations.
This commit implements comprehensive support for Bitrix24Partners following the established DDD/CQRS patterns in the codebase.
Features Implemented:
Core Domain
Use Cases (CQRS)
CLI Tools
Testing
Technical Details:
Resolves: #70