feat(contact): optimize contact import and pagination performance#40
Merged
DavidsonGomes merged 2 commits intoMay 5, 2026
Conversation
- Add support for person/company type differentiation - Add processing for social profiles (linkedin, facebook, instagram, twitter, github) - Add location attributes (city, country, country_code) - Add custom attributes auto-mapping for unknown fields - Add empresas_vinculadas field for linking persons to companies - Import companies first, then persons, then process linkages - Updated CSV sample with all 19 supported fields
Reviewer's GuideRefactors the contact CSV import pipeline to classify and import companies and persons separately with richer field mapping and logging, introduces tax ID sanitization and improved phone formatting in ContactManager, adds database indexes and a LEFT JOIN-based scope to speed up resolved contacts listing, caches contact count in the contacts API, and introduces roles/user_roles models to identify admin users via evo-auth roles for admin mailers, along with Docker entrypoint and schema/migration adjustments. Sequence diagram for the optimized contact CSV import processsequenceDiagram
actor Admin
participant DataImportJob
participant ContactManager
participant Contact
participant Mailer as AdministratorNotifications_AccountNotificationMailer
Admin->>DataImportJob: enqueue(data_import)
DataImportJob->>DataImportJob: perform(data_import)
DataImportJob->>DataImportJob: process_import_file
DataImportJob->>DataImportJob: parse_csv_and_classify_contacts
loop each CSV row
DataImportJob->>ContactManager: build_contact(params)
ContactManager-->>DataImportJob: contact
alt tipo == company
DataImportJob->>DataImportJob: classify as company_rows
else tipo == person
DataImportJob->>DataImportJob: classify as person_rows
end
end
DataImportJob->>DataImportJob: import_companies(company_rows)
DataImportJob->>Contact: import(companies)
Contact-->>DataImportJob: import_result_companies
DataImportJob->>DataImportJob: import_persons(person_rows)
DataImportJob->>Contact: import(persons)
Contact-->>DataImportJob: import_result_persons
DataImportJob->>DataImportJob: process_company_linkages(person_rows)
loop each person with empresas_vinculadas
DataImportJob->>Contact: companies.find_by(name, type company)
alt company exists
DataImportJob->>Contact: contact.add_company(company)
else company missing
DataImportJob->>Contact: create!(type company, name)
Contact-->>DataImportJob: new_company
DataImportJob->>Contact: contact.add_company(new_company)
end
end
DataImportJob->>DataImportJob: update_data_import_status
DataImportJob->>DataImportJob: save_failed_records_csv
alt success
DataImportJob->>Mailer: contact_import_complete(data_import)
Mailer-->>Admin: completion email
else CSV::MalformedCSVError
DataImportJob->>DataImportJob: handle_csv_error
DataImportJob->>Mailer: contact_import_failed
Mailer-->>Admin: failure email
end
Entity relationship diagram for users, roles, and user_roleserDiagram
users {
uuid id
varchar email
}
roles {
uuid id
varchar key
varchar name
}
user_roles {
uuid id
uuid user_id
uuid role_id
uuid granted_by_id
}
users ||--o{ user_roles : has_many
roles ||--o{ user_roles : has_many
users ||--o{ user_roles : grants
Class diagram for ContactManager and contact import mappingclassDiagram
class DataImport_ContactManager {
+find_or_initialize_contact(params)
+find_existing_contact(params)
+build_contact(params)
+find_contact_by_identifier(params)
+find_contact_by_email(params)
+find_contact_by_phone_number(params)
+find_contact_by_tax_id(params)
+format_phone_number(phone_number)
+sanitize_tax_id(tax_id)
+update_contact_with_merged_attributes(params, contact)
-update_contact_attributes(params, contact)
-build_name(params)
-process_location_attributes(params, contact)
-process_social_attributes(params, contact)
-process_description(params, contact)
-process_custom_attributes(params, contact)
}
class Contact {
+String name
+String last_name
+String email
+String phone_number
+String tax_id
+String type
+String website
+String industry
+String country_code
+Hash additional_attributes
+Hash custom_attributes
+add_company(company)
+self.import(records)
+self.companies()
+self.resolved_contacts()
}
DataImport_ContactManager ..> Contact : builds_and_updates
class DataImportJob {
+perform(data_import)
-process_import_file()
-parse_csv_and_classify_contacts()
-append_rejected_contact(row, contact, rejected_contacts)
-import_contacts(contacts)
-import_companies(company_rows)
-import_persons(person_rows)
-process_company_linkages(person_rows)
-update_data_import_status(processed_records, rejected_records)
-save_failed_records_csv(rejected_contacts)
-handle_csv_error(error)
-send_import_notification_to_admin()
-send_import_failed_notification_to_admin()
}
DataImportJob ..> DataImport_ContactManager : uses
DataImportJob ..> Contact : imports
Class diagram for User, Role, and UserRole relationshipsclassDiagram
class User {
+UUID id
+String email
+has_many user_roles
+has_many roles
}
class Role {
+UUID id
+String key
+String name
+has_many user_roles
+has_many users
+administrator?()
+self.administrator_role()
+self.administrator_users()
}
class UserRole {
+UUID id
+UUID user_id
+UUID role_id
+UUID granted_by_id
+belongs_to user
+belongs_to role
+belongs_to granted_by
}
User "1" -- "*" UserRole : has_many
Role "1" -- "*" UserRole : has_many
UserRole "*" -- "1" User : user
UserRole "*" -- "1" Role : role
UserRole "*" -- "0..1" User : granted_by
class AdministratorNotifications_BaseMailer {
-admin_emails()
-settings_url(section)
}
AdministratorNotifications_BaseMailer ..> User : queries
AdministratorNotifications_BaseMailer ..> Role : filters_by_role_key
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
3 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
format_phone_numberimplementation strips all non-digits before checkingstart_with?('+'), so an existing leading+will never be preserved and all formatted numbers will lose the plus sign; consider preserving a leading+before removing other non-digit characters. Role.administrator_roleusesfind_by(key: %w[account_owner administrator admin]), which will look for a single row whosekeyequals the entire array; if you intend to match any of those keys, switch towhere(key: %w[account_owner administrator admin]).firstor a similar approach.- The schema.rb changes include removals/additions for several unrelated tables and foreign keys (e.g., events, sessions, user_tours); it would be good to verify schema was generated from a clean database so only the intended contact-related index changes are committed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `format_phone_number` implementation strips all non-digits before checking `start_with?('+')`, so an existing leading `+` will never be preserved and all formatted numbers will lose the plus sign; consider preserving a leading `+` before removing other non-digit characters.
- `Role.administrator_role` uses `find_by(key: %w[account_owner administrator admin])`, which will look for a single row whose `key` equals the entire array; if you intend to match any of those keys, switch to `where(key: %w[account_owner administrator admin]).first` or a similar approach.
- The schema.rb changes include removals/additions for several unrelated tables and foreign keys (e.g., events, sessions, user_tours); it would be good to verify schema was generated from a clean database so only the intended contact-related index changes are committed.
## Individual Comments
### Comment 1
<location path="app/services/data_import/contact_manager.rb" line_range="55-59" />
<code_context>
+ Contact.find_by(tax_id: tax_id)
+ end
+
def format_phone_number(phone_number)
- phone_number.start_with?('+') ? phone_number : "+#{phone_number}"
+ return unless phone_number.present?
+
+ cleaned = phone_number.to_s.gsub(/\D/, '')
+ cleaned.start_with?('+') ? cleaned : "+#{cleaned}"
+ end
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Phone normalization logic strips non-digits and then checks for '+', which can never be present.
Because `gsub(/�5CD/, '')` removes `+`, `cleaned.start_with?('+')` is always false, so `+` will always be prepended, even when the original already had a country code. If you want `+<digits>` as the normalized form, either check the original value before stripping, or always prepend `+` after stripping and rely on callers to expect that format. For example:
```ruby
cleaned = phone_number.to_s.gsub(/\D/, '')
"+#{cleaned}" if cleaned.present?
```
</issue_to_address>
### Comment 2
<location path="app/models/contact.rb" line_range="199-201" />
<code_context>
- where(
- "contacts.email <> '' OR contacts.phone_number <> '' OR contacts.identifier <> '' OR contacts.id IN (SELECT DISTINCT contact_id FROM contact_inboxes WHERE contact_id IS NOT NULL)"
+ # This uses LEFT JOIN for better performance
+ joins(
+ "LEFT JOIN contact_inboxes ON contact_inboxes.contact_id = contacts.id"
+ ).where(
+ "contacts.email <> '' OR contacts.phone_number <> '' OR contacts.identifier <> '' OR contact_inboxes.id IS NOT NULL"
)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a LEFT JOIN with a non-null condition on contact_inboxes may introduce duplicate contacts and unexpected pagination behavior.
This change alters the behavior of `resolved_contacts`: the LEFT JOIN plus `WHERE contact_inboxes.id IS NOT NULL` will now return one row per `contact_inboxes` record, so contacts with multiple inboxes will appear multiple times unless the caller uses `DISTINCT`. If callers expect one row per contact (for pagination, counts, etc.), this can introduce subtle bugs. Consider preserving the previous “one row per contact” behavior by adding `.distinct` or otherwise de-duplicating, e.g.:
```ruby
joins("LEFT JOIN contact_inboxes ON contact_inboxes.contact_id = contacts.id")
.where("contacts.email <> '' OR contacts.phone_number <> '' OR contacts.identifier <> '' OR contact_inboxes.id IS NOT NULL")
.distinct
```
</issue_to_address>
### Comment 3
<location path="app/mailers/administrator_notifications/base_mailer.rb" line_range="25" />
<code_context>
def admin_emails
- User.where(role: 'administrator').pluck(:email)
+ User.joins(:roles).where(roles: { key: %w[account_owner administrator admin] }).pluck(:email)
end
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using joins on roles can return duplicate admin emails when a user has multiple admin roles.
This could lead to users receiving the same email multiple times if they hold more than one admin-type role. To prevent that, add `distinct` before plucking:
```ruby
User.joins(:roles)
.where(roles: { key: %w[account_owner administrator admin] })
.distinct
.pluck(:email)
```
</issue_to_address>
### Comment 4
<location path="app/jobs/data_import_job.rb" line_range="100" />
<code_context>
def append_rejected_contact(row, contact, rejected_contacts)
row['errors'] = contact.errors.full_messages.join(', ')
rejected_contacts << row
+ Rails.logger.debug "📊 DataImportJob: Rejected contact - name: #{row[:nome] || row[:name]}, errors: #{row['errors']}"
end
</code_context>
<issue_to_address>
**nitpick (bug_risk):** Mixed symbol and string access on CSV row may always log a blank name.
Because `row` comes from CSV data it will typically have string keys. Here you’re assigning `row['errors']` but reading the name via symbol keys (`row[:nome] || row[:name]`), so the logged name will likely be `nil` even when present.
Consider normalizing the keys before use (e.g. `row = row.to_h.with_indifferent_access`) or consistently using string keys:
```ruby
name = row['nome'] || row['name']
Rails.logger.debug "📊 DataImportJob: Rejected contact - name: #{name}, errors: #{row['errors']}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
55
to
+59
| def format_phone_number(phone_number) | ||
| phone_number.start_with?('+') ? phone_number : "+#{phone_number}" | ||
| return unless phone_number.present? | ||
|
|
||
| cleaned = phone_number.to_s.gsub(/\D/, '') | ||
| cleaned.start_with?('+') ? cleaned : "+#{cleaned}" |
There was a problem hiding this comment.
issue (bug_risk): Phone normalization logic strips non-digits and then checks for '+', which can never be present.
Because gsub(/�5CD/, '') removes +, cleaned.start_with?('+') is always false, so + will always be prepended, even when the original already had a country code. If you want +<digits> as the normalized form, either check the original value before stripping, or always prepend + after stripping and rely on callers to expect that format. For example:
cleaned = phone_number.to_s.gsub(/\D/, '')
"+#{cleaned}" if cleaned.present?
Comment on lines
+199
to
+201
| joins( | ||
| "LEFT JOIN contact_inboxes ON contact_inboxes.contact_id = contacts.id" | ||
| ).where( |
There was a problem hiding this comment.
issue (bug_risk): Using a LEFT JOIN with a non-null condition on contact_inboxes may introduce duplicate contacts and unexpected pagination behavior.
This change alters the behavior of resolved_contacts: the LEFT JOIN plus WHERE contact_inboxes.id IS NOT NULL will now return one row per contact_inboxes record, so contacts with multiple inboxes will appear multiple times unless the caller uses DISTINCT. If callers expect one row per contact (for pagination, counts, etc.), this can introduce subtle bugs. Consider preserving the previous “one row per contact” behavior by adding .distinct or otherwise de-duplicating, e.g.:
joins("LEFT JOIN contact_inboxes ON contact_inboxes.contact_id = contacts.id")
.where("contacts.email <> '' OR contacts.phone_number <> '' OR contacts.identifier <> '' OR contact_inboxes.id IS NOT NULL")
.distinct|
|
||
| def admin_emails | ||
| User.where(role: 'administrator').pluck(:email) | ||
| User.joins(:roles).where(roles: { key: %w[account_owner administrator admin] }).pluck(:email) |
There was a problem hiding this comment.
suggestion (bug_risk): Using joins on roles can return duplicate admin emails when a user has multiple admin roles.
This could lead to users receiving the same email multiple times if they hold more than one admin-type role. To prevent that, add distinct before plucking:
User.joins(:roles)
.where(roles: { key: %w[account_owner administrator admin] })
.distinct
.pluck(:email)| def append_rejected_contact(row, contact, rejected_contacts) | ||
| row['errors'] = contact.errors.full_messages.join(', ') | ||
| rejected_contacts << row | ||
| Rails.logger.debug "📊 DataImportJob: Rejected contact - name: #{row[:nome] || row[:name]}, errors: #{row['errors']}" |
There was a problem hiding this comment.
nitpick (bug_risk): Mixed symbol and string access on CSV row may always log a blank name.
Because row comes from CSV data it will typically have string keys. Here you’re assigning row['errors'] but reading the name via symbol keys (row[:nome] || row[:name]), so the logged name will likely be nil even when present.
Consider normalizing the keys before use (e.g. row = row.to_h.with_indifferent_access) or consistently using string keys:
name = row['nome'] || row['name']
Rails.logger.debug "📊 DataImportJob: Rejected contact - name: #{name}, errors: #{row['errors']}"
DavidsonGomes
added
needs-review
lpcoutinho
added a commit
to lpcoutinho/evo-ai-crm-community
that referenced
this pull request
May 9, 2026
Reverted the migration timestamp renames from the original PR evolution-foundation#40 feedback. The develop branch decided to keep the original timestamps: - 20241020000100_optimize_contacts_performance.rb (kept) - 20251117132621_add_type_to_contacts.rb (kept) This commit aligns our branch with develop, keeping only the test coverage added in the previous commit (a638778).
Contributor
Author
|
@DavidsonGomes - Oi Davidson, tudo bem? Gostaria de compartilhar o status atual sobre os pontos que você mencionou no feedback do PR #40: PR #52 - Cobertura de Testes ✅Criei o PR #52 com cobertura de testes para o que foi mergeado:
Status dos 4 PRs Mencionados
O que FALTA: Sanitização tax_idAtualmente há apenas sanitização básica:
O que pode ser implementado:
Como o PR #40 já foi mergeado, podemos criar um PR separado focado apenas em melhorar a sanitização/validação de tax_id (CPF/CNPJ). Isso seria mais simples de revisar e testar. O que você acha? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Objetivo
Otimizar o processo de importação de contatos e melhorar a performance da listagem de contatos com mais de 1000 registros.
✅ Alterações Realizadas
1. Sanitização de CPF/CNPJ na Importação
sanitize_tax_idnoContactManagerfind_contact_by_tax_ideupdate_contact_attributes2. Otimização de Performance
contact_inboxese contatosresolved_contactsotimizada comLEFT JOIN3. Correções
format_phone_numberpara manter prefixo+🧪 Testes
tax_idé armazenado apenas com númerosSummary by Sourcery
Optimize contact import, enrichment, and listing performance while integrating role-based admin notifications.
New Features:
Bug Fixes:
Enhancements:
Build: