-
Notifications
You must be signed in to change notification settings - Fork 23
feat(contact): optimize contact import and pagination performance #40
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1095,7 +1095,7 @@ DEPENDENCIES | |
| working_hours | ||
|
|
||
| RUBY VERSION | ||
| ruby 3.4.4p34 | ||
| ruby 3.4.4p34 | ||
|
|
||
| BUNDLED WITH | ||
| 2.5.16 | ||
| 4.0.11 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ def settings_url(section) | |
| private | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 User.joins(:roles)
.where(roles: { key: %w[account_owner administrator admin] })
.distinct
.pluck(:email) |
||
| end | ||
|
|
||
| def liquid_locals | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,9 +195,11 @@ def webhook_data | |
| def self.resolved_contacts | ||
| # Include contacts that have email, phone_number, or identifier | ||
| # Also include contacts that have contact_inboxes (have at least one conversation) | ||
| # This ensures Telegram and other social media contacts appear in the list | ||
| 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( | ||
|
Comment on lines
+199
to
+201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
| "contacts.email <> '' OR contacts.phone_number <> '' OR contacts.identifier <> '' OR contact_inboxes.id IS NOT NULL" | ||
| ) | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| # Role model - synced from evo-auth-service | ||
| # This model provides read-only access to roles managed by evo-auth-service | ||
| class Role < ApplicationRecord | ||
| # Evolution Reference Model - managed by evo-auth-service | ||
| # This model serves only as a reference to sync data from evo-auth-service | ||
|
|
||
| self.table_name = 'roles' | ||
|
|
||
| # Read-only model - data is synced from evo-auth-service | ||
| has_many :user_roles, dependent: :destroy_async | ||
| has_many :users, through: :user_roles | ||
|
|
||
| validates :key, presence: true, uniqueness: true | ||
| validates :name, presence: true | ||
|
|
||
| # Check if this is an administrator role | ||
| def administrator? | ||
| key.in?(%w[account_owner administrator admin]) | ||
| end | ||
|
|
||
| # Find administrator role | ||
| def self.administrator_role | ||
| find_by(key: %w[account_owner administrator admin]) | ||
| end | ||
|
|
||
| # Find users with administrator roles | ||
| def self.administrator_users | ||
| Role.where(key: %w[account_owner administrator admin]).flat_map(&:users).uniq | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| # UserRole model - joins table for users and roles | ||
| # This model provides read-only access to user_roles managed by evo-auth-service | ||
| class UserRole < ApplicationRecord | ||
| # Evolution Reference Model - managed by evo-auth-service | ||
| # This model serves only as a reference to sync data from evo-auth-service | ||
|
|
||
| self.table_name = 'user_roles' | ||
|
|
||
| belongs_to :user | ||
| belongs_to :role | ||
| belongs_to :granted_by, class_name: 'User', optional: true | ||
|
|
||
| validates :user, presence: true | ||
| validates :role, presence: true | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick (bug_risk): Mixed symbol and string access on CSV row may always log a blank name.
Because
rowcomes from CSV data it will typically have string keys. Here you’re assigningrow['errors']but reading the name via symbol keys (row[:nome] || row[:name]), so the logged name will likely benileven when present.Consider normalizing the keys before use (e.g.
row = row.to_h.with_indifferent_access) or consistently using string keys: